qsv: enforcing continuous memory layout

Message ID 20180728085354.11068-1-maxim.d33@gmail.com
State New
Headers show
Series
  • qsv: enforcing continuous memory layout
Related show

Commit Message

Maxym Dmytrychenko July 28, 2018, 8:53 a.m.
---
 libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

Comments

Luca Barbato July 28, 2018, 9:54 a.m. | #1
On 28/07/2018 10:53, maxim_d33 wrote:
> ---
>  libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index e349a075f..9fb1ae01b 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1061,6 +1061,7 @@ static int submit_frame(QSVEncContext *q, const AVFrame *frame,
>  {
>      QSVFrame *qf;
>      int ret;
> +    AVFrame* aligned_frame;
>  
>      ret = get_free_frame(q, &qf);
>      if (ret < 0)
> @@ -1081,22 +1082,35 @@ static int submit_frame(QSVEncContext *q, const AVFrame *frame,
>              qf->surface.Data.MemId = &q->frames_ctx.mids[ret];
>          }
>      } else {
> -        /* make a copy if the input is not padded as libmfx requires */
> -        if (frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) {

This can be kept

> -            qf->frame->height = FFALIGN(frame->height, q->height_align);
> -            qf->frame->width  = FFALIGN(frame->width, q->width_align);

I think

> -            ret = ff_get_buffer(q->avctx, qf->frame, AV_GET_BUFFER_FLAG_REF);
> +        /* make a copy if
> +           - the input is not padded
> +           - allocations are not continious for data[0]/data[1]
> +           required by libmfx */
> +          if ((frame->data[1] - frame->data[0] != frame->linesize[0] * FFALIGN(frame->height, q->height_align)) ||
> +            (frame->height & 31 || frame->linesize[0] & (q->width_align - 1))) {
> +            aligned_frame = av_frame_alloc();
> +            if (!aligned_frame)
> +                return AVERROR(ENOMEM);

I think you can use qf->frame instead of creating another all the time.

> +            aligned_frame->format = frame->format;
> +            aligned_frame->height = FFALIGN(frame->height, q->height_align);
> +            aligned_frame->width  = FFALIGN(frame->width, q->width_align);
> +
> +            ret = av_frame_get_buffer(aligned_frame,q->width_align);
>              if (ret < 0)
>                  return ret;
>  
> -            qf->frame->height = frame->height;
> -            qf->frame->width  = frame->width;
> -            ret = av_frame_copy(qf->frame, frame);
> +            aligned_frame->height = frame->height;
> +            aligned_frame->width  = frame->width;
> +
> +            ret = av_frame_copy(aligned_frame, frame);
>              if (ret < 0) {
> -                av_frame_unref(qf->frame);
> +                av_frame_unref(aligned_frame);
>                  return ret;
>              }
> +
> +            av_frame_free(&qf->frame);
> +            qf->frame = aligned_frame;
>          } else {
>              ret = av_frame_ref(qf->frame, frame);
>              if (ret < 0)
> 

lu
Diego Biurrun July 28, 2018, 10:20 a.m. | #2
On Sat, Jul 28, 2018 at 10:53:54AM +0200, maxim_d33 wrote:
> ---
>  libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)

Looks like your Git is not set up properly.

Diego
Maxym Dmytrychenko July 28, 2018, 10:50 p.m. | #3
Hi Diego!

On Sat, Jul 28, 2018 at 12:20 PM Diego Biurrun <diego@biurrun.de> wrote:

> On Sat, Jul 28, 2018 at 10:53:54AM +0200, maxim_d33 wrote:
> > ---
> >  libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
>
> Looks like your Git is not set up properly.
>
>
what do you mean exactly, Diego?
I was squashing it before sending - may be because of this.


> Diego
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel


regards
Max
Diego Biurrun July 29, 2018, 5:38 p.m. | #4
On Sun, Jul 29, 2018 at 12:50:42AM +0200, Maxym Dmytrychenko wrote:
> On Sat, Jul 28, 2018 at 12:20 PM Diego Biurrun <diego@biurrun.de> wrote:
> > On Sat, Jul 28, 2018 at 10:53:54AM +0200, maxim_d33 wrote:
> > > ---
> > >  libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
> > >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > Looks like your Git is not set up properly.
> >
> >
> what do you mean exactly, Diego?
> I was squashing it before sending - may be because of this.

The Git author name looks strange: maxim_d33. Probably because your
Git user.name is not set and falls back on your username.

Diego
Li, Zhong July 30, 2018, 5:39 a.m. | #5
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of Luca
> Barbato
> Sent: Saturday, July 28, 2018 5:55 PM
> To: libav-devel@libav.org
> Subject: Re: [libav-devel] [PATCH] qsv: enforcing continuous memory layout
> 
> On 28/07/2018 10:53, maxim_d33 wrote:
> > ---
> >  libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index
> > e349a075f..9fb1ae01b 100644
> > --- a/libavcodec/qsvenc.c
> > +++ b/libavcodec/qsvenc.c
> > @@ -1061,6 +1061,7 @@ static int submit_frame(QSVEncContext *q,
> const
> > AVFrame *frame,  {
> >      QSVFrame *qf;
> >      int ret;
> > +    AVFrame* aligned_frame;
> >
> >      ret = get_free_frame(q, &qf);
> >      if (ret < 0)
> > @@ -1081,22 +1082,35 @@ static int submit_frame(QSVEncContext *q,
> const AVFrame *frame,
> >              qf->surface.Data.MemId = &q->frames_ctx.mids[ret];
> >          }
> >      } else {
> > -        /* make a copy if the input is not padded as libmfx requires */
> > -        if (frame->height & 31 || frame->linesize[0] & (q->width_align -
> 1)) {
> 
> This can be kept
> 
> > -            qf->frame->height = FFALIGN(frame->height,
> q->height_align);
> > -            qf->frame->width  = FFALIGN(frame->width,
> q->width_align);
> 
> I think
> 
> > -            ret = ff_get_buffer(q->avctx, qf->frame,
> AV_GET_BUFFER_FLAG_REF);
> > +        /* make a copy if
> > +           - the input is not padded
> > +           - allocations are not continious for data[0]/data[1]
> > +           required by libmfx */
> > +          if ((frame->data[1] - frame->data[0] != frame->linesize[0] *
> FFALIGN(frame->height, q->height_align)) ||

Thanks a lot for this patch, should be useful for http://trac.ffmpeg.org/ticket/5708. 
IIRC, this is only required for NV12 format. For YUV420p, is it must too?
If yes, I guess the best fix is not to making a copy (it will introduce performance drop). Continuous layout is required by spec, not only for qsv.

Checked https://www.fourcc.org/pixel-format/yuv-nv12/: 
"A format in which all Y samples are found first in memory as an array of unsigned char with an even number of lines (possibly with a larger stride for memory alignment), _followed immediately_ by an array of unsigned char containing interleaved Cb and Cr samples (such that if addressed as a little-endian WORD type, Cb would be in the LSBs and Cr would be in the MSBs) with the same total stride as the Y samples. This is the preferred 4:2:0 pixel format."

So I think the best fixing it in the initial allocation. 

> > +            (frame->height & 31 || frame->linesize[0] &
> (q->width_align - 1))) {
> > +            aligned_frame = av_frame_alloc();
> > +            if (!aligned_frame)
> > +                return AVERROR(ENOMEM);
> 
> I think you can use qf->frame instead of creating another all the time.
> 
> > +            aligned_frame->format = frame->format;
> > +            aligned_frame->height = FFALIGN(frame->height,
> q->height_align);
> > +            aligned_frame->width  = FFALIGN(frame->width,
> > + q->width_align);
> > +
> > +            ret = av_frame_get_buffer(aligned_frame,q->width_align);

Could this make sure the reallocated aligned_frame is continuous memory? 

> >              if (ret < 0)
> >                  return ret;
> >
> > -            qf->frame->height = frame->height;
> > -            qf->frame->width  = frame->width;
> > -            ret = av_frame_copy(qf->frame, frame);
> > +            aligned_frame->height = frame->height;
> > +            aligned_frame->width  = frame->width;
> > +
> > +            ret = av_frame_copy(aligned_frame, frame);
> >              if (ret < 0) {
> > -                av_frame_unref(qf->frame);
> > +                av_frame_unref(aligned_frame);
> >                  return ret;
> >              }
> > +
> > +            av_frame_free(&qf->frame);
> > +            qf->frame = aligned_frame;
> >          } else {
> >              ret = av_frame_ref(qf->frame, frame);
> >              if (ret < 0)
> >
> 
> lu
Luca Barbato July 30, 2018, 11:19 a.m. | #6
On 30/07/2018 07:39, Li, Zhong wrote:
> Could this make sure the reallocated aligned_frame is continuous memory? 

I made so it is in [libav-devel] [PATCH] frame: Simplify the video
allocation.

lu
Luca Barbato July 30, 2018, 11:21 a.m. | #7
On 30/07/2018 07:39, Li, Zhong wrote:
> If yes, I guess the best fix is not to making a copy (it will
> introduce performance drop). Continuous layout is required by spec,
> not only for qsv.

Currently nothing prevents anybody to implement a frame allocator that
does not respects that.

Incidentally the avcodec default frame allocator might enjoy something
along the lines of what I did for the generic frame allocator in avutil

Probably would be a good idea to update it as well.

lu
Maxym Dmytrychenko July 30, 2018, 4:04 p.m. | #8
On Sun, Jul 29, 2018 at 7:38 PM Diego Biurrun <diego@biurrun.de> wrote:

> On Sun, Jul 29, 2018 at 12:50:42AM +0200, Maxym Dmytrychenko wrote:
> > On Sat, Jul 28, 2018 at 12:20 PM Diego Biurrun <diego@biurrun.de> wrote:
> > > On Sat, Jul 28, 2018 at 10:53:54AM +0200, maxim_d33 wrote:
> > > > ---
> > > >  libavcodec/qsvenc.c | 34 ++++++++++++++++++++++++----------
> > > >  1 file changed, 24 insertions(+), 10 deletions(-)
> > >
> > > Looks like your Git is not set up properly.
> > >
> > >
> > what do you mean exactly, Diego?
> > I was squashing it before sending - may be because of this.
>
> The Git author name looks strange: maxim_d33. Probably because your
> Git user.name is not set and falls back on your username.
>
> Diego
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel


ok, should be fixed at v2

regards
Max

Patch

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index e349a075f..9fb1ae01b 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1061,6 +1061,7 @@  static int submit_frame(QSVEncContext *q, const AVFrame *frame,
 {
     QSVFrame *qf;
     int ret;
+    AVFrame* aligned_frame;
 
     ret = get_free_frame(q, &qf);
     if (ret < 0)
@@ -1081,22 +1082,35 @@  static int submit_frame(QSVEncContext *q, const AVFrame *frame,
             qf->surface.Data.MemId = &q->frames_ctx.mids[ret];
         }
     } else {
-        /* make a copy if the input is not padded as libmfx requires */
-        if (frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) {
-            qf->frame->height = FFALIGN(frame->height, q->height_align);
-            qf->frame->width  = FFALIGN(frame->width, q->width_align);
-
-            ret = ff_get_buffer(q->avctx, qf->frame, AV_GET_BUFFER_FLAG_REF);
+        /* make a copy if
+           - the input is not padded
+           - allocations are not continious for data[0]/data[1]
+           required by libmfx */
+          if ((frame->data[1] - frame->data[0] != frame->linesize[0] * FFALIGN(frame->height, q->height_align)) ||
+            (frame->height & 31 || frame->linesize[0] & (q->width_align - 1))) {
+            aligned_frame = av_frame_alloc();
+            if (!aligned_frame)
+                return AVERROR(ENOMEM);
+
+            aligned_frame->format = frame->format;
+            aligned_frame->height = FFALIGN(frame->height, q->height_align);
+            aligned_frame->width  = FFALIGN(frame->width, q->width_align);
+
+            ret = av_frame_get_buffer(aligned_frame,q->width_align);
             if (ret < 0)
                 return ret;
 
-            qf->frame->height = frame->height;
-            qf->frame->width  = frame->width;
-            ret = av_frame_copy(qf->frame, frame);
+            aligned_frame->height = frame->height;
+            aligned_frame->width  = frame->width;
+
+            ret = av_frame_copy(aligned_frame, frame);
             if (ret < 0) {
-                av_frame_unref(qf->frame);
+                av_frame_unref(aligned_frame);
                 return ret;
             }
+
+            av_frame_free(&qf->frame);
+            qf->frame = aligned_frame;
         } else {
             ret = av_frame_ref(qf->frame, frame);
             if (ret < 0)