diff mbox series

[FFmpeg-devel] avformat/utils: Preserve AV_PKT_FLAG_CORRUPT

Message ID 20210318001926.29398-1-pkoshevoy@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/utils: Preserve AV_PKT_FLAG_CORRUPT
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate warning Make fate failed

Commit Message

Pavel Koshevoy March 18, 2021, 12:19 a.m. UTC
Preserve AV_PKT_FLAG_CORRUPT so the caller can decide whether to drop
the packet.
---
 libavformat/utils.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Marton Balint March 18, 2021, 8:51 p.m. UTC | #1
On Wed, 17 Mar 2021, Pavel Koshevoy wrote:

> Preserve AV_PKT_FLAG_CORRUPT so the caller can decide whether to drop
> the packet.

LGTM, but Michael was against it last time:

https://patchwork.ffmpeg.org/project/ffmpeg/patch/20181009233214.8785-2-cus@passwd.hu/

Regards,
Marton

> ---
> libavformat/utils.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index a73f944e6e..0dc978e3d2 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -1494,7 +1494,8 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt,
>         out_pkt->pts          = st->parser->pts;
>         out_pkt->dts          = st->parser->dts;
>         out_pkt->pos          = st->parser->pos;
> -        out_pkt->flags       |= pkt->flags & AV_PKT_FLAG_DISCARD;
> +        out_pkt->flags       |= pkt->flags & (AV_PKT_FLAG_CORRUPT |
> +                                              AV_PKT_FLAG_DISCARD);
>
>         if (st->need_parsing == AVSTREAM_PARSE_FULL_RAW)
>             out_pkt->pos = st->parser->frame_offset;
> -- 
> 2.26.2
>
> _______________________________________________
> 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".
Pavel Koshevoy March 18, 2021, 8:57 p.m. UTC | #2
On Thu, Mar 18, 2021 at 2:51 PM Marton Balint <cus@passwd.hu> wrote:

>
>
> On Wed, 17 Mar 2021, Pavel Koshevoy wrote:
>
> > Preserve AV_PKT_FLAG_CORRUPT so the caller can decide whether to drop
> > the packet.
>
> LGTM, but Michael was against it last time:
>
>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20181009233214.8785-2-cus@passwd.hu/
>
>
>

I have a source where a corrupt packet is able to poison the video decoder
so that the subsequent video is decoded with severe visual artifacts.  My
workaround is to detect corrupt packets, drop them and  re-create the video
decoder on the next non-corrupt packet.  This workaround is working well so
far.

Pavel.




> > ---
> > libavformat/utils.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index a73f944e6e..0dc978e3d2 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -1494,7 +1494,8 @@ static int parse_packet(AVFormatContext *s,
> AVPacket *pkt,
> >         out_pkt->pts          = st->parser->pts;
> >         out_pkt->dts          = st->parser->dts;
> >         out_pkt->pos          = st->parser->pos;
> > -        out_pkt->flags       |= pkt->flags & AV_PKT_FLAG_DISCARD;
> > +        out_pkt->flags       |= pkt->flags & (AV_PKT_FLAG_CORRUPT |
> > +                                              AV_PKT_FLAG_DISCARD);
> >
> >         if (st->need_parsing == AVSTREAM_PARSE_FULL_RAW)
> >             out_pkt->pos = st->parser->frame_offset;
> > --
> > 2.26.2
> >
> > _______________________________________________
> > 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".
> _______________________________________________
> 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".
Pavel Koshevoy March 18, 2021, 9:11 p.m. UTC | #3
On Thu, Mar 18, 2021 at 2:57 PM Pavel Koshevoy <pkoshevoy@gmail.com> wrote:

>
>
> On Thu, Mar 18, 2021 at 2:51 PM Marton Balint <cus@passwd.hu> wrote:
>
>>
>>
>> On Wed, 17 Mar 2021, Pavel Koshevoy wrote:
>>
>> > Preserve AV_PKT_FLAG_CORRUPT so the caller can decide whether to drop
>> > the packet.
>>
>> LGTM, but Michael was against it last time:
>>
>>
>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20181009233214.8785-2-cus@passwd.hu/
>>
>>
>>
>
> I have a source where a corrupt packet is able to poison the video decoder
> so that the subsequent video is decoded with severe visual artifacts.  My
> workaround is to detect corrupt packets, drop them and  re-create the video
> decoder on the next non-corrupt packet.  This workaround is working well so
> far.
>
> Pavel.
>


Although, the video decoder being poisoned seems to be a regression in
ffmpeg ... the problem doesn't occur with ffmpeg git snapshot from
20190318, 15d016be30bd24cdba514c7c888e9da0286b5647

Pavel.
Michael Niedermayer March 19, 2021, 1:52 p.m. UTC | #4
On Thu, Mar 18, 2021 at 02:57:59PM -0600, Pavel Koshevoy wrote:
> On Thu, Mar 18, 2021 at 2:51 PM Marton Balint <cus@passwd.hu> wrote:
> 
> >
> >
> > On Wed, 17 Mar 2021, Pavel Koshevoy wrote:
> >
> > > Preserve AV_PKT_FLAG_CORRUPT so the caller can decide whether to drop
> > > the packet.
> >
> > LGTM, but Michael was against it last time:
> >
> >
> > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20181009233214.8785-2-cus@passwd.hu/
> >
> >
> >
> 
> I have a source where a corrupt packet is able to poison the video decoder
> so that the subsequent video is decoded with severe visual artifacts. 

Why dont you fix the video decoder?
Note, if droping packets works better in general there is a bug in the video
decoders handling of it. 


> My workaround 
> is to detect corrupt packets, drop them and  re-create the video
> decoder on the next non-corrupt packet.  This workaround is working well so
> far.

[...]
Michael Niedermayer March 19, 2021, 1:58 p.m. UTC | #5
On Wed, Mar 17, 2021 at 06:19:26PM -0600, Pavel Koshevoy wrote:
> Preserve AV_PKT_FLAG_CORRUPT so the caller can decide whether to drop
> the packet.
> ---
>  libavformat/utils.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index a73f944e6e..0dc978e3d2 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -1494,7 +1494,8 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt,
>          out_pkt->pts          = st->parser->pts;
>          out_pkt->dts          = st->parser->dts;
>          out_pkt->pos          = st->parser->pos;
> -        out_pkt->flags       |= pkt->flags & AV_PKT_FLAG_DISCARD;
> +        out_pkt->flags       |= pkt->flags & (AV_PKT_FLAG_CORRUPT |
> +                                              AV_PKT_FLAG_DISCARD);

1. assume the parsers input are a stream of 4kb packets, the output is lets say 101kb
packets. Assumingh one input packet is corrupted which output packet if any
gets the flag set and why is that the correct one ?

2. assume the parsers input are a stream of 128kb packets, the output is lets say 3kb
packets. Assuming one input packet is corrupted which output packet if any
gets the flag set and why is that the correct one ?

Thanks

[...]
Pavel Koshevoy March 19, 2021, 3:02 p.m. UTC | #6
On Fri, Mar 19, 2021 at 7:58 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Wed, Mar 17, 2021 at 06:19:26PM -0600, Pavel Koshevoy wrote:
> > Preserve AV_PKT_FLAG_CORRUPT so the caller can decide whether to drop
> > the packet.
> > ---
> >  libavformat/utils.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index a73f944e6e..0dc978e3d2 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -1494,7 +1494,8 @@ static int parse_packet(AVFormatContext *s,
> AVPacket *pkt,
> >          out_pkt->pts          = st->parser->pts;
> >          out_pkt->dts          = st->parser->dts;
> >          out_pkt->pos          = st->parser->pos;
> > -        out_pkt->flags       |= pkt->flags & AV_PKT_FLAG_DISCARD;
> > +        out_pkt->flags       |= pkt->flags & (AV_PKT_FLAG_CORRUPT |
> > +                                              AV_PKT_FLAG_DISCARD);
>
> 1. assume the parsers input are a stream of 4kb packets, the output is
> lets say 101kb
> packets. Assumingh one input packet is corrupted which output packet if any
> gets the flag set and why is that the correct one ?
>
> 2. assume the parsers input are a stream of 128kb packets, the output is
> lets say 3kb
> packets. Assuming one input packet is corrupted which output packet if any
> gets the flag set and why is that the correct one ?
>


In both cases -- any output packet that may contain any part of the
corrupted input packet should be marked corrupted as well.
How likely is the 2nd case?  In my case the source is a UDP mpegts stream,
so input packets are small (188 bytes), much smaller than the typical
output packet size.

Pavel.
Michael Niedermayer March 19, 2021, 4:46 p.m. UTC | #7
On Fri, Mar 19, 2021 at 09:02:44AM -0600, Pavel Koshevoy wrote:
> On Fri, Mar 19, 2021 at 7:58 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Wed, Mar 17, 2021 at 06:19:26PM -0600, Pavel Koshevoy wrote:
> > > Preserve AV_PKT_FLAG_CORRUPT so the caller can decide whether to drop
> > > the packet.
> > > ---
> > >  libavformat/utils.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index a73f944e6e..0dc978e3d2 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -1494,7 +1494,8 @@ static int parse_packet(AVFormatContext *s,
> > AVPacket *pkt,
> > >          out_pkt->pts          = st->parser->pts;
> > >          out_pkt->dts          = st->parser->dts;
> > >          out_pkt->pos          = st->parser->pos;
> > > -        out_pkt->flags       |= pkt->flags & AV_PKT_FLAG_DISCARD;
> > > +        out_pkt->flags       |= pkt->flags & (AV_PKT_FLAG_CORRUPT |
> > > +                                              AV_PKT_FLAG_DISCARD);
> >
> > 1. assume the parsers input are a stream of 4kb packets, the output is
> > lets say 101kb
> > packets. Assumingh one input packet is corrupted which output packet if any
> > gets the flag set and why is that the correct one ?
> >
> > 2. assume the parsers input are a stream of 128kb packets, the output is
> > lets say 3kb
> > packets. Assuming one input packet is corrupted which output packet if any
> > gets the flag set and why is that the correct one ?
> >
> 
> 
> In both cases -- any output packet that may contain any part of the
> corrupted input packet should be marked corrupted as well.

Thats a reasonable choice but i dont think thats what the patch does.
But even if this is fixed, there are still some umcomforatble cases
For example if you have a 128k input packet with 1 corrupted byte
and that is split to 128 1kb packets, marking every as corrupted
feels uncomfortable to me as 127 of them are not corrupted.
That is the meaning of AV_PKT_FLAG_CORRUPT is defined as 
"The packet content is corrupted" but in the example here in 99% of the
packets marked as corrupted there would be no corruption.

Now to make it a bit worse, we have parsers that work by reading
the next packet size and then packaging that. Corruption an happen
in that size itself. At that point the following packets can be corrupted
just because things got out of sync without any damage in the bytes of
the packets.

Maybe side data instead of a single bit flag would be better.
Specifying what range of the packet is corrupted.
For example a single corrupted 188byte input packet shouldnt cause
a 100kb size frame to be discarded, That could contain multiple
independantly decodeable slices which are known to be not corrupted


> How likely is the 2nd case?  In my case the source is a UDP mpegts stream,
> so input packets are small (188 bytes), much smaller than the typical
> output packet size.

I expect its common for audio codecs with small frames


> 
> Pavel.
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavformat/utils.c b/libavformat/utils.c
index a73f944e6e..0dc978e3d2 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -1494,7 +1494,8 @@  static int parse_packet(AVFormatContext *s, AVPacket *pkt,
         out_pkt->pts          = st->parser->pts;
         out_pkt->dts          = st->parser->dts;
         out_pkt->pos          = st->parser->pos;
-        out_pkt->flags       |= pkt->flags & AV_PKT_FLAG_DISCARD;
+        out_pkt->flags       |= pkt->flags & (AV_PKT_FLAG_CORRUPT |
+                                              AV_PKT_FLAG_DISCARD);
 
         if (st->need_parsing == AVSTREAM_PARSE_FULL_RAW)
             out_pkt->pos = st->parser->frame_offset;