[Ffmpeg-devel] H.264 encoder

[Michael Niedermayer]

Hi

On Tue, Jun 06, 2006 at 10:26:54AM +0200, Panagiotis Issaris wrote:
> Hi,
> 
> A while ago we sent a message to this mailinglist, announcing the fact
> that we were working on a H.264 encoder for libavcodec. We have released
> a new patch, which is available at the following location:
> http://research.edm.uhasselt.be/h264/
> 
> Our main focus is on the baseline profile and the current features of
> the encoder are the following:
>  - one slice per frame is used
>  - I frames using intra 16x16 encoding
>  - P frames using 16x16 macroblocks (no subblocks)
>  - a basic motion estimation algorithm
>  - a basic rate control algorithm
>  - motion vector search is full-pixel (a qpel implementation is
> available but is not enabled by default)
>  - the previous frame is used as reference frame
>  - deblocking filter is enabled
> 
> We felt that now was a good time to ask for some additional feedback. 

it contains a few pretty large tables which could maybe be generated at
runtime ...

some tables are int while all values would fit in char

some tables (ex: h264cavlc_encode_vlc_totalzeros_chromadc) have more
entries then ever used

some tables based code could be replaced with tables-less (of coarse only
if its not slower)
for example:
+       static const int length_table[15][16] = 
+       {
+               { 1,2,3,3 },
+               { 1,2,2 },
+               { 1,1 }
+       };
+       
+       static const int code_table[15][16] = 
+       {
+               { 1,1,1,0 },
+               { 1,1,0 },
+               { 1,0 }
+       };
+       
+       put_bits(b,length_table[vlcnum][value],code_table[vlcnum][value]);

with
if(vlcnum + value == 3) put_bits(b, value  , 0);
else                    put_bits(b, value+1, 1);


some non table code would be much simpler with a table
for example:
+       switch(nc)
+       {
+       case 0:
+       case 1:
+               lookup_table = 0;
+               break;
+       case 2:
+       case 3:
+               lookup_table = 1;
+               break;
+       case 4:
+       case 5:
+       case 6:
+       case 7:
+               lookup_table = 2;
+               break;
+       default:
+               lookup_table = 3;
+       }


then theres some do nothing code like:
+    case PIX_FMT_RGBA32:
+
+    case PIX_FMT_YUV444P:
+    case PIX_FMT_YUV422P:
+    case PIX_FMT_GRAY8:
+    case PIX_FMT_YUV411P:
+    case PIX_FMT_YUV410P:
+    default:


theres mmx code in libavcodec instead of libavcodec/i386

code duplication, ive not checked too carefully but loop filter & motion
compensation at least appear to be duplicated from the h264 decoder



> The patch is not intended for direct adoption into the main libavcodec
> tree, but we'd be happy to make the required changes if the encoder is
> considered adequate.

for a encoder to be accepted in lavc it must at least:
* be clean & simple = no code where its immedeatly obvious how to do the
  same thing in half the number of lines
* no redundant code
* processor specific code must be in the respective directories

for this specific case i think further requireing that the encoder
must be at least one of the following 3 seems logical
* be simpler then x264
* produce better quality/bitrate then x264 at some fixed "speed"
* produce better quality/bitrate then x264 at the slowest speed of each encoder

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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is