[FFmpeg-devel] [PATCH v2 1/4] avcodec/h264_redundant_pps_bsf: Don't remove PPS

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Andreas Rheinhardt:
>> There is no check for whether these supposedly redundant PPS
>> are actually redundant. One could check via memcmp which would
>> work in practice* (because all content buffers are initially
>> zero-allocated), but this is not portable as compilers may
>> trash padding inside structures as they wish.
>>
>> In case the PPS is not really redundant the output is garbage.
>> This happens with several files from the FATE-suite. E.g.
>> h264-conformance/CVCANLMA2_Sony_C.jsv doesn't decode correctly
>> any more, whereas h264-conformance/CABA3_TOSHIBA_E.264 even
>> fails in ff_cbs_write_packet(), because the inferred value
>> of num_ref_idx_l0_active_minus1 mismatches with the value set
>> in the slice (this happens when num_ref_idx_l0_default_active_minus1
>> changes in the PPS; the value in the slice header is inferred from
>> the original PPS's num_ref_idx_l0_default_active_minus1).
>>
>> *: Unless slice_group_id is used, i.e. unless slice_group_map_type
>> is six.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>>  libavcodec/h264_redundant_pps_bsf.c | 11 -----------
>>  1 file changed, 11 deletions(-)
>>
>> diff --git a/libavcodec/h264_redundant_pps_bsf.c b/libavcodec/h264_redundant_pps_bsf.c
>> index f8bab1f109..df9a88a705 100644
>> --- a/libavcodec/h264_redundant_pps_bsf.c
>> +++ b/libavcodec/h264_redundant_pps_bsf.c
>> @@ -80,26 +80,15 @@ static int h264_redundant_pps_update_fragment(AVBSFContext *bsf,
>>                                                CodedBitstreamFragment *au)
>>  {
>>      H264RedundantPPSContext *ctx = bsf->priv_data;
>> -    int au_has_sps;
>>      int err, i;
>>  
>> -    au_has_sps = 0;
>>      for (i = 0; i < au->nb_units; i++) {
>>          CodedBitstreamUnit *nal = &au->units[i];
>>  
>> -        if (nal->type == H264_NAL_SPS)
>> -            au_has_sps = 1;
>>          if (nal->type == H264_NAL_PPS) {
>>              err = h264_redundant_pps_fixup_pps(ctx, nal);
>>              if (err < 0)
>>                  return err;
>> -            if (!au_has_sps) {
>> -                av_log(bsf, AV_LOG_VERBOSE, "Deleting redundant PPS "
>> -                       "at %"PRId64".\n", pkt->pts);
>> -                ff_cbs_delete_unit(au, i);
>> -                i--;
>> -                continue;
>> -            }
>>          }
>>          if (nal->type == H264_NAL_SLICE ||
>>              nal->type == H264_NAL_IDR_SLICE) {
> 
> I just noticed that the documentation contains the sentence "A new
> single global PPS is created, and all of the redundant PPSs within the
> stream are removed". As the observations in the commit message show,
> this is just not true because every PPS in an access unit without SPS is
> considered redundant, making this filter dangerous. So I would simply
> delete this sentence from the documentation. Is everyone ok with this?
> 

Will apply this patchset tomorrow unless there are objections.

- Andreas