| Message ID | 20191215220029.28817-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
On 12/15/2019 7:00 PM, Michael Niedermayer wrote: > Fixes: Ticket7249 (No longer displaying a scary red message) > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/h264_ps.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > index e8738d8502..74f12f8979 100644 > --- a/libavcodec/h264_ps.c > +++ b/libavcodec/h264_ps.c > @@ -203,6 +203,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx > sps->num_units_in_tick = num_units_in_tick; > sps->time_scale = time_scale; > } > + if (get_bits_left(gb) == 0) { Doing this check here sounds like it's tailored specifically for the sample in the ticket. What if another file is truncated one bit after this point? Or one bit before? You will get the scary red message for it. Will you also add a check for it? We can't clutter this file with new get_bits_left() checks every time a user shares their broken samples with us just to get a less scary output in their terminals. A better "fix" for this is mapping the AV_EF_IGNORE_ERR err_recognition avctx flag to the ignore_truncation parameter in ff_h264_decode_seq_parameter_set(), and removing the fallback call that hard enables it in the decoder. That way, the user will get warnings instead of errors when they use that flag to request these truncation errors to be ignored. > + av_log(avctx, AV_LOG_WARNING, "Truncated VUI before fixed_frame_rate_flag\n"); > + return 0; > + } > sps->fixed_frame_rate_flag = get_bits1(gb); > } > >
Am So., 15. Dez. 2019 um 23:25 Uhr schrieb James Almer <jamrial@gmail.com>: > > On 12/15/2019 7:00 PM, Michael Niedermayer wrote: > > Fixes: Ticket7249 (No longer displaying a scary red message) > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/h264_ps.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > > index e8738d8502..74f12f8979 100644 > > --- a/libavcodec/h264_ps.c > > +++ b/libavcodec/h264_ps.c > > @@ -203,6 +203,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx > > sps->num_units_in_tick = num_units_in_tick; > > sps->time_scale = time_scale; > > } > > + if (get_bits_left(gb) == 0) { > > Doing this check here sounds like it's tailored specifically for the > sample in the ticket. What if another file is truncated one bit after > this point? Or one bit before? You will get the scary red message for > it. Will you also add a check for it? > We can't clutter this file with new get_bits_left() checks every time a > user shares their broken samples with us just to get a less scary output > in their terminals. Do you know of another proprietary or open-source encoder that encodes streams with such a truncation? Carl Eugen
On 12/15/2019 8:50 PM, Carl Eugen Hoyos wrote: > Am So., 15. Dez. 2019 um 23:25 Uhr schrieb James Almer <jamrial@gmail.com>: >> >> On 12/15/2019 7:00 PM, Michael Niedermayer wrote: >>> Fixes: Ticket7249 (No longer displaying a scary red message) >>> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/h264_ps.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c >>> index e8738d8502..74f12f8979 100644 >>> --- a/libavcodec/h264_ps.c >>> +++ b/libavcodec/h264_ps.c >>> @@ -203,6 +203,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx >>> sps->num_units_in_tick = num_units_in_tick; >>> sps->time_scale = time_scale; >>> } >>> + if (get_bits_left(gb) == 0) { >> >> Doing this check here sounds like it's tailored specifically for the >> sample in the ticket. What if another file is truncated one bit after >> this point? Or one bit before? You will get the scary red message for >> it. Will you also add a check for it? >> We can't clutter this file with new get_bits_left() checks every time a >> user shares their broken samples with us just to get a less scary output >> in their terminals. > > Do you know of another proprietary or open-source encoder that encodes > streams with such a truncation? There was an old hevc encoder that did something like that. We ended up adding a workaround for it as well. And i can imagine capturing a transport stream with bad network conditions could result in all kinds randomly placed truncation. This patch is not fixing anything, it's just removing an error level log message with a warning one so it's "less scary", since ultimately the file is decodeable with or without it because truncation is ignored by the decoder. I gave a suggestion to properly remove the scary messages and prevent littering the decoder with custom checks in arbitrary places.
Am Mo., 16. Dez. 2019 um 01:00 Uhr schrieb James Almer <jamrial@gmail.com>: > > On 12/15/2019 8:50 PM, Carl Eugen Hoyos wrote: > > Am So., 15. Dez. 2019 um 23:25 Uhr schrieb James Almer <jamrial@gmail.com>: > >> > >> On 12/15/2019 7:00 PM, Michael Niedermayer wrote: > >>> Fixes: Ticket7249 (No longer displaying a scary red message) > >>> > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavcodec/h264_ps.c | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > >>> index e8738d8502..74f12f8979 100644 > >>> --- a/libavcodec/h264_ps.c > >>> +++ b/libavcodec/h264_ps.c > >>> @@ -203,6 +203,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx > >>> sps->num_units_in_tick = num_units_in_tick; > >>> sps->time_scale = time_scale; > >>> } > >>> + if (get_bits_left(gb) == 0) { > >> > >> Doing this check here sounds like it's tailored specifically for the > >> sample in the ticket. What if another file is truncated one bit after > >> this point? Or one bit before? You will get the scary red message for > >> it. Will you also add a check for it? > >> We can't clutter this file with new get_bits_left() checks every time a > >> user shares their broken samples with us just to get a less scary output > >> in their terminals. > > > > Do you know of another proprietary or open-source encoder that encodes > > streams with such a truncation? > > There was an old hevc encoder that did something like that. We ended up > adding a workaround for it as well. > And i can imagine capturing a > transport stream with bad network conditions could result in all kinds > randomly placed truncation. I don't think a "random" truncation as produced by reception or network issues can lead to similar messages as in the ticket. Carl Eugen
On 12/15/2019 9:03 PM, Carl Eugen Hoyos wrote: > Am Mo., 16. Dez. 2019 um 01:00 Uhr schrieb James Almer <jamrial@gmail.com>: >> >> On 12/15/2019 8:50 PM, Carl Eugen Hoyos wrote: >>> Am So., 15. Dez. 2019 um 23:25 Uhr schrieb James Almer <jamrial@gmail.com>: >>>> >>>> On 12/15/2019 7:00 PM, Michael Niedermayer wrote: >>>>> Fixes: Ticket7249 (No longer displaying a scary red message) >>>>> >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>>> --- >>>>> libavcodec/h264_ps.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c >>>>> index e8738d8502..74f12f8979 100644 >>>>> --- a/libavcodec/h264_ps.c >>>>> +++ b/libavcodec/h264_ps.c >>>>> @@ -203,6 +203,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx >>>>> sps->num_units_in_tick = num_units_in_tick; >>>>> sps->time_scale = time_scale; >>>>> } >>>>> + if (get_bits_left(gb) == 0) { >>>> >>>> Doing this check here sounds like it's tailored specifically for the >>>> sample in the ticket. What if another file is truncated one bit after >>>> this point? Or one bit before? You will get the scary red message for >>>> it. Will you also add a check for it? >>>> We can't clutter this file with new get_bits_left() checks every time a >>>> user shares their broken samples with us just to get a less scary output >>>> in their terminals. >>> >>> Do you know of another proprietary or open-source encoder that encodes >>> streams with such a truncation? >> >> There was an old hevc encoder that did something like that. We ended up >> adding a workaround for it as well. > >> And i can imagine capturing a >> transport stream with bad network conditions could result in all kinds >> randomly placed truncation. > > I don't think a "random" truncation as produced by reception or > network issues can lead to similar messages as in the ticket. Can you elaborate why? If a sample stored in some container is truncated in the VUI one bit after this point, will it not still print the overread error level log message? > > Carl Eugen > _______________________________________________ > 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 12/15/2019 7:00 PM, Michael Niedermayer wrote: > Fixes: Ticket7249 (No longer displaying a scary red message) > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/h264_ps.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > index e8738d8502..74f12f8979 100644 > --- a/libavcodec/h264_ps.c > +++ b/libavcodec/h264_ps.c > @@ -203,6 +203,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx > sps->num_units_in_tick = num_units_in_tick; > sps->time_scale = time_scale; > } > + if (get_bits_left(gb) == 0) { > + av_log(avctx, AV_LOG_WARNING, "Truncated VUI before fixed_frame_rate_flag\n"); This log message is wrong, for that matter. The VUI in these packets does contain fixed_frame_rate_flag and two more bits. The reason get_bits_left(gb) is zero at this point is because ff_h2645_packet_split() skips what it assumes are the rbsp_stop_one_bit and any potential rbsp_alignment_zero_bits at the end of a NAL, and in this case fixed_frame_rate_flag happened to be the last 1. > + return 0; > + } > sps->fixed_frame_rate_flag = get_bits1(gb); > } > >
Am Mo., 16. Dez. 2019 um 01:07 Uhr schrieb James Almer <jamrial@gmail.com>: > > On 12/15/2019 9:03 PM, Carl Eugen Hoyos wrote: > > Am Mo., 16. Dez. 2019 um 01:00 Uhr schrieb James Almer <jamrial@gmail.com>: > >> > >> On 12/15/2019 8:50 PM, Carl Eugen Hoyos wrote: > >>> Am So., 15. Dez. 2019 um 23:25 Uhr schrieb James Almer <jamrial@gmail.com>: > >>>> > >>>> On 12/15/2019 7:00 PM, Michael Niedermayer wrote: > >>>>> Fixes: Ticket7249 (No longer displaying a scary red message) > >>>>> > >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>>>> --- > >>>>> libavcodec/h264_ps.c | 4 ++++ > >>>>> 1 file changed, 4 insertions(+) > >>>>> > >>>>> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > >>>>> index e8738d8502..74f12f8979 100644 > >>>>> --- a/libavcodec/h264_ps.c > >>>>> +++ b/libavcodec/h264_ps.c > >>>>> @@ -203,6 +203,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx > >>>>> sps->num_units_in_tick = num_units_in_tick; > >>>>> sps->time_scale = time_scale; > >>>>> } > >>>>> + if (get_bits_left(gb) == 0) { > >>>> > >>>> Doing this check here sounds like it's tailored specifically for the > >>>> sample in the ticket. What if another file is truncated one bit after > >>>> this point? Or one bit before? You will get the scary red message for > >>>> it. Will you also add a check for it? > >>>> We can't clutter this file with new get_bits_left() checks every time a > >>>> user shares their broken samples with us just to get a less scary output > >>>> in their terminals. > >>> > >>> Do you know of another proprietary or open-source encoder that encodes > >>> streams with such a truncation? > >> > >> There was an old hevc encoder that did something like that. We ended up > >> adding a workaround for it as well. > > > >> And i can imagine capturing a > >> transport stream with bad network conditions could result in all kinds > >> randomly placed truncation. > > > > I don't think a "random" truncation as produced by reception or > > network issues can lead to similar messages as in the ticket. > > Can you elaborate why? If a sample stored in some container is truncated > in the VUI one bit after this point, will it not still print the > overread error level log message? But not continuously. Carl Eugen
On Mon, Dec 16, 2019 at 11:52:21AM +0100, Carl Eugen Hoyos wrote: > Am Mo., 16. Dez. 2019 um 01:07 Uhr schrieb James Almer <jamrial@gmail.com>: > > > > On 12/15/2019 9:03 PM, Carl Eugen Hoyos wrote: > > > Am Mo., 16. Dez. 2019 um 01:00 Uhr schrieb James Almer <jamrial@gmail.com>: > > >> > > >> On 12/15/2019 8:50 PM, Carl Eugen Hoyos wrote: > > >>> Am So., 15. Dez. 2019 um 23:25 Uhr schrieb James Almer <jamrial@gmail.com>: > > >>>> > > >>>> On 12/15/2019 7:00 PM, Michael Niedermayer wrote: > > >>>>> Fixes: Ticket7249 (No longer displaying a scary red message) > > >>>>> > > >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > >>>>> --- > > >>>>> libavcodec/h264_ps.c | 4 ++++ > > >>>>> 1 file changed, 4 insertions(+) > > >>>>> > > >>>>> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c > > >>>>> index e8738d8502..74f12f8979 100644 > > >>>>> --- a/libavcodec/h264_ps.c > > >>>>> +++ b/libavcodec/h264_ps.c > > >>>>> @@ -203,6 +203,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx > > >>>>> sps->num_units_in_tick = num_units_in_tick; > > >>>>> sps->time_scale = time_scale; > > >>>>> } > > >>>>> + if (get_bits_left(gb) == 0) { > > >>>> > > >>>> Doing this check here sounds like it's tailored specifically for the > > >>>> sample in the ticket. What if another file is truncated one bit after > > >>>> this point? Or one bit before? You will get the scary red message for > > >>>> it. Will you also add a check for it? > > >>>> We can't clutter this file with new get_bits_left() checks every time a > > >>>> user shares their broken samples with us just to get a less scary output > > >>>> in their terminals. > > >>> > > >>> Do you know of another proprietary or open-source encoder that encodes > > >>> streams with such a truncation? > > >> > > >> There was an old hevc encoder that did something like that. We ended up > > >> adding a workaround for it as well. > > > > > >> And i can imagine capturing a > > >> transport stream with bad network conditions could result in all kinds > > >> randomly placed truncation. > > > > > > I don't think a "random" truncation as produced by reception or > > > network issues can lead to similar messages as in the ticket. > > > > Can you elaborate why? If a sample stored in some container is truncated > > in the VUI one bit after this point, will it not still print the > > overread error level log message? > > But not continuously. I could imagine some global header being truncated for some reason and then that truncated header being repeated with each random access point. PS: off topic but i have a problem with receiving mails from james ATM (didnt receive the 2 mails you replied to yet) thx [...]
diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c index e8738d8502..74f12f8979 100644 --- a/libavcodec/h264_ps.c +++ b/libavcodec/h264_ps.c @@ -203,6 +203,10 @@ static inline int decode_vui_parameters(GetBitContext *gb, AVCodecContext *avctx sps->num_units_in_tick = num_units_in_tick; sps->time_scale = time_scale; } + if (get_bits_left(gb) == 0) { + av_log(avctx, AV_LOG_WARNING, "Truncated VUI before fixed_frame_rate_flag\n"); + return 0; + } sps->fixed_frame_rate_flag = get_bits1(gb); }
Fixes: Ticket7249 (No longer displaying a scary red message) Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/h264_ps.c | 4 ++++ 1 file changed, 4 insertions(+)