diff mbox series

[FFmpeg-devel,v6,15/22] cbs_h264: Add support for frame packing arrangement SEI messages

Message ID 20200727163237.23371-16-sw@jkqxz.net
State New
Headers show
Series CBS unit type tables and assorted related stuff
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Mark Thompson July 27, 2020, 4:32 p.m. UTC
---
 libavcodec/cbs_h264.h                 | 24 ++++++++++++++++
 libavcodec/cbs_h2645.c                |  1 +
 libavcodec/cbs_h264_syntax_template.c | 40 +++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)

Comments

Andreas Rheinhardt Aug. 11, 2020, 8:13 p.m. UTC | #1
Mark Thompson:
> ---
>  libavcodec/cbs_h264.h                 | 24 ++++++++++++++++
>  libavcodec/cbs_h2645.c                |  1 +
>  libavcodec/cbs_h264_syntax_template.c | 40 +++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
> 
> diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h
> index 88313629f5..ca717a3b57 100644
> --- a/libavcodec/cbs_h264.h
> +++ b/libavcodec/cbs_h264.h
> @@ -296,6 +296,29 @@ typedef struct H264RawSEIRecoveryPoint {
>      uint8_t changing_slice_group_idc;
>  } H264RawSEIRecoveryPoint;
>  
> +typedef struct H264RawSEIFramePackingArrangement {
> +    uint32_t frame_packing_arrangement_id;
> +    uint8_t  frame_packing_arrangement_cancel_flag;
> +
> +    uint8_t  frame_packing_arrangement_type;
> +    uint8_t  quincunx_sampling_flag;
> +    uint8_t  content_interpretation_type;
> +    uint8_t  spatial_flipping_flag;
> +    uint8_t  frame0_flipped_flag;
> +    uint8_t  field_views_flag;
> +    uint8_t  current_frame_is_frame0_flag;
> +    uint8_t  frame0_self_contained_flag;
> +    uint8_t  frame1_self_contained_flag;
> +    uint8_t  frame0_grid_position_x;
> +    uint8_t  frame0_grid_position_y;
> +    uint8_t  frame1_grid_position_x;
> +    uint8_t  frame1_grid_position_y;
> +    uint8_t  frame_packing_arrangement_reserved_byte;
> +    uint16_t frame_packing_arrangement_repetition_period;
> +
> +    uint8_t  frame_packing_arrangement_extension_flag;
> +} H264RawSEIFramePackingArrangement;
> +
>  typedef struct H264RawSEIDisplayOrientation {
>      uint8_t display_orientation_cancel_flag;
>      uint8_t hor_flip;
> @@ -329,6 +352,7 @@ typedef struct H264RawSEIPayload {
>          H264RawSEIUserDataRegistered user_data_registered;
>          H264RawSEIUserDataUnregistered user_data_unregistered;
>          H264RawSEIRecoveryPoint recovery_point;
> +        H264RawSEIFramePackingArrangement frame_packing_arrangement;
>          H264RawSEIDisplayOrientation display_orientation;
>          H264RawSEIMasteringDisplayColourVolume mastering_display_colour_volume;
>          H264RawSEIAlternativeTransferCharacteristics
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index 603017d8fe..1677731db9 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -1331,6 +1331,7 @@ void ff_cbs_h264_free_sei_payload(H264RawSEIPayload *payload)
>      case H264_SEI_TYPE_PIC_TIMING:
>      case H264_SEI_TYPE_PAN_SCAN_RECT:
>      case H264_SEI_TYPE_RECOVERY_POINT:
> +    case H264_SEI_TYPE_FRAME_PACKING:
>      case H264_SEI_TYPE_DISPLAY_ORIENTATION:
>      case H264_SEI_TYPE_MASTERING_DISPLAY_COLOUR_VOLUME:
>      case H264_SEI_TYPE_ALTERNATIVE_TRANSFER:
> diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c
> index b65460996b..7880b1472c 100644
> --- a/libavcodec/cbs_h264_syntax_template.c
> +++ b/libavcodec/cbs_h264_syntax_template.c
> @@ -779,6 +779,42 @@ static int FUNC(sei_recovery_point)(CodedBitstreamContext *ctx, RWContext *rw,
>      return 0;
>  }
>  
> +static int FUNC(sei_frame_packing_arrangement)(CodedBitstreamContext *ctx, RWContext *rw,
> +                                               H264RawSEIFramePackingArrangement *current)
> +{
> +    int err;
> +
> +    HEADER("Frame Packing Arrangement");
> +
> +    ue(frame_packing_arrangement_id, 0, UINT32_MAX - 1);
> +    flag(frame_packing_arrangement_cancel_flag);
> +
> +    if (!current->frame_packing_arrangement_cancel_flag) {
> +        u(7, frame_packing_arrangement_type, 0, 7);

Only values 0-7 are currently defined, yet all other values are reserved
for future use. Same goes for content_interpretation_type and
frame_packing_arrangement_reserved_byte. We typically don't error out if
values reserved for future use are encountered (see for example the
matrix and transfer stuff in the VUI).

The same goes for content_interpretation_type and
frame_packing_arrangement_reserved_byte.

> +        flag(quincunx_sampling_flag);
> +        u(6, content_interpretation_type, 0, 2);
> +        flag(spatial_flipping_flag);
> +        flag(frame0_flipped_flag);
> +        flag(field_views_flag);
> +        flag(current_frame_is_frame0_flag);
> +        flag(frame0_self_contained_flag);
> +        flag(frame1_self_contained_flag);
> +        if (!current->quincunx_sampling_flag &&
> +            current->frame_packing_arrangement_type != 5) {
> +            ub(4, frame0_grid_position_x);
> +            ub(4, frame0_grid_position_y);
> +            ub(4, frame1_grid_position_x);
> +            ub(4, frame1_grid_position_y);
> +        }
> +        u(8, frame_packing_arrangement_reserved_byte, 0, 0);
> +        ue(frame_packing_arrangement_repetition_period, 0, 16384);
> +    }
> +
> +    flag(frame_packing_arrangement_extension_flag);

If this flag is set (where the value 1 is illegal for samples conforming
to the current version of the spec), then there will be further data
which should be ignored by decoders conforming to this version of the
standard. Chances are that this SEI message will run afoul of the
byte-alignment check or the incorrect SEI payload length check below
(which mysteriously does not check for overreads, but only for
underreads). I wish we could simply passthrough the data that follows,
but unfortunately we don't know whether the data is already byte-aligned
or not (unless the last byte of the SEI is zero). We could skip all the
remaining bits and infer frame_packing_arrangement_extension_flag to be
zero if it is 1.

Notice that it is possible for one of the reserved values of one of the
fields above to require the presence of extension data, i.e. if we
simply pass the above fields through and ignore the extension, we might
create invalid output. I don't have a good solution for this.

- Andreas
Mark Thompson Aug. 12, 2020, 9:04 p.m. UTC | #2
On 11/08/2020 21:13, Andreas Rheinhardt wrote:
> Mark Thompson:
>> ---
>>   libavcodec/cbs_h264.h                 | 24 ++++++++++++++++
>>   libavcodec/cbs_h2645.c                |  1 +
>>   libavcodec/cbs_h264_syntax_template.c | 40 +++++++++++++++++++++++++++
>>   3 files changed, 65 insertions(+)
>>
>> diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h
>> index 88313629f5..ca717a3b57 100644
>> --- a/libavcodec/cbs_h264.h
>> +++ b/libavcodec/cbs_h264.h
>> @@ -296,6 +296,29 @@ typedef struct H264RawSEIRecoveryPoint {
>>       uint8_t changing_slice_group_idc;
>>   } H264RawSEIRecoveryPoint;
>>   
>> +typedef struct H264RawSEIFramePackingArrangement {
>> +    uint32_t frame_packing_arrangement_id;
>> +    uint8_t  frame_packing_arrangement_cancel_flag;
>> +
>> +    uint8_t  frame_packing_arrangement_type;
>> +    uint8_t  quincunx_sampling_flag;
>> +    uint8_t  content_interpretation_type;
>> +    uint8_t  spatial_flipping_flag;
>> +    uint8_t  frame0_flipped_flag;
>> +    uint8_t  field_views_flag;
>> +    uint8_t  current_frame_is_frame0_flag;
>> +    uint8_t  frame0_self_contained_flag;
>> +    uint8_t  frame1_self_contained_flag;
>> +    uint8_t  frame0_grid_position_x;
>> +    uint8_t  frame0_grid_position_y;
>> +    uint8_t  frame1_grid_position_x;
>> +    uint8_t  frame1_grid_position_y;
>> +    uint8_t  frame_packing_arrangement_reserved_byte;
>> +    uint16_t frame_packing_arrangement_repetition_period;
>> +
>> +    uint8_t  frame_packing_arrangement_extension_flag;
>> +} H264RawSEIFramePackingArrangement;
>> +
>>   typedef struct H264RawSEIDisplayOrientation {
>>       uint8_t display_orientation_cancel_flag;
>>       uint8_t hor_flip;
>> @@ -329,6 +352,7 @@ typedef struct H264RawSEIPayload {
>>           H264RawSEIUserDataRegistered user_data_registered;
>>           H264RawSEIUserDataUnregistered user_data_unregistered;
>>           H264RawSEIRecoveryPoint recovery_point;
>> +        H264RawSEIFramePackingArrangement frame_packing_arrangement;
>>           H264RawSEIDisplayOrientation display_orientation;
>>           H264RawSEIMasteringDisplayColourVolume mastering_display_colour_volume;
>>           H264RawSEIAlternativeTransferCharacteristics
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index 603017d8fe..1677731db9 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -1331,6 +1331,7 @@ void ff_cbs_h264_free_sei_payload(H264RawSEIPayload *payload)
>>       case H264_SEI_TYPE_PIC_TIMING:
>>       case H264_SEI_TYPE_PAN_SCAN_RECT:
>>       case H264_SEI_TYPE_RECOVERY_POINT:
>> +    case H264_SEI_TYPE_FRAME_PACKING:
>>       case H264_SEI_TYPE_DISPLAY_ORIENTATION:
>>       case H264_SEI_TYPE_MASTERING_DISPLAY_COLOUR_VOLUME:
>>       case H264_SEI_TYPE_ALTERNATIVE_TRANSFER:
>> diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c
>> index b65460996b..7880b1472c 100644
>> --- a/libavcodec/cbs_h264_syntax_template.c
>> +++ b/libavcodec/cbs_h264_syntax_template.c
>> @@ -779,6 +779,42 @@ static int FUNC(sei_recovery_point)(CodedBitstreamContext *ctx, RWContext *rw,
>>       return 0;
>>   }
>>   
>> +static int FUNC(sei_frame_packing_arrangement)(CodedBitstreamContext *ctx, RWContext *rw,
>> +                                               H264RawSEIFramePackingArrangement *current)
>> +{
>> +    int err;
>> +
>> +    HEADER("Frame Packing Arrangement");
>> +
>> +    ue(frame_packing_arrangement_id, 0, UINT32_MAX - 1);
>> +    flag(frame_packing_arrangement_cancel_flag);
>> +
>> +    if (!current->frame_packing_arrangement_cancel_flag) {
>> +        u(7, frame_packing_arrangement_type, 0, 7);
> 
> Only values 0-7 are currently defined, yet all other values are reserved
> for future use. Same goes for content_interpretation_type and
> frame_packing_arrangement_reserved_byte. We typically don't error out if
> values reserved for future use are encountered (see for example the
> matrix and transfer stuff in the VUI).
> 
> The same goes for content_interpretation_type and
> frame_packing_arrangement_reserved_byte.
> 
>> +        flag(quincunx_sampling_flag);
>> +        u(6, content_interpretation_type, 0, 2);
>> +        flag(spatial_flipping_flag);
>> +        flag(frame0_flipped_flag);
>> +        flag(field_views_flag);
>> +        flag(current_frame_is_frame0_flag);
>> +        flag(frame0_self_contained_flag);
>> +        flag(frame1_self_contained_flag);
>> +        if (!current->quincunx_sampling_flag &&
>> +            current->frame_packing_arrangement_type != 5) {
>> +            ub(4, frame0_grid_position_x);
>> +            ub(4, frame0_grid_position_y);
>> +            ub(4, frame1_grid_position_x);
>> +            ub(4, frame1_grid_position_y);
>> +        }
>> +        u(8, frame_packing_arrangement_reserved_byte, 0, 0);
>> +        ue(frame_packing_arrangement_repetition_period, 0, 16384);
>> +    }
>> +
>> +    flag(frame_packing_arrangement_extension_flag);
> 
> If this flag is set (where the value 1 is illegal for samples conforming
> to the current version of the spec), then there will be further data
> which should be ignored by decoders conforming to this version of the
> standard. Chances are that this SEI message will run afoul of the
> byte-alignment check or the incorrect SEI payload length check below
> (which mysteriously does not check for overreads, but only for
> underreads). I wish we could simply passthrough the data that follows,
> but unfortunately we don't know whether the data is already byte-aligned
> or not (unless the last byte of the SEI is zero). We could skip all the
> remaining bits and infer frame_packing_arrangement_extension_flag to be
> zero if it is 1.
> 
> Notice that it is possible for one of the reserved values of one of the
> fields above to require the presence of extension data, i.e. if we
> simply pass the above fields through and ignore the extension, we might
> create invalid output. I don't have a good solution for this.

As discussed on IRC, the one change I've made here is to disallow extensions by changing this line to:

+    u(1, frame_packing_arrangement_extension_flag, 0, 0);

Thanks,

- Mark
diff mbox series

Patch

diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h
index 88313629f5..ca717a3b57 100644
--- a/libavcodec/cbs_h264.h
+++ b/libavcodec/cbs_h264.h
@@ -296,6 +296,29 @@  typedef struct H264RawSEIRecoveryPoint {
     uint8_t changing_slice_group_idc;
 } H264RawSEIRecoveryPoint;
 
+typedef struct H264RawSEIFramePackingArrangement {
+    uint32_t frame_packing_arrangement_id;
+    uint8_t  frame_packing_arrangement_cancel_flag;
+
+    uint8_t  frame_packing_arrangement_type;
+    uint8_t  quincunx_sampling_flag;
+    uint8_t  content_interpretation_type;
+    uint8_t  spatial_flipping_flag;
+    uint8_t  frame0_flipped_flag;
+    uint8_t  field_views_flag;
+    uint8_t  current_frame_is_frame0_flag;
+    uint8_t  frame0_self_contained_flag;
+    uint8_t  frame1_self_contained_flag;
+    uint8_t  frame0_grid_position_x;
+    uint8_t  frame0_grid_position_y;
+    uint8_t  frame1_grid_position_x;
+    uint8_t  frame1_grid_position_y;
+    uint8_t  frame_packing_arrangement_reserved_byte;
+    uint16_t frame_packing_arrangement_repetition_period;
+
+    uint8_t  frame_packing_arrangement_extension_flag;
+} H264RawSEIFramePackingArrangement;
+
 typedef struct H264RawSEIDisplayOrientation {
     uint8_t display_orientation_cancel_flag;
     uint8_t hor_flip;
@@ -329,6 +352,7 @@  typedef struct H264RawSEIPayload {
         H264RawSEIUserDataRegistered user_data_registered;
         H264RawSEIUserDataUnregistered user_data_unregistered;
         H264RawSEIRecoveryPoint recovery_point;
+        H264RawSEIFramePackingArrangement frame_packing_arrangement;
         H264RawSEIDisplayOrientation display_orientation;
         H264RawSEIMasteringDisplayColourVolume mastering_display_colour_volume;
         H264RawSEIAlternativeTransferCharacteristics
diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index 603017d8fe..1677731db9 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -1331,6 +1331,7 @@  void ff_cbs_h264_free_sei_payload(H264RawSEIPayload *payload)
     case H264_SEI_TYPE_PIC_TIMING:
     case H264_SEI_TYPE_PAN_SCAN_RECT:
     case H264_SEI_TYPE_RECOVERY_POINT:
+    case H264_SEI_TYPE_FRAME_PACKING:
     case H264_SEI_TYPE_DISPLAY_ORIENTATION:
     case H264_SEI_TYPE_MASTERING_DISPLAY_COLOUR_VOLUME:
     case H264_SEI_TYPE_ALTERNATIVE_TRANSFER:
diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c
index b65460996b..7880b1472c 100644
--- a/libavcodec/cbs_h264_syntax_template.c
+++ b/libavcodec/cbs_h264_syntax_template.c
@@ -779,6 +779,42 @@  static int FUNC(sei_recovery_point)(CodedBitstreamContext *ctx, RWContext *rw,
     return 0;
 }
 
+static int FUNC(sei_frame_packing_arrangement)(CodedBitstreamContext *ctx, RWContext *rw,
+                                               H264RawSEIFramePackingArrangement *current)
+{
+    int err;
+
+    HEADER("Frame Packing Arrangement");
+
+    ue(frame_packing_arrangement_id, 0, UINT32_MAX - 1);
+    flag(frame_packing_arrangement_cancel_flag);
+
+    if (!current->frame_packing_arrangement_cancel_flag) {
+        u(7, frame_packing_arrangement_type, 0, 7);
+        flag(quincunx_sampling_flag);
+        u(6, content_interpretation_type, 0, 2);
+        flag(spatial_flipping_flag);
+        flag(frame0_flipped_flag);
+        flag(field_views_flag);
+        flag(current_frame_is_frame0_flag);
+        flag(frame0_self_contained_flag);
+        flag(frame1_self_contained_flag);
+        if (!current->quincunx_sampling_flag &&
+            current->frame_packing_arrangement_type != 5) {
+            ub(4, frame0_grid_position_x);
+            ub(4, frame0_grid_position_y);
+            ub(4, frame1_grid_position_x);
+            ub(4, frame1_grid_position_y);
+        }
+        u(8, frame_packing_arrangement_reserved_byte, 0, 0);
+        ue(frame_packing_arrangement_repetition_period, 0, 16384);
+    }
+
+    flag(frame_packing_arrangement_extension_flag);
+
+    return 0;
+}
+
 static int FUNC(sei_display_orientation)(CodedBitstreamContext *ctx, RWContext *rw,
                                          H264RawSEIDisplayOrientation *current)
 {
@@ -879,6 +915,10 @@  static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw,
         CHECK(FUNC(sei_display_orientation)
               (ctx, rw, &current->payload.display_orientation));
         break;
+    case H264_SEI_TYPE_FRAME_PACKING:
+        CHECK(FUNC(sei_frame_packing_arrangement)
+              (ctx, rw, &current->payload.frame_packing_arrangement));
+        break;
     case H264_SEI_TYPE_MASTERING_DISPLAY_COLOUR_VOLUME:
         CHECK(FUNC(sei_mastering_display_colour_volume)
               (ctx, rw, &current->payload.mastering_display_colour_volume));