| Message ID | GV1P250MB0737575554F114CCDEDBD6618F4C9@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/4] avcodec/h264_redundant_pps_bsf: Don't remove PPS | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/configure_x86 | warning | Failed to run configure |
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@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
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@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
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) {
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@outlook.com> --- libavcodec/h264_redundant_pps_bsf.c | 11 ----------- 1 file changed, 11 deletions(-)