[Ffmpeg-devel] FFmpeg Theora encoding patch

[Michael Niedermayer]

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?


[...]
> 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


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


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


[...]

> +/** Internal stuff below */
> +#define NOT_IMPLEMENTED_YET 0;

?


> +
> +/**
> +    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


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


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


[...]
> +    /** 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;
}



[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070106/627b5ef9/attachment.pgp>