qsvenc: AV_PIX_FMT_QSV path should release frame

diff mbox series

Fixes high memory usage, now is back to normal.

Can be checked as:
-hwaccel qsv -c:v h264_qsv -i ../h264-conformance/CANL2_Sony_E.jsv -c:v
h264_qsv -b:v 2000k -y qsv.mp4
---
 libavcodec/qsvenc.c | 3 +++
 1 file changed, 3 insertions(+)

On 18/09/2018 09:54, Maxym Dmytrychenko wrote:
> Fixes high memory usage, now is back to normal.
> 
> Can be checked as:
> -hwaccel qsv -c:v h264_qsv -i ../h264-conformance/CANL2_Sony_E.jsv -c:v
> h264_qsv -b:v 2000k -y qsv.mp4
> ---
>  libavcodec/qsvenc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index 611449cbe..17a0559f3 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1028,6 +1028,9 @@ static void clear_unused_frames(QSVEncContext *q)
>      QSVFrame *cur = q->work_frames;
>      while (cur) {
>          if (cur->used && !cur->surface.Data.Locked) {
> +            if (cur->frame->format == AV_PIX_FMT_QSV) {
> +                av_frame_unref(cur->frame);
> +            }
>              cur->used = 0;
>          }
>          cur = cur->next;
> 

Please put a note about why this is needed (and safe to do).

lu

> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of Luca
> Barbato
> Sent: Wednesday, September 19, 2018 12:11 AM
> To: libav-devel@libav.org
> Subject: Re: [libav-devel] [PATCH] qsvenc: AV_PIX_FMT_QSV path should
> release frame
> 
> On 18/09/2018 09:54, Maxym Dmytrychenko wrote:
> > Fixes high memory usage, now is back to normal.
> >
> > Can be checked as:
> > -hwaccel qsv -c:v h264_qsv -i ../h264-conformance/CANL2_Sony_E.jsv
> > -c:v h264_qsv -b:v 2000k -y qsv.mp4
> > ---
> >  libavcodec/qsvenc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index
> > 611449cbe..17a0559f3 100644
> > --- a/libavcodec/qsvenc.c
> > +++ b/libavcodec/qsvenc.c
> > @@ -1028,6 +1028,9 @@ static void
> clear_unused_frames(QSVEncContext *q)
> >      QSVFrame *cur = q->work_frames;
> >      while (cur) {
> >          if (cur->used && !cur->surface.Data.Locked) {
> > +            if (cur->frame->format == AV_PIX_FMT_QSV) {
> > +                av_frame_unref(cur->frame);
> > +            }
> >              cur->used = 0;
> >          }
> >          cur = cur->next;
> >
> 
> Please put a note about why this is needed (and safe to do).
> 
> lu

I am still confused by the original commit (https://patchwork.libav.org/patch/64323/ ) which introduce the regression:
         if (cur->used && !cur->surface.Data.Locked) {
-            av_frame_unref(cur->frame);
             cur->used = 0;
         }
It made sense for qsv decoding + qsv encoding pipeline(both GPU memory mode, thus format = AV_PIX_FMT_QSV, and system memory mode, thus format != AV_PIX_FMT_QSV). 
Because if it is unlocked, qsv decoder doesn't need this surface any more, then we can release it. 
Removing "av_frame_unref(cur->frame)" is breaking qsv decoding + qsv encoding pipeline. The patch above just fix GPU memory mode case, but doesn’t work for system mode case of qsv decoding+ qsv encoding.

On Tue, Sep 18, 2018 at 6:45 PM Li, Zhong <zhong.li@intel.com> wrote:

> > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> Luca
> > Barbato
> > Sent: Wednesday, September 19, 2018 12:11 AM
> > To: libav-devel@libav.org
> > Subject: Re: [libav-devel] [PATCH] qsvenc: AV_PIX_FMT_QSV path should
> > release frame
> >
> > On 18/09/2018 09:54, Maxym Dmytrychenko wrote:
> > > Fixes high memory usage, now is back to normal.
> > >
> > > Can be checked as:
> > > -hwaccel qsv -c:v h264_qsv -i ../h264-conformance/CANL2_Sony_E.jsv
> > > -c:v h264_qsv -b:v 2000k -y qsv.mp4
> > > ---
> > >  libavcodec/qsvenc.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index
> > > 611449cbe..17a0559f3 100644
> > > --- a/libavcodec/qsvenc.c
> > > +++ b/libavcodec/qsvenc.c
> > > @@ -1028,6 +1028,9 @@ static void
> > clear_unused_frames(QSVEncContext *q)
> > >      QSVFrame *cur = q->work_frames;
> > >      while (cur) {
> > >          if (cur->used && !cur->surface.Data.Locked) {
> > > +            if (cur->frame->format == AV_PIX_FMT_QSV) {
> > > +                av_frame_unref(cur->frame);
> > > +            }
> > >              cur->used = 0;
> > >          }
> > >          cur = cur->next;
> > >
> >
> > Please put a note about why this is needed (and safe to do).
> >
> > lu
>
> I am still confused by the original commit (
> https://patchwork.libav.org/patch/64323/ ) which introduce the regression:
>          if (cur->used && !cur->surface.Data.Locked) {
> -            av_frame_unref(cur->frame);
>              cur->used = 0;
>          }
> It made sense for qsv decoding + qsv encoding pipeline(both GPU memory
> mode, thus format = AV_PIX_FMT_QSV, and system memory mode, thus format !=
> AV_PIX_FMT_QSV).
> Because if it is unlocked, qsv decoder doesn't need this surface any more,
> then we can release it.
> Removing "av_frame_unref(cur->frame)" is breaking qsv decoding + qsv
> encoding pipeline. The patch above just fix GPU memory mode case, but
> doesn’t work for system mode case of qsv decoding+ qsv encoding.
>
>
>
should be fixed by now for any case, or ?

regards
Max

On Tue, 2018-09-18 at 16:45 +0000, Li, Zhong wrote:
> > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf
> > Of Luca
> > Barbato
> > Sent: Wednesday, September 19, 2018 12:11 AM
> > To: libav-devel@libav.org
> > Subject: Re: [libav-devel] [PATCH] qsvenc: AV_PIX_FMT_QSV path
> > should
> > release frame
> > 
> > On 18/09/2018 09:54, Maxym Dmytrychenko wrote:
> > > Fixes high memory usage, now is back to normal.
> > > 
> > > Can be checked as:
> > > -hwaccel qsv -c:v h264_qsv -i ../h264-
> > > conformance/CANL2_Sony_E.jsv
> > > -c:v h264_qsv -b:v 2000k -y qsv.mp4
> > > ---
> > >  libavcodec/qsvenc.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index
> > > 611449cbe..17a0559f3 100644
> > > --- a/libavcodec/qsvenc.c
> > > +++ b/libavcodec/qsvenc.c
> > > @@ -1028,6 +1028,9 @@ static void
> > 
> > clear_unused_frames(QSVEncContext *q)
> > >      QSVFrame *cur = q->work_frames;
> > >      while (cur) {
> > >          if (cur->used && !cur->surface.Data.Locked) {
> > > +            if (cur->frame->format == AV_PIX_FMT_QSV) {
> > > +                av_frame_unref(cur->frame);
> > > +            }
> > >              cur->used = 0;
> > >          }
> > >          cur = cur->next;
> > > 
> > 
> > Please put a note about why this is needed (and safe to do).
> > 
> > lu
> 
> I am still confused by the original commit
> (https://patchwork.libav.org/patch/64323/ ) which introduce the
> regression:
Since this is a regression and the commit which introduced it is
identified, I suggest to explicitly mark it in commit message. For
example with:

Fixes: "qsv: enforcing continuous memory layout"

>          if (cur->used && !cur->surface.Data.Locked) {
> -            av_frame_unref(cur->frame);
>              cur->used = 0;
>          }
> It made sense for qsv decoding + qsv encoding pipeline(both GPU
> memory mode, thus format = AV_PIX_FMT_QSV, and system memory mode,
> thus format != AV_PIX_FMT_QSV). 
> Because if it is unlocked, qsv decoder doesn't need this surface any
> more, then we can release it. 
> Removing "av_frame_unref(cur->frame)" is breaking qsv decoding + qsv
> encoding pipeline.
This is not reflected in the commit message which just claims that
there is "a high memory usage". But what I see is transcoding don't
work returning errors. I believe this should be highlighted int he
commit message.

>  The patch above just fix GPU memory mode case, but doesn’t work for
> system mode case of qsv decoding+ qsv encoding. 
> 
> 
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel

On Tue, 2018-09-18 at 20:37 +0200, Maxym Dmytrychenko wrote:
> On Tue, Sep 18, 2018 at 6:45 PM Li, Zhong <zhong.li@intel.com> wrote:
> 
> > > From: libav-devel [mailto:libav-devel-bounces@libav.org] On
> > > Behalf Of
> > 
> > Luca
> > > Barbato
> > > Sent: Wednesday, September 19, 2018 12:11 AM
> > > To: libav-devel@libav.org
> > > Subject: Re: [libav-devel] [PATCH] qsvenc: AV_PIX_FMT_QSV path
> > > should
> > > release frame
> > > 
> > > On 18/09/2018 09:54, Maxym Dmytrychenko wrote:
> > > > Fixes high memory usage, now is back to normal.
> > > > 
> > > > Can be checked as:
> > > > -hwaccel qsv -c:v h264_qsv -i ../h264-
> > > > conformance/CANL2_Sony_E.jsv
> > > > -c:v h264_qsv -b:v 2000k -y qsv.mp4
> > > > ---
> > > >  libavcodec/qsvenc.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index
> > > > 611449cbe..17a0559f3 100644
> > > > --- a/libavcodec/qsvenc.c
> > > > +++ b/libavcodec/qsvenc.c
> > > > @@ -1028,6 +1028,9 @@ static void
> > > 
> > > clear_unused_frames(QSVEncContext *q)
> > > >      QSVFrame *cur = q->work_frames;
> > > >      while (cur) {
> > > >          if (cur->used && !cur->surface.Data.Locked) {
> > > > +            if (cur->frame->format == AV_PIX_FMT_QSV) {
> > > > +                av_frame_unref(cur->frame);

It seems that you are trying to reuse the memory which you have
allocated to story a properly aligned incoming frame. However, just
reviewing the code you should do this if and only if you _have_
allocated this additional memory. If you did not allocate it, then code
should remain the same, i.e. have this unref.

The problem is (and I think that Zhong points this out as well) that
the condition when you allocate the memory does not match the condition
when you avoid a call to unref.


Maybe you will simply introduce some flag and check it? For example, 

if (cur->used && !cur->surface.Data.Locked) {
            if (!cur->was_copied) {
                av_frame_unref(cur->frame);
...

and when you created a copy:
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))) {

cur->was_copied = 1;


> > > > +            }
> > > >              cur->used = 0;
> > > >          }
> > > >          cur = cur->next;
> > > > 
> > > 
> > > Please put a note about why this is needed (and safe to do).
> > > 
> > > lu
> > 
> > I am still confused by the original commit (
> > https://patchwork.libav.org/patch/64323/ ) which introduce the
> > regression:
> >          if (cur->used && !cur->surface.Data.Locked) {
> > -            av_frame_unref(cur->frame);
> >              cur->used = 0;
> >          }
> > It made sense for qsv decoding + qsv encoding pipeline(both GPU
> > memory
> > mode, thus format = AV_PIX_FMT_QSV, and system memory mode, thus
> > format !=
> > AV_PIX_FMT_QSV).
> > Because if it is unlocked, qsv decoder doesn't need this surface
> > any more,
> > then we can release it.
> > Removing "av_frame_unref(cur->frame)" is breaking qsv decoding +
> > qsv
> > encoding pipeline. The patch above just fix GPU memory mode case,
> > but
> > doesn’t work for system mode case of qsv decoding+ qsv encoding.
> > 
> > 
> > 
> 
> should be fixed by now for any case, or ?

Which command lines we need to verify? Can someone list them? Looks
like we need
1. Transcoding pipeline which works on video memory between decoder and
encoder
2. Encoding pipeline which accepts system memory in 2 variants:
2.a) Correctly aligned
2.b) Incorrectly aligned, i.e. which requires copying of the frame.

Unfortunately I am not too familiar with avconv options to judge
correctness of those command lines which I will compose myself. Thus, I
would prefer someone else to list them here.

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