[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 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