[FFmpeg-devel] [PATCH 2/2] avcodec/utils: keep CORRUPT and TRUSTED packet flags when parsing packets
Michael Niedermayer
michael at niedermayer.cc
Fri Oct 12 02:45:56 EEST 2018
On Thu, Oct 11, 2018 at 10:39:54PM +0200, Marton Balint wrote:
>
>
> On Thu, 11 Oct 2018, Michael Niedermayer wrote:
>
> >On Wed, Oct 10, 2018 at 01:32:14AM +0200, Marton Balint wrote:
> >>An FF_ macro got defined in avcodec.h to store the flags which need to be
> >>propagated when parsers split packets so this won't be forgotten when a new
> >>packet flag is introduced.
> >>
> >>(I wonder if DISPOSABLE also fits here, or maybe some special handling is
> >>needed like it is done for the keyframe flag?)
> >>---
> >> libavcodec/avcodec.h | 9 ++++++++-
> >> libavcodec/version.h | 2 +-
> >> libavformat/utils.c | 2 +-
> >> tests/ref/fate/flv-demux | 2 +-
> >> tests/ref/fate/iv8-demux | 2 +-
> >> tests/ref/fate/segment-mp4-to-ts | 6 +++---
> >> tests/ref/fate/ts-demux | 2 +-
> >> 7 files changed, 16 insertions(+), 9 deletions(-)
> >>
> >>diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> >>index 705a3ce4f3..9a3f9b6226 100644
> >>--- a/libavcodec/avcodec.h
> >>+++ b/libavcodec/avcodec.h
> >>@@ -1493,7 +1493,14 @@ typedef struct AVPacket {
> >> * be discarded by the decoder. I.e. Non-reference frames.
> >> */
> >> #define AV_PKT_FLAG_DISPOSABLE 0x0010
> >>-
> >>+/**
> >>+ * Packet flags which must always be kept when parsers split packets
> >>+ */
> >>+#define FF_PKT_FLAGS_KEEP_WHEN_PARSING (\
> >>+ AV_PKT_FLAG_CORRUPT | \
> >>+ AV_PKT_FLAG_DISCARD | \
> >>+ AV_PKT_FLAG_TRUSTED | \
> >>+ 0)
> >>
> >> enum AVSideDataParamChangeFlags {
> >> AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT = 0x0001,
> >>diff --git a/libavcodec/version.h b/libavcodec/version.h
> >>index 97d134851f..79c5dc6773 100644
> >>--- a/libavcodec/version.h
> >>+++ b/libavcodec/version.h
> >>@@ -29,7 +29,7 @@
> >>
> >> #define LIBAVCODEC_VERSION_MAJOR 58
> >> #define LIBAVCODEC_VERSION_MINOR 32
> >>-#define LIBAVCODEC_VERSION_MICRO 100
> >>+#define LIBAVCODEC_VERSION_MICRO 101
> >>
> >> #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> >> LIBAVCODEC_VERSION_MINOR, \
> >>diff --git a/libavformat/utils.c b/libavformat/utils.c
> >>index a8ac90213e..351bd88fa5 100644
> >>--- a/libavformat/utils.c
> >>+++ b/libavformat/utils.c
> >>@@ -1511,7 +1511,7 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt, int stream_index)
> >> 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 & FF_PKT_FLAGS_KEEP_WHEN_PARSING;
> >
> >I think this is wrong
> >this looks like the packet flags are not passed through the parsers, so they would
> >not neccessarily be attached to the correct packets, there could be a delay
> >from a buffer ...
>
> True, this solution is not perfect, but probably better than what we have
> now, which is losing the corrupt flag entirely.
>
> >
> >more so, these flags cannot be handled identically
> >for example if there is output packet formed from concatenating a trusted and
> >untrusted input packet the output cannot be trusted because it is not just
> >trusted data
>
> OK, TRUSTED should be removed from the flags which are propagated.
>
> >
> >compared to corrupt, if there is output packet formed from concatenating a
> >corrupt and non-corrupt input packet the output still is corrupt
> >
> >It gets further complicated if only part of a AV_PKT_FLAG_CORRUPT input is
> >used. You cant mark the output as corrupt in this case because as documented
> >AV_PKT_FLAG_CORRUPT means the packet IS corrupt but a subpart of it may be
> >corrupt or may be undamaged.
>
> IMHO there is no harm in setting corrupt flag if part of the (or the whole)
> packet comes from a corrupt source. Suspicion is enough. Losing the flag is
> a much bigger issue.
ATM the documentation says "The packet content is corrupted"
If you want to set the flag when this is not true then the documentation
would need to be changed. Otherwise the code would violate the API
and the change of the documentation strictly speaking breaks API
and needs to be documented in APIChanges.
Yes iam quite aware that is taking things very strict and litteral here
but i think thats what we should aim for with APIs. The documentation
should be strictly correct or should make it clear when/where it is
not.
>
> >A finer granularity of corruption likelyness would be needed here.
> >(checksums matching, probably ok - no checksums, possibly corrupt,
> >certainly some corrupt, certain significant corruption)
> >Independant of this there could be information about packet truncation.
> >a packet content could be known to be correct but possibly incomplete
> >All these cases differ in how they would have to be passed on when packets
> >are merged / split
>
> Seems a bit of overdesign to me, a typical user app either bails out on
> error, or try decoding no matter how corrupted the input is, so not much
> point in signalling stuff between. That is also why I think that it is OK if
> we define AV_PKT_FLAG_CORRUPT as _suspected_ corruption, because it does not
> affect either use case.
I think archival people would like to know exactly which parts of their
archive are corrupted
>
> Anyway, I still think setting the flag as it is done for AV_PKT_FLAG_DISCARD
> is better than ignoring it. Do you see a way to implement propagating the
> flag in a more sophisticated way with reasonable amount of work? On first
> look, the parser.c code seems pretty scary.
Well, i dont have a magic bullet here. But for timestamps, file position
and keyframe flags we go to great lengths to keep them attached to the
correct packet. Why should we not do this for the flags ?
also with the corrupt flag not only could the current code mark
undamaged parts of output packets as AV_PKT_FLAG_CORRUPT but also
the other way around.
If for example there are 5 input packets which comprise 1 output
packet, the corrupt flag would only be taken from one and so theres
a good chance a corrupt input would not be maked as corrupt on
output.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20181012/9594eeb8/attachment.sig>
More information about the ffmpeg-devel
mailing list