[Ffmpeg-devel] FFmpeg Theora encoding patch

[Paul Richards]

On 07/01/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> Hi
>
> On Sun, Jan 07, 2007 at 03:41:46PM +0000, Paul Richards wrote:
> > Hi,
> > I have rolled in many of these suggestions.  See below for details.. :)
> >
> > I'll post the revised patch to a new thread once I've rolled in the
> > suggestions from everyone.
> >
> >
> > On 06/01/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> > >Hi
> > >
> > >On Sat, Jan 06, 2007 at 03:51:28PM +0000, Paul Richards wrote:
> > >> Hi,
> > >> Attached is my patch to add theora encoding to ffmpeg's libavcodec (by
> > >> using libtheora).  I am requesting help to fix the bug I mention below
> > >> and am seeking general comments before I submit the patch properly.
> > >>
> > >> Files encoded using this encoder have a problem playing in VLC.  The
> > >> files will not play unless "Drop late frames" has been unticked in the
> > >> advanced video settings.  Hopefully someone can tell me where this
> > >> problem comes from.  I am not sure which API (FFmpeg or libtheora) I
> > >> am misusing.
> > >
> > >are these .avi or .ogg?
> > >and does it work if you create the same theora file with another tool then
> > >ffmpeg?
> > >and what does "ffmpeg -v 9 -i yourfile" say?
> > >
> >
> > I get lots of debugging info (presumably from vp3 decoder), none of
> > the output looks like error.  Here are the first few lines:
> >
> > [theora @ 0x458594]Theora bitstream version 30200
> > [theora @ 0x458594]344 bits left in packet 81
> > [theora @ 0x458594]7 bits left in packet 82
> > Input #0, avi, from 'test.avi':
>             ^^^
> theora in avi is not a standard thing as the extradata format is not
> officially specified so you should not expect it to work with all players
>

So would my patch with broken VLC playback be acceptable?  I'd be
happy to later fix the implementation if theora in avi ever does get
officially specified.


>
> [...]
>
> > >[...]
> > >> Index: libavcodec/theora_enc.c
> > >> ===================================================================
> > >> --- libavcodec/theora_enc.c   (revision 0)
> > >> +++ libavcodec/theora_enc.c   (revision 0)
> > >[...]
> > >> +/**
> > >> + * @file theora_enc.c
> > >> + * Theora encoder using libtheora.
> > >> + *
> > >> + * A lot of this is copy / paste from other output codecs in
> > >> + * libavcodec or pure guesswork (or both).
> > >> + *
> > >> + * I have used t_ prefixes on variables which are libtheora types
> > >> + * and o_ prefixes on variables which are libogg types.
> > >> + *
> > >> + * @author Paul Richards <paul.richards at gmail.com>
> > >> + */
> > >> +
> > >> +/** FFmpeg includes */
> > >
> > >this is not the correct doxygen syntax to cover >1 element RTFM of doxygen
> > >
> >
> > So apart from changing /** to /*! and swapping @... with \..., I think
> > my doxygen was correct.  Or was there something else you took issue
> > with?
> >
> > >
> > >> +#include "avcodec.h"
> > >> +#include "log.h"
>
> what i meant was that this should be
>
> /** FFmpeg includes */
> //@{
> #include "avcodec.h"
> #include "log.h"
> //@}
>
> but then maybe the whole comment isnt that usefull i dunno, also iam not
> sure if doxygen supports such grouped comments around #include ...
>
> about /** vs /*! both are fine, though /** is more common currently in
> libav* so i would prefer that you keep that
>
>

Ahh I see..  It wasn't the file-level comment but the include comments
that were incorrect.  Those have changed to C-style now anyway, since
(as you point out) they aren't very interesting comments.

> [...]
>
> > >
> > >[...]
> > >> +    /** Now call into theora_encode_YUVin */
> > >> +    result = theora_encode_YUVin( &(h->t_state), &t_yuv_buffer );
> > >> +    switch (result) {
> > >> +        case 0:
> > >> +            /** Success */
> > >> +            break;
> > >> +        case -1:
> > >> +            av_log(avc_context, AV_LOG_ERROR, "theora_encode_YUVin
> > >failed (differing frame sizes)\n");
> > >> +            return -1;
> > >> +        case OC_EINVAL:
> > >> +            av_log(avc_context, AV_LOG_ERROR, "theora_encode_YUVin
> > >failed (encoder is not ready or is finished)\n");
> > >> +            return -1;
> > >> +        default:
> > >> +            av_log(avc_context, AV_LOG_ERROR, "theora_encode_YUVin
> > >failed [%d]\n", result);
> > >> +            return -1;
> > >> +    }
> > >
> > >id write
> > >
> > >if(result){
> > >    char *err= "?";
> > >    if     (result==-1       ) err= "differing frame sizes";
> > >    else if(result==OC_EINVAL) err= "encoder is not ready or is finished";
> > >    av_log(avc_context, AV_LOG_ERROR, "theora_encode_YUVin failed
> > >    [%d](%s)\n", result, err);
> > >    return -1;
> > >}
> > >
> > >
> >
> > I'm afraid I've stuck with the switch statement.  It's probably a
> > matter of taste but I think it more clearly shows my intent.  If you
> > feel strongly about this then let me know and I'll switch the code to
> > what you suggest.
>
> i dont care if its a switch or not, what i do care about is that your
> method is more complex (return -1 being duplicated on several branches and
> "theora_encode_YUVin failed" being duplicated as subpart of several strings
> if gcc can get rid of these then iam fine with them but i doubt that ...
> simply looking at the size of the generated .o file should give a pretty
> definite awnser which is simplest ...
>
> maybe you would prefer: ?
>
> if(result){
>     char *error_message;
>     switch(result){
>         case -1:
>             error_message= "differing frame sizes";
>             break;
>         case OC_EINVAL:
>             error_message= "encoder is not ready or is finished";
>             break;
>         deafult:
>             error_message= "?";
>     }
>     av_log(avc_context, AV_LOG_ERROR, "theora_encode_YUVin failed (%s)[%d]\n", error_message, result);
>     rerurn -1;
> }
>

Ok.  I'll factor out some of the code like you suggest and submit a
another patch.


-- 
Paul Richards