On 07/01/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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
>
So would my patch with broken VLC playback be acceptable? I'd be
happy to later fix the implementation if theora in avi ever does get
officially specified.
>
> [...]
>
> > >[...]
> > >> 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
>
>
Ahh I see.. It wasn't the file-level comment but the include comments
that were incorrect. Those have changed to C-style now anyway, since
(as you point out) they aren't very interesting 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.
>
> 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;
> }
>
Ok. I'll factor out some of the code like you suggest and submit a
another patch.
--
Paul Richards
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4