[FFmpeg-devel] [PATCH] Correctly set low_delay_hrd_flag when rewriting fixed_frame_rate_flag in h264_metadata bitstream filter

[Keshav Varma]

Rereading the spec, I think it only makes sense to update the
low_delay_hrd_flag parameter when the bitstream filter option for
fixed_frame_rate_flag is 1 to avoid clobbering the existing value in
other cases. I tried to work through all the scenarios below:

<fixed_frame_rate_flag>/<low_delay_hrd_flag> (BSF options) -> Resulting flags:

0/0 (tick_rate=10) -> 0/0
0/1 (tick_rate=10) -> 0/1
1/0 (tick_rate=10) -> 1/0
1/1 (tick_rate=10) -> 1/1
0/0 (fixed_frame_rate_flag=0) -> 0/1
0/1 (fixed_frame_rate_flag=0) -> 0/1
1/0 (fixed_frame_rate_flag=0) -> 0/0
1/1 (fixed_frame_rate_flag=0) -> 0/1
0/0 (fixed_frame_rate_flag=1) -> 1/0
0/1 (fixed_frame_rate_flag=1) -> 1/0
1/0 (fixed_frame_rate_flag=1) -> 1/0
1/1 (fixed_frame_rate_flag=1) -> 1/0


Signed-off-by: Keshav Varma <keshavdv at gmail.com>
---
 libavcodec/h264_metadata_bsf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
index 9df99cbae3..3e558a5166 100644
--- a/libavcodec/h264_metadata_bsf.c
+++ b/libavcodec/h264_metadata_bsf.c
@@ -228,7 +228,13 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
         sps->vui.timing_info_present_flag = 1;
         need_vui = 1;
     }
+
+    // Set fixed frame rate flag and update low_delay_hrd_flag to match
     SET_VUI_FIELD(fixed_frame_rate_flag);
+    if (ctx->fixed_frame_rate_flag) {
+        sps->vui.low_delay_hrd_flag = 0;
+    }
+
     if (ctx->zero_new_constraint_set_flags) {
         sps->constraint_set4_flag = 0;
         sps->constraint_set5_flag = 0;
--
2.31.1


On Fri, Feb 4, 2022 at 3:00 PM Keshav Varma <keshavdv at gmail.com> wrote:
>
> After changes like ef13faf, the h264_metadata bitstream filter stopped
> working when using the fixed_frame_rate_flag option on an input stream
> that doesn't contain VUI because the default inferred value of
> low_hrd_delay_flag seems to be 1. ffmpeg used to raise a warning, but
> proceeded anyway but now aborts after the other fixes since the output
> is rightfully invalid.
>
> I believe this change makes the bitstream filter conform to page 403
> of the ITU spec: "When fixed_frame_rate_flag is equal to 1,
> low_delay_hrd_flag shall be equal to 0. When low_delay_hrd_flag is not
> present, its value shall be inferred to be equal to 1 −
> fixed_frame_rate_flag."
>
> Signed-off-by: Keshav Varma <keshavdv at gmail.com>
> ---
>  libavcodec/h264_metadata_bsf.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index 9df99cbae3..59a7eba546 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -228,7 +228,13 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
>          sps->vui.timing_info_present_flag = 1;
>          need_vui = 1;
>      }
> -    SET_VUI_FIELD(fixed_frame_rate_flag);
> +
> +    // Set fixed frame rate flag and update low_delay_hrd_flag to match
> +    if (ctx->fixed_frame_rate_flag >= 0) {
> +        sps->vui.fixed_frame_rate_flag = ctx->fixed_frame_rate_flag;
> +        sps->vui.low_delay_hrd_flag = 1 - sps->vui.fixed_frame_rate_flag;
> +    }
> +
>      if (ctx->zero_new_constraint_set_flags) {
>          sps->constraint_set4_flag = 0;
>          sps->constraint_set5_flag = 0;
> --
> 2.31.1