[Ffmpeg-devel] H.264 encoder
Panagiotis Issaris
takis.issaris
Wed Oct 4 17:51:25 CEST 2006
Hi,
Op woensdag 4 oktober 2006 13:11, schreef Michael Niedermayer:
> > > [...]
> > I would hope so :-) I can't know for sure, as I do not dare reading the x264
> > code as it is GPL licensed and we want our code to be LGPL licensed. I'd think
> > reading code of other H.264 encoders might cause me to use similar constructs,
> > which might not be liked by f.e. the x264 authors.
>
> not looking at others code will lead only to one thing, lower quality of your
> encoder
Indeed. But I do not think it would be fair to the x264 authors, to peek at their
code and use their long-sought-after constructs. Furthermore, I'd think this would
not be legal, as implementors of closed source H.264 encoders could do the same:
look at x264, and use the same tricks in their commercial closed source product.
> also for simplicity comparission simply looking at the object and source code
> sizes will do just fine, no need to read the code
True.
> > [...]
> > >> What specifically needs to be done now to get the patch integrated in
> > >> libavcodec?
> > >>
> > >
> > > alot, the code looks somewhat messy, more details are below,
> > > also note that if there where many similar issues i generally
> > > mentioned just one but for acceptance all need to be fixed
> > >
> > I hope it looks a bit less messy now.
>
> yes, but theres still some work left to do before it can be applied
> for example all the issues i mentioned which you "ignored", below
> is some quick and incomplete review, ill do a more complete one
> when you dealt with all issues either by fixing or explaining why
> you cant fix ...
Let me assure you that it was not my intention to ignore you at all. The issues
you highlighted which have not yet been addressed, have not been so for the sole
reason that I did not understand how to do it yet.
The reason I am sending this patch anyway, is that someone posted a question about
the patch a few days ago, and I figured it would be best to send the code again. Hoping I'd
get more -much appreciated- remarks, and kinda hoping for someone else to get interested
and starting helping out. I'm not attaching an updated patch yet, as I think you wouldn't
like it :) as I still could not address all issues you mentioned yet.
Someone tried helping out before, but it wasn't easy for him as there is just some
patches online. To make life easier for anyone interesting in getting a H.264 encoder
in FFmpeg, I've put my GIT repository online.
You can clone it with: git clone http://lumumba.uhasselt.be/~takis/git/ffmpeg-h264.git
There's also another repository online, which is just tracking the main FFmpeg SVN tree:
git clone http://lumumba.uhasselt.be/~takis/git/ffmpeg.git
I've also put a webinterface (gitweb) online at:
http://issaris.org/gitweb/gitweb.cgi
This one's really slow, as it is hosted on my home P166 with 32meg RAM.
I think I saw someone expressing his annoyance about SVN on this mailinglist before (I think
it was you Michael?). I would really recommend having a look at GIT as I find it a _very_
handy tool.
> > > [...]
> > >
> > >> + int x,y;
> > >> +
> > >> + for (y = 0 ; y < 4 ; y++)
> > >> + for (x = 0 ; x < 4 ; x++)
> > >> + mb->Y_nonzero[y][x] = 0;
> > >>
> > >
> > > memset() ?
Fixed.
> > >
> > >
> > >
> > >> + }
> > >> +
> > >> + if (CodedBlockPatternChroma == 0)
> > >> + {
> > >> + int x,y;
> > >> +
> > >> + for (y = 0 ; y < 2 ; y++)
> > >> + {
> > >> + for (x = 0 ; x < 2 ; x++)
> > >> + {
> > >> + mb->U_nonzero[y][x] = 0;
> > >> + mb->V_nonzero[y][x] = 0;
> > >>
> > >
> > > again memset() should be useable here, also look at fill_rectangle() from
> > > h264.c
Fixed.
> [...]
> > + if (val > 0)
> > + val--;
> > + else // < 0
> > + val++;
>
> val -= (val>>31)|1;
Fixed.
>
> [...]
> > +void ff_h264_hadamard_quant_4x4_c(DCTELEM Y[4][4], int QP)
> > +{
> > + int qbits = 15 + div6[QP];
> > + int f2 = ((1 << qbits) / 3)*2;
>
> always replace division by constants by multiplications by the reciprocal
> >>something unless the code is irrelevant speedwise (init code)
Fixed.
>
> [...]
> > + buf = (uint8_t *)av_malloc(s);
>
> useless cast
Fixed.
>
> [...]
> > + for (y = 0 ; y < 2 ; y++)
> > + {
> > + for (x = 0 ; x < 2 ; x++)
> > + {
> > + copy_mb->U_nonzero[y][x] = 16;
> > + copy_mb->V_nonzero[y][x] = 16;
> > + }
> > + }
>
> memset()
Is that possible? U_nonzero is of type int. Hmm. I think the picture_fill function
might be helpful for this.
> [...]
> > +#define H264_JUST_CLIP(x) \
> > +{\
> > + int16_t val = x;\
> > + if (val > 2047)\
> > + x = 2047; \
> > + else if (val < -2047) \
> > + x = -2047;\
>
> clip()
Fixed.
> [...]
> > + static const int8_t mbtype_map[4][3][2] =
> > + {
> > + {
> > + { 1, 13 }, // 0 0 0, 0 0 1
> > + { 5, 17 }, // 0 1 0, 0 1 1
> > + { 9, 21 } // 0 2 0, 0 2 1
> > + },
> > + {
> > + { 2, 14 }, // 1 0 0, 1 0 1
> > + { 6, 18 }, // 1 1 0, 1 1 1
> > + { 10, 22 } // 1 2 0, 1 2 1
> > + },
> > + {
> > + { 3, 15 }, // 2 0 0, 2 0 1
> > + { 7, 19 }, // 2 1 0, 2 1 1
> > + { 11, 23 } // 2 2 0, 2 2 1
> > + },
> > + {
> > + { 4, 16 }, // 3 0 0, 3 0 1
> > + { 8, 20 }, // 3 1 0, 3 1 1
> > + { 12, 24 } // 3 2 0, 3 2 1
> > + }
> > + };
>
> mbtype_map[a][b][c] -> 1 + a + 4*(b + 3*c)
Fixed.
>
> [...]
> > + int x = (i%2)+X;
> > + int y = (i/2)+Y;
>
> &1 and >>1
Fixed.
>
> > +
> > + int k;
> > +
> > + for (k = 0 ; k < 15 ; k++)
> > + coefficients[k] = residual->part4x4Y[y][x][zigzagy[k+1]][zigzagx[k+1]];
> > + ff_h264_neighbour_count_nonzero(mb,NEIGHBOUR_SUBTYPE_Y,x,y,&nA,&nB);
> > + mb->Y_nonzero[y][x] = h264cavlc_encode(b,coefficients,15,nA,nB,0);
> > + }
> > + }
> > + }
> > + else
> > + memset(mb->Y_nonzero, 0, sizeof(mb->Y_nonzero));
> > +
> > + if (CodedBlockPatternChroma == 0)
> > + {
> > + memset(mb->U_nonzero, 0, sizeof(mb->U_nonzero));
> > + memset(mb->V_nonzero, 0, sizeof(mb->V_nonzero));
> > + return;
> > + }
> > +
> > + if (CodedBlockPatternChroma != 0)
> > + {
> > + H264_ENCODE_INTRA16X16_RESIDUAL_COEFFICIENTS(UD);
> > + H264_ENCODE_INTRA16X16_RESIDUAL_COEFFICIENTS(VD);
> > + }
> > +
> > + if (CodedBlockPatternChroma == 2)
> > + {
> > + for (i = 0 ; i < 4 ; i++)
> > + {
> > + int x = (i%2);
> > + int y = (i/2);
>
> &1 and >>1
Fixed.
>
> > +
> > + int k;
> > +
> > + for (k = 0 ; k < 15 ; k++)
> > + coefficients[k] = residual->part4x4U[y][x][zigzagy[k+1]][zigzagx[k+1]];
> > + ff_h264_neighbour_count_nonzero(mb,NEIGHBOUR_SUBTYPE_U,x,y,&nA,&nB);
> > + mb->U_nonzero[y][x] = h264cavlc_encode(b,coefficients,15,nA,nB,0);
> > + }
> > +
> > + for (i = 0 ; i < 4 ; i++)
> > + {
> > + int x = (i%2);
> > + int y = (i/2);
>
> &1 and >>1
Fixed.
>
> [...]
> > + for (y = 0 ; y < 2 ; y++)
> > + {
> > + for (x = 0 ; x < 2 ; x++)
> > + {
> > + mb->U_nonzero[y][x] = 0;
> > + mb->V_nonzero[y][x] = 0;
> > + }
> > + }
>
> memset()
Fixed.
> [...]
> > + 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
>
>
> [...]
> > +#ifdef CONFIG_ENCODERS
> > +AVCodec h264_encoder = {
> > + "ffh264",
> > + CODEC_TYPE_VIDEO,
> > + CODEC_ID_FFH264,
> > + sizeof(H264Context),
> > + ff_h264_encoder_init,
> > + ff_h264_encode,
> > + ff_h264_encoder_close,
>
> list of supported pix_fmts missing
Fixed.
Thanks for the quick review.
With friendly regards,
Takis
More information about the ffmpeg-devel
mailing list