[Ffmpeg-devel] Re: [PATCH] FFmpeg theora encoding

[Diego Biurrun]

On Sat, Jan 13, 2007 at 12:31:12PM +0000, Paul Richards wrote:
> On 12/01/07, Diego Biurrun <diego at biurrun.de> wrote:
> >
> >Thanks, I like this one much better.  However, I think that the
> >following
> >
> >> --- libavcodec/Makefile       (revision 7438)
> >> +++ libavcodec/Makefile       (working copy)
> >> @@ -257,6 +257,7 @@
> >>  OBJS-$(CONFIG_LIBFAAD)                 += faad.o
> >>  OBJS-$(CONFIG_LIBGSM)                  += libgsm.o
> >>  OBJS-$(CONFIG_LIBMP3LAME)              += mp3lameaudio.o
> >> +OBJS-$(CONFIG_THEORA_ENCODER)          += libtheoraenc.o
> >
> >should be
> >
> >  +OBJS-$(CONFIG_LIBTHEORA)               += libtheoraenc.o
> >
> >as CONFIG_LIBTHEORA is what configure will add to config.mak - if I am
> >not mistaken...
> 
> You are correct.  CONFIG_THEORA_ENCODER is always enabled and is
> independent of the CONFIG_LIBTHEORA option.  This led me to fix a bug
> I had in allcodecs.c.
> These have both been corrected along with the one trailing whitespace
> character I could find.

Yes, fixed.  The patch is OK from my side apart from the following
issue:

> --- libavcodec/allcodecs.c	(revision 7444)
> +++ libavcodec/allcodecs.c	(working copy)
> @@ -198,6 +198,9 @@
> +#ifdef CONFIG_LIBTHEORA
> +    REGISTER_ENCODER(THEORA, theora);
> +#endif
> --- libavcodec/avcodec.h	(revision 7444)
> +++ libavcodec/avcodec.h	(working copy)
> @@ -2182,6 +2182,7 @@
>  extern AVCodec snow_encoder;
> +extern AVCodec theora_encoder;
>  extern AVCodec vorbis_encoder;
> --- libavcodec/libtheoraenc.c	(revision 0)
> +++ libavcodec/libtheoraenc.c	(revision 0)
> @@ -0,0 +1,255 @@
> +AVCodec theora_encoder =
> +{
> +    .name = "theora",

IMO the encoder should be named libtheora in all cases so as not to
conflict with a possible future native encoder.  Also, it's inconsistent
to name it theora in some places and libtheora in others.

Diego