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

[Aurelien Jacobs]

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 ?

> +    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.

Aurel