Message ID | CAMrYdZcAtvLjS8=H5=ypH6kJAC3hzDmq-Wcfi2cFcUUb9+HiaQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] Correctly set low_delay_hrd_flag when rewriting fixed_frame_rate_flag in h264_metadata bitstream filter | expand |
Context | Check | Description |
---|---|---|
andriy/commit_msg_aarch64_jetson | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
andriy/commit_msg_x86 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
andriy/commit_msg_ppc | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
andriy/make_ppc | success | Make finished |
andriy/make_fate_ppc | success | Make fate finished |
andriy/make_aarch64_jetson | success | Make finished |
andriy/make_fate_aarch64_jetson | success | Make fate finished |
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@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@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@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
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;
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@gmail.com> --- libavcodec/h264_metadata_bsf.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) -- 2.31.1