hwcontext_qsv: Fix qsv hwupload failure issue

Message ID 1516929418-4260-1-git-send-email-ruiling.song@intel.com
State New
Headers show
Series
  • hwcontext_qsv: Fix qsv hwupload failure issue
Related show

Commit Message

Ruiling Song Jan. 26, 2018, 1:16 a.m.
From: "Ruiling, Song" <ruiling.song@intel.com>

1.) the initial_pool_size need to be set instead of zero.
2.) the PicStruct is required by MediaSDK, so give a default value.

A simple command to reproduce the issue:
avconv -i INPUT -init_hw_device qsv=foo -filter_hw_device foo -vf format=nv12,hwupload -c:v h264_qsv ... OUTPUT

Signed-off-by: Ruiling Song <ruiling.song@intel.com>
---
 libavutil/hwcontext_qsv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Li, Zhong Jan. 26, 2018, 5:56 a.m. | #1
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> Ruiling Song
> Sent: Friday, January 26, 2018 9:17 AM
> To: libav-devel@libav.org
> Subject: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
> 
> From: "Ruiling, Song" <ruiling.song@intel.com>
> 
> 1.) the initial_pool_size need to be set instead of zero.
> 2.) the PicStruct is required by MediaSDK, so give a default value.
> 
> A simple command to reproduce the issue:
> avconv -i INPUT -init_hw_device qsv=foo -filter_hw_device foo -vf
> format=nv12,hwupload -c:v h264_qsv ... OUTPUT
> 
> Signed-off-by: Ruiling Song <ruiling.song@intel.com>
> ---
>  libavutil/hwcontext_qsv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c index
> 9270b22..14f26c6 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -313,6 +313,7 @@ static int qsv_init_surface(AVHWFramesContext *ctx,
> mfxFrameSurface1 *surf)
>      surf->Info.CropH          = ctx->height;
>      surf->Info.FrameRateExtN  = 25;
>      surf->Info.FrameRateExtD  = 1;
> +    surf->Info.PicStruct      = MFX_PICSTRUCT_PROGRESSIVE;
> 
>      return 0;
>  }
> @@ -325,8 +326,7 @@ static int qsv_init_pool(AVHWFramesContext *ctx,
> uint32_t fourcc)
>      int i, ret = 0;
> 
>      if (ctx->initial_pool_size <= 0) {
> -        av_log(ctx, AV_LOG_ERROR, "QSV requires a fixed frame pool
> size\n");

Should keep this log message as a warning be better? 

> -        return AVERROR(EINVAL);
> +        ctx->initial_pool_size = 64;
>      }
> 
>      s->surfaces_internal = av_mallocz_array(ctx->initial_pool_size,
> --
> 2.7.4
Luca Barbato Jan. 26, 2018, 8:42 a.m. | #2
On 26/01/2018 02:16, Ruiling Song wrote:
> +        ctx->initial_pool_size = 64;

Maybe 64 could be defined with a self descriptive name or you might 
write a comment on why 64 is picked.

The best solution would be adding the ability to query components before 
assembling the transcoding chain and get the minimum number of surfaces 
required before instantiating it, but it might take a little bit of time 
to appear...

lu
Maxym Dmytrychenko Jan. 26, 2018, 8:50 a.m. | #3
+ as sort of default value is 32

I would check why frames_ctx->initial_pool_size or NumFrameSuggested
 cannot be used

log message as a warning is a good idea.




On Fri, Jan 26, 2018 at 9:42 AM, Luca Barbato <lu_zero@gentoo.org> wrote:

> On 26/01/2018 02:16, Ruiling Song wrote:
>
>> +        ctx->initial_pool_size = 64;
>>
>
> Maybe 64 could be defined with a self descriptive name or you might write
> a comment on why 64 is picked.
>
> The best solution would be adding the ability to query components before
> assembling the transcoding chain and get the minimum number of surfaces
> required before instantiating it, but it might take a little bit of time to
> appear...
>
> lu
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
wm4 Jan. 26, 2018, 9:15 a.m. | #4
On Fri, 26 Jan 2018 05:56:46 +0000
"Li, Zhong" <zhong.li@intel.com> wrote:

> > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> > Ruiling Song
> > Sent: Friday, January 26, 2018 9:17 AM
> > To: libav-devel@libav.org
> > Subject: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
> > 
> > From: "Ruiling, Song" <ruiling.song@intel.com>
> > 
> > 1.) the initial_pool_size need to be set instead of zero.
> > 2.) the PicStruct is required by MediaSDK, so give a default value.
> > 
> > A simple command to reproduce the issue:
> > avconv -i INPUT -init_hw_device qsv=foo -filter_hw_device foo -vf
> > format=nv12,hwupload -c:v h264_qsv ... OUTPUT
> > 
> > Signed-off-by: Ruiling Song <ruiling.song@intel.com>
> > ---
> >  libavutil/hwcontext_qsv.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c index
> > 9270b22..14f26c6 100644
> > --- a/libavutil/hwcontext_qsv.c
> > +++ b/libavutil/hwcontext_qsv.c
> > @@ -313,6 +313,7 @@ static int qsv_init_surface(AVHWFramesContext *ctx,
> > mfxFrameSurface1 *surf)
> >      surf->Info.CropH          = ctx->height;
> >      surf->Info.FrameRateExtN  = 25;
> >      surf->Info.FrameRateExtD  = 1;
> > +    surf->Info.PicStruct      = MFX_PICSTRUCT_PROGRESSIVE;
> > 
> >      return 0;
> >  }
> > @@ -325,8 +326,7 @@ static int qsv_init_pool(AVHWFramesContext *ctx,
> > uint32_t fourcc)
> >      int i, ret = 0;
> > 
> >      if (ctx->initial_pool_size <= 0) {
> > -        av_log(ctx, AV_LOG_ERROR, "QSV requires a fixed frame pool
> > size\n");  
> 
> Should keep this log message as a warning be better? 
> 
> > -        return AVERROR(EINVAL);
> > +        ctx->initial_pool_size = 64;

That doesn't really feel appropriate. If a fixed size pool is required,
the user should simply be forced to specify a size. Making it use a
random value doesn't make too much sense to me, and the required size
is going to depend on the caller's use cases. In addition 64 by default
sounds like a huge waste of memory.
Mark Thompson Jan. 26, 2018, 11:04 a.m. | #5
On 26/01/18 09:15, wm4 wrote:
> On Fri, 26 Jan 2018 05:56:46 +0000
> "Li, Zhong" <zhong.li@intel.com> wrote:
> 
>>> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
>>> Ruiling Song
>>> Sent: Friday, January 26, 2018 9:17 AM
>>> To: libav-devel@libav.org
>>> Subject: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
>>>
>>> From: "Ruiling, Song" <ruiling.song@intel.com>
>>>
>>> 1.) the initial_pool_size need to be set instead of zero.
>>> 2.) the PicStruct is required by MediaSDK, so give a default value.
>>>
>>> A simple command to reproduce the issue:
>>> avconv -i INPUT -init_hw_device qsv=foo -filter_hw_device foo -vf
>>> format=nv12,hwupload -c:v h264_qsv ... OUTPUT
>>>
>>> Signed-off-by: Ruiling Song <ruiling.song@intel.com>
>>> ---
>>>  libavutil/hwcontext_qsv.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c index
>>> 9270b22..14f26c6 100644
>>> --- a/libavutil/hwcontext_qsv.c
>>> +++ b/libavutil/hwcontext_qsv.c
>>> @@ -313,6 +313,7 @@ static int qsv_init_surface(AVHWFramesContext *ctx,
>>> mfxFrameSurface1 *surf)
>>>      surf->Info.CropH          = ctx->height;
>>>      surf->Info.FrameRateExtN  = 25;
>>>      surf->Info.FrameRateExtD  = 1;
>>> +    surf->Info.PicStruct      = MFX_PICSTRUCT_PROGRESSIVE;
>>>
>>>      return 0;
>>>  }
>>> @@ -325,8 +326,7 @@ static int qsv_init_pool(AVHWFramesContext *ctx,
>>> uint32_t fourcc)
>>>      int i, ret = 0;
>>>
>>>      if (ctx->initial_pool_size <= 0) {
>>> -        av_log(ctx, AV_LOG_ERROR, "QSV requires a fixed frame pool
>>> size\n");  
>>
>> Should keep this log message as a warning be better? 
>>
>>> -        return AVERROR(EINVAL);
>>> +        ctx->initial_pool_size = 64;
> 
> That doesn't really feel appropriate. If a fixed size pool is required,
> the user should simply be forced to specify a size. Making it use a
> random value doesn't make too much sense to me, and the required size
> is going to depend on the caller's use cases. In addition 64 by default
> sounds like a huge waste of memory.

Agree.

Maybe it would be a good idea to resurrect the set at <https://lists.libav.org/pipermail/libav-devel/2017-September/084776.html>, in particular there is <https://lists.libav.org/pipermail/libav-devel/2017-September/084778.html> for exactly this case.  I don't know if we want to modify how this works to be more like what was used in libavcodec, though.

Thoughts?  I can send the set again if that would be helpful.

- Mark
wm4 Jan. 26, 2018, 11:20 a.m. | #6
On Fri, 26 Jan 2018 11:04:12 +0000
Mark Thompson <sw@jkqxz.net> wrote:

> On 26/01/18 09:15, wm4 wrote:
> > On Fri, 26 Jan 2018 05:56:46 +0000
> > "Li, Zhong" <zhong.li@intel.com> wrote:
> >   
> >>> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> >>> Ruiling Song
> >>> Sent: Friday, January 26, 2018 9:17 AM
> >>> To: libav-devel@libav.org
> >>> Subject: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
> >>>
> >>> From: "Ruiling, Song" <ruiling.song@intel.com>
> >>>
> >>> 1.) the initial_pool_size need to be set instead of zero.
> >>> 2.) the PicStruct is required by MediaSDK, so give a default value.
> >>>
> >>> A simple command to reproduce the issue:
> >>> avconv -i INPUT -init_hw_device qsv=foo -filter_hw_device foo -vf
> >>> format=nv12,hwupload -c:v h264_qsv ... OUTPUT
> >>>
> >>> Signed-off-by: Ruiling Song <ruiling.song@intel.com>
> >>> ---
> >>>  libavutil/hwcontext_qsv.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c index
> >>> 9270b22..14f26c6 100644
> >>> --- a/libavutil/hwcontext_qsv.c
> >>> +++ b/libavutil/hwcontext_qsv.c
> >>> @@ -313,6 +313,7 @@ static int qsv_init_surface(AVHWFramesContext *ctx,
> >>> mfxFrameSurface1 *surf)
> >>>      surf->Info.CropH          = ctx->height;
> >>>      surf->Info.FrameRateExtN  = 25;
> >>>      surf->Info.FrameRateExtD  = 1;
> >>> +    surf->Info.PicStruct      = MFX_PICSTRUCT_PROGRESSIVE;
> >>>
> >>>      return 0;
> >>>  }
> >>> @@ -325,8 +326,7 @@ static int qsv_init_pool(AVHWFramesContext *ctx,
> >>> uint32_t fourcc)
> >>>      int i, ret = 0;
> >>>
> >>>      if (ctx->initial_pool_size <= 0) {
> >>> -        av_log(ctx, AV_LOG_ERROR, "QSV requires a fixed frame pool
> >>> size\n");    
> >>
> >> Should keep this log message as a warning be better? 
> >>  
> >>> -        return AVERROR(EINVAL);
> >>> +        ctx->initial_pool_size = 64;  
> > 
> > That doesn't really feel appropriate. If a fixed size pool is required,
> > the user should simply be forced to specify a size. Making it use a
> > random value doesn't make too much sense to me, and the required size
> > is going to depend on the caller's use cases. In addition 64 by default
> > sounds like a huge waste of memory.  
> 
> Agree.
> 
> Maybe it would be a good idea to resurrect the set at <https://lists.libav.org/pipermail/libav-devel/2017-September/084776.html>, in particular there is <https://lists.libav.org/pipermail/libav-devel/2017-September/084778.html> for exactly this case.  I don't know if we want to modify how this works to be more like what was used in libavcodec, though.
> 
> Thoughts?  I can send the set again if that would be helpful.

That sounds generally like a good idea, although I'm not sure how
exactly the details should work.

In theory, it would be nice if libavfilter would have an automagic way
to figure out the optimal pool sizes (and it'd have to involve decoder
and encoder somehow), but it doesn't look like we'll get such a thing
any time soon. So manual configuration it is, which should be still
better than putting random sizes into the code to evade the problem.
Luca Barbato Jan. 26, 2018, 1:26 p.m. | #7
On 26/01/2018 12:04, Mark Thompson wrote:
> Thoughts?  I can send the set again if that would be helpful.

Please do :)

I'm still afraid that we should add yet another call to query the number 
of surfaces needed by each component and then call the setup with the 
actual value once we have it...

lu
Ruiling Song Jan. 30, 2018, 3:33 a.m. | #8
> -----Original Message-----
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of Luca
> Barbato
> Sent: Friday, January 26, 2018 9:26 PM
> To: libav-devel@libav.org
> Subject: Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
> 
> On 26/01/2018 12:04, Mark Thompson wrote:
> > Thoughts?  I can send the set again if that would be helpful.
> 
> Please do :)
> 
> I'm still afraid that we should add yet another call to query the number
> of surfaces needed by each component and then call the setup with the
> actual value once we have it...
It sounds like a good idea. But I don't know too much about Libav internals.
So I am not sure if you can share more idea on this. How far we are from it?
And what kind of problems that need to be addressed before we arrive at that situation?

Ruiling
> 
> lu
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
Ruiling Song Jan. 30, 2018, 3:35 a.m. | #9
> -----Original Message-----
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of wm4
> Sent: Friday, January 26, 2018 5:15 PM
> To: libav-devel@libav.org
> Subject: Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
> 
> On Fri, 26 Jan 2018 05:56:46 +0000
> "Li, Zhong" <zhong.li@intel.com> wrote:
> 
> > > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> > > Ruiling Song
> > > Sent: Friday, January 26, 2018 9:17 AM
> > > To: libav-devel@libav.org
> > > Subject: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
> > >
> > > From: "Ruiling, Song" <ruiling.song@intel.com>
> > >
> > > 1.) the initial_pool_size need to be set instead of zero.
> > > 2.) the PicStruct is required by MediaSDK, so give a default value.
> > >
> > > A simple command to reproduce the issue:
> > > avconv -i INPUT -init_hw_device qsv=foo -filter_hw_device foo -vf
> > > format=nv12,hwupload -c:v h264_qsv ... OUTPUT
> > >
> > > Signed-off-by: Ruiling Song <ruiling.song@intel.com>
> > > ---
> > >  libavutil/hwcontext_qsv.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c index
> > > 9270b22..14f26c6 100644
> > > --- a/libavutil/hwcontext_qsv.c
> > > +++ b/libavutil/hwcontext_qsv.c
> > > @@ -313,6 +313,7 @@ static int qsv_init_surface(AVHWFramesContext *ctx,
> > > mfxFrameSurface1 *surf)
> > >      surf->Info.CropH          = ctx->height;
> > >      surf->Info.FrameRateExtN  = 25;
> > >      surf->Info.FrameRateExtD  = 1;
> > > +    surf->Info.PicStruct      = MFX_PICSTRUCT_PROGRESSIVE;
> > >
> > >      return 0;
> > >  }
> > > @@ -325,8 +326,7 @@ static int qsv_init_pool(AVHWFramesContext *ctx,
> > > uint32_t fourcc)
> > >      int i, ret = 0;
> > >
> > >      if (ctx->initial_pool_size <= 0) {
> > > -        av_log(ctx, AV_LOG_ERROR, "QSV requires a fixed frame pool
> > > size\n");
> >
> > Should keep this log message as a warning be better?
> >
> > > -        return AVERROR(EINVAL);
> > > +        ctx->initial_pool_size = 64;
> 
> That doesn't really feel appropriate. If a fixed size pool is required,
> the user should simply be forced to specify a size. Making it use a
> random value doesn't make too much sense to me, and the required size
> is going to depend on the caller's use cases. In addition 64 by default
> sounds like a huge waste of memory.

Thanks for your comment!
But I think it is better if we don't force the user to explicitly setup a value to get it work.
Less parameters means less burden for end-users. If this rule is not applicable here, please correct me.
I am not sure why a default workable value is not as good here. Could you share me more thought?
And there are more places that set default values to initial_pool_size:
Inside libavfilter/qsvvpp.c it also sets initial_pool_size to 64.
Inside avtools/avcov_qsv.c, it sets initial_pool_size to 32.
I am not sure do you have any comment on this? Will be 32 looks a little better?

Ruiling

> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
Luca Barbato Jan. 30, 2018, 5:01 a.m. | #10
On 30/01/2018 04:33, Song, Ruiling wrote:
>> -----Original Message-----
>> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of Luca
>> Barbato
>> Sent: Friday, January 26, 2018 9:26 PM
>> To: libav-devel@libav.org
>> Subject: Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
>>
>> On 26/01/2018 12:04, Mark Thompson wrote:
>>> Thoughts?  I can send the set again if that would be helpful.
>>
>> Please do :)
>>
>> I'm still afraid that we should add yet another call to query the number
>> of surfaces needed by each component and then call the setup with the
>> actual value once we have it...
> It sounds like a good idea. But I don't know too much about Libav internals.
> So I am not sure if you can share more idea on this. How far we are from it?
> And what kind of problems that need to be addressed before we arrive at that situation?

Mark last set should address most of it :). (Thank you Mark!)

Testing it is more than welcome.

lu
Mark Thompson Jan. 30, 2018, 10:16 p.m. | #11
On 30/01/18 03:35, Song, Ruiling wrote:
>> -----Original Message-----
>> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of wm4
>> Sent: Friday, January 26, 2018 5:15 PM
>> To: libav-devel@libav.org
>> Subject: Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
>>
>> On Fri, 26 Jan 2018 05:56:46 +0000
>> "Li, Zhong" <zhong.li@intel.com> wrote:
>>
>>>> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
>>>> Ruiling Song
>>>> Sent: Friday, January 26, 2018 9:17 AM
>>>> To: libav-devel@libav.org
>>>> Subject: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue
>>>>
>>>> From: "Ruiling, Song" <ruiling.song@intel.com>
>>>>
>>>> 1.) the initial_pool_size need to be set instead of zero.
>>>> 2.) the PicStruct is required by MediaSDK, so give a default value.
>>>>
>>>> A simple command to reproduce the issue:
>>>> avconv -i INPUT -init_hw_device qsv=foo -filter_hw_device foo -vf
>>>> format=nv12,hwupload -c:v h264_qsv ... OUTPUT
>>>>
>>>> Signed-off-by: Ruiling Song <ruiling.song@intel.com>
>>>> ---
>>>>  libavutil/hwcontext_qsv.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c index
>>>> 9270b22..14f26c6 100644
>>>> --- a/libavutil/hwcontext_qsv.c
>>>> +++ b/libavutil/hwcontext_qsv.c
>>>> @@ -313,6 +313,7 @@ static int qsv_init_surface(AVHWFramesContext *ctx,
>>>> mfxFrameSurface1 *surf)
>>>>      surf->Info.CropH          = ctx->height;
>>>>      surf->Info.FrameRateExtN  = 25;
>>>>      surf->Info.FrameRateExtD  = 1;
>>>> +    surf->Info.PicStruct      = MFX_PICSTRUCT_PROGRESSIVE;
>>>>
>>>>      return 0;
>>>>  }
>>>> @@ -325,8 +326,7 @@ static int qsv_init_pool(AVHWFramesContext *ctx,
>>>> uint32_t fourcc)
>>>>      int i, ret = 0;
>>>>
>>>>      if (ctx->initial_pool_size <= 0) {
>>>> -        av_log(ctx, AV_LOG_ERROR, "QSV requires a fixed frame pool
>>>> size\n");
>>>
>>> Should keep this log message as a warning be better?
>>>
>>>> -        return AVERROR(EINVAL);
>>>> +        ctx->initial_pool_size = 64;
>>
>> That doesn't really feel appropriate. If a fixed size pool is required,
>> the user should simply be forced to specify a size. Making it use a
>> random value doesn't make too much sense to me, and the required size
>> is going to depend on the caller's use cases. In addition 64 by default
>> sounds like a huge waste of memory.
> 
> Thanks for your comment!
> But I think it is better if we don't force the user to explicitly setup a value to get it work.
> Less parameters means less burden for end-users. If this rule is not applicable here, please correct me.
> I am not sure why a default workable value is not as good here. Could you share me more thought?
> And there are more places that set default values to initial_pool_size:
> Inside libavfilter/qsvvpp.c it also sets initial_pool_size to 64.
> Inside avtools/avcov_qsv.c, it sets initial_pool_size to 32.
> I am not sure do you have any comment on this? Will be 32 looks a little better?

IMO any fixed number is a problem.  The user can always break it by holding on to more frames - the lookahead option in the libmfx encoder is the easiest way to eat lots of frames, but there is nothing stopping the user just not giving the frames back for a long time.  The advantage of the extra_hw_frames option is that it actually codifies how many frames the user is allowed to hold, so that if they do hold more it is clear where the error is rather than the answer being "um, that doesn't work, sorry".

It is unclear what the default should be before that number is applied, but probably not something particularly large because it is very memory-wasteful if you have 64 surfaces and only ever use 2 (currently a common problem when chaining filters together - it's only the encoder which wants many).  Obviously that can all solved by some sort of magic autonegotiation, but noone has yet offered a plan of how that could work.

- Mark
Li, Zhong Jan. 31, 2018, 9:19 a.m. | #12
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of Mark
> Thompson
> Sent: Wednesday, January 31, 2018 6:17 AM
> To: libav-devel@libav.org
> Subject: Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure
> issue
> 
> On 30/01/18 03:35, Song, Ruiling wrote:
> >> -----Original Message-----
> >> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> >> wm4
> >> Sent: Friday, January 26, 2018 5:15 PM
> >> To: libav-devel@libav.org
> >> Subject: Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload
> >> failure issue
> >>
> >> On Fri, 26 Jan 2018 05:56:46 +0000
> >> "Li, Zhong" <zhong.li@intel.com> wrote:
> >>
> >>>> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf
> >>>> Of Ruiling Song
> >>>> Sent: Friday, January 26, 2018 9:17 AM
> >>>> To: libav-devel@libav.org
> >>>> Subject: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload
> >>>> failure issue
> >>>>
> >>>> From: "Ruiling, Song" <ruiling.song@intel.com>
> >>>>
> >>>> 1.) the initial_pool_size need to be set instead of zero.
> >>>> 2.) the PicStruct is required by MediaSDK, so give a default value.
> >>>>
> >>>> A simple command to reproduce the issue:
> >>>> avconv -i INPUT -init_hw_device qsv=foo -filter_hw_device foo -vf
> >>>> format=nv12,hwupload -c:v h264_qsv ... OUTPUT
> >>>>
> >>>> Signed-off-by: Ruiling Song <ruiling.song@intel.com>
> >>>> ---
> >>>>  libavutil/hwcontext_qsv.c | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> >>>> index
> >>>> 9270b22..14f26c6 100644
> >>>> --- a/libavutil/hwcontext_qsv.c
> >>>> +++ b/libavutil/hwcontext_qsv.c
> >>>> @@ -313,6 +313,7 @@ static int
> qsv_init_surface(AVHWFramesContext
> >>>> *ctx,
> >>>> mfxFrameSurface1 *surf)
> >>>>      surf->Info.CropH          = ctx->height;
> >>>>      surf->Info.FrameRateExtN  = 25;
> >>>>      surf->Info.FrameRateExtD  = 1;
> >>>> +    surf->Info.PicStruct      = MFX_PICSTRUCT_PROGRESSIVE;
> >>>>
> >>>>      return 0;
> >>>>  }
> >>>> @@ -325,8 +326,7 @@ static int qsv_init_pool(AVHWFramesContext
> >>>> *ctx, uint32_t fourcc)
> >>>>      int i, ret = 0;
> >>>>
> >>>>      if (ctx->initial_pool_size <= 0) {
> >>>> -        av_log(ctx, AV_LOG_ERROR, "QSV requires a fixed frame
> pool
> >>>> size\n");
> >>>
> >>> Should keep this log message as a warning be better?
> >>>
> >>>> -        return AVERROR(EINVAL);
> >>>> +        ctx->initial_pool_size = 64;
> >>
> >> That doesn't really feel appropriate. If a fixed size pool is
> >> required, the user should simply be forced to specify a size. Making
> >> it use a random value doesn't make too much sense to me, and the
> >> required size is going to depend on the caller's use cases. In
> >> addition 64 by default sounds like a huge waste of memory.
> >
> > Thanks for your comment!
> > But I think it is better if we don't force the user to explicitly setup a value to
> get it work.
> > Less parameters means less burden for end-users. If this rule is not
> applicable here, please correct me.
> > I am not sure why a default workable value is not as good here. Could you
> share me more thought?
> > And there are more places that set default values to initial_pool_size:
> > Inside libavfilter/qsvvpp.c it also sets initial_pool_size to 64.
> > Inside avtools/avcov_qsv.c, it sets initial_pool_size to 32.
> > I am not sure do you have any comment on this? Will be 32 looks a little
> better?
> 
> IMO any fixed number is a problem.  The user can always break it by
> holding on to more frames - the lookahead option in the libmfx encoder is
> the easiest way to eat lots of frames, but there is nothing stopping the user
> just not giving the frames back for a long time.  The advantage of the
> extra_hw_frames option is that it actually codifies how many frames the
> user is allowed to hold, so that if they do hold more it is clear where the
> error is rather than the answer being "um, that doesn't work, sorry".
> 
> It is unclear what the default should be before that number is applied, but
> probably not something particularly large because it is very
> memory-wasteful if you have 64 surfaces and only ever use 2 (currently a
> common problem when chaining filters together - it's only the encoder
> which wants many).  Obviously that can all solved by some sort of magic
> autonegotiation, but noone has yet offered a plan of how that could work.
> 
> - Mark

I think it is a good idea to add the extra_hw_frames option to make it more flexible.
But I see default value is -1. IMHO it is equal to forcing user to set this option else it will fail. It becomes a burden for user and they have to know what's the exact number of extra_hw_frames should be set.
So I think it is a good idea to set a default value (though I also don't know why it is 64) when user hasn't set it.
Yes it will waste memory but IMHO it is not a big problem because it only takes effect when extra_hw_frames is unset and better than pipeline crash.

Patch

diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index 9270b22..14f26c6 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -313,6 +313,7 @@  static int qsv_init_surface(AVHWFramesContext *ctx, mfxFrameSurface1 *surf)
     surf->Info.CropH          = ctx->height;
     surf->Info.FrameRateExtN  = 25;
     surf->Info.FrameRateExtD  = 1;
+    surf->Info.PicStruct      = MFX_PICSTRUCT_PROGRESSIVE;
 
     return 0;
 }
@@ -325,8 +326,7 @@  static int qsv_init_pool(AVHWFramesContext *ctx, uint32_t fourcc)
     int i, ret = 0;
 
     if (ctx->initial_pool_size <= 0) {
-        av_log(ctx, AV_LOG_ERROR, "QSV requires a fixed frame pool size\n");
-        return AVERROR(EINVAL);
+        ctx->initial_pool_size = 64;
     }
 
     s->surfaces_internal = av_mallocz_array(ctx->initial_pool_size,