[v2] qsv: enforcing continuous memory layout

Message ID 20180730160237.8776-1-maxim.d33@gmail.com
State Committed
Commit 325aa63dd1a3abc2453914d0bc111d297833d725
Headers show
Series
  • [v2] qsv: enforcing continuous memory layout
Related show

Commit Message

Maxym Dmytrychenko July 30, 2018, 4:02 p.m.
we need to make sure that memory allocation for Y/UV planes is continuous and re-used from a
pool
---
 libavcodec/qsvenc.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Luca Barbato July 30, 2018, 4:15 p.m. | #1
On 30/07/2018 18:02, Maxym Dmytrychenko wrote:
> we need to make sure that memory allocation for Y/UV planes is continuous and re-used from a
> pool
> ---

I'm afraid this would break the already-proper-frame codepath.

I would simplify the default avcoded allocator and call it directly.


>  libavcodec/qsvenc.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index e349a075f..c74b3ae31 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1014,7 +1014,6 @@ static void clear_unused_frames(QSVEncContext *q)
>      QSVFrame *cur = q->work_frames;
>      while (cur) {
>          if (cur->used && !cur->surface.Data.Locked) {
> -            av_frame_unref(cur->frame);
>              cur->used = 0;
>          }
>          cur = cur->next;
> @@ -1082,16 +1081,23 @@ static int submit_frame(QSVEncContext *q, const AVFrame *frame,
>          }
>      } else {
>          /* make a copy if the input is not padded as libmfx requires */
> -        if (frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) {
> +        /* and to make allocation continious for data[0]/data[1] */
> +         if ((frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) ||
> +            (frame->data[1] - frame->data[0] != frame->linesize[0] * FFALIGN(qf->frame->height, q->height_align))) {
>              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);
> -            if (ret < 0)
> -                return ret;
> +            qf->frame->format = frame->format;
> +
> +            if (!qf->frame->data[0]) {
> +                ret = av_frame_get_buffer(qf->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);
>              if (ret < 0) {
>                  av_frame_unref(qf->frame);
>
Rogozhkin, Dmitry V July 30, 2018, 5:55 p.m. | #2
On Mon, 2018-07-30 at 18:02 +0200, Maxym Dmytrychenko wrote:
> we need to make sure that memory allocation for Y/UV planes is
> continuous and re-used from a
> pool

Could you, please, be more explicit in the commit message otherwise we
slip the discussion since not everyone here in the mailing list may
understand the origin of the problem?

I suggest to cover the following topics:
1. What's the problem? Why you submitted this patch? I.e. there was
random failures during some operations (which: decoding, encoding,
transcoding?)
2. Provide specific command line(s) with the examples where the issue
can be observed.
3. Highlight the root cause: hardware requires continuous memory
allocation and can't work with planes being allocated separately.

> ---
>  libavcodec/qsvenc.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index e349a075f..c74b3ae31 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1014,7 +1014,6 @@ static void clear_unused_frames(QSVEncContext
> *q)
>      QSVFrame *cur = q->work_frames;
>      while (cur) {
>          if (cur->used && !cur->surface.Data.Locked) {
> -            av_frame_unref(cur->frame);

Hm. This call looked logical: you don't need a frame and you dispose of
 it. Thus, it should either return to the pool or be completely
disposed. Why you removed this call? and where frame is being disposed now?

>              cur->used = 0;
>          }
>          cur = cur->next;
> @@ -1082,16 +1081,23 @@ static int submit_frame(QSVEncContext *q,
> const AVFrame *frame,
>          }
>      } else {
>          /* make a copy if the input is not padded as libmfx requires
> */
> -        if (frame->height & 31 || frame->linesize[0] & (q-
> >width_align - 1)) {
> +        /* and to make allocation continious for data[0]/data[1] */
> +         if ((frame->height & 31 || frame->linesize[0] & (q-
> >width_align - 1)) ||
> +            (frame->data[1] - frame->data[0] != frame->linesize[0] *
> FFALIGN(qf->frame->height, q->height_align))) {

So, if frame is allocated in an incompatible way, you trigger a copy,
right?

To be honest I consider this as a partial solution only. It is good
when you want to be compatible with the bigger range of 3rd party
components and for any reason you can't make sure that memory is
allocated in a compatible way right away...

Could we, please, discuss whether it is possible to have compatible
memory allocation from the very beginning? Question to libav guys: does
libav account that hardware usually requires continuous memory
allocations of the video memory? I.e. is there an libav API to handle
this?

Essentially to handle such a case negotiation stage between components
is needed. QSV in that case would inform other components that it
accepts only memory allocated in this particular way or that this is a
preferred option meaning that compatibility with non-continuous memory
will come with a price.

>              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);
> -            if (ret < 0)
> -                return ret;
> +            qf->frame->format = frame->format;
> +
> +            if (!qf->frame->data[0]) {
> +                ret = av_frame_get_buffer(qf->frame, q-
> >width_align);

I was expecting to see some flag like AV_MEMORY_CONTINOUS to make sure
you get really continuous memory or a call to custom memory allocation
function which will make sure of it. The above call looks to be general
libav call, thus, generally speaking no guarantee memory will be
continuous.

> +                if (ret < 0)
> +                    return ret;
> +            }
>  
>              qf->frame->height = frame->height;
>              qf->frame->width  = frame->width;
> +
>              ret = av_frame_copy(qf->frame, frame);
>              if (ret < 0) {
>                  av_frame_unref(qf->frame);
Maxym Dmytrychenko Aug. 5, 2018, 2:45 p.m. | #3
On Mon, Jul 30, 2018 at 6:15 PM Luca Barbato <lu_zero@gentoo.org> wrote:

> On 30/07/2018 18:02, Maxym Dmytrychenko wrote:
> > we need to make sure that memory allocation for Y/UV planes is
> continuous and re-used from a
> > pool
> > ---
>
> I'm afraid this would break the already-proper-frame codepath.
>
> I would simplify the default avcoded allocator and call it directly.
>
>
Note that:

+            if (!qf->frame->data[0]) {
+                ret = av_frame_get_buffer(qf->frame, q->width_align);

would keep the pool running and dont see it leaking, or?




>
> >  libavcodec/qsvenc.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> > index e349a075f..c74b3ae31 100644
> > --- a/libavcodec/qsvenc.c
> > +++ b/libavcodec/qsvenc.c
> > @@ -1014,7 +1014,6 @@ static void clear_unused_frames(QSVEncContext *q)
> >      QSVFrame *cur = q->work_frames;
> >      while (cur) {
> >          if (cur->used && !cur->surface.Data.Locked) {
> > -            av_frame_unref(cur->frame);
> >              cur->used = 0;
> >          }
> >          cur = cur->next;
> > @@ -1082,16 +1081,23 @@ static int submit_frame(QSVEncContext *q, const
> AVFrame *frame,
> >          }
> >      } else {
> >          /* make a copy if the input is not padded as libmfx requires */
> > -        if (frame->height & 31 || frame->linesize[0] & (q->width_align
> - 1)) {
> > +        /* and to make allocation continious for data[0]/data[1] */
> > +         if ((frame->height & 31 || frame->linesize[0] &
> (q->width_align - 1)) ||
> > +            (frame->data[1] - frame->data[0] != frame->linesize[0] *
> FFALIGN(qf->frame->height, q->height_align))) {
> >              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);
> > -            if (ret < 0)
> > -                return ret;
> > +            qf->frame->format = frame->format;
> > +
> > +            if (!qf->frame->data[0]) {
> > +                ret = av_frame_get_buffer(qf->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);
> >              if (ret < 0) {
> >                  av_frame_unref(qf->frame);
> >
>
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
Luca Barbato Aug. 6, 2018, 8:43 p.m. | #4
On 05/08/2018 16:45, Maxym Dmytrychenko wrote:
> Note that:
> 
> +            if (!qf->frame->data[0]) {
> +                ret = av_frame_get_buffer(qf->frame, q->width_align);
> 
> would keep the pool running and dont see it leaking, or?

You seem right, I guess the patch is in the good direction, reworking
the code to use the normal framepool might be a good idea though.

lu

Patch

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index e349a075f..c74b3ae31 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1014,7 +1014,6 @@  static void clear_unused_frames(QSVEncContext *q)
     QSVFrame *cur = q->work_frames;
     while (cur) {
         if (cur->used && !cur->surface.Data.Locked) {
-            av_frame_unref(cur->frame);
             cur->used = 0;
         }
         cur = cur->next;
@@ -1082,16 +1081,23 @@  static int submit_frame(QSVEncContext *q, const AVFrame *frame,
         }
     } else {
         /* make a copy if the input is not padded as libmfx requires */
-        if (frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) {
+        /* and to make allocation continious for data[0]/data[1] */
+         if ((frame->height & 31 || frame->linesize[0] & (q->width_align - 1)) ||
+            (frame->data[1] - frame->data[0] != frame->linesize[0] * FFALIGN(qf->frame->height, q->height_align))) {
             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);
-            if (ret < 0)
-                return ret;
+            qf->frame->format = frame->format;
+
+            if (!qf->frame->data[0]) {
+                ret = av_frame_get_buffer(qf->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);
             if (ret < 0) {
                 av_frame_unref(qf->frame);