[Ffmpeg-devel] FFmpeg Theora encoding patch
Michael Niedermayer
michaelni
Sun Jan 7 18:50:32 CET 2007
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
[...]
> >[...]
> >> 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
[...]
> >
> >[...]
> >> + /** 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;
}
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/20070107/7f2a2ecd/attachment.pgp>
More information about the ffmpeg-devel
mailing list