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