Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Li, Zhong]

> From: libav-devel [mailto:libav-devel-boun...@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-boun...@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 +
> >> "Li, Zhong" <zhong...@intel.com> wrote:
> >>
> >>>> From: libav-devel [mailto:libav-devel-boun...@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.s...@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.s...@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

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Mark Thompson]

On 30/01/18 03:35, Song, Ruiling wrote:
>> -Original Message-
>> From: libav-devel [mailto:libav-devel-boun...@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 +
>> "Li, Zhong" <zhong...@intel.com> wrote:
>>
>>>> From: libav-devel [mailto:libav-devel-boun...@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.s...@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.s...@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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Luca Barbato]

On 30/01/2018 04:33, Song, Ruiling wrote:

-Original Message-
From: libav-devel [mailto:libav-devel-boun...@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

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Song, Ruiling]

> -Original Message-
> From: libav-devel [mailto:libav-devel-boun...@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 +
> "Li, Zhong" <zhong...@intel.com> wrote:
> 
> > > From: libav-devel [mailto:libav-devel-boun...@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.s...@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.s...@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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Song, Ruiling]

> -Original Message-
> From: libav-devel [mailto:libav-devel-boun...@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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Luca Barbato]

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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[wm4]

On Fri, 26 Jan 2018 11:04:12 +
Mark Thompson <s...@jkqxz.net> wrote:

> On 26/01/18 09:15, wm4 wrote:
> > On Fri, 26 Jan 2018 05:56:46 +
> > "Li, Zhong" <zhong...@intel.com> wrote:
> >   
> >>> From: libav-devel [mailto:libav-devel-boun...@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.s...@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.s...@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.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Mark Thompson]

On 26/01/18 09:15, wm4 wrote:
> On Fri, 26 Jan 2018 05:56:46 +
> "Li, Zhong" <zhong...@intel.com> wrote:
> 
>>> From: libav-devel [mailto:libav-devel-boun...@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.s...@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.s...@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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[wm4]

On Fri, 26 Jan 2018 05:56:46 +
"Li, Zhong" <zhong...@intel.com> wrote:

> > From: libav-devel [mailto:libav-devel-boun...@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.s...@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.s...@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.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Maxym Dmytrychenko]

+ 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  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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Luca Barbato]

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

Re: [libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Li, Zhong]

> From: libav-devel [mailto:libav-devel-boun...@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.s...@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.s...@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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH] hwcontext_qsv: Fix qsv hwupload failure issue

[Ruiling Song]

From: "Ruiling, Song" 

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 
---
 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");
-return AVERROR(EINVAL);
+ctx->initial_pool_size = 64;
 }
 
 s->surfaces_internal = av_mallocz_array(ctx->initial_pool_size,
-- 
2.7.4

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel