[FFmpeg-devel] [PATCH] h264dec: h264_select_output_frame() - fill correctly has_b_frames based on the level

[Jaroslav Kysela]

Dne 9.11.2017 v 21:38 Jaroslav Kysela napsal(a):
> The current code does not handle correctly the situation when sps->num_reoder_frames
> was set using the level. The incorrect has_b_frames results in the wrong DTS guess and
> malformed output (wrong 'Non-monotonous DTS' fixup).
> 
> sps->num_reorder_frames is set in the function ff_h264_decode_seq_parameter_set(),
> see comment: 'if the maximum delay is not stored in the SPS, derive it based on the
> level'.
> 
> Issue: #6810
> ---
>  libavcodec/h264_slice.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 35dcabd611..04b10656e0 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -1287,6 +1287,7 @@ static int h264_select_output_frame(H264Context *h)
>      h->mmco_reset = 0;
>  
>      if (sps->bitstream_restriction_flag ||
> +        sps->ref_frame_count ||
>          h->avctx->strict_std_compliance >= FF_COMPLIANCE_STRICT) {
>          h->avctx->has_b_frames = FFMAX(h->avctx->has_b_frames, sps->num_reorder_frames);
>      }
> 

Hendrik Leppkes wrote:

> This behavior has actually been intentional in the past to keep the
> decode delay lower.

The source of this check is:

  commit fb19e1443bc45e192545d3485ddb9c76e7d77855
  Author: Michael Niedermayer <michaelni at gmx.at>
  Date:   Sat Jul 19 16:16:00 2008 +0000

    Take the brain amputated spec literally if the user asks for it
(-strict 1).
    That is, add 16 frames delay, cache trashing and av desync.
    fixes at least the following reference bitstreams:
    CABA3_Sony_C.jsv
    CACQP3_Sony_D.jsv
    CAMANL1_TOSHIBA_B.264
    CANL3_Sony_C.jsv
    CVBS3_Sony_C.jsv
    CVMANL1_TOSHIBA_B.264

    Originally committed as revision 14308 to
svn://svn.ffmpeg.org/ffmpeg/trun

But it does not take in account the level based num_reorder_frames (when
ref_frame_count is available). I think that the original idea was to
avoid to set has_b_frames unconditionally to 16. With the proposed
change, has_b_frames are 4 for my problematic input stream.

Again, the output stream is broken (timestamps) when the source stream
has those parameters, so I think that the default behavior of ffmpeg
should be changed.

> If this is supposed to be changed (which needs to be discussed first),
> the std_compliance value should probably be adjusted to NORMAL instead
> of strict, instead of adding the additional condition here
> (num_reorder_frames is only set from the profile if ref_frame_count is
> > 0 in the first place).

Perhaps, my original patch in issue #6810 is better then - remove all
conditions and keep only:

  h->avctx->has_b_frames = FFMAX(h->avctx->has_b_frames,
sps->num_reorder_frames);

Please CC: me, I'm not subscribed to the list.

				Thanks,
					Jaroslav


-- 
Jaroslav Kysela <perex at perex.cz>