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

[Denis Fortin]

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/vc1enc.c	(r??vision 0)
> > +++ libavcodec/vc1enc.c	(r??vision 0)
> > @@ -0,0 +1,791 @@
> > +#include "common.h"
> > +#include "vc1.h"
> > +#include "vc1enc.h"
> > +#include "vc1data.h"
> 
> Add the standard license header here.
Ok will do.

> > --- 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 
...

> > @@ -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 ?

Thanks for the quick review,
Denis