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

[Fu, Linjie]

> -----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.

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