[FFmpeg-devel] [PATCH] hevc_mp4toannexb: Do not duplicate parameter sets

[Andreas Rheinhardt]

Andriy Gelman:
> From: Andriy Gelman <andriy.gelman at gmail.com>
> 
> Fixes #7799
> 
> Currently, the mp4toannexb filter always inserts extradata at the start
> of each IRAP unit. This can lead to duplication of parameter sets if the
> demuxed packet from mdat atom already contains a version of the
> parameters.
> 
> As in ticket #7799 this can also lead to decoding errors when the
> parameter sets of the IRAP frames are different from the ones stored in
> extradata.
> 
> This commit avoids duplicating the parameter sets if they are already
> present in the demuxed packet.
> 
> This commit also makes an update to the hevc-bsf-mp4toannexb fate
> test since the result before this patch contained duplicate vps/sps/pps
> nal units.
> ---
>  libavcodec/hevc_mp4toannexb_bsf.c | 9 ++++++++-
>  tests/fate/hevc.mak               | 2 +-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
> index 09bce5b34c..5c27306b09 100644
> --- a/libavcodec/hevc_mp4toannexb_bsf.c
> +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> @@ -123,6 +123,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
>  
>      int got_irap = 0;
>      int i, ret = 0;
> +    int vps_detected, sps_detected, pps_detected = 0;
>  
You are only initiallizing pps_detected.

>      ret = ff_bsf_get_packet(ctx, &in);
>      if (ret < 0)
> @@ -146,9 +147,15 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
>  
>          nalu_type = (bytestream2_peek_byte(&gb) >> 1) & 0x3f;
>  
> +        switch (nalu_type) {
> +          case HEVC_NAL_VPS: vps_detected = 1; break;
> +          case HEVC_NAL_SPS: sps_detected = 1; break;
> +          case HEVC_NAL_PPS: pps_detected = 1; break;
> +        }
> +
>          /* prepend extradata to IRAP frames */
>          is_irap       = nalu_type >= 16 && nalu_type <= 23;
> -        add_extradata = is_irap && !got_irap;
> +        add_extradata = is_irap && !got_irap && !(vps_detected && sps_detected && pps_detected);
>          extra_size    = add_extradata * ctx->par_out->extradata_size;
>          got_irap     |= is_irap;
>  
There are two things that I don't like about this approach:
1. Image an input file like this: VPS, SPS and PPS in extradata and
then after a few GOPs there is an inband PPS as part of a VPS, SPS and
PPS combination where the PPS differs from the PPS in the extradata.
After this change in parameter sets there are no further inband
parameter sets. With your proposal, the extradata would again be
inserted into access units with IRAP frames after the change in
extradata and it would be the old, outdated extradata, thereby
breaking decoding (the original mp4 file would probably not be able
fully seekable in this case, but that would be another story).
2. If the sample in #7799 contained only the parameter set that
actually changed inband, your proposal would still add a whole
VPS+SPS+PPS combination and therefore still make the sample unplayable.

- Andreas