diff mbox series

[FFmpeg-devel] avcodec/h264_metadata_bsf: Allow zeroing constraint_set4_flag and constraint_set5_flag

Message ID 20210623161243.488097-1-derek.buitenhuis@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel] avcodec/h264_metadata_bsf: Allow zeroing constraint_set4_flag and constraint_set5_flag | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Derek Buitenhuis June 23, 2021, 4:12 p.m. UTC
These bits are reserved in earlie versions of the H.264 spec, and
some poor hardware decoders require they are zero. Thus, it is useful
to be able to zero these on streams that may have them set. The result
is still a valid H.264 bitstream.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
---
e.g. x264 wrote these bits for several months before reverting that
functionality, since it broke several hardware decoders.
---
 doc/bitstream_filters.texi     | 5 +++++
 libavcodec/h264_metadata_bsf.c | 8 ++++++++
 2 files changed, 13 insertions(+)

Comments

James Almer June 24, 2021, 1:23 a.m. UTC | #1
On 6/23/2021 1:12 PM, Derek Buitenhuis wrote:
> These bits are reserved in earlie versions of the H.264 spec, and
> some poor hardware decoders require they are zero. Thus, it is useful
> to be able to zero these on streams that may have them set. The result
> is still a valid H.264 bitstream.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> e.g. x264 wrote these bits for several months before reverting that
> functionality, since it broke several hardware decoders.
> ---
>   doc/bitstream_filters.texi     | 5 +++++
>   libavcodec/h264_metadata_bsf.c | 8 ++++++++
>   2 files changed, 13 insertions(+)
> 
> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
> index 60e729484d..81220638d4 100644
> --- a/doc/bitstream_filters.texi
> +++ b/doc/bitstream_filters.texi
> @@ -253,6 +253,11 @@ Set whether the stream has fixed framerate - typically this indicates
>   that the framerate is exactly half the tick rate, but the exact
>   meaning is dependent on interlacing and the picture structure (see
>   H.264 section E.2.1 and table E-6).
> +@item zero_new_constraint_set_flags
> +Zero constraint_set4_flag and constraint_set5_flag in the SPS. These
> +bits were reserved in a previous version of the H.264 spec, and thus
> +some hardware encoders require these to be zero. The result of zeroing

Decoders.

> +this is still a valid bitstream.
>   
>   @item crop_left
>   @item crop_right
> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index ef74cba560..452a8ec5dc 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -61,6 +61,7 @@ typedef struct H264MetadataContext {
>   
>       AVRational tick_rate;
>       int fixed_frame_rate_flag;
> +    int zero_new_constraint_set_flags;
>   
>       int crop_left;
>       int crop_right;
> @@ -228,6 +229,10 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
>           need_vui = 1;
>       }
>       SET_VUI_FIELD(fixed_frame_rate_flag);
> +    if (ctx->zero_new_constraint_set_flags) {
> +        sps->constraint_set4_flag = 0;
> +        sps->constraint_set5_flag = 0;
> +    }
>   
>       if (sps->separate_colour_plane_flag || sps->chroma_format_idc == 0) {
>           crop_unit_x = 1;
> @@ -618,6 +623,9 @@ static const AVOption h264_metadata_options[] = {
>       { "fixed_frame_rate_flag", "Set VUI fixed frame rate flag",
>           OFFSET(fixed_frame_rate_flag), AV_OPT_TYPE_INT,
>           { .i64 = -1 }, -1, 1, FLAGS },
> +    { "zero_new_constraint_set_flags", "Set constraint_set4_flag / constraint_set5_flag to zero",
> +        OFFSET(zero_new_constraint_set_flags), AV_OPT_TYPE_BOOL,
> +        { .i64 = 0 }, 0, 1, FLAGS },
>   
>       { "crop_left", "Set left border crop offset",
>           OFFSET(crop_left), AV_OPT_TYPE_INT,

I guess it's fine, but Mark may have some comments about it.
Derek Buitenhuis June 24, 2021, 1:21 p.m. UTC | #2
On 24/06/2021 02:23, James Almer wrote:
>> +@item zero_new_constraint_set_flags
>> +Zero constraint_set4_flag and constraint_set5_flag in the SPS. These
>> +bits were reserved in a previous version of the H.264 spec, and thus
>> +some hardware encoders require these to be zero. The result of zeroing
> 
> Decoders.

Fixed locally. I also fixed the 'earlie' typo in the commit message.

> I guess it's fine, but Mark may have some comments about it.

I'll wait a bit and see if he has any.

- Derek
Derek Buitenhuis June 27, 2021, 11:56 a.m. UTC | #3
On 6/24/2021 2:21 PM, Derek Buitenhuis wrote:
> I'll wait a bit and see if he has any.

Ping.

- Derek
Derek Buitenhuis June 28, 2021, 1:25 p.m. UTC | #4
On 6/27/2021 12:56 PM, Derek Buitenhuis wrote:
> On 6/24/2021 2:21 PM, Derek Buitenhuis wrote:
>> I'll wait a bit and see if he has any.
> 
> Ping.

Will push tonight or tomorrow with typo fixes + lavc version bump
if there are no objections.

- Derek
diff mbox series

Patch

diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
index 60e729484d..81220638d4 100644
--- a/doc/bitstream_filters.texi
+++ b/doc/bitstream_filters.texi
@@ -253,6 +253,11 @@  Set whether the stream has fixed framerate - typically this indicates
 that the framerate is exactly half the tick rate, but the exact
 meaning is dependent on interlacing and the picture structure (see
 H.264 section E.2.1 and table E-6).
+@item zero_new_constraint_set_flags
+Zero constraint_set4_flag and constraint_set5_flag in the SPS. These
+bits were reserved in a previous version of the H.264 spec, and thus
+some hardware encoders require these to be zero. The result of zeroing
+this is still a valid bitstream.
 
 @item crop_left
 @item crop_right
diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
index ef74cba560..452a8ec5dc 100644
--- a/libavcodec/h264_metadata_bsf.c
+++ b/libavcodec/h264_metadata_bsf.c
@@ -61,6 +61,7 @@  typedef struct H264MetadataContext {
 
     AVRational tick_rate;
     int fixed_frame_rate_flag;
+    int zero_new_constraint_set_flags;
 
     int crop_left;
     int crop_right;
@@ -228,6 +229,10 @@  static int h264_metadata_update_sps(AVBSFContext *bsf,
         need_vui = 1;
     }
     SET_VUI_FIELD(fixed_frame_rate_flag);
+    if (ctx->zero_new_constraint_set_flags) {
+        sps->constraint_set4_flag = 0;
+        sps->constraint_set5_flag = 0;
+    }
 
     if (sps->separate_colour_plane_flag || sps->chroma_format_idc == 0) {
         crop_unit_x = 1;
@@ -618,6 +623,9 @@  static const AVOption h264_metadata_options[] = {
     { "fixed_frame_rate_flag", "Set VUI fixed frame rate flag",
         OFFSET(fixed_frame_rate_flag), AV_OPT_TYPE_INT,
         { .i64 = -1 }, -1, 1, FLAGS },
+    { "zero_new_constraint_set_flags", "Set constraint_set4_flag / constraint_set5_flag to zero",
+        OFFSET(zero_new_constraint_set_flags), AV_OPT_TYPE_BOOL,
+        { .i64 = 0 }, 0, 1, FLAGS },
 
     { "crop_left", "Set left border crop offset",
         OFFSET(crop_left), AV_OPT_TYPE_INT,