| Message ID | AS8P250MB07446A13546EC73DD133F2158F94A@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/mpegvideo_enc: Don't copy non-existent padding | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
Andreas Rheinhardt: > The allocated buffer of an AVFrame need not be height * linesize > big. In case there is padding between lines, the last line need > not have this padding. Pathological examples exist with height == 1 > (where linesize is not really meaningful); non-pathological examples > are produced by the separatefields filter, which simply offsets > the pointers and doubles linesize, so that the frame created from > the bottom field has only height - 1 lines with padding (containing > the top field) and one line without padding. > > The mpegvideo encoders sometimes copy the input frames for padding/ > alignment reasons and when doing so, there is a fast path in which > everything is copied with one memcpy() of height * linesize. And > this is wrong as explained above. Use av_image_copy_plane() instead > of our ad-hoc code for copying. > > Fixes ticket #10754 (discovered by Zeng Yunxiang). > > (The above discussion presupposes linesize > 0 and would need some > adaptation for negative linesizes; the code removed seems even more > buggy for negative linesizes.) > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > 1. One could also use av_image_copy(). > 2. The "direct" (non-copy) path only ensures a height mod 16, yet > the non-direct mode sometimes pads to 32 (for non-progressive mpeg2). > Seems fishy. > > libavcodec/mpegvideo_enc.c | 19 ++++--------------- > 1 file changed, 4 insertions(+), 15 deletions(-) > > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c > index 6d2d417454..fac9bb7ae7 100644 > --- a/libavcodec/mpegvideo_enc.c > +++ b/libavcodec/mpegvideo_enc.c > @@ -36,6 +36,7 @@ > #include <stdint.h> > > #include "libavutil/emms.h" > +#include "libavutil/imgutils.h" > #include "libavutil/internal.h" > #include "libavutil/intmath.h" > #include "libavutil/mathematics.h" > @@ -1201,28 +1202,16 @@ static int load_input_picture(MpegEncContext *s, const AVFrame *pic_arg) > int w = s->width >> h_shift; > int h = s->height >> v_shift; > const uint8_t *src = pic_arg->data[i]; > - uint8_t *dst = pic->f->data[i]; > + uint8_t *dst = pic->f->data[i] + !s->avctx->rc_buffer_size * INPLACE_OFFSET; > int vpad = 16; > > + av_image_copy_plane(dst, dst_stride, src, src_stride, w, h); > + > if ( s->codec_id == AV_CODEC_ID_MPEG2VIDEO > && !s->progressive_sequence > && FFALIGN(s->height, 32) - s->height > 16) > vpad = 32; > > - if (!s->avctx->rc_buffer_size) > - dst += INPLACE_OFFSET; > - > - if (src_stride == dst_stride) > - memcpy(dst, src, src_stride * h); > - else { > - int h2 = h; > - uint8_t *dst2 = dst; > - while (h2--) { > - memcpy(dst2, src, w); > - dst2 += dst_stride; > - src += src_stride; > - } > - } > if ((s->width & 15) || (s->height & (vpad-1))) { > s->mpvencdsp.draw_edges(dst, dst_stride, > w, h, Will apply this patch tomorrow unless there are objections. - Andreas
diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c index 6d2d417454..fac9bb7ae7 100644 --- a/libavcodec/mpegvideo_enc.c +++ b/libavcodec/mpegvideo_enc.c @@ -36,6 +36,7 @@ #include <stdint.h> #include "libavutil/emms.h" +#include "libavutil/imgutils.h" #include "libavutil/internal.h" #include "libavutil/intmath.h" #include "libavutil/mathematics.h" @@ -1201,28 +1202,16 @@ static int load_input_picture(MpegEncContext *s, const AVFrame *pic_arg) int w = s->width >> h_shift; int h = s->height >> v_shift; const uint8_t *src = pic_arg->data[i]; - uint8_t *dst = pic->f->data[i]; + uint8_t *dst = pic->f->data[i] + !s->avctx->rc_buffer_size * INPLACE_OFFSET; int vpad = 16; + av_image_copy_plane(dst, dst_stride, src, src_stride, w, h); + if ( s->codec_id == AV_CODEC_ID_MPEG2VIDEO && !s->progressive_sequence && FFALIGN(s->height, 32) - s->height > 16) vpad = 32; - if (!s->avctx->rc_buffer_size) - dst += INPLACE_OFFSET; - - if (src_stride == dst_stride) - memcpy(dst, src, src_stride * h); - else { - int h2 = h; - uint8_t *dst2 = dst; - while (h2--) { - memcpy(dst2, src, w); - dst2 += dst_stride; - src += src_stride; - } - } if ((s->width & 15) || (s->height & (vpad-1))) { s->mpvencdsp.draw_edges(dst, dst_stride, w, h,
The allocated buffer of an AVFrame need not be height * linesize big. In case there is padding between lines, the last line need not have this padding. Pathological examples exist with height == 1 (where linesize is not really meaningful); non-pathological examples are produced by the separatefields filter, which simply offsets the pointers and doubles linesize, so that the frame created from the bottom field has only height - 1 lines with padding (containing the top field) and one line without padding. The mpegvideo encoders sometimes copy the input frames for padding/ alignment reasons and when doing so, there is a fast path in which everything is copied with one memcpy() of height * linesize. And this is wrong as explained above. Use av_image_copy_plane() instead of our ad-hoc code for copying. Fixes ticket #10754 (discovered by Zeng Yunxiang). (The above discussion presupposes linesize > 0 and would need some adaptation for negative linesizes; the code removed seems even more buggy for negative linesizes.) Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- 1. One could also use av_image_copy(). 2. The "direct" (non-copy) path only ensures a height mod 16, yet the non-direct mode sometimes pads to 32 (for non-progressive mpeg2). Seems fishy. libavcodec/mpegvideo_enc.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)