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

[Fu, Linjie]

> -----Original Message-----
> From: Li, Zhong
> Sent: Monday, June 3, 2019 13:28
> To: Fu, Linjie <linjie.fu at intel.com>; FFmpeg development discussions and
> patches <ffmpeg-devel at 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 at intel.com>; FFmpeg development discussions and
> > patches <ffmpeg-devel at 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 at ffmpeg.org>
> > > Cc: Fu, Linjie <linjie.fu at intel.com>
> > > Subject: RE: [FFmpeg-devel] [PATCH, v2 1/2] lavf/qsvvpp: allocate
> > > continuous memory
> > >
> > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> > > Behalf
> > > > Of Linjie Fu
> > > > Sent: Thursday, May 30, 2019 1:01 AM
> > > > To: ffmpeg-devel at ffmpeg.org
> > > > Cc: Fu, Linjie <linjie.fu at 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 at 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