[FFmpeg-devel] [PATCH] WebM mux/demux

[David Conrad]

On May 20, 2010, at 7:46 PM, James Zern wrote:

> On Thu, May 20, 2010 at 19:23, Aurelien Jacobs <aurel at gnuage.org> wrote:
>> On Thu, May 20, 2010 at 04:27:02AM -0400, David Conrad wrote:
>>> On May 19, 2010, at 6:14 PM, Ronald S. Bultje wrote:
>>> 
>>>> Hi,
>>>> 
>>>> On Wed, May 19, 2010 at 5:22 PM, David Conrad <lessen42 at gmail.com> wrote:
>>>>> (for certain definitions of fixed...)
>>>> [..]
>>>>> +        int probelen = strlen(matroska_doctypes[i]);
>>>>> +        for (n = 4+size; n <= 4+size+total-(probelen-1); n++)
>>>>> +            if (!memcmp(p->buf+n, matroska_doctypes[i], probelen-1))
>>>> 
>>>> Why -1? The original code has -1 because sizeof(str) includes the
>>>> terminating zero. strlen() doesn't count that, so the -1 becomes
>>>> unnecessary.
>>> 
>>> Zombie coding.
>>> 
>>>> Otherwise yes, but Baptiste is maintainer.
>>> 
>>> You meant Aurel?
>>> 
>> 
>>> commit d21f6fd54a9b624321ad208959c5de38f138b324
>>> Author: David Conrad <lessen42 at gmail.com>
>>> Date:   Wed May 19 13:10:33 2010 -0400
>>> 
>>>     matroskadec: Add webm doctype
>>> 
>>>     Based on a Google patch
>>> 
>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>> index 1bde1af..a287b36 100644
>>> --- a/libavformat/matroskadec.c
>>> +++ b/libavformat/matroskadec.c
>>> @@ -209,6 +209,7 @@ typedef struct {
>>> 
>>>  typedef struct {
>>>      AVFormatContext *ctx;
>>> +    const char *doctype;
>> 
>> Why ? It don't look useful... Could be a local var.
>>> [...]
>>>          av_log(matroska->ctx, AV_LOG_ERROR,
>>>                 "EBML header using unsupported features\n"
>>> -               "(EBML version %"PRIu64", doctype %s, doc version %"PRIu64")\n",
>>> -               ebml.version, ebml.doctype, ebml.doctype_version);
>>> +               "(EBML version %"PRIu64", doc version %"PRIu64")\n",
>>> +               ebml.version, ebml.doctype_version);
>> 
>> I'm not sure it's a good idea to remove doctype from this message.
>> Does it do any harm ?

Not really, kept.

>>> +    for (i = 0; i < FF_ARRAY_ELEMS(matroska_doctypes); i++)
>>> +        if (!strcmp(ebml.doctype, matroska_doctypes[i])) {
>>> +            matroska->doctype = matroska_doctypes[i];
>>> +            break;
>>> +        }
>>> +    if (!matroska->doctype) {
>> 
>> Here you could just do :
>>  if (i >= FF_ARRAY_ELEMS(matroska_doctypes))
>> and totally drop matroska->doctype.
>> 
>> Except this, patch looks OK.
>> 
> Here is a roll-up of what was discussed on the demux side, including a
> free for the ebml on the patch welcome returns.
> In addition this also stores the doctype similar to how the mov
> demuxer stores the major brand/minor version.

Applied, thanks