Re: [PATCH] Add Dirac support to ffmpeg

Michael,
Thanks for the prompt reply. We will resubmit the patch after applying
your comments.

Regards,
Anuradha Suraparaju
On Sun, 2005-07-10 at 00:46 +0200, Michael Niedermayer wrote:
> Hi
> 
> On Thursday 07 July 2005 14:38, Anuradha Suraparaju wrote:
> > **Dirac patch for ffmpeg
> > **Tested with ffmpeg cvs (as of July 7, 2007 10:00am) and
> >   FFMpeg-20050707
> > **Author(s): Andrew Kennedy (dirac_dev <at> users.sourceforge.net) (main)
> >              Anuradha Suraparaju (asuraparaju <at> users.sourceforge.net)
> >              (minor mods)
> >
> > Dirac is an open-source video codec currently under development. Both
> > the encoder and decoder have been integrated with ffmpeg. This patch is
> > only a basic integration such that only the salient paremeters are
> > currently passed from ffmpeg to Dirac. As Dirac develops, especially
> > with the integration of  a formalised bit-stream syntax,  then more
> > complex screnarios can be dealt with. Encoded Dirac files, with the
> > right patch, can be played back using MPlayer.
> [...]
> 
> > +dirac_encoder_presets_t GetDiracPreset(AVCodecContext *avccontext)
> 
> shouldnt that and a few other functions be static to avoid namespace 
> pollution, or are they used outside dirac.c?
> 
> 
> > +       default:
> > +               av_log (avccontext, AV_LOG_ERROR, "this codec supports only 
> YUV420P\n");
> > +               return format420;
> > +       
> > +       }
> > +       
> > +       return format420;
> 
> the last return is unreachable, IMHO it should be removed
> 
> 
> it seems GetFrameBufferSize() could be replaced by avpicture_get_size()
> 
> 
> > +static unsigned char* dirac_videobuffer = 0;
> > +static int dirac_videobuf_len =0;
> > +static unsigned char dirac_videobuf_code[5];
> > +static int dirac_videobuf_code_len = 0;
> 
> non constant static/globals arent acceptable, it should be possible to use 
> several demuxers or decoders at once
> 
> 
> > +         }
> > +
> > +           dirac_videobuffer[dirac_videobuf_len++] = byte;
> 
> i dont care what indention style is used as long as its not the randomized one
> 
> 
> > +    av_free(dirac_videobuffer);
> > +    dirac_videobuffer=0;
> 
> this could be replaced by av_freep()
> 
> 
> and if i understand the demuxing code in drc.c correctly, then it does only 
> split the stream into packets, this should be done in a AVParser instead for 
> raw "containers", and the remaining (de)muxing code might fit better in 
> libavformat/raw.c instead of a separate file, but a separate file is fine too 
> if preferred
> 
--

--