[Ffmpeg-devel] [PATCH] lowres chroma bug

[Oleg Metelitsa]

Hello Michael,

>> Please find attached a patch, that solves an
>> "extreme chroma bug" problem in lowres=2 mode
>> reported by Reimar Doffinger 6 months ago:
>>
>> Is the proposed patch OK ?

MN> no because it slows the non lowres code down

Sorry, I did not mean old Reimar's patch which might slow down the non
lowres code. I am trying to propose the new patch that solves the same
problem.  I  think it must not affect the speed of FFMPEG code because
it really adds only one extra register-to register move instruction.

Could  you  consider  my  patch again? If it is not OK, I have another
solution to the "lowres chroma bug" problem.

Please find a patch below and a short explanation why the "lowres
chroma bug" appears.

Index: libavcodec/i386/dsputil_h264_template_mmx.c
===================================================================
--- dsputil_h264_template_mmx.c (revision 7818)
+++ dsputil_h264_template_mmx.c (working copy)
@@ -317,8 +317,9 @@
             H264_CHROMA_OP4(%0, %%mm1, %%mm3)
-            "movd %%mm1, %0\n\t"
-            : "=m" (dst[0]) : "m" (ff_pw_32));
+            "movd %%mm1, %%edx\n\t"
+            "mov %%dx, %0\n\t"
+            : "=m" (dst[0]) : "m" (ff_pw_32): "%edx");
         dst += stride;
     }
===================================================================

In case of mpeg4 decoding, the "H264_CHROMA_MC2_TMPL" macros is
responsible for handling 2x2 blocks in lowres=2 mode.

The "H264_CHROMA_MC2_TMPL" macros in the original FFMPEG code has the
following problem. It must write a 16-bit word into memory, but it
uses 32-bit movd command for this task.

As  a  result, a 16-bit word is written into the memory + a "garbage"
16-bit  word.  The rows in the image are processed from left to right.
It  was  expected  that  the  garbage  will  be rewritten in the next
writing circle.

Unfortunately,  it  does  not  always  work  as  intended!  The  codec
processes  not  one  but  two  rows  simultaneously. When we write the
endings  of  two  consecutive  rows  (say,  rows  (x)  and (x+1)), the
garbage,  which  is  written  after the very end of the first row (row
(x)),  goes into the very beginning of the second row (row (x+1)). But
the  beginning  of  row  (x+1)  has  been already processed and we are
actually writing garbage on the top of valid image data !

As  a  result, we have garbage at the beginning of every second row of
an image.

There are 3 possible ways to solve the problem:

1)  See the new proposed patch above. It works in the following way: I
changed  a  32-bit movd instruction to a 16-bit mov. As a result there
will be no garbage  written in the picture any more.

2)  We  can  add  to our 16-bit another 16 bit that were can read from
picture and than write 16+16=32 bit. I tried this solution too, but it
was slower than solution 1)

3)  We can add two extra pixels to image in lowres=2 mode:
image_stride =  image_width  +  2
chroma_stride = image_width/2 + 2
Then  the garbage will be written to those extra (dummy) pixels rather
than to the beginning of the next row.

The  last  solution is also OK, and I can send the patch containing 3)
if you like.


Best regards,

Oleg.