diff mbox

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

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

Commit Message

Fu, Linjie May 29, 2019, 5 p.m. UTC
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_image_fill_pointers to arrange data according to pixfmt.

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
[v2]: use av_image_fill_pointers

 libavfilter/qsvvpp.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

Comments

Zhong Li May 31, 2019, 9:20 a.m. UTC | #1
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Linjie Fu

> Sent: Thursday, May 30, 2019 1:01 AM

> To: ffmpeg-devel@ffmpeg.org

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

> Subject: [FFmpeg-devel] [PATCH, v2 1/2] 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_image_fill_pointers to arrange data according to pixfmt.

> 

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

> ---

> [v2]: use av_image_fill_pointers

> 

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

>  1 file changed, 27 insertions(+), 5 deletions(-)

> 

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

> 06efdf5089..6eeed7e632 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"

> @@ -51,6 +52,7 @@ struct QSVVPPContext {

>      enum AVPixelFormat  out_sw_format;   /* Real output format */

>      mfxVideoParam       vpp_param;

>      mfxFrameInfo       *frame_infos;     /* frame info for each

> input */

> +    AVBufferPool       *pool;

> 

>      /* members related to the input/output surface */

>      int                 in_mem_mode;

> @@ -375,10 +377,24 @@ 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));

> +         * 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;


Should be better to return AVERROR(ENOMEM)? 

> +        }

> +

> +        out_frame->frame->linesize[0] = FFALIGN(outlink->w, 128);

> +        out_frame->frame->linesize[1] = out_frame->frame->linesize[0];

> +        out_frame->frame->buf[0]      = av_buffer_pool_get(s->pool);

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

> +

> +        if (!out_frame->frame->buf[0])

> +            return NULL;


Same as frame alloc.

> +

> +        av_image_fill_pointers(out_frame->frame->data,

> out_frame->frame->format,

> +                                FFALIGN(outlink->h, 64),

> out_frame->frame->buf[0]->data,

> +

> + out_frame->frame->linesize);

>          if (!out_frame->frame)

>              return NULL;

> 

> @@ -483,8 +499,13 @@ static int init_vpp_session(AVFilterContext *avctx,

> QSVVPPContext *s)

> 

>          av_buffer_unref(&outlink->hw_frames_ctx);

>          outlink->hw_frames_ctx = out_frames_ref;

> -    } else

> +    } else {

>          s->out_mem_mode = MFX_MEMTYPE_SYSTEM_MEMORY;

> +        s->pool =

> av_buffer_pool_init(av_image_get_buffer_size(outlink->format,

> +

> FFALIGN(outlink->w, 128),

> +

> FFALIGN(outlink->h, 64), 1),

> +                                        av_buffer_allocz);


1. What is the benefit to use a pool? Comparing with directly alloc a buffer use av_buffer_allocz()? 
2. av_buffer_allocz() will memset the whole buffer and make performance drop. Is it really necessary here? 
  If no (I believe so), just use av_buffer_alloc()
Fu, Linjie June 3, 2019, 5:17 a.m. UTC | #2
> -----Original Message-----

> From: Li, Zhong

> Sent: Friday, May 31, 2019 17:20

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

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

> Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> continuous memory

> 

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

> Behalf

> > Of Linjie Fu

> > Sent: Thursday, May 30, 2019 1:01 AM

> > To: ffmpeg-devel@ffmpeg.org

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

> > Subject: [FFmpeg-devel] [PATCH, v2 1/2] 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_image_fill_pointers to arrange data according to pixfmt.

> >

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

> > ---

> > [v2]: use av_image_fill_pointers

> >

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

> >  1 file changed, 27 insertions(+), 5 deletions(-)

> >

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

> > 06efdf5089..6eeed7e632 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"

> > @@ -51,6 +52,7 @@ struct QSVVPPContext {

> >      enum AVPixelFormat  out_sw_format;   /* Real output format */

> >      mfxVideoParam       vpp_param;

> >      mfxFrameInfo       *frame_infos;     /* frame info for each

> > input */

> > +    AVBufferPool       *pool;

> >

> >      /* members related to the input/output surface */

> >      int                 in_mem_mode;

> > @@ -375,10 +377,24 @@ 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));

> > +         * 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;

> 

> Should be better to return AVERROR(ENOMEM)?


Will refine.

> 

> > +        }

> > +

> > +        out_frame->frame->linesize[0] = FFALIGN(outlink->w, 128);

> > +        out_frame->frame->linesize[1] = out_frame->frame->linesize[0];

> > +        out_frame->frame->buf[0]      = av_buffer_pool_get(s->pool);

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

> > +

> > +        if (!out_frame->frame->buf[0])

> > +            return NULL;

> 

> Same as frame alloc.


Will refine.

> 

> > +

> > +        av_image_fill_pointers(out_frame->frame->data,

> > out_frame->frame->format,

> > +                                FFALIGN(outlink->h, 64),

> > out_frame->frame->buf[0]->data,

> > +

> > + out_frame->frame->linesize);

> >          if (!out_frame->frame)

> >              return NULL;

> >

> > @@ -483,8 +499,13 @@ static int init_vpp_session(AVFilterContext *avctx,

> > QSVVPPContext *s)

> >

> >          av_buffer_unref(&outlink->hw_frames_ctx);

> >          outlink->hw_frames_ctx = out_frames_ref;

> > -    } else

> > +    } else {

> >          s->out_mem_mode = MFX_MEMTYPE_SYSTEM_MEMORY;

> > +        s->pool =

> > av_buffer_pool_init(av_image_get_buffer_size(outlink->format,

> > +

> > FFALIGN(outlink->w, 128),

> > +

> > FFALIGN(outlink->h, 64), 1),

> > +                                        av_buffer_allocz);

> 

> 1. What is the benefit to use a pool? Comparing with directly alloc a buffer

> use av_buffer_allocz()?

Directly allocate seems to be better.

> 2. av_buffer_allocz() will memset the whole buffer and make performance

> drop. Is it really necessary here?

>   If no (I believe so), just use av_buffer_alloc()

I' not quite sure whether the non-initialized padding data introduced by
the alignment will cause some potential run2run issues if we do encoding next.
But as the usages in lavu/frame.c and lavc/avpacket.c, av_buffer_alloc() is enough.

Will refine and send a new version today.

Thanks.
Linjie
Zhong Li June 3, 2019, 5:27 a.m. UTC | #3
> -----Original Message-----

> From: Fu, Linjie

> Sent: Monday, June 3, 2019 1:17 PM

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

> patches <ffmpeg-devel@ffmpeg.org>

> Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> continuous memory

> 

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

> > From: Li, Zhong

> > Sent: Friday, May 31, 2019 17:20

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

> > devel@ffmpeg.org>

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

> > Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> > continuous memory

> >

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

> > Behalf

> > > Of Linjie Fu

> > > Sent: Thursday, May 30, 2019 1:01 AM

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

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

> > > Subject: [FFmpeg-devel] [PATCH, v2 1/2] 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_image_fill_pointers to arrange data according to pixfmt.

> > >

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

> > > ---

> > > [v2]: use av_image_fill_pointers

> > >

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

> > >  1 file changed, 27 insertions(+), 5 deletions(-)

> > >

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

> > > 06efdf5089..6eeed7e632 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"

> > > @@ -51,6 +52,7 @@ struct QSVVPPContext {

> > >      enum AVPixelFormat  out_sw_format;   /* Real output format

> */

> > >      mfxVideoParam       vpp_param;

> > >      mfxFrameInfo       *frame_infos;     /* frame info for each

> > > input */

> > > +    AVBufferPool       *pool;

> > >

> > >      /* members related to the input/output surface */

> > >      int                 in_mem_mode;

> > > @@ -375,10 +377,24 @@ 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));

> > > +         * 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;

> >

> > Should be better to return AVERROR(ENOMEM)?

> 

> Will refine.

> 

> >

> > > +        }

> > > +

> > > +        out_frame->frame->linesize[0] = FFALIGN(outlink->w, 128);

> > > +        out_frame->frame->linesize[1] =

> out_frame->frame->linesize[0];

> > > +        out_frame->frame->buf[0]      =

> av_buffer_pool_get(s->pool);

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

> > > +

> > > +        if (!out_frame->frame->buf[0])

> > > +            return NULL;

> >

> > Same as frame alloc.

> 

> Will refine.

> > 1. What is the benefit to use a pool? Comparing with directly alloc a

> > buffer use ()?

> Directly allocate seems to be better.


I am thinking using av_frame_get_buffer() will make life easier or not. 
It can make sure memory continuous and call av_buffer_alloc() and av_image_fill_pointers() internally.
Fu, Linjie June 3, 2019, 7:09 a.m. UTC | #4
> -----Original Message-----

> From: Li, Zhong

> Sent: Monday, June 3, 2019 13:28

> To: Fu, Linjie <linjie.fu@intel.com>; FFmpeg development discussions and

> patches <ffmpeg-devel@ffmpeg.org>

> Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> continuous memory

> 

> 

> 

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

> > From: Fu, Linjie

> > Sent: Monday, June 3, 2019 1:17 PM

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

> > patches <ffmpeg-devel@ffmpeg.org>

> > Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> > continuous memory

> >

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

> > > From: Li, Zhong

> > > Sent: Friday, May 31, 2019 17:20

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

> > > devel@ffmpeg.org>

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

> > > Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> > > continuous memory

> > >

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

> > > Behalf

> > > > Of Linjie Fu

> > > > Sent: Thursday, May 30, 2019 1:01 AM

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

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

> > > > Subject: [FFmpeg-devel] [PATCH, v2 1/2] 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_image_fill_pointers to arrange data according to pixfmt.

> > > >

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

> > > > ---

> > > > [v2]: use av_image_fill_pointers

> > > >

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

> > > >  1 file changed, 27 insertions(+), 5 deletions(-)

> > > >

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

> > > > 06efdf5089..6eeed7e632 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"

> > > > @@ -51,6 +52,7 @@ struct QSVVPPContext {

> > > >      enum AVPixelFormat  out_sw_format;   /* Real output format

> > */

> > > >      mfxVideoParam       vpp_param;

> > > >      mfxFrameInfo       *frame_infos;     /* frame info for each

> > > > input */

> > > > +    AVBufferPool       *pool;

> > > >

> > > >      /* members related to the input/output surface */

> > > >      int                 in_mem_mode;

> > > > @@ -375,10 +377,24 @@ 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));

> > > > +         * 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;

> > >

> > > Should be better to return AVERROR(ENOMEM)?

> >

> > Will refine.

> >

> > >

> > > > +        }

> > > > +

> > > > +        out_frame->frame->linesize[0] = FFALIGN(outlink->w, 128);

> > > > +        out_frame->frame->linesize[1] =

> > out_frame->frame->linesize[0];

> > > > +        out_frame->frame->buf[0]      =

> > av_buffer_pool_get(s->pool);

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

> > > > +

> > > > +        if (!out_frame->frame->buf[0])

> > > > +            return NULL;

> > >

> > > Same as frame alloc.

> >

> > Will refine.


Thinking about this again, I see all return values for error handling in submit_frame
and query_frame is NULL.
So I prefer to keep this to match the whole behavior in qsvvpp.c, or we can send
another patch to replace them all.

> > > 1. What is the benefit to use a pool? Comparing with directly alloc a

> > > buffer use ()?

> > Directly allocate seems to be better.

> 

> I am thinking using av_frame_get_buffer() will make life easier or not.

> It can make sure memory continuous and call av_buffer_alloc() and

> av_image_fill_pointers() internally.


It works in continuously allocating memory,  but will the padded_height be a constraint? 
In get_video_buffer(), padded_height is aligned by 32 in hard-coded way,
But 64 alignment is needed.

Thanks,
Linjie
Zhong Li June 3, 2019, 7:23 a.m. UTC | #5
> -----Original Message-----

> From: Fu, Linjie

> Sent: Monday, June 3, 2019 3:10 PM

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

> patches <ffmpeg-devel@ffmpeg.org>

> Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> continuous memory

> 

> 

> 

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

> > From: Li, Zhong

> > Sent: Monday, June 3, 2019 13:28

> > To: Fu, Linjie <linjie.fu@intel.com>; FFmpeg development discussions

> > and patches <ffmpeg-devel@ffmpeg.org>

> > Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> > continuous memory

> >

> >

> >

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

> > > From: Fu, Linjie

> > > Sent: Monday, June 3, 2019 1:17 PM

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

> > > and patches <ffmpeg-devel@ffmpeg.org>

> > > Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> > > continuous memory

> > >

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

> > > > From: Li, Zhong

> > > > Sent: Friday, May 31, 2019 17:20

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

> > > > devel@ffmpeg.org>

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

> > > > Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate

> > > > continuous memory

> > > >

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

> > > > Behalf

> > > > > Of Linjie Fu

> > > > > Sent: Thursday, May 30, 2019 1:01 AM

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

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

> > > > > Subject: [FFmpeg-devel] [PATCH, v2 1/2] 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_image_fill_pointers to arrange data according to pixfmt.

> > > > >

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

> > > > > ---

> > > > > [v2]: use av_image_fill_pointers

> > > > >

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

> > > > >  1 file changed, 27 insertions(+), 5 deletions(-)

> > > > >

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

> > > > > 06efdf5089..6eeed7e632 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"

> > > > > @@ -51,6 +52,7 @@ struct QSVVPPContext {

> > > > >      enum AVPixelFormat  out_sw_format;   /* Real output

> format

> > > */

> > > > >      mfxVideoParam       vpp_param;

> > > > >      mfxFrameInfo       *frame_infos;     /* frame info for

> each

> > > > > input */

> > > > > +    AVBufferPool       *pool;

> > > > >

> > > > >      /* members related to the input/output surface */

> > > > >      int                 in_mem_mode;

> > > > > @@ -375,10 +377,24 @@ 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));

> > > > > +         * 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;

> > > >

> > > > Should be better to return AVERROR(ENOMEM)?

> > >

> > > Will refine.

> > >

> > > >

> > > > > +        }

> > > > > +

> > > > > +        out_frame->frame->linesize[0] = FFALIGN(outlink->w,

> 128);

> > > > > +        out_frame->frame->linesize[1] =

> > > out_frame->frame->linesize[0];

> > > > > +        out_frame->frame->buf[0]      =

> > > av_buffer_pool_get(s->pool);

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

> > > > > +

> > > > > +        if (!out_frame->frame->buf[0])

> > > > > +            return NULL;

> > > >

> > > > Same as frame alloc.

> > >

> > > Will refine.

> 

> Thinking about this again, I see all return values for error handling in

> submit_frame and query_frame is NULL.

> So I prefer to keep this to match the whole behavior in qsvvpp.c, or we can

> send another patch to replace them all.


Thus means other places should be modified too. Fine to me to fix it with a separated patch. 

> > > > 1. What is the benefit to use a pool? Comparing with directly

> > > > alloc a buffer use ()?

> > > Directly allocate seems to be better.

> >

> > I am thinking using av_frame_get_buffer() will make life easier or not.

> > It can make sure memory continuous and call av_buffer_alloc() and

> > av_image_fill_pointers() internally.

> 

> It works in continuously allocating memory,  but will the padded_height be

> a constraint?

> In get_video_buffer(), padded_height is aligned by 32 in hard-coded way, But

> 64 alignment is needed.


Making the height has been 64 alignment before call it should be ok IMHO.
diff mbox

Patch

diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c
index 06efdf5089..6eeed7e632 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"
@@ -51,6 +52,7 @@  struct QSVVPPContext {
     enum AVPixelFormat  out_sw_format;   /* Real output format */
     mfxVideoParam       vpp_param;
     mfxFrameInfo       *frame_infos;     /* frame info for each input */
+    AVBufferPool       *pool;
 
     /* members related to the input/output surface */
     int                 in_mem_mode;
@@ -375,10 +377,24 @@  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));
+         * 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->linesize[0] = FFALIGN(outlink->w, 128);
+        out_frame->frame->linesize[1] = out_frame->frame->linesize[0];
+        out_frame->frame->buf[0]      = av_buffer_pool_get(s->pool);
+        out_frame->frame->format      = outlink->format;
+
+        if (!out_frame->frame->buf[0])
+            return NULL;
+
+        av_image_fill_pointers(out_frame->frame->data, out_frame->frame->format,
+                                FFALIGN(outlink->h, 64), out_frame->frame->buf[0]->data,
+                                                            out_frame->frame->linesize);
         if (!out_frame->frame)
             return NULL;
 
@@ -483,8 +499,13 @@  static int init_vpp_session(AVFilterContext *avctx, QSVVPPContext *s)
 
         av_buffer_unref(&outlink->hw_frames_ctx);
         outlink->hw_frames_ctx = out_frames_ref;
-    } else
+    } else {
         s->out_mem_mode = MFX_MEMTYPE_SYSTEM_MEMORY;
+        s->pool = av_buffer_pool_init(av_image_get_buffer_size(outlink->format,
+                                                        FFALIGN(outlink->w, 128),
+                                                        FFALIGN(outlink->h, 64), 1),
+                                        av_buffer_allocz);
+    }
 
     /* extract the properties of the "master" session given to us */
     ret = MFXQueryIMPL(device_hwctx->session, &impl);
@@ -674,6 +695,7 @@  int ff_qsvvpp_free(QSVVPPContext **vpp)
     /* release all the resources */
     clear_frame_list(&s->in_frame_list);
     clear_frame_list(&s->out_frame_list);
+    av_buffer_pool_uninit(&s->pool);
     av_freep(&s->surface_ptrs_in);
     av_freep(&s->surface_ptrs_out);
     av_freep(&s->ext_buffers);