[FFmpeg-devel] mpeg2dec: fix decoding field pictures

Submitted by Nekopanda on Feb. 8, 2018, 5:03 p.m.

Details

Message ID 1518109404-29932-1-git-send-email-pianoyayaninth@yahoo.co.jp
State New
Headers show

Commit Message

Nekopanda Feb. 8, 2018, 5:03 p.m.
- Fix field selection for skipped macroblocks

For B field pictures, the spec says,

> The prediction shall be made from the field of the same parity as the field being predicted.

I did it.

- Fix motion vector rounding for chroma components

In 16x8 motion compensation, for lower 16x8 region, the input to mpeg_motion() for motion_y was "motion_y + 16", which causes wrong rounding. For 4:2:0, chroma scaling for y is dividing by two and rounding toward zero. When motion_y < 0 and motion_y + 16 > 0, the rounding direction of "motion_y" and "motion_y + 16" is different and rounding "motion_y + 16" would be incorrect.

We should input "motion_y" as is to round correctly. I add "is_16x8" flag to do that.
---
 libavcodec/mpeg12dec.c        |  2 ++
 libavcodec/mpegvideo_motion.c | 30 ++++++++++++++++--------------
 2 files changed, 18 insertions(+), 14 deletions(-)

Comments

Michael Niedermayer Feb. 9, 2018, 12:55 a.m.
On Fri, Feb 09, 2018 at 02:03:24AM +0900, Nekopanda wrote:
> - Fix field selection for skipped macroblocks
> 
> For B field pictures, the spec says,
> 
> > The prediction shall be made from the field of the same parity as the field being predicted.
> 
> I did it.
> 
> - Fix motion vector rounding for chroma components
> 
> In 16x8 motion compensation, for lower 16x8 region, the input to mpeg_motion() for motion_y was "motion_y + 16", which causes wrong rounding. For 4:2:0, chroma scaling for y is dividing by two and rounding toward zero. When motion_y < 0 and motion_y + 16 > 0, the rounding direction of "motion_y" and "motion_y + 16" is different and rounding "motion_y + 16" would be incorrect.
> 
> We should input "motion_y" as is to round correctly. I add "is_16x8" flag to do that.
> ---
>  libavcodec/mpeg12dec.c        |  2 ++
>  libavcodec/mpegvideo_motion.c | 30 ++++++++++++++++--------------
>  2 files changed, 18 insertions(+), 14 deletions(-)

How did you find this bug ?

do you have a testcase / file which shows artifacts without this change ?

assuming the change is correct then the patch needs to update several fate
checksums. As is it would break make fate

thanks

[...]
Nekopanda Feb. 9, 2018, 2:07 a.m.
> How did you find this bug ?

> do you have a testcase / file which shows artifacts without this change ?

Yes, I have files that show artifacts without this change. Recently, Japanese satellite television started to use field pictures widely, and many of that show artifacts. We need this change to remove the artifacts.

> assuming the change is correct then the patch needs to update several fate
> checksums. As is it would break make fate

currently the following tests fail with this change.

filter-w3fdif-complex
filter-w3fdif-simple
filter-yadif10
filter-yadif16
filter-yadif-mode0
filter-yadif-mode1
mpeg2-field-enc
mpeg2-ticket186
mpeg2-ticket6677



----- Original Message -----
> From: Michael Niedermayer <michael@niedermayer.cc>
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Cc: 
> Date: 2018/2/9, Fri 09:55
> Subject: Re: [FFmpeg-devel] [PATCH] mpeg2dec: fix decoding field pictures
> 
> On Fri, Feb 09, 2018 at 02:03:24AM +0900, Nekopanda wrote:
>>  - Fix field selection for skipped macroblocks
>> 
>>  For B field pictures, the spec says,
>> 
>>  > The prediction shall be made from the field of the same parity as the 
> field being predicted.
>> 
>>  I did it.
>> 
>>  - Fix motion vector rounding for chroma components
>> 
>>  In 16x8 motion compensation, for lower 16x8 region, the input to 
> mpeg_motion() for motion_y was "motion_y + 16", which causes wrong 
> rounding. For 4:2:0, chroma scaling for y is dividing by two and rounding toward 
> zero. When motion_y < 0 and motion_y + 16 > 0, the rounding direction of 
> "motion_y" and "motion_y + 16" is different and rounding 
> "motion_y + 16" would be incorrect.
>> 
>>  We should input "motion_y" as is to round correctly. I add 
> "is_16x8" flag to do that.
>>  ---
>>   libavcodec/mpeg12dec.c        |  2 ++
>>   libavcodec/mpegvideo_motion.c | 30 ++++++++++++++++--------------
>>   2 files changed, 18 insertions(+), 14 deletions(-)
> 
> How did you find this bug ?
> 
> do you have a testcase / file which shows artifacts without this change ?
> 
> assuming the change is correct then the patch needs to update several fate
> checksums. As is it would break make fate
> 
> thanks
> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> "Nothing to hide" only works if the folks in power share the values of
> you and everyone you know entirely and always will -- Tom Scott
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Carl Eugen Hoyos Feb. 9, 2018, 2:23 a.m.
2018-02-09 3:07 GMT+01:00 Nekopanda <pianoyayaninth-at-yahoo.co.jp@ffmpeg.org>:
>> How did you find this bug ?
>>
>> do you have a testcase / file which shows artifacts without this change ?
>
> Yes, I have files that show artifacts without this change. Recently,
> Japanese satellite television started to use field pictures widely,
> and many of that show artifacts. We need this change to remove
> the artifacts.

Could you provide a sample with visible artefacts?

Thank you, Carl Eugen
Nekopanda Feb. 9, 2018, 2:51 a.m.
> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>

> 
> 2018-02-09 3:07 GMT+01:00 Nekopanda 
> <pianoyayaninth-at-yahoo.co.jp@ffmpeg.org>:
>>>  How did you find this bug ?
>>> 
>>>  do you have a testcase / file which shows artifacts without this change 
> ?
>> 
>>  Yes, I have files that show artifacts without this change. Recently,
>>  Japanese satellite television started to use field pictures widely,
>>  and many of that show artifacts. We need this change to remove
>>  the artifacts.
> 
> Could you provide a sample with visible artefacts?

Sorry, all of them are copyrighted materials and difficult to share.
Carl Eugen Hoyos Feb. 9, 2018, 10:31 a.m.
2018-02-09 3:51 GMT+01:00 Nekopanda <pianoyayaninth-at-yahoo.co.jp@ffmpeg.org>:
>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>

>>>  Yes, I have files that show artifacts without this change. Recently,
>>>  Japanese satellite television started to use field pictures widely,
>>>  and many of that show artifacts. We need this change to remove
>>>  the artifacts.
>>
>> Could you provide a sample with visible artefacts?
>
> Sorry, all of them are copyrighted materials and difficult to share.

Please find a few seconds of commercials that show the issue
and share them.

Many thousand samples were provided here, very few were
not copyrighted.

Carl Eugen
Nekopanda Feb. 9, 2018, 12:28 p.m.
> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>

> 
> 2018-02-09 3:51 GMT+01:00 Nekopanda 
> <pianoyayaninth-at-yahoo.co.jp@ffmpeg.org>:
>>>  From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> 
>>>>   Yes, I have files that show artifacts without this change. 
> Recently,
>>>>   Japanese satellite television started to use field pictures 
> widely,
>>>>   and many of that show artifacts. We need this change to remove
>>>>   the artifacts.
>>> 
>>>  Could you provide a sample with visible artefacts?
>> 
>>  Sorry, all of them are copyrighted materials and difficult to share.
> 
> Please find a few seconds of commercials that show the issue
> and share them.
> 
> Many thousand samples were provided here, very few were
> not copyrighted.


OK. Here is.

http://nekopandanet.sakura.ne.jp/tmp/v/field-pic-bug.mpg

Decoded frame with artifacts
http://nekopandanet.sakura.ne.jp/tmp/v/pic-09.png
Michael Niedermayer Feb. 9, 2018, 10:56 p.m.
On Fri, Feb 09, 2018 at 02:03:24AM +0900, Nekopanda wrote:
> - Fix field selection for skipped macroblocks
> 
> For B field pictures, the spec says,
> 
> > The prediction shall be made from the field of the same parity as the field being predicted.
> 
> I did it.
> 
> - Fix motion vector rounding for chroma components
> 
> In 16x8 motion compensation, for lower 16x8 region, the input to mpeg_motion() for motion_y was "motion_y + 16", which causes wrong rounding. For 4:2:0, chroma scaling for y is dividing by two and rounding toward zero. When motion_y < 0 and motion_y + 16 > 0, the rounding direction of "motion_y" and "motion_y + 16" is different and rounding "motion_y + 16" would be incorrect.
> 
> We should input "motion_y" as is to round correctly. I add "is_16x8" flag to do that.

please split this patch in 2.
I think these are 2 independant bugfixes.

also please update the fate checksums so make fate does not break after
either patch

thx

[...]
Nekopanda Feb. 10, 2018, 9:50 a.m.
> On Fri, Feb 09, 2018 at 02:03:24AM +0900, Nekopanda wrote:

>>  - Fix field selection for skipped macroblocks
>> 
>>  For B field pictures, the spec says,
>> 
>>  > The prediction shall be made from the field of the same parity as the 
> field being predicted.
>> 
>>  I did it.
>> 
>>  - Fix motion vector rounding for chroma components
>> 
>>  In 16x8 motion compensation, for lower 16x8 region, the input to 
> mpeg_motion() for motion_y was "motion_y + 16", which causes wrong 
> rounding. For 4:2:0, chroma scaling for y is dividing by two and rounding toward 
> zero. When motion_y < 0 and motion_y + 16 > 0, the rounding direction of 
> "motion_y" and "motion_y + 16" is different and rounding 
> "motion_y + 16" would be incorrect.
>> 
>>  We should input "motion_y" as is to round correctly. I add 
> "is_16x8" flag to do that.
> 
> please split this patch in 2.
> I think these are 2 independant bugfixes.
> 
> also please update the fate checksums so make fate does not break after
> either patch


I split into 2 patches and sent them. They also include new checksums.

Patch hide | download patch | download mbox

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index f5f2c69..9e076e8 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1969,6 +1969,8 @@  static int mpeg_decode_slice(MpegEncContext *s, int mb_y,
                     s->mv[0][0][1] = s->last_mv[0][0][1];
                     s->mv[1][0][0] = s->last_mv[1][0][0];
                     s->mv[1][0][1] = s->last_mv[1][0][1];
+                    s->field_select[0][0] = (s->picture_structure - 1) & 1;
+                    s->field_select[1][0] = (s->picture_structure - 1) & 1;
                 }
             }
         }
diff --git a/libavcodec/mpegvideo_motion.c b/libavcodec/mpegvideo_motion.c
index c913504..5624c10 100644
--- a/libavcodec/mpegvideo_motion.c
+++ b/libavcodec/mpegvideo_motion.c
@@ -239,20 +239,22 @@  void mpeg_motion_internal(MpegEncContext *s,
                           int motion_y,
                           int h,
                           int is_mpeg12,
+                          int is_16x8,
                           int mb_y)
 {
     uint8_t *ptr_y, *ptr_cb, *ptr_cr;
     int dxy, uvdxy, mx, my, src_x, src_y,
-        uvsrc_x, uvsrc_y, v_edge_pos;
+        uvsrc_x, uvsrc_y, v_edge_pos, block_y_half;
     ptrdiff_t uvlinesize, linesize;
 
     v_edge_pos = s->v_edge_pos >> field_based;
     linesize   = s->current_picture.f->linesize[0] << field_based;
     uvlinesize = s->current_picture.f->linesize[1] << field_based;
+    block_y_half = (field_based | is_16x8);
 
     dxy   = ((motion_y & 1) << 1) | (motion_x & 1);
     src_x = s->mb_x * 16 + (motion_x >> 1);
-    src_y = (mb_y << (4 - field_based)) + (motion_y >> 1);
+    src_y = (mb_y << (4 - block_y_half)) + (motion_y >> 1);
 
     if (!is_mpeg12 && s->out_format == FMT_H263) {
         if ((s->workaround_bugs & FF_BUG_HPEL_CHROMA) && field_based) {
@@ -260,7 +262,7 @@  void mpeg_motion_internal(MpegEncContext *s,
             my      = motion_y >> 1;
             uvdxy   = ((my & 1) << 1) | (mx & 1);
             uvsrc_x = s->mb_x * 8 + (mx >> 1);
-            uvsrc_y = (mb_y << (3 - field_based)) + (my >> 1);
+            uvsrc_y = (mb_y << (3 - block_y_half)) + (my >> 1);
         } else {
             uvdxy   = dxy | (motion_y & 2) | ((motion_x & 2) >> 1);
             uvsrc_x = src_x >> 1;
@@ -279,7 +281,7 @@  void mpeg_motion_internal(MpegEncContext *s,
             my      = motion_y / 2;
             uvdxy   = ((my & 1) << 1) | (mx & 1);
             uvsrc_x = s->mb_x * 8 + (mx >> 1);
-            uvsrc_y = (mb_y << (3 - field_based)) + (my >> 1);
+            uvsrc_y = (mb_y << (3 - block_y_half)) + (my >> 1);
         } else {
             if (s->chroma_x_shift) {
                 // Chroma422
@@ -370,18 +372,18 @@  static void mpeg_motion(MpegEncContext *s,
                         uint8_t *dest_y, uint8_t *dest_cb, uint8_t *dest_cr,
                         int field_select, uint8_t **ref_picture,
                         op_pixels_func (*pix_op)[4],
-                        int motion_x, int motion_y, int h, int mb_y)
+                        int motion_x, int motion_y, int h, int is_16x8, int mb_y)
 {
 #if !CONFIG_SMALL
     if (s->out_format == FMT_MPEG1)
         mpeg_motion_internal(s, dest_y, dest_cb, dest_cr, 0, 0,
                              field_select, ref_picture, pix_op,
-                             motion_x, motion_y, h, 1, mb_y);
+                             motion_x, motion_y, h, 1, is_16x8, mb_y);
     else
 #endif
         mpeg_motion_internal(s, dest_y, dest_cb, dest_cr, 0, 0,
                              field_select, ref_picture, pix_op,
-                             motion_x, motion_y, h, 0, mb_y);
+                             motion_x, motion_y, h, 0, is_16x8, mb_y);
 }
 
 static void mpeg_motion_field(MpegEncContext *s, uint8_t *dest_y,
@@ -395,12 +397,12 @@  static void mpeg_motion_field(MpegEncContext *s, uint8_t *dest_y,
     if (s->out_format == FMT_MPEG1)
         mpeg_motion_internal(s, dest_y, dest_cb, dest_cr, 1,
                              bottom_field, field_select, ref_picture, pix_op,
-                             motion_x, motion_y, h, 1, mb_y);
+                             motion_x, motion_y, h, 1, 0, mb_y);
     else
 #endif
         mpeg_motion_internal(s, dest_y, dest_cb, dest_cr, 1,
                              bottom_field, field_select, ref_picture, pix_op,
-                             motion_x, motion_y, h, 0, mb_y);
+                             motion_x, motion_y, h, 0, 0, mb_y);
 }
 
 // FIXME: SIMDify, avg variant, 16x16 version
@@ -870,7 +872,7 @@  static av_always_inline void mpv_motion_internal(MpegEncContext *s,
         } else {
             mpeg_motion(s, dest_y, dest_cb, dest_cr, 0,
                         ref_picture, pix_op,
-                        s->mv[dir][0][0], s->mv[dir][0][1], 16, mb_y);
+                        s->mv[dir][0][0], s->mv[dir][0][1], 16, 0, mb_y);
         }
         break;
     case MV_TYPE_8X8:
@@ -907,7 +909,7 @@  static av_always_inline void mpv_motion_internal(MpegEncContext *s,
             mpeg_motion(s, dest_y, dest_cb, dest_cr,
                         s->field_select[dir][0],
                         ref_picture, pix_op,
-                        s->mv[dir][0][0], s->mv[dir][0][1], 16, mb_y >> 1);
+                        s->mv[dir][0][0], s->mv[dir][0][1], 16, 0, mb_y >> 1);
         }
         break;
     case MV_TYPE_16X8:
@@ -924,8 +926,8 @@  static av_always_inline void mpv_motion_internal(MpegEncContext *s,
             mpeg_motion(s, dest_y, dest_cb, dest_cr,
                         s->field_select[dir][i],
                         ref2picture, pix_op,
-                        s->mv[dir][i][0], s->mv[dir][i][1] + 16 * i,
-                        8, mb_y >> 1);
+                        s->mv[dir][i][0], s->mv[dir][i][1],
+                        8, 1, (mb_y & ~1) + i);
 
             dest_y  += 16 * s->linesize;
             dest_cb += (16 >> s->chroma_y_shift) * s->uvlinesize;
@@ -952,7 +954,7 @@  static av_always_inline void mpv_motion_internal(MpegEncContext *s,
                             s->picture_structure != i + 1,
                             ref_picture, pix_op,
                             s->mv[dir][2 * i][0], s->mv[dir][2 * i][1],
-                            16, mb_y >> 1);
+                            16, 0, mb_y >> 1);
 
                 // after put we make avg of the same block
                 pix_op = s->hdsp.avg_pixels_tab;