| Message ID | 20210318001926.29398-1-pkoshevoy@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/utils: Preserve AV_PKT_FLAG_CORRUPT | expand |
| 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 |
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".
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".
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.
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. [...]
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 [...]
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.
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 --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;