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

[Li, Zhong]

> -----Original Message-----
> From: Fu, Linjie
> Sent: Monday, June 3, 2019 3:10 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: 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.

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.