| Message ID | 9e58eb38-02a5-f1d9-1fb4-a25315ddf362@jkqxz.net |
|---|---|
| State | Accepted |
| Commit | bd211bb866f8bf5c372589fc44dc06d1a9509c0a |
| Headers | show |
On 10/8/2017 4:49 PM, Mark Thompson wrote: > Yeah, ok, I agree. Patch changed as enclosing. > > > libavcodec/vaapi_decode.c | 1 - > libavcodec/vaapi_encode_h264.c | 12 ++++-------- > 2 files changed, 4 insertions(+), 9 deletions(-) Looks OK to me. I assume we don't care about the old lib versions and hardware wanting to use the profile. - Derek
On Sun, Oct 08, 2017 at 16:49:58 +0100, Mark Thompson wrote: > switch (avctx->profile) { > + case FF_PROFILE_H264_BASELINE: > + av_log(avctx, AV_LOG_WARNING, "H.264 baseline profile is not " > + "supported, using constrained baseline profile instead.\n"); > + avctx->profile = FF_PROFILE_H264_CONSTRAINED_BASELINE; > case FF_PROFILE_H264_CONSTRAINED_BASELINE: I recall a discussion that linting/analysis tools such as Coverty require a fall-through to be marked as such. > /* fall-through */ > case FF_PROFILE_H264_CONSTRAINED_BASELINE: Cheers, Moritz
On Sun, 8 Oct 2017 16:49:58 +0100 Mark Thompson <sw@jkqxz.net> wrote: > This has been deprecated in libva2 because hardware does not and will not > support it. Therefore never consider it for decode, and for encode assume > the user meant constrained baseline profile instead. > --- > On 08/10/17 16:44, Derek Buitenhuis wrote: > > On 10/8/2017 4:11 PM, Mark Thompson wrote: > >> + case FF_PROFILE_H264_BASELINE: > >> + // Baseline profile is not supported, assume the user meant > >> + // constrained baseline instead. > >> + avctx->profile = FF_PROFILE_H264_CONSTRAINED_BASELINE; > > > > Trying to automatically (and silently!) guess what the user wanted > > is never a good idea, IMO. At the very least, print a warning. > > Yeah, ok, I agree. Patch changed as enclosing. > > > libavcodec/vaapi_decode.c | 1 - > libavcodec/vaapi_encode_h264.c | 12 ++++-------- > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c > index cf58aae4c6..4f0ff84e01 100644 > --- a/libavcodec/vaapi_decode.c > +++ b/libavcodec/vaapi_decode.c > @@ -246,7 +246,6 @@ static const struct { > MAP(MPEG4, MPEG4_MAIN, MPEG4Main ), > MAP(H264, H264_CONSTRAINED_BASELINE, > H264ConstrainedBaseline), > - MAP(H264, H264_BASELINE, H264Baseline), > MAP(H264, H264_MAIN, H264Main ), > MAP(H264, H264_HIGH, H264High ), > #if VA_CHECK_VERSION(0, 37, 0) > diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c > index 549867ef3f..efde80b08e 100644 > --- a/libavcodec/vaapi_encode_h264.c > +++ b/libavcodec/vaapi_encode_h264.c > @@ -1175,6 +1175,10 @@ static av_cold int vaapi_encode_h264_init(AVCodecContext *avctx) > ctx->codec = &vaapi_encode_type_h264; > > switch (avctx->profile) { > + case FF_PROFILE_H264_BASELINE: > + av_log(avctx, AV_LOG_WARNING, "H.264 baseline profile is not " > + "supported, using constrained baseline profile instead.\n"); > + avctx->profile = FF_PROFILE_H264_CONSTRAINED_BASELINE; > case FF_PROFILE_H264_CONSTRAINED_BASELINE: > ctx->va_profile = VAProfileH264ConstrainedBaseline; > if (avctx->max_b_frames != 0) { > @@ -1183,14 +1187,6 @@ static av_cold int vaapi_encode_h264_init(AVCodecContext *avctx) > "doesn't support encoding with B frames, disabling them.\n"); > } > break; > - case FF_PROFILE_H264_BASELINE: > - ctx->va_profile = VAProfileH264Baseline; > - if (avctx->max_b_frames != 0) { > - avctx->max_b_frames = 0; > - av_log(avctx, AV_LOG_WARNING, "H.264 baseline profile " > - "doesn't support encoding with B frames, disabling them.\n"); > - } > - break; > case FF_PROFILE_H264_MAIN: > ctx->va_profile = VAProfileH264Main; > break; Shouldn't trying to decode baseline video just fall back to sw decoding?
2017-10-09 13:21 GMT+02:00 wm4 <nfxjfg@googlemail.com>:
> Shouldn't trying to decode baseline video just fall back to sw decoding?
Given that software doesn't support the specific features of baseline either,
I don't think this helps.
I was under the impression the only sane thing to do when reading
baseline H.264 is printing a warning and continue as if constraint
baseline was detected.
Carl Eugen
On 09/10/17 22:48, Carl Eugen Hoyos wrote: > 2017-10-09 13:21 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > >> Shouldn't trying to decode baseline video just fall back to sw decoding? > > Given that software doesn't support the specific features of baseline either, > I don't think this helps. > I was under the impression the only sane thing to do when reading > baseline H.264 is printing a warning and continue as if constraint > baseline was detected. At least with software decode we would know that it has failed - an inconvenient property of many hardware decoders is their reluctance to report errors usefully or at all. Hence the warning on using the ALLOW_PROFILE_MISMATCH flag, which does allow the attempt to decode baseline profile streams with hardware: """ If the stream is actually not supported then the behaviour is undefined, and may include returning entirely incorrect output while indicating success. """ I think the behaviour we have now with this option to continue if the user really wants (and is aware of the possible consequences) is correct. Thanks, - Mark
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Mark Thompson > Sent: Tuesday, October 10, 2017 6:36 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH 2/6] vaapi: Remove H.264 baseline > profile > > On 09/10/17 22:48, Carl Eugen Hoyos wrote: > > 2017-10-09 13:21 GMT+02:00 wm4 <nfxjfg@googlemail.com>: > > > >> Shouldn't trying to decode baseline video just fall back to sw decoding? > > > > Given that software doesn't support the specific features of baseline > > either, I don't think this helps. > > I was under the impression the only sane thing to do when reading > > baseline H.264 is printing a warning and continue as if constraint > > baseline was detected. > > At least with software decode we would know that it has failed - an > inconvenient property of many hardware decoders is their reluctance to > report errors usefully or at all. > > Hence the warning on using the ALLOW_PROFILE_MISMATCH flag, which > does allow the attempt to decode baseline profile streams with hardware: > """ > If the stream is actually not supported then the behaviour is undefined, and > may include returning entirely incorrect output while indicating success. > """ > > I think the behaviour we have now with this option to continue if the user > really wants (and is aware of the possible consequences) is correct. > [Li, Zhong] There advanced features of baseline profile are not supported：FMO, SAO, and Redundant slices. So another way is to check a baseline clip has some of these there features or not (ie, slice_group_number can be check when decoding PPS), If yes, fall back to sw decoding. Else, set the profile to be constrained profile. > Thanks, > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 11/10/17 09:10, Li, Zhong wrote: >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf >> Of Mark Thompson >> Sent: Tuesday, October 10, 2017 6:36 AM >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH 2/6] vaapi: Remove H.264 baseline >> profile >> >> On 09/10/17 22:48, Carl Eugen Hoyos wrote: >>> 2017-10-09 13:21 GMT+02:00 wm4 <nfxjfg@googlemail.com>: >>> >>>> Shouldn't trying to decode baseline video just fall back to sw decoding? >>> >>> Given that software doesn't support the specific features of baseline >>> either, I don't think this helps. >>> I was under the impression the only sane thing to do when reading >>> baseline H.264 is printing a warning and continue as if constraint >>> baseline was detected. >> >> At least with software decode we would know that it has failed - an >> inconvenient property of many hardware decoders is their reluctance to >> report errors usefully or at all. >> >> Hence the warning on using the ALLOW_PROFILE_MISMATCH flag, which >> does allow the attempt to decode baseline profile streams with hardware: >> """ >> If the stream is actually not supported then the behaviour is undefined, and >> may include returning entirely incorrect output while indicating success. >> """ >> >> I think the behaviour we have now with this option to continue if the user >> really wants (and is aware of the possible consequences) is correct. >> > > [Li, Zhong] There advanced features of baseline profile are not supported：FMO, SAO, and Redundant slices. > So another way is to check a baseline clip has some of these there features or not (ie, slice_group_number can be check when decoding PPS), > If yes, fall back to sw decoding. Else, set the profile to be constrained profile. Unfortunately, you have to examine the whole stream to determine this sort of thing. For a more concrete example in a perhaps more plausible case, consider an H.264 main profile hardware decoder and an H.264 high profile stream. At the start of the stream, we can examine the first packet and observe that all constraints for main profile are met: no scaling matrices, no 8x8 transform, etc. So, we can start using the main profile hardware decoder, and everything works for a while. Then, at some point in the middle of the stream, a new PPS turns up containing scaling matrices and enabling the 8x8 transform. Now what? We can't fall back to software decoding because this isn't a stream boundary - we need to use the reference frames which are currently stored in hardware surfaces. So the output is now going to be broken because we made the wrong choice initially, when it could have worked. Therefore: the profile information in the stream is the only method we should be using to determine profile conformance - trying to infer other values is doomed to disastrous failure in edge cases, so should only be done if the user explicitly requests it (e.g. with the ALLOW_PROFILE_MISMATCH flag). If an encoder declares a higher profile than it actually uses then that is unhelpful but there is little we can do about it. - Mark
2017-10-11 12:26 GMT+02:00 Mark Thompson <sw@jkqxz.net>: > If an encoder declares a higher profile than it actually uses then > that is unhelpful but there is little we can do about it. We have been through all this years ago: The only sane thing to do is to ignore these values (and if you want, print a warning). For the specific original issue here: Is there a real-world baseline encoder, or actually: a real-world baseline-encoded file? Because we know that many real-world constrained-baseline files were encoded with profile "baseline": Carl Eugen
diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c index cf58aae4c6..4f0ff84e01 100644 --- a/libavcodec/vaapi_decode.c +++ b/libavcodec/vaapi_decode.c @@ -246,7 +246,6 @@ static const struct { MAP(MPEG4, MPEG4_MAIN, MPEG4Main ), MAP(H264, H264_CONSTRAINED_BASELINE, H264ConstrainedBaseline), - MAP(H264, H264_BASELINE, H264Baseline), MAP(H264, H264_MAIN, H264Main ), MAP(H264, H264_HIGH, H264High ), #if VA_CHECK_VERSION(0, 37, 0) diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c index 549867ef3f..efde80b08e 100644 --- a/libavcodec/vaapi_encode_h264.c +++ b/libavcodec/vaapi_encode_h264.c @@ -1175,6 +1175,10 @@ static av_cold int vaapi_encode_h264_init(AVCodecContext *avctx) ctx->codec = &vaapi_encode_type_h264; switch (avctx->profile) { + case FF_PROFILE_H264_BASELINE: + av_log(avctx, AV_LOG_WARNING, "H.264 baseline profile is not " + "supported, using constrained baseline profile instead.\n"); + avctx->profile = FF_PROFILE_H264_CONSTRAINED_BASELINE; case FF_PROFILE_H264_CONSTRAINED_BASELINE: ctx->va_profile = VAProfileH264ConstrainedBaseline; if (avctx->max_b_frames != 0) { @@ -1183,14 +1187,6 @@ static av_cold int vaapi_encode_h264_init(AVCodecContext *avctx) "doesn't support encoding with B frames, disabling them.\n"); } break; - case FF_PROFILE_H264_BASELINE: - ctx->va_profile = VAProfileH264Baseline; - if (avctx->max_b_frames != 0) { - avctx->max_b_frames = 0; - av_log(avctx, AV_LOG_WARNING, "H.264 baseline profile " - "doesn't support encoding with B frames, disabling them.\n"); - } - break; case FF_PROFILE_H264_MAIN: ctx->va_profile = VAProfileH264Main; break;