| Message ID | 20190603222153.19810-1-antonin.gouzer@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 04.06.2019, at 00:21, Antonin Gouzer <antonin.gouzer@gmail.com> wrote: > --- > Some codecs editors had miss interpreted the H264 standart and > have coded a wrong size in the SEI data. > size = SEI size + 1. > The SEI data is detected as "truncated" > Ex: https://drive.google.com/file/d/1cNtLwnfPnyJnYqE7OYhU3SCoLRtuXIUM/view?usp=sharing > Command: > ffprobe -strict experimental -print_format xml -show_frames -read_intervals %+0.04 truncated.h264 > This (simple) patch add the possibility to read this false truncated SEI data. > by setting strict_std_compliance = FF_COMPLIANCE_EXPERIMENTAL. > The error remain logged. > > Thanks in advance ! > > @@ -425,27 +425,28 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, > } while (get_bits(gb, 8) == 255); > > if (size > get_bits_left(gb) / 8) { > - av_log(logctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d\n", > + av_log(avctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d\n", > type, 8*size, get_bits_left(gb)); > - return AVERROR_INVALIDDATA; > + if (avctx->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) > + return AVERROR_INVALIDDATA; > } > next = get_bits_count(gb) + 8 * size; This doesn't seem right, shouldn't you adjust "size"? Also if this is reasonably common shouldn't we accept at least the exactly off-by-one case at the default settings?
Hello, Thanks for your response. It's difficult to say if this is a common issue. I have hundred of thousands of files like this from an editor. Even the off by one Size is not standart compliant and it would be incorrect to not report it as an error/warning My first idea was to allow the user to attempt reading such data but not as a default behaviour. The patch is larger than my use case and allow size off by N. Do you think that it would be better to reduce the patch to a unique use case: truncated SEI of picture timing type, off by one size ? Regards Le mar. 4 juin 2019 à 08:12, Reimar Döffinger <Reimar.Doeffinger@gmx.de> a écrit : > > > > On 04.06.2019, at 00:21, Antonin Gouzer <antonin.gouzer@gmail.com> wrote: > > > --- > > Some codecs editors had miss interpreted the H264 standart and > > have coded a wrong size in the SEI data. > > size = SEI size + 1. > > The SEI data is detected as "truncated" > > Ex: https://drive.google.com/file/d/1cNtLwnfPnyJnYqE7OYhU3SCoLRtuXIUM/view?usp=sharing > > Command: > > ffprobe -strict experimental -print_format xml -show_frames -read_intervals %+0.04 truncated.h264 > > This (simple) patch add the possibility to read this false truncated SEI data. > > by setting strict_std_compliance = FF_COMPLIANCE_EXPERIMENTAL. > > The error remain logged. > > > > Thanks in advance ! > > > > > @@ -425,27 +425,28 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, > > } while (get_bits(gb, 8) == 255); > > > > if (size > get_bits_left(gb) / 8) { > > - av_log(logctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d\n", > > + av_log(avctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d\n", > > type, 8*size, get_bits_left(gb)); > > - return AVERROR_INVALIDDATA; > > + if (avctx->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) > > + return AVERROR_INVALIDDATA; > > } > > next = get_bits_count(gb) + 8 * size; > > This doesn't seem right, shouldn't you adjust "size"? > Also if this is reasonably common shouldn't we accept at least the exactly off-by-one case at the default settings? > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 04.06.2019, at 22:33, Antonin Gouzer <antonin.gouzer@gmail.com> wrote: > Hello, > Thanks for your response. > It's difficult to say if this is a common issue. > I have hundred of thousands of files like this from an editor. > > Even the off by one Size is not standart compliant and it would be > incorrect to not report it as an error/warning Warning yes, but as far as I know FFmpeg policy generally is to support all files that exist in the wild without extra options. So I would maybe start with suggesting if you can convince the authors of that editor to fix it (doesn't fix existing files, but at least we won't have more of them generated). Secondly, I would suggest to accept (with warning) the off-by-one case, unless stricter than default compliance is requested. Lastly, up to you what to do about the case where it is off by more than one. I think it should probably still be rejected at default compliance selection, but don't care whether to not support it at all or at experimental level. Though last word is up to the maintainer.
diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c index d4eb9c0dab..28cd7577ed 100644 --- a/libavcodec/h264_sei.c +++ b/libavcodec/h264_sei.c @@ -402,7 +402,7 @@ static int decode_alternative_transfer(H264SEIAlternativeTransfer *h, } int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, - const H264ParamSets *ps, void *logctx) + const H264ParamSets *ps, AVCodecContext *avctx) { int master_ret = 0; @@ -425,27 +425,28 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, } while (get_bits(gb, 8) == 255); if (size > get_bits_left(gb) / 8) { - av_log(logctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d\n", + av_log(avctx, AV_LOG_ERROR, "SEI type %d size %d truncated at %d\n", type, 8*size, get_bits_left(gb)); - return AVERROR_INVALIDDATA; + if (avctx->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) + return AVERROR_INVALIDDATA; } next = get_bits_count(gb) + 8 * size; switch (type) { case H264_SEI_TYPE_PIC_TIMING: // Picture timing SEI - ret = decode_picture_timing(&h->picture_timing, gb, ps, logctx); + ret = decode_picture_timing(&h->picture_timing, gb, ps, avctx); break; case H264_SEI_TYPE_USER_DATA_REGISTERED: - ret = decode_registered_user_data(h, gb, logctx, size); + ret = decode_registered_user_data(h, gb, avctx, size); break; case H264_SEI_TYPE_USER_DATA_UNREGISTERED: - ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size); + ret = decode_unregistered_user_data(&h->unregistered, gb, avctx, size); break; case H264_SEI_TYPE_RECOVERY_POINT: - ret = decode_recovery_point(&h->recovery_point, gb, logctx); + ret = decode_recovery_point(&h->recovery_point, gb, avctx); break; case H264_SEI_TYPE_BUFFERING_PERIOD: - ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx); + ret = decode_buffering_period(&h->buffering_period, gb, ps, avctx); break; case H264_SEI_TYPE_FRAME_PACKING: ret = decode_frame_packing_arrangement(&h->frame_packing, gb); @@ -460,7 +461,7 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, ret = decode_alternative_transfer(&h->alternative_transfer, gb); break; default: - av_log(logctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type); + av_log(avctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type); } if (ret < 0 && ret != AVERROR_PS_NOT_FOUND) return ret; diff --git a/libavcodec/h264_sei.h b/libavcodec/h264_sei.h index a75c3aa175..32f3d67096 100644 --- a/libavcodec/h264_sei.h +++ b/libavcodec/h264_sei.h @@ -190,7 +190,7 @@ typedef struct H264SEIContext { struct H264ParamSets; int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, - const struct H264ParamSets *ps, void *logctx); + const struct H264ParamSets *ps, AVCodecContext *avctx); /** * Reset SEI values at the beginning of the frame.