RetroSearch Browse

Sun Jan 7 18:50:32 CET 2007 · https://ffmpeg.org/pipermail/ffmpeg-devel/2007-January/027133.html

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>

Home - News ( United States | United Kingdom | Italy | Germany ) - Football scores

Showing content from https://ffmpeg.org/pipermail/ffmpeg-devel/2007-January/027133.html below:

[Ffmpeg-devel] FFmpeg Theora encoding patch