[FFmpeg-devel] [PATCH] avcodec/mpegvideo_enc: Don't copy non-existent padding

[FFmpeg-devel] [PATCH] avcodec/mpegvideo_enc: Don't copy non-existent padding

[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,
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo_enc: Don't copy non-existent padding

[Andreas Rheinhardt]

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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".