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