[FFmpeg-devel] [Ffmpeg-devel][PATCH] WMV 3 encoding support (intra main profile)

[Diego Biurrun]

On Tue, May 01, 2007 at 05:55:44PM +0200, Denis Fortin wrote:
> On mar, 2007-05-01 at 17:44 +0200, Diego Biurrun wrote:
> > On Tue, May 01, 2007 at 05:16:16PM +0200, Denis Fortin wrote:
> > > 
> > > I am working on WMV3/VC-1 encoding support.
> > > I was holding out a patch until i get at least P frames, but i have been
> > > out of sync from svn for too long and i still have some bugs in mc ; so
> > > i guess it's time to get some feedback about intra frame main profile
> > > VC1 support.
> > > I tried to reuse as much code as possible from ffmpeg, ratecontrol,
> > > motion estimation ...
> > > 
> > > Summary : 
> > > -modify some msmpeg4.c functions to handle vc1
> > > -rl.h: switch table_vlc from uint16_t to uint32_t for new vlc tables
> > > -vc1.h: move some definitions from vc1.c to this new filw
> > > -vc1enc.c: new functions (this file is included in msmpeg4.c, like
> > > wmv2.c is)
> > 
> > Is that necessary?  I find the inclusion of C files ugly.
> > 
> > > Comments? 
> > 
> > Quick comments below to spare Michael some reviewing trouble:
> > You have tabs and trailing whitespace in your patch, this needs to go.
> Ok i'll clean up.
> 
> > > --- libavcodec/msmpeg4data.h	(r??vision 8855)
> > > +++ libavcodec/msmpeg4data.h	(copie de travail)
> > > @@ -586,11 +586,182 @@
> > >   29, 30, 31, 32, 33, 34, 35, 36,
> > >  };
> > >  
> > > -extern const uint16_t inter_vlc[103][2];
> > > +/*shamefull copy paste from vc1acdata.h*/
> > > +static const uint32_t vc1_high_rate_intra_vlc[163][2] = {
> > 
> > Michael will refuse this if it's duplicated.
> One can argue that there's already table duplication between vc1acdata.h and msmpeg4data.h
> vc1_ac_tables[0] == table0_vlc
> vc1_ac_tables[1] == table4_vlc 
> ...

Time to refactor that part of the code then I would say :)

If this means that you can split your patch into multiple small parts -
all the better.

> > > @@ -606,10 +777,11 @@
> > >  
> > >  static RLTable rl_table[NB_RL_TABLES] = {
> > >      /* intra luminance tables */
> > > +    /* low motion  */
> > >      {
> > >          132,
> > >          85,
> > 
> > All of these could go in a separate patch, same for the others.
> I need to remove these comments ?

No, send them in as a separate patch, it's likely to get committed right
away.

> Thanks for the quick review,

You are welcome.

Diego