| Message ID | 1518109404-29932-1-git-send-email-pianoyayaninth@yahoo.co.jp |
|---|---|
| State | New |
| Headers | show |
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 [...]
> 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 >
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
> 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.
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
> 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
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 [...]
> 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.
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;
- 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(-)