[FFmpeg-devel,v4,1/2] lavf/qsvvpp:allocate continuous memory

Submitted by Linjie Fu on June 18, 2019, 2:51 p.m.

Details

Message ID 20190618145144.11949-1-linjie.fu@intel.com
State New
Headers show

Commit Message

Linjie Fu June 18, 2019, 2:51 p.m.
Mediasdk calls CMRT to copy from video to system memory and requires
memory to be continuously allocated across Y and UV.

Add a new path to allocate continuous memory when using system out.
Use get_continuous_buffer to arrange data according to pixfmt.

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
 libavfilter/qsvvpp.c | 67 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 62 insertions(+), 5 deletions(-)

Comments

Maxym Dmytrychenko June 18, 2019, 7:05 a.m.
On Tue, Jun 18, 2019 at 9:52 AM Linjie Fu <linjie.fu@intel.com> wrote:

> Mediasdk calls CMRT to copy from video to system memory and requires
> memory to be continuously allocated across Y and UV.
>
> Add a new path to allocate continuous memory when using system out.
> Use get_continuous_buffer to arrange data according to pixfmt.
>
>
it would be good to keep such continuous allocations consistent with
encoder's implementation,
see
https://github.com/FFmpeg/FFmpeg/blob/8f6e65183354d1d402ae80c71cba2759fe152018/libavcodec/qsvenc.c#L1218

if needed, needs to be checked before re-allocation.


> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavfilter/qsvvpp.c | 67 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
> index 8d5ff2eb65..07fe53573d 100644
> --- a/libavfilter/qsvvpp.c
> +++ b/libavfilter/qsvvpp.c
> @@ -27,6 +27,7 @@
>  #include "libavutil/hwcontext_qsv.h"
>  #include "libavutil/time.h"
>  #include "libavutil/pixdesc.h"
> +#include "libavutil/imgutils.h"
>
>  #include "internal.h"
>  #include "qsvvpp.h"
> @@ -346,6 +347,57 @@ static QSVFrame *submit_frame(QSVVPPContext *s,
> AVFilterLink *inlink, AVFrame *p
>      return qsv_frame;
>  }
>
> +static int get_continuous_buffer(AVFrame *frame, int align)
> +{
> +    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
> +    int ret, i, padded_height;
> +
> +    if (!desc)
> +        return AVERROR(EINVAL);
> +
> +    if ((ret = av_image_check_size(frame->width, frame->height, 0, NULL))
> < 0)
> +        return ret;
> +
> +    if (!frame->linesize[0]) {
> +        if (align <= 0)
> +            align = 32; /* STRIDE_ALIGN. Should be av_cpu_max_align() */
> +
> +        for (i=1; i<=align; i+=i) {
> +            ret = av_image_fill_linesizes(frame->linesize, frame->format,
> +                                          FFALIGN(frame->width, i));
> +            if (ret < 0)
> +                return ret;
> +            if (!(frame->linesize[0] & (align-1)))
> +                break;
> +        }
> +
> +        for (i = 0; i < 4 && frame->linesize[i]; i++)
> +            frame->linesize[i] = FFALIGN(frame->linesize[i], align);
> +    }
> +
> +    padded_height = FFALIGN(frame->height, 64);
> +    if ((ret = av_image_fill_pointers(frame->data, frame->format,
> padded_height,
> +                                      NULL, frame->linesize)) < 0)
> +        return ret;
> +
> +    frame->buf[0] = av_buffer_alloc(ret);
> +    if (!frame->buf[0]) {
> +        ret = AVERROR(ENOMEM);
> +        goto fail;
> +    }
> +
> +    if ((ret = av_image_fill_pointers(frame->data, frame->format,
> padded_height,
> +                                      frame->buf[0]->data,
> frame->linesize)) < 0)
> +        goto fail;
> +
> +    frame->extended_data = frame->data;
> +
> +    return 0;
> +fail:
> +    av_frame_unref(frame);
> +    return ret;
> +}
> +
>  /* get the output surface */
>  static QSVFrame *query_frame(QSVVPPContext *s, AVFilterLink *outlink)
>  {
> @@ -375,15 +427,20 @@ static QSVFrame *query_frame(QSVVPPContext *s,
> AVFilterLink *outlink)
>          out_frame->surface = (mfxFrameSurface1
> *)out_frame->frame->data[3];
>      } else {
>          /* Get a frame with aligned dimensions.
> -         * Libmfx need system memory being 128x64 aligned */
> -        out_frame->frame = ff_get_video_buffer(outlink,
> -                                               FFALIGN(outlink->w, 128),
> -                                               FFALIGN(outlink->h, 64));
> -        if (!out_frame->frame)
> +         * Libmfx need system memory being 128x64 aligned
> +         * and continuously allocated across Y and UV */
> +        out_frame->frame = av_frame_alloc();
> +        if (!out_frame->frame) {
>              return NULL;
> +        }
>
>          out_frame->frame->width  = outlink->w;
>          out_frame->frame->height = outlink->h;
> +        out_frame->frame->format = outlink->format;
> +
> +        ret = get_continuous_buffer(out_frame->frame, 128);
> +        if (ret < 0)
> +            return NULL;
>
>          ret = map_frame_to_surface(out_frame->frame,
>                                    &out_frame->surface_internal);
> --
> 2.17.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".


regards
Max
Linjie Fu June 18, 2019, 8:12 a.m.
From: Max Dmitrichenko [mailto:maxim.d33@gmail.com]

Sent: Tuesday, June 18, 2019 15:06
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: Fu, Linjie <linjie.fu@intel.com>
Subject: Re: [FFmpeg-devel] [PATCH, v4 1/2] lavf/qsvvpp:allocate continuous memory



On Tue, Jun 18, 2019 at 9:52 AM Linjie Fu <linjie.fu@intel.com<mailto:linjie.fu@intel.com>> wrote:
Mediasdk calls CMRT to copy from video to system memory and requires
memory to be continuously allocated across Y and UV.

Add a new path to allocate continuous memory when using system out.
Use get_continuous_buffer to arrange data according to pixfmt.

it would be good to keep such continuous allocations consistent with encoder's implementation,
see https://github.com/FFmpeg/FFmpeg/blob/8f6e65183354d1d402ae80c71cba2759fe152018/libavcodec/qsvenc.c#L1218

if needed, needs to be checked before re-allocation.

Thanks for the advice and I agree consistency is important.
And I’ve thought about the behavior in qsvenc.c as well to make sure the memory is continuous.

However, I got some concerns:

1.       av_frame_get_buffer() will introduce plane_padding, and it will lead to un-continuous data[0]/data[1];

And the paddings  will lead to wrong offset of U/V, and will cause garbage on chroma filed of image. (when using mirror methods)

So I choose to use get_continuous_buffer() which is similar but introduced no paddings.

2.       Since here is the first the position to allocate memory for output frame, directly allocate a continuous memory seems to be good IMHO.

Thanks,
Linjie

Patch hide | download patch | download mbox

diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
index 8d5ff2eb65..07fe53573d 100644
--- a/libavfilter/qsvvpp.c
+++ b/libavfilter/qsvvpp.c
@@ -27,6 +27,7 @@ 
 #include "libavutil/hwcontext_qsv.h"
 #include "libavutil/time.h"
 #include "libavutil/pixdesc.h"
+#include "libavutil/imgutils.h"
 
 #include "internal.h"
 #include "qsvvpp.h"
@@ -346,6 +347,57 @@  static QSVFrame *submit_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame *p
     return qsv_frame;
 }
 
+static int get_continuous_buffer(AVFrame *frame, int align)
+{
+    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
+    int ret, i, padded_height;
+
+    if (!desc)
+        return AVERROR(EINVAL);
+
+    if ((ret = av_image_check_size(frame->width, frame->height, 0, NULL)) < 0)
+        return ret;
+
+    if (!frame->linesize[0]) {
+        if (align <= 0)
+            align = 32; /* STRIDE_ALIGN. Should be av_cpu_max_align() */
+
+        for (i=1; i<=align; i+=i) {
+            ret = av_image_fill_linesizes(frame->linesize, frame->format,
+                                          FFALIGN(frame->width, i));
+            if (ret < 0)
+                return ret;
+            if (!(frame->linesize[0] & (align-1)))
+                break;
+        }
+
+        for (i = 0; i < 4 && frame->linesize[i]; i++)
+            frame->linesize[i] = FFALIGN(frame->linesize[i], align);
+    }
+
+    padded_height = FFALIGN(frame->height, 64);
+    if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height,
+                                      NULL, frame->linesize)) < 0)
+        return ret;
+
+    frame->buf[0] = av_buffer_alloc(ret);
+    if (!frame->buf[0]) {
+        ret = AVERROR(ENOMEM);
+        goto fail;
+    }
+
+    if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height,
+                                      frame->buf[0]->data, frame->linesize)) < 0)
+        goto fail;
+
+    frame->extended_data = frame->data;
+
+    return 0;
+fail:
+    av_frame_unref(frame);
+    return ret;
+}
+
 /* get the output surface */
 static QSVFrame *query_frame(QSVVPPContext *s, AVFilterLink *outlink)
 {
@@ -375,15 +427,20 @@  static QSVFrame *query_frame(QSVVPPContext *s, AVFilterLink *outlink)
         out_frame->surface = (mfxFrameSurface1 *)out_frame->frame->data[3];
     } else {
         /* Get a frame with aligned dimensions.
-         * Libmfx need system memory being 128x64 aligned */
-        out_frame->frame = ff_get_video_buffer(outlink,
-                                               FFALIGN(outlink->w, 128),
-                                               FFALIGN(outlink->h, 64));
-        if (!out_frame->frame)
+         * Libmfx need system memory being 128x64 aligned
+         * and continuously allocated across Y and UV */
+        out_frame->frame = av_frame_alloc();
+        if (!out_frame->frame) {
             return NULL;
+        }
 
         out_frame->frame->width  = outlink->w;
         out_frame->frame->height = outlink->h;
+        out_frame->frame->format = outlink->format;
+
+        ret = get_continuous_buffer(out_frame->frame, 128);
+        if (ret < 0)
+            return NULL;
 
         ret = map_frame_to_surface(out_frame->frame,
                                   &out_frame->surface_internal);