[Ffmpeg-devel] H.264 encoder

[Panagiotis Issaris]

Hi,

On wo, 2006-10-04 at 13:11 +0200, Michael Niedermayer wrote:
> > > [...]
> > >
> > >
> > >>>> code duplication, ive not checked too carefully but loop filter & motion
> > >>>> compensation at least appear to be duplicated from the h264 decoder
> > >>>>
> > >>>
> > >>>
> > >> We didn't find a way to easily reuse the loop filter from the decoder, which
> > >> is why we wrote the routine ourselves. The motion compensation code in
> > >>
> > >
> > > hmm, where is the problem?
> 
> ping ...
> 
> anyway if theres significant code duplication then the patch wont be accepted
The h264.c filtering code seems to be using MpegEncContext. I'm
postponing this one, until after I complete the "easier" issues.

I haven't quoted all your remarks, as I'm trying to tackle them one by
one. No ignoring intended.

> > > [...]
> > >
> > >> +
> > >> +// inblock is transposed, outblock isn't
> > >> +void ff_h264_dct_c(DCTELEM inblock[4][4],DCTELEM outblock[4][4])
> > >> +{
> > >> +    DCTELEM pieces[4][4];
> > >> +
> > >> +    pieces[0][0] = inblock[0][0]+inblock[1][0]+inblock[2][0]+inblock[3][0];
> > >> +    pieces[0][1] = inblock[0][1]+inblock[1][1]+inblock[2][1]+inblock[3][1];
> > >> +    pieces[0][2] = inblock[0][2]+inblock[1][2]+inblock[2][2]+inblock[3][2];
> > >> +    pieces[0][3] = inblock[0][3]+inblock[1][3]+inblock[2][3]+inblock[3][3];
> > >> +
> > >> +    pieces[1][0] = (inblock[0][0]<<1)+inblock[1][0]-inblock[2][0]-(inblock[3][0]<<1);
> > >> +    pieces[1][1] = (inblock[0][1]<<1)+inblock[1][1]-inblock[2][1]-(inblock[3][1]<<1);
> > >> +    pieces[1][2] = (inblock[0][2]<<1)+inblock[1][2]-inblock[2][2]-(inblock[3][2]<<1);
> > >> +    pieces[1][3] = (inblock[0][3]<<1)+inblock[1][3]-inblock[2][3]-(inblock[3][3]<<1);
> > >> +
> > >> +    pieces[2][0] = inblock[0][0]-inblock[1][0]-inblock[2][0]+inblock[3][0];
> > >> +    pieces[2][1] = inblock[0][1]-inblock[1][1]-inblock[2][1]+inblock[3][1];
> > >> +    pieces[2][2] = inblock[0][2]-inblock[1][2]-inblock[2][2]+inblock[3][2];
> > >> +    pieces[2][3] = inblock[0][3]-inblock[1][3]-inblock[2][3]+inblock[3][3];
> > >> +
> > >> +    pieces[3][0] = inblock[0][0]-(inblock[1][0]<<1)+(inblock[2][0]<<1)-inblock[3][0];
> > >> +    pieces[3][1] = inblock[0][1]-(inblock[1][1]<<1)+(inblock[2][1]<<1)-inblock[3][1];
> > >> +    pieces[3][2] = inblock[0][2]-(inblock[1][2]<<1)+(inblock[2][2]<<1)-inblock[3][2];
> > >> +    pieces[3][3] = inblock[0][3]-(inblock[1][3]<<1)+(inblock[2][3]<<1)-inblock[3][3];
> > >> +
> > >> +    outblock[0][0] = pieces[0][0]+pieces[0][1]+pieces[0][2]+pieces[0][3];
> > >> +    outblock[0][1] = pieces[1][0]+pieces[1][1]+pieces[1][2]+pieces[1][3];
> > >> +    outblock[0][2] = pieces[2][0]+pieces[2][1]+pieces[2][2]+pieces[2][3];
> > >> +    outblock[0][3] = pieces[3][0]+pieces[3][1]+pieces[3][2]+pieces[3][3];
> > >> +
> > >> +    outblock[1][0] = (pieces[0][0] << 1)+pieces[0][1]-pieces[0][2]-(pieces[0][3]<<1);
> > >> +    outblock[1][1] = (pieces[1][0] << 1)+pieces[1][1]-pieces[1][2]-(pieces[1][3]<<1);
> > >> +    outblock[1][2] = (pieces[2][0] << 1)+pieces[2][1]-pieces[2][2]-(pieces[2][3]<<1);
> > >> +    outblock[1][3] = (pieces[3][0] << 1)+pieces[3][1]-pieces[3][2]-(pieces[3][3]<<1);
> > >> +
> > >> +    outblock[2][0] = pieces[0][0]-pieces[0][1]-pieces[0][2]+pieces[0][3];
> > >> +    outblock[2][1] = pieces[1][0]-pieces[1][1]-pieces[1][2]+pieces[1][3];
> > >> +    outblock[2][2] = pieces[2][0]-pieces[2][1]-pieces[2][2]+pieces[2][3];
> > >> +    outblock[2][3] = pieces[3][0]-pieces[3][1]-pieces[3][2]+pieces[3][3];
> > >> +
> > >> +    outblock[3][0] = pieces[0][0]-(pieces[0][1]<<1)+(pieces[0][2]<<1)-pieces[0][3];
> > >> +    outblock[3][1] = pieces[1][0]-(pieces[1][1]<<1)+(pieces[1][2]<<1)-pieces[1][3];
> > >> +    outblock[3][2] = pieces[2][0]-(pieces[2][1]<<1)+(pieces[2][2]<<1)-pieces[2][3];
> > >> +    outblock[3][3] = pieces[3][0]-(pieces[3][1]<<1)+(pieces[3][2]<<1)-pieces[3][3];
> > >> +}
> > >>
> > >
> > > h264.c h264_diff_dct_c() looks so much nicer, is this faster?
The h264_diff_dct_c() function had been #if 0'ed out. Has it ever been
tested and confirmed working correctly?

I'm asking because I'm getting image corruption with this function
integrated, and knowing if this function is correct can help me find the
problem.

> [...]
> > +        if (topavail && w == 16 && h == 16 && srcleft->topblock != 0 && srcleft->topblock->available)
> > +        {
> > +            // Plane prediction
> 
> use h264.c:pred*x*_plane_c and the other ones from h264.c
Fixed.


With friendly regards,
Takis