[FFmpeg-devel] [PATCH 2/2] avcodec/h26[45]_metadata_bsf: Use separate contexts for reading/writing

[Andreas Rheinhardt]

Mark Thompson:
> On 06/07/2020 01:53, Andreas Rheinhardt wrote:
>> Currently, both bsfs used the same CodedBitstreamContext for reading and
>> writing; as a consequence, the state of the writer's context at the
>> beginning of writing a fragment is exactly the state of the reader after
>> having read the fragment; in particular, the writer might not have
>> encountered one of its active parameter sets yet.
>>
>> This is not nice and may lead to invalid output even when the input
>> is completely spec-compliant: Think of an access unit containing
>> a primary coded picture referencing a PPS with id id (that is known from
>> an earlier access unit/from extradata), then a new version of the PPS
>> with id id and then a redundant coded picture that is also referencing
>> the PPS with id id. This is spec-compliant, as the standard allows to
>> overwrite a PPS with a different PPS in between coded pictures and not
>> only at the beginning of an access unit. In this scenario, the reader
>> would read the primary coded picture with the old PPS and the redundant
>> coded picture with the new PPS (as it should); yet the writer would
>> write both with the new PPS as extradata which might lead to errors or
>> to invalid data being output without any error (e.g. if the two PPS
>> differed in redundant_pic_cnt_present_flag).
>>
>> The above scenario does not directly translate to HEVC as long as one
>> restricts oneself to input with nuh_layer_id == 0 only (as cbs_h265
>> does: it currently strips away any NAL unit with nuh_layer_id > 0 when
>> decomposing); if one doesn't the same issue as above can happen.
>>
>> If one also allowed input packets to contain more than one access unit,
>> issues like the above can happen even without redundant coded
>> pictures/multiple layers.
>>
>> Therefore this commit uses separate contexts for reader and writer.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> This is an alternative to James patch [1] which instead uses separate
>> reader and writer parameter sets in the same CodedBitstreamContext.
> 
> I do prefer this approach.
> 
>> The diff would be bigger if it were not for the preceding patch (in this
>> case one would have to choose one of the two contexts to add/delete
>> units and as soon as one has to do this, one notices that none of the
>> two choices make any sense).
>>
>>   libavcodec/h264_metadata_bsf.c | 23 ++++++++++++++---------
>>   libavcodec/h265_metadata_bsf.c | 23 ++++++++++++++---------
>>   2 files changed, 28 insertions(+), 18 deletions(-)
>>
>> diff --git a/libavcodec/h264_metadata_bsf.c
>> b/libavcodec/h264_metadata_bsf.c
>> index 09aa1765fd..9f081a3879 100644
>> --- a/libavcodec/h264_metadata_bsf.c
>> +++ b/libavcodec/h264_metadata_bsf.c
>> @@ -49,7 +49,8 @@ enum {
>>   typedef struct H264MetadataContext {
>>       const AVClass *class;
>>   -    CodedBitstreamContext *cbc;
>> +    CodedBitstreamContext *cbc_in;
>> +    CodedBitstreamContext *cbc_out;
> 
> The old name "cbc" there was just a placeholder because it couldn't be
> "".  Given that, just "in" and "out" might be nicer, or "read" and
> "write"?  (Feel free to ignore this if you don't agree.)
> 
I opted for "input" and "output", thereby making the naming consistent
with h264_redundant_pps.

- Andreas