theora dev - Jan 2007 - FFmpeg Theora encoding patch

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.



-- 
Paul Richards
-------------- next part --------------
A non-text attachment was scrubbed...
Name: theora.patch
Type: text/x-patch
Size: 14485 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/theora-dev/attachments/20070106/8cf9f672/theora.bin

PS. Has been tested on Mac OS X (gcc 4.0.1) and Linux AMD64 (gcc
4.1.2), both compile without any warnings.


On 06/01/07, Paul Richards <paul.richards@gmail.com>
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.
>
>
>
> --
> Paul Richards
>
>
>

-- 
Paul Richards

On 06/01/07, M?ns Rullg?rd <mru@inprovide.com>
wrote:
> "Paul Richards" <paul.richards@gmail.com> writes:
>
> > 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.
> >
> > Index: libavcodec/vp3.c
> > ==================================================================>
> --- libavcodec/vp3.c  (revision 7409)
> > +++ libavcodec/vp3.c  (working copy)
> > @@ -2643,7 +2643,6 @@
> >      NULL
> >  };
> >
> > -#ifndef CONFIG_LIBTHEORA
> >  AVCodec theora_decoder = {
> >      "theora",
> >      CODEC_TYPE_VIDEO,
> > @@ -2656,4 +2655,3 @@
> >      0,
> >      NULL
> >  };
> > -#endif
>
> Why?
The author of the vp3 decoder assumed that when libtheora was hooked
up it would take over the job of theora decoding.  My patch only uses
libtheora for encoding, and so the vp3 theora decoder is still needed
despite CONFIG_LIBTHEORA now being defined.
>
> > +OBJS-$(CONFIG_THEORA_ENCODER)          += theora_enc.o
>
> Personally I'd prefer calling the file libtheoraenc.c or something
> similar.  The name theoraenc.c should be reserved for a possible
> future native implementation.
>
Ok, I've renamed the source file locally.  When I roll up the other
changes suggested on this thread I'll email my (hopefully final)
patch.


-- 
Paul Richards

On 06/01/07, William Volkman <wkvtheora@netshark.com>
wrote:
> Hello Paul,
> On Sat, 2007-01-06 at 15:51 +0000, Paul Richards wrote:
> > 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.
>
> What bitrate are you using?  I observe that files have trouble
> in playback both in vlc and the example player when the bitrate
> value is too high.
>
I have tested both PAL resolution at around 500kbit and also a tiny
320x240 video at a much lower bitrate.  Both have the VLC problem.



-- 
Paul Richards

Paul,
> The author of the vp3 decoder assumed that when libtheora was hooked
> up it would take over the job of theora decoding.  My patch only uses
> libtheora for encoding, and so the vp3 theora decoder is still needed
> despite CONFIG_LIBTHEORA now being defined.
How much harder would it be to include calls to the Theora decoder as
well?  That would ensure that you are using a decoder that is compatible
with the encoder.

Ed Okerson

On Sat, Jan 06, 2007 at 03:51:28PM +0000, Paul Richards
wrote:
> Hi,
> ==================================================================>
+static int encode_init(AVCodecContext* avc_context)
> +{
> +    theora_info t_info;
> +    theora_comment t_comment;
> +    ogg_packet o_packet;
> +    unsigned int offset;
> +    TheoraContext *h = avc_context->priv_data;
> +
> +    /** Check pixel format contraints */
> +    if (check_constraints(avc_context) != 0) {
> +        return -1;
> +    }
> +    
> +    /** Set up the theora_info struct */
> +    theora_info_init( &t_info );
> +    t_info.width = avc_context->width;
> +    t_info.height = avc_context->height;
> +    t_info.frame_width = avc_context->width;
> +    t_info.frame_height = avc_context->height;
> +    t_info.offset_x = 0;
> +    t_info.offset_y = 0;
> +    t_info.fps_numerator = avc_context->time_base.den;
> +    t_info.fps_denominator = avc_context->time_base.num;
This is a naive and probably dumb question ... but why do you flip the
numerator and denominator here?
> +    if (avc_context->sample_aspect_ratio.num != 0) {
> +        t_info.aspect_numerator = avc_context->sample_aspect_ratio.num;
> +        t_info.aspect_denominator =
avc_context->sample_aspect_ratio.den;
> +    } else {
> +        t_info.aspect_numerator = 1;
> +        t_info.aspect_denominator = 1;

-Eric Rz.

In AVCodecContext the time_base gives the time period between frames.
In theora_info they want the frame rate..  These are the inverse of
each other.  :)

I perhaps should have put a little comment there.


On 01/02/07, Eric Dantan Rzewnicki <eric@zhevny.com>
wrote:
> On Sat, Jan 06, 2007 at 03:51:28PM +0000, Paul Richards wrote:
> > Hi,
> > ==================================================================>
> +static int encode_init(AVCodecContext* avc_context)
> > +{
> > +    theora_info t_info;
> > +    theora_comment t_comment;
> > +    ogg_packet o_packet;
> > +    unsigned int offset;
> > +    TheoraContext *h = avc_context->priv_data;
> > +
> > +    /** Check pixel format contraints */
> > +    if (check_constraints(avc_context) != 0) {
> > +        return -1;
> > +    }
> > +
> > +    /** Set up the theora_info struct */
> > +    theora_info_init( &t_info );
> > +    t_info.width = avc_context->width;
> > +    t_info.height = avc_context->height;
> > +    t_info.frame_width = avc_context->width;
> > +    t_info.frame_height = avc_context->height;
> > +    t_info.offset_x = 0;
> > +    t_info.offset_y = 0;
> > +    t_info.fps_numerator = avc_context->time_base.den;
> > +    t_info.fps_denominator = avc_context->time_base.num;
>
> This is a naive and probably dumb question ... but why do you flip the
> numerator and denominator here?
>
> > +    if (avc_context->sample_aspect_ratio.num != 0) {
> > +        t_info.aspect_numerator =
avc_context->sample_aspect_ratio.num;
> > +        t_info.aspect_denominator =
avc_context->sample_aspect_ratio.den;
> > +    } else {
> > +        t_info.aspect_numerator = 1;
> > +        t_info.aspect_denominator = 1;
>
>
> -Eric Rz.
>

-- 
Paul Richards

On Thu, Feb 01, 2007 at 09:09:46AM +0000, Paul Richards
wrote:
> In AVCodecContext the time_base gives the time period between frames.
> In theora_info they want the frame rate..  These are the inverse of
> each other.  :)
> 
> I perhaps should have put a little comment there.
That makes sense. Sorry for the silly question.

-Eric Rz.