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.
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.
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.
Regards, Marton _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
