| Message ID | 20200727163237.23371-16-sw@jkqxz.net |
|---|---|
| State | New |
| Headers | show |
| Series | CBS unit type tables and assorted related stuff | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
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 --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, ¤t->payload.display_orientation)); break; + case H264_SEI_TYPE_FRAME_PACKING: + CHECK(FUNC(sei_frame_packing_arrangement) + (ctx, rw, ¤t->payload.frame_packing_arrangement)); + break; case H264_SEI_TYPE_MASTERING_DISPLAY_COLOUR_VOLUME: CHECK(FUNC(sei_mastering_display_colour_volume) (ctx, rw, ¤t->payload.mastering_display_colour_volume));