[FFmpeg-devel,v3] lavf/qsvvpp:allocate continuous memory

Submitted by Linjie Fu on June 4, 2019, 3:59 p.m.

Details

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

Commit Message

Linjie Fu June 4, 2019, 3:59 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 av_frame_get_buffer to arrange data according to pixfmt, and remove
the introduced plane_padding.

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

Comments

Zhong Li June 14, 2019, 3:34 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Linjie Fu

> Sent: Tuesday, June 4, 2019 11:59 PM

> To: ffmpeg-devel@ffmpeg.org

> Cc: Fu, Linjie <linjie.fu@intel.com>

> Subject: [FFmpeg-devel] [PATCH,v3] lavf/qsvvpp:allocate continuous

> memory

> 

> 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 av_frame_get_buffer to arrange data according to pixfmt, and remove

> the introduced plane_padding.

> 

> Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> ---

>  libavfilter/qsvvpp.c | 26 ++++++++++++++++++++------

>  1 file changed, 20 insertions(+), 6 deletions(-)

> 

> diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c index

> 5cd1d5d345..c06171444f 100644

> --- a/libavfilter/qsvvpp.c

> +++ b/libavfilter/qsvvpp.c

> @@ -350,7 +350,7 @@ static QSVFrame *query_frame(QSVVPPContext *s,

> AVFilterLink *outlink)  {

>      AVFilterContext *ctx = outlink->src;

>      QSVFrame        *out_frame;

> -    int              ret;

> +    int              i, ret;

> 

>      clear_unused_frames(s->out_frame_list);

> 

> @@ -374,12 +374,26 @@ 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  = FFALIGN(outlink->w, 128);

> +        out_frame->frame->height = FFALIGN(outlink->h, 64);

> +        out_frame->frame->format = outlink->format;

> +

> +        ret = av_frame_get_buffer(out_frame->frame, 128);

> +        if (ret < 0)

> +            return NULL;

> +

> +        /* remove plane_padding introduced by av_frame_get_buffer */

Should be well explained in the comments why need to reomove.

> +        for (i = 1; i < 4; i++) {

> +            if (out_frame->frame->data[i])

> +                out_frame->frame->data[i] -= i * 128;

> +        }


Looks like tricky workaround. If padding bytes size changed in av_frame_get_buffer, will break qsv vpp.
Linjie Fu June 14, 2019, 5:38 a.m.
> -----Original Message-----

> From: Li, Zhong

> Sent: Friday, June 14, 2019 11:34

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Cc: Fu, Linjie <linjie.fu@intel.com>

> Subject: RE: [FFmpeg-devel] [PATCH,v3] lavf/qsvvpp:allocate continuous

> memory

> 

> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > Of Linjie Fu

> > Sent: Tuesday, June 4, 2019 11:59 PM

> > To: ffmpeg-devel@ffmpeg.org

> > Cc: Fu, Linjie <linjie.fu@intel.com>

> > Subject: [FFmpeg-devel] [PATCH,v3] lavf/qsvvpp:allocate continuous

> > memory

> >

> > 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 av_frame_get_buffer to arrange data according to pixfmt, and remove

> > the introduced plane_padding.

> >

> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> > ---

> >  libavfilter/qsvvpp.c | 26 ++++++++++++++++++++------

> >  1 file changed, 20 insertions(+), 6 deletions(-)

> >

> > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c index

> > 5cd1d5d345..c06171444f 100644

> > --- a/libavfilter/qsvvpp.c

> > +++ b/libavfilter/qsvvpp.c

> > @@ -350,7 +350,7 @@ static QSVFrame *query_frame(QSVVPPContext *s,

> > AVFilterLink *outlink)  {

> >      AVFilterContext *ctx = outlink->src;

> >      QSVFrame        *out_frame;

> > -    int              ret;

> > +    int              i, ret;

> >

> >      clear_unused_frames(s->out_frame_list);

> >

> > @@ -374,12 +374,26 @@ 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  = FFALIGN(outlink->w, 128);

> > +        out_frame->frame->height = FFALIGN(outlink->h, 64);

> > +        out_frame->frame->format = outlink->format;

> > +

> > +        ret = av_frame_get_buffer(out_frame->frame, 128);

> > +        if (ret < 0)

> > +            return NULL;

> > +

> > +        /* remove plane_padding introduced by av_frame_get_buffer */

> Should be well explained in the comments why need to reomove.


Tried to explain that continuous memory is need for  libmfx in commit message
and before av_frame_get_buffer was called.

If that's not enough, would you please offer some advices on how to explain better?

> 

> > +        for (i = 1; i < 4; i++) {

> > +            if (out_frame->frame->data[i])

> > +                out_frame->frame->data[i] -= i * 128;

> > +        }

> 

> Looks like tricky workaround. If padding bytes size changed in

> av_frame_get_buffer, will break qsv vpp.


Calls av_frame_get_buffer-> get_video_buffer could allocate memory at one time and
make the code clearer, but will introduce plane_padding:

plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align);

The padding bytes was initialized by variable "align", and "align" was set to 128 as a requirement.
So the padding bytes will always be set to align and I think it won't break vpp pipeline.

In fact, it's a reverse action of padding_bytes being introduced in get_video_buffer():
for (i = 1; i < 4; i++) {
        if (frame->data[i])
            frame->data[i] += i * plane_padding;
    }

To make it more clear and less tricky, I thought 128 can be replaced by variable or Macro like:
Int align = plane_padding = 128;
or
#define PLANE_PADDING 128

Thanks,
Linjie
Zhong Li June 14, 2019, 7:08 a.m.
> From: Fu, Linjie

> Sent: Friday, June 14, 2019 1:39 PM

> To: Li, Zhong <zhong.li@intel.com>; FFmpeg development discussions and

> patches <ffmpeg-devel@ffmpeg.org>

> Subject: RE: [FFmpeg-devel] [PATCH,v3] lavf/qsvvpp:allocate continuous

> memory

> 

> > -----Original Message-----

> > From: Li, Zhong

> > Sent: Friday, June 14, 2019 11:34

> > To: FFmpeg development discussions and patches <ffmpeg-

> > devel@ffmpeg.org>

> > Cc: Fu, Linjie <linjie.fu@intel.com>

> > Subject: RE: [FFmpeg-devel] [PATCH,v3] lavf/qsvvpp:allocate continuous

> > memory

> >

> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> > Behalf

> > > Of Linjie Fu

> > > Sent: Tuesday, June 4, 2019 11:59 PM

> > > To: ffmpeg-devel@ffmpeg.org

> > > Cc: Fu, Linjie <linjie.fu@intel.com>

> > > Subject: [FFmpeg-devel] [PATCH,v3] lavf/qsvvpp:allocate continuous

> > > memory

> > >

> > > 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 av_frame_get_buffer to arrange data according to pixfmt, and

> > > remove the introduced plane_padding.

> > >

> > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> > > ---

> > >  libavfilter/qsvvpp.c | 26 ++++++++++++++++++++------

> > >  1 file changed, 20 insertions(+), 6 deletions(-)

> > >

> > > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c index

> > > 5cd1d5d345..c06171444f 100644

> > > --- a/libavfilter/qsvvpp.c

> > > +++ b/libavfilter/qsvvpp.c

> > > @@ -350,7 +350,7 @@ static QSVFrame *query_frame(QSVVPPContext

> *s,

> > > AVFilterLink *outlink)  {

> > >      AVFilterContext *ctx = outlink->src;

> > >      QSVFrame        *out_frame;

> > > -    int              ret;

> > > +    int              i, ret;

> > >

> > >      clear_unused_frames(s->out_frame_list);

> > >

> > > @@ -374,12 +374,26 @@ 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  = FFALIGN(outlink->w, 128);

> > > +        out_frame->frame->height = FFALIGN(outlink->h, 64);

> > > +        out_frame->frame->format = outlink->format;

> > > +

> > > +        ret = av_frame_get_buffer(out_frame->frame, 128);

> > > +        if (ret < 0)

> > > +            return NULL;

> > > +

> > > +        /* remove plane_padding introduced by av_frame_get_buffer

> > > + */

> > Should be well explained in the comments why need to reomove.

> 

> Tried to explain that continuous memory is need for  libmfx in commit

> message and before av_frame_get_buffer was called.

> 

> If that's not enough, would you please offer some advices on how to explain

> better?

> 

> >

> > > +        for (i = 1; i < 4; i++) {

> > > +            if (out_frame->frame->data[i])

> > > +                out_frame->frame->data[i] -= i * 128;

> > > +        }

> >

> > Looks like tricky workaround. If padding bytes size changed in

> > av_frame_get_buffer, will break qsv vpp.

> 

> Calls av_frame_get_buffer-> get_video_buffer could allocate memory at

> one time and make the code clearer, but will introduce plane_padding:

> 

> plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align);

> 

> The padding bytes was initialized by variable "align", and "align" was set to

> 128 as a requirement.

> So the padding bytes will always be set to align and I think it won't break vpp

> pipeline.


This is just current status, if you check git log of frame.c, plane_padding size was changed serval times. 
If it is continuous to change, then will break qsv vpp.

Patch hide | download patch | download mbox

diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
index 5cd1d5d345..c06171444f 100644
--- a/libavfilter/qsvvpp.c
+++ b/libavfilter/qsvvpp.c
@@ -350,7 +350,7 @@  static QSVFrame *query_frame(QSVVPPContext *s, AVFilterLink *outlink)
 {
     AVFilterContext *ctx = outlink->src;
     QSVFrame        *out_frame;
-    int              ret;
+    int              i, ret;
 
     clear_unused_frames(s->out_frame_list);
 
@@ -374,12 +374,26 @@  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  = FFALIGN(outlink->w, 128);
+        out_frame->frame->height = FFALIGN(outlink->h, 64);
+        out_frame->frame->format = outlink->format;
+
+        ret = av_frame_get_buffer(out_frame->frame, 128);
+        if (ret < 0)
+            return NULL;
+
+        /* remove plane_padding introduced by av_frame_get_buffer */
+        for (i = 1; i < 4; i++) {
+            if (out_frame->frame->data[i])
+                out_frame->frame->data[i] -= i * 128;
+        }
 
         out_frame->frame->width  = outlink->w;
         out_frame->frame->height = outlink->h;