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

[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?

- Andreas