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

[Paul Richards]

On 15/01/07, Diego Biurrun <diego at biurrun.de> wrote:
> 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.
>

Done.


-- 
Paul Richards
-------------- next part --------------
A non-text attachment was scrubbed...
Name: theora.patch
Type: text/x-patch
Size: 13184 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070117/63e59e16/attachment.bin>