| Message ID | 7cc9c3fc-d8c6-6345-b358-ef3db1215dc6@jkqxz.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] vaapi_decode: Improve logging around codec/profile selection | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Mark Thompson > Sent: Sunday, April 12, 2020 21:00 > To: ffmpeg-devel@ffmpeg.org > Subject: [FFmpeg-devel] [PATCH] vaapi_decode: Improve logging around > codec/profile selection > > --- > On 12/04/2020 13:14, Mark Thompson wrote: > > ... This does rather suggest that the error messages in that file should be > clearer, though - it would be nice if it could distinguish between "this codec > isn't supported by libavcodec at all", "this codec might work but hasn't built > into this libavcodec" and "this codec is supported by libavcodec but not by > your hardware". > > Something like this? > > > libavcodec/vaapi_decode.c | 39 +++++++++++++++++++++++++++++++---- > ---- > 1 file changed, 31 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c > index 54a0ecb47a..a191850e36 100644 > --- a/libavcodec/vaapi_decode.c > +++ b/libavcodec/vaapi_decode.c > @@ -429,6 +429,7 @@ static int > vaapi_decode_make_config(AVCodecContext *avctx, > const AVCodecDescriptor *codec_desc; > VAProfile *profile_list = NULL, matched_va_profile, va_profile; > int profile_count, exact_match, matched_ff_profile, codec_profile; > + int found_codec, found_profile; > > AVHWDeviceContext *device = (AVHWDeviceContext*)device_ref- > >data; > AVVAAPIDeviceContext *hwctx = device->hwctx; > @@ -457,15 +458,19 @@ static int > vaapi_decode_make_config(AVCodecContext *avctx, > } > > matched_va_profile = VAProfileNone; > + found_codec = found_profile = 0; > exact_match = 0; > > for (i = 0; i < FF_ARRAY_ELEMS(vaapi_profile_map); i++) { > int profile_match = 0; > if (avctx->codec_id != vaapi_profile_map[i].codec_id) > continue; > + found_codec = 1; > if (avctx->profile == vaapi_profile_map[i].codec_profile || > - vaapi_profile_map[i].codec_profile == FF_PROFILE_UNKNOWN) > + vaapi_profile_map[i].codec_profile == FF_PROFILE_UNKNOWN) { > profile_match = 1; > + found_profile = 1; > + } > > va_profile = vaapi_profile_map[i].profile_parser ? > vaapi_profile_map[i].profile_parser(avctx) : > @@ -487,24 +492,42 @@ static int > vaapi_decode_make_config(AVCodecContext *avctx, > } > av_freep(&profile_list); > > - if (matched_va_profile == VAProfileNone) { > - av_log(avctx, AV_LOG_ERROR, "No support for codec %s " > - "profile %d.\n", codec_desc->name, avctx->profile); > + if (!found_codec) { > + av_log(avctx, AV_LOG_ERROR, "This libavcodec build does not " > + "support VAAPI decoding of codec %s.\n", > + codec_desc->name); > + err = AVERROR(ENOSYS); > + goto fail; > + } > + if (!found_profile && !(avctx->hwaccel_flags & > + AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH)) { > + // We allow this case with profile-mismatch enabled to support > + // things like trying to decode H.264 extended profile. > + av_log(avctx, AV_LOG_ERROR, "This libavcodec build does not " > + "support VAAPI decoding of codec %s profile %d.\n", > + codec_desc->name, avctx->profile); > err = AVERROR(ENOSYS); > goto fail; > } > + if (matched_va_profile == VAProfileNone) { > + av_log(avctx, AV_LOG_ERROR, "This VAAPI driver does not " > + "support decoding of codec %s.\n", > + codec_desc->name); > + err = AVERROR(EINVAL); > + goto fail; > + } > if (!exact_match) { > if (avctx->hwaccel_flags & > AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH) { > - av_log(avctx, AV_LOG_VERBOSE, "Codec %s profile %d not " > - "supported for hardware decode.\n", > + av_log(avctx, AV_LOG_WARNING, "This VAAPI driver does not " > + "support decoding of codec %s profile %d.\n", > codec_desc->name, avctx->profile); > av_log(avctx, AV_LOG_WARNING, "Using possibly-" > "incompatible profile %d instead.\n", > matched_ff_profile); > } else { > - av_log(avctx, AV_LOG_VERBOSE, "Codec %s profile %d not " > - "supported for hardware decode.\n", > + av_log(avctx, AV_LOG_ERROR, "This VAAPI driver does not " > + "support decoding of codec %s profile %d.\n", > codec_desc->name, avctx->profile); > err = AVERROR(EINVAL); > goto fail; > -- Generally makes sense, however there is one concern if I got it correctly: If a codec is not supported by VAAPI in current libavcodec build, ff_get_format() would not select VAAPI as the HW acceleration. Instead, it would fallback to the native software decoding path, and won't trigger the (!found_codec) logic. ./configure --enable-vaapi --disable-hwaccel=hevc_vaapi $ ffmpeg -v debug -hwaccel vaapi -i ./hevc_rext_decode/Main_422_10_A_RExt_Sony_2.bin -f null - Log: [hevc @ 0x5592f7b0fec0] Format yuv422p10le chosen by get_format(). VAAPI error return would not be triggered. - Linjie
On 12/04/2020 18:19, Fu, Linjie wrote: >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> Mark Thompson >> Sent: Sunday, April 12, 2020 21:00 >> To: ffmpeg-devel@ffmpeg.org >> Subject: [FFmpeg-devel] [PATCH] vaapi_decode: Improve logging around >> codec/profile selection >> >> --- >> On 12/04/2020 13:14, Mark Thompson wrote: >>> ... This does rather suggest that the error messages in that file should be >> clearer, though - it would be nice if it could distinguish between "this codec >> isn't supported by libavcodec at all", "this codec might work but hasn't built >> into this libavcodec" and "this codec is supported by libavcodec but not by >> your hardware". >> >> Something like this? >> >> >> libavcodec/vaapi_decode.c | 39 +++++++++++++++++++++++++++++++---- >> ---- >> 1 file changed, 31 insertions(+), 8 deletions(-) >> >> diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c >> index 54a0ecb47a..a191850e36 100644 >> --- a/libavcodec/vaapi_decode.c >> +++ b/libavcodec/vaapi_decode.c >> @@ -429,6 +429,7 @@ static int >> vaapi_decode_make_config(AVCodecContext *avctx, >> const AVCodecDescriptor *codec_desc; >> VAProfile *profile_list = NULL, matched_va_profile, va_profile; >> int profile_count, exact_match, matched_ff_profile, codec_profile; >> + int found_codec, found_profile; >> >> AVHWDeviceContext *device = (AVHWDeviceContext*)device_ref- >>> data; >> AVVAAPIDeviceContext *hwctx = device->hwctx; >> @@ -457,15 +458,19 @@ static int >> vaapi_decode_make_config(AVCodecContext *avctx, >> } >> >> matched_va_profile = VAProfileNone; >> + found_codec = found_profile = 0; >> exact_match = 0; >> >> for (i = 0; i < FF_ARRAY_ELEMS(vaapi_profile_map); i++) { >> int profile_match = 0; >> if (avctx->codec_id != vaapi_profile_map[i].codec_id) >> continue; >> + found_codec = 1; >> if (avctx->profile == vaapi_profile_map[i].codec_profile || >> - vaapi_profile_map[i].codec_profile == FF_PROFILE_UNKNOWN) >> + vaapi_profile_map[i].codec_profile == FF_PROFILE_UNKNOWN) { >> profile_match = 1; >> + found_profile = 1; >> + } >> >> va_profile = vaapi_profile_map[i].profile_parser ? >> vaapi_profile_map[i].profile_parser(avctx) : >> @@ -487,24 +492,42 @@ static int >> vaapi_decode_make_config(AVCodecContext *avctx, >> } >> av_freep(&profile_list); >> >> - if (matched_va_profile == VAProfileNone) { >> - av_log(avctx, AV_LOG_ERROR, "No support for codec %s " >> - "profile %d.\n", codec_desc->name, avctx->profile); >> + if (!found_codec) { >> + av_log(avctx, AV_LOG_ERROR, "This libavcodec build does not " >> + "support VAAPI decoding of codec %s.\n", >> + codec_desc->name); >> + err = AVERROR(ENOSYS); >> + goto fail; >> + } >> + if (!found_profile && !(avctx->hwaccel_flags & >> + AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH)) { >> + // We allow this case with profile-mismatch enabled to support >> + // things like trying to decode H.264 extended profile. >> + av_log(avctx, AV_LOG_ERROR, "This libavcodec build does not " >> + "support VAAPI decoding of codec %s profile %d.\n", >> + codec_desc->name, avctx->profile); >> err = AVERROR(ENOSYS); >> goto fail; >> } >> + if (matched_va_profile == VAProfileNone) { >> + av_log(avctx, AV_LOG_ERROR, "This VAAPI driver does not " >> + "support decoding of codec %s.\n", >> + codec_desc->name); >> + err = AVERROR(EINVAL); >> + goto fail; >> + } >> if (!exact_match) { >> if (avctx->hwaccel_flags & >> AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH) { >> - av_log(avctx, AV_LOG_VERBOSE, "Codec %s profile %d not " >> - "supported for hardware decode.\n", >> + av_log(avctx, AV_LOG_WARNING, "This VAAPI driver does not " >> + "support decoding of codec %s profile %d.\n", >> codec_desc->name, avctx->profile); >> av_log(avctx, AV_LOG_WARNING, "Using possibly-" >> "incompatible profile %d instead.\n", >> matched_ff_profile); >> } else { >> - av_log(avctx, AV_LOG_VERBOSE, "Codec %s profile %d not " >> - "supported for hardware decode.\n", >> + av_log(avctx, AV_LOG_ERROR, "This VAAPI driver does not " >> + "support decoding of codec %s profile %d.\n", >> codec_desc->name, avctx->profile); >> err = AVERROR(EINVAL); >> goto fail; >> -- > Generally makes sense, however there is one concern if I got it correctly: > > If a codec is not supported by VAAPI in current libavcodec build, ff_get_format() > would not select VAAPI as the HW acceleration. > Instead, it would fallback to the native software decoding path, and won't trigger > the (!found_codec) logic. > > ./configure --enable-vaapi --disable-hwaccel=hevc_vaapi > > $ ffmpeg -v debug -hwaccel vaapi -i ./hevc_rext_decode/Main_422_10_A_RExt_Sony_2.bin -f null - > > Log: > [hevc @ 0x5592f7b0fec0] Format yuv422p10le chosen by get_format(). > > VAAPI error return would not be triggered. Yeah, it's not covering that case because of how get_format() works - the caller can't choose the VAAPI format, so we never get here. If the caller really wanted VAAPI then they need to deal with it externally (for ffmpeg, you want something like the proposed -decode_format option: <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-November/236205.html>). The case where the "no codec" message is triggered is when you can pick it (the hwaccel is enabled) but the build still doesn't have the codec in the table (for example if it were built against older libva headers). - Mark
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Mark Thompson > Sent: Monday, April 13, 2020 21:14 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH] vaapi_decode: Improve logging around > codec/profile selection > > On 12/04/2020 18:19, Fu, Linjie wrote: > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > >> Mark Thompson > >> Sent: Sunday, April 12, 2020 21:00 > >> To: ffmpeg-devel@ffmpeg.org > >> Subject: [FFmpeg-devel] [PATCH] vaapi_decode: Improve logging around > >> codec/profile selection > >> > >> --- > >> On 12/04/2020 13:14, Mark Thompson wrote: > >>> ... This does rather suggest that the error messages in that file should be > >> clearer, though - it would be nice if it could distinguish between "this > codec > >> isn't supported by libavcodec at all", "this codec might work but hasn't > built > >> into this libavcodec" and "this codec is supported by libavcodec but not by > >> your hardware". > >> > >> Something like this? > >> > >> > >> libavcodec/vaapi_decode.c | 39 > +++++++++++++++++++++++++++++++---- > >> ---- > >> 1 file changed, 31 insertions(+), 8 deletions(-) > >> > >> -- > > Generally makes sense, however there is one concern if I got it correctly: > > > > If a codec is not supported by VAAPI in current libavcodec build, > ff_get_format() > > would not select VAAPI as the HW acceleration. > > Instead, it would fallback to the native software decoding path, and won't > trigger > > the (!found_codec) logic. > > > > ./configure --enable-vaapi --disable-hwaccel=hevc_vaapi > > > > $ ffmpeg -v debug -hwaccel vaapi - > i ./hevc_rext_decode/Main_422_10_A_RExt_Sony_2.bin -f null - > > > > Log: > > [hevc @ 0x5592f7b0fec0] Format yuv422p10le chosen by get_format(). > > > > VAAPI error return would not be triggered. > > Yeah, it's not covering that case because of how get_format() works - the > caller can't choose the VAAPI format, so we never get here. If the caller > really wanted VAAPI then they need to deal with it externally (for ffmpeg, > you want something like the proposed -decode_format option: > <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018- > November/236205.html>). > > The case where the "no codec" message is triggered is when you can pick it > (the hwaccel is enabled) but the build still doesn't have the codec in the table > (for example if it were built against older libva headers). > Yep, got the point and looks reasonable, thanks for elaboration. - Linjie
diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c index 54a0ecb47a..a191850e36 100644 --- a/libavcodec/vaapi_decode.c +++ b/libavcodec/vaapi_decode.c @@ -429,6 +429,7 @@ static int vaapi_decode_make_config(AVCodecContext *avctx, const AVCodecDescriptor *codec_desc; VAProfile *profile_list = NULL, matched_va_profile, va_profile; int profile_count, exact_match, matched_ff_profile, codec_profile; + int found_codec, found_profile; AVHWDeviceContext *device = (AVHWDeviceContext*)device_ref->data; AVVAAPIDeviceContext *hwctx = device->hwctx; @@ -457,15 +458,19 @@ static int vaapi_decode_make_config(AVCodecContext *avctx, } matched_va_profile = VAProfileNone; + found_codec = found_profile = 0; exact_match = 0; for (i = 0; i < FF_ARRAY_ELEMS(vaapi_profile_map); i++) { int profile_match = 0; if (avctx->codec_id != vaapi_profile_map[i].codec_id) continue; + found_codec = 1; if (avctx->profile == vaapi_profile_map[i].codec_profile || - vaapi_profile_map[i].codec_profile == FF_PROFILE_UNKNOWN) + vaapi_profile_map[i].codec_profile == FF_PROFILE_UNKNOWN) { profile_match = 1; + found_profile = 1; + } va_profile = vaapi_profile_map[i].profile_parser ? vaapi_profile_map[i].profile_parser(avctx) : @@ -487,24 +492,42 @@ static int vaapi_decode_make_config(AVCodecContext *avctx, } av_freep(&profile_list); - if (matched_va_profile == VAProfileNone) { - av_log(avctx, AV_LOG_ERROR, "No support for codec %s " - "profile %d.\n", codec_desc->name, avctx->profile); + if (!found_codec) { + av_log(avctx, AV_LOG_ERROR, "This libavcodec build does not " + "support VAAPI decoding of codec %s.\n", + codec_desc->name); + err = AVERROR(ENOSYS); + goto fail; + } + if (!found_profile && !(avctx->hwaccel_flags & + AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH)) { + // We allow this case with profile-mismatch enabled to support + // things like trying to decode H.264 extended profile. + av_log(avctx, AV_LOG_ERROR, "This libavcodec build does not " + "support VAAPI decoding of codec %s profile %d.\n", + codec_desc->name, avctx->profile); err = AVERROR(ENOSYS); goto fail; } + if (matched_va_profile == VAProfileNone) { + av_log(avctx, AV_LOG_ERROR, "This VAAPI driver does not " + "support decoding of codec %s.\n", + codec_desc->name); + err = AVERROR(EINVAL); + goto fail; + } if (!exact_match) { if (avctx->hwaccel_flags & AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH) { - av_log(avctx, AV_LOG_VERBOSE, "Codec %s profile %d not " - "supported for hardware decode.\n", + av_log(avctx, AV_LOG_WARNING, "This VAAPI driver does not " + "support decoding of codec %s profile %d.\n", codec_desc->name, avctx->profile); av_log(avctx, AV_LOG_WARNING, "Using possibly-" "incompatible profile %d instead.\n", matched_ff_profile); } else { - av_log(avctx, AV_LOG_VERBOSE, "Codec %s profile %d not " - "supported for hardware decode.\n", + av_log(avctx, AV_LOG_ERROR, "This VAAPI driver does not " + "support decoding of codec %s profile %d.\n", codec_desc->name, avctx->profile); err = AVERROR(EINVAL); goto fail;
--- On 12/04/2020 13:14, Mark Thompson wrote: > ... This does rather suggest that the error messages in that file should be clearer, though - it would be nice if it could distinguish between "this codec isn't supported by libavcodec at all", "this codec might work but hasn't built into this libavcodec" and "this codec is supported by libavcodec but not by your hardware". Something like this? libavcodec/vaapi_decode.c | 39 +++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-)