diff mbox

[FFmpeg-devel,1/2] avcodec/h264_ps: Check for truncation at fixed_frame_rate_flag

Message ID 20191215220029.28817-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Dec. 15, 2019, 10 p.m. UTC
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(+)

Comments

James Almer Dec. 15, 2019, 10:25 p.m. UTC | #1
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);
>      }
>  
>
Carl Eugen Hoyos Dec. 15, 2019, 11:50 p.m. UTC | #2
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
James Almer Dec. 16, 2019, midnight UTC | #3
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.
Carl Eugen Hoyos Dec. 16, 2019, 12:03 a.m. UTC | #4
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
James Almer Dec. 16, 2019, 12:06 a.m. UTC | #5
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".
>
James Almer Dec. 16, 2019, 4:23 a.m. UTC | #6
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);
>      }
>  
>
Carl Eugen Hoyos Dec. 16, 2019, 10:52 a.m. UTC | #7
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
Michael Niedermayer Dec. 16, 2019, 11:53 p.m. UTC | #8
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 mbox

Patch

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);
     }