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

[Mark Thompson]

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 

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

[Andreas Rheinhardt]

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