[Ffmpeg-devel] FFmpeg Theora encoding patch

[Paul Richards]

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':
  Duration: 00:00:07.4, start: 0.000000, bitrate: 190 kb/s
  Stream #0.0, 1/15: Video: theora, yuv420p, 320x240, 1/15, 15.00 fps(r)
Output #0, avi, to 'test3.avi':
  Stream #0.0, 1/90000: Video: mpeg4, yuv420p, 320x240, 1/15, q=2-31,
200 kb/s, 15.00 fps(c)
Stream mapping:
  Stream #0.0 -> #0.0
[theora @ 0x458594]Theora bitstream version 30200
[theora @ 0x458594]344 bits left in packet 81
[theora @ 0x458594]7 bits left in packet 82
Press [q] to stop encoding
1471320 dezicycles in init_frame, 1 runs, 0 skips
1075320 dezicycles in unpack_superblocks, 1 runs, 0 skips
152640 dezicycles in unpack_modes, 1 runs, 0 skips
4440 dezicycles in unpack_vectors, 1 runs, 0 skips
...

>
> [...]
> > 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"
>
> [...]
> > +/** TheoraContext */
> > +typedef struct TheoraContext{
>
> this comment is giga useless if thats the only thing to say about it then it
> would be better not to say it ...
>

True.  Comment has been axed.

>
> > +    theora_state t_state;
> > +} TheoraContext;
> > +
> > +/** Forward declares for the internal functions below */
> > +static int encode_init(AVCodecContext* avctx);
> > +static int encode_close(AVCodecContext* avctx);
> > +static int encode_frame(AVCodecContext* avctx, uint8_t *buf, int buf_size, void *data);
>
> always try to order code so as to avoid forward declarations ...
>
>

Done.

> [...]
>
> > +/** Internal stuff below */
> > +#define NOT_IMPLEMENTED_YET 0;
>
> ?

I was using "assert(NOT_IMPLEMENTED_YET)" so I'd get "readable"
run-time assertions.  Forgot to remove the define.

>
>
> > +
> > +/**
> > +    Checks that an AVCodexContext has a pixel format we understand.
> > +*/
> > +static int check_constraints(AVCodecContext* avc_context)
> > +{
> > +    /**
> > +        Currently only support the following input formats:
> > +
> > +        PIX_FMT_YUV420P:
> > +        Planar YUV 4:2:0, 12bpp,
> > +        (1 Cr & Cb sample per 2x2 Y samples).
> > +    */
> > +    if (avc_context->pix_fmt != PIX_FMT_YUV420P) {
> > +        av_log(avc_context, AV_LOG_ERROR, "have only implemented PIX_FMT_YUV420P input");
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
>
> theres a much simpler way to achive this, and that is to set AVCodec.pix_fmts
>

Excellent, done. :)

>
> [...]
> > +    t_info.keyframe_frequency = 64;
> > +    t_info.keyframe_frequency_force = 64;
>
> AVCodecContext.gop_size
>
>
> > +    t_info.keyframe_data_target_bitrate = t_info.target_bitrate * 1.5;
> > +    t_info.keyframe_auto_threshold = 80;
> > +    t_info.keyframe_mindistance = 8;
>
> AVCodecContext.keyint_min
>

Also done.

>
> [...]
> > +static int encode_frame(
> > +    AVCodecContext* avc_context,
> > +    uint8_t *outbuf,
> > +    int buf_size,
> > +    void *data)
> > +{
> > +    yuv_buffer t_yuv_buffer;
> > +    TheoraContext *h = avc_context->priv_data;
> > +    AVFrame *frame = data;
> > +    ogg_packet o_packet;
> > +    int result;
> > +
> > +    /** Check pixel format contraints */
> > +    if (check_constraints(avc_context) != 0) {
> > +        return -1;
> > +    }
> > +
> > +    /** Copy planes to the theora yuv_buffer */
>
> doxygen doesnt support such comments in the middle of the code i _think_
> at least not last time i checked ...
>

Ok, have swapped these for normal C-style 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.


-- 
Paul Richards