[FFmpeg-devel] [PATCH 2/2] avcodec/utils: keep CORRUPT and TRUSTED packet flags when parsing packets

[Marton Balint]

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