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

[Michael Niedermayer]

Hi

On Tue, May 01, 2007 at 09:04:15PM +0200, Denis Fortin wrote:
> Let's start with an easy patch :
> add some comments to msmpeg4data.h tables.
> 
> > > 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 ...
> > > I tought it was a good idea at first but now i'm not that sure because
> > > to have correct mc i had to mess mpegvideo.c:qpel_motion which is not
> > > vc1 specific.
> > > So i'm interested to know what is the endorsed way to add a mpeg-like
> > > encoder inside ffmpeg (and reuse some algorithms already coded).
> 
> You noticed that some code is duplicated in the previous patch, so what
> is the best way to avoid code duplication yet maintain readability. I'd
> like to avoid some
> #ifdef VC1ENCODER
> 	if(s->msmpeg)
> #endif
> all over the code.

i too like readable code and i think readable+fast+no duplication is possible
though i cant give you any generic tips here as i have none, each individual
case (MC, ratecontrol, ...) has to be investigated one by one on how to do
it best and cleanest ...


[...]
> > > 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
> > 
> > new files should always be diffed agianst their "parent" file if one exists
> the idea here is to "diff libavcodec/vc1.c libavcodec/vc1.h" ?
> How to apply such a patch ?

with patch but thats our problem not yours :)


[...]
> > > -        scantable= s->intra_scantable.permutated;
> > > +        scantable= s->intra_scantable.scantable;
> > 
> > why?
> I have a problem with permutated :
> when i started vc1 encoder permutated was working (no permutation:
> permutated==scantable), when i synced with newer svn permutated was
> wrong for me(permutated!=scantable). Where do i need to look to fix this
> issue.

is the code above used for msmpeg4 too? if so the change is wrong

if all the (dct->quantize->encode->dequant->idct) is your "private"
code then idct_permutation is wrong i guess, it should be the
"no permutation" id as none of the used code does any permutation

if the used idct implementation expects some permutation then you
have to provide coefficients in that permutation either by permuting
after encoding or after quantization and adapting the encoding
accordingly


[...]
> > [...]
> > 
> > > +    /* dc pred */
> > > +    if(s->msmpeg4_version>=6){
> > >      s->dc_val[0][xy           ] =
> > >      s->dc_val[0][xy + 1       ] =
> > >      s->dc_val[0][xy     + wrap] =
> > > +        s->dc_val[0][xy + 1 + wrap] = 0;
> > > +    } else {
> > > +        s->dc_val[0][xy           ] =
> > > +        s->dc_val[0][xy + 1       ] =
> > > +        s->dc_val[0][xy     + wrap] =
> > >      s->dc_val[0][xy + 1 + wrap] = 1024;
> > > +    }
> > 
> > code duplication
> you want this ?  
> s->msmpeg4_version>=6 ? 0 : 1024

yes

[...]
> > > +
> > > +    src  = block;
> > > +    dest = block;
> > > +    
> > > +    for(i=0 ; i < 8 ; i++){
> > > +        t1 = src[0]  + src[56];
> > > +        t2 = src[0]  - src[56];
> > > +        t3 = src[8]  + src[48];
> > > +        t4 = src[8]  - src[48];
> > > +        t5 = src[16] + src[40];
> > > +        t6 = src[16] - src[40];
> > > +        t7 = src[24] + src[32];
> > > +        t8 = src[24] - src[32];
> > > +        
> > > +        dest[0]  = 12 * (t1 + t3 + t5 + t7);
> > > +        dest[8]  = 16 * t2 + 15 * t4 + 9 * t6 + 4 * t8;
> > > +        dest[16] = 16 * (t1 - t7) + 6 * ( t3 - t5);
> > > +        dest[24] = 15 * t2 - 4 * t4 - 16 * t6 - 9 * t8;
> > > +        dest[32] = 12 * ( t1 - t3 - t5 + t7);
> > > +        dest[40] = 9 * t2 - 16 * t4 + 4 * t6 + 15 * t8;
> > > +        dest[48] = 6 * (t1 - t7) - 16 * (t3 - t5);
> > > +        dest[56] = 4 * t2 - 9 * t4 + 15 * t6 - 16 * t8;
> > 
> > i bet this can be simplified alot, i suggest you look at the exiting dct code
> > as the same factorizations are likely applicable here
> maybe later :-)

ok

comment patch ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20070502/8b779dbb/attachment.pgp>