| Message ID | 20180420072747.15054-1-haihao.xiang@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On 20/04/18 08:27, Haihao Xiang wrote: > Similar to cbs_h264, cbs_h265_{read, write}_nal_unit() can handle HEVC > prefix SEI NAL units now. Currently mastering display colour volume SEI > message is added only, we may add more SEI message if needed later > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > --- > libavcodec/cbs_h2645.c | 43 ++++++++++ > libavcodec/cbs_h265.h | 38 +++++++++ > libavcodec/cbs_h265_syntax_template.c | 150 ++++++++++++++++++++++++++++++++++ > 3 files changed, 231 insertions(+) > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > index 5585831cf6..6fc5832966 100644 > --- a/libavcodec/cbs_h2645.c > +++ b/libavcodec/cbs_h2645.c > @@ -29,6 +29,7 @@ > #include "h264_sei.h" > #include "h2645_parse.h" > #include "hevc.h" > +#include "hevc_sei.h" > > > static int cbs_read_ue_golomb(CodedBitstreamContext *ctx, GetBitContext *gbc, > @@ -465,6 +466,26 @@ static void cbs_h265_free_slice(void *unit, uint8_t *content) > av_freep(&content); > } > > +static void cbs_h265_free_sei_payload(H265RawSEIPayload *payload) > +{ > + switch (payload->payload_type) { > + case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: > + break; > + default: > + av_buffer_unref(&payload->payload.other.data_ref); > + break; > + } > +} > + > +static void cbs_h265_free_sei(void *unit, uint8_t *content) > +{ > + H265RawSEI *sei = (H265RawSEI*)content; > + int i; > + for (i = 0; i < sei->payload_count; i++) > + cbs_h265_free_sei_payload(&sei->payload[i]); > + av_freep(&content); > +} > + > static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > const H2645Packet *packet) > @@ -972,6 +993,20 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx, > } > break; > > + case HEVC_NAL_SEI_PREFIX: I don't think it would be too hard to handling SEI_SUFFIX here too? (The different set of valid payloads doesn't matter when we aren't checking that below.) > + err = ff_cbs_alloc_unit_content(ctx, unit, sizeof(H265RawSEI), > + &cbs_h265_free_sei); > + > + if (err < 0) > + return err; > + > + err = cbs_h265_read_sei(ctx, &gbc, unit->content); > + > + if (err < 0) > + return err; > + > + break; > + > default: > return AVERROR(ENOSYS); > } > @@ -1212,6 +1247,14 @@ static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, > } > break; > > + case HEVC_NAL_SEI_PREFIX: > + err = cbs_h265_write_sei(ctx, pbc, unit->content); > + > + if (err < 0) > + return err; > + > + break; > + Please make these cases match the styling of the others. > default: > av_log(ctx->log_ctx, AV_LOG_ERROR, "Write unimplemented for " > "NAL unit type %"PRIu32".\n", unit->type); > diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h > index 33e71fc234..e37c1b8d94 100644 > --- a/libavcodec/cbs_h265.h > +++ b/libavcodec/cbs_h265.h > @@ -25,6 +25,14 @@ > #include "cbs_h2645.h" > #include "hevc.h" > > +enum { > + // This limit is arbitrary - it is sufficient for one message of each > + // type plus some repeats, and will therefore easily cover all sane > + // streams. However, it is possible to make technically-valid streams > + // for which it will fail (for example, by including a large number of > + // user-data-unregistered messages). > + H265_MAX_SEI_PAYLOADS = 64, > +}; > > typedef struct H265RawNALUnitHeader { > uint8_t forbidden_zero_bit; > @@ -516,6 +524,36 @@ typedef struct H265RawSlice { > AVBufferRef *data_ref; > } H265RawSlice; > > +typedef struct H265RawSEIMasteringDiplayColorVolume { The H.265 standard uses English spelling of colour, not USAian. > + struct { > + uint16_t x; > + uint16_t y; > + } display_primaries[3]; Make it two arrays, display_primaries_x and display_primaries_y, so that it matches the standard. > + uint16_t white_point_x; > + uint16_t white_point_y; > + uint32_t max_display_mastering_luminance; > + uint32_t min_display_mastering_luminance; > +} H265RawSEIMasteringDiplayColorVolume; > + > +typedef struct H265RawSEIPayload { > + uint32_t payload_type; > + uint32_t payload_size; > + union { > + H265RawSEIMasteringDiplayColorVolume mastering_display; > + struct { > + uint8_t *data; > + size_t data_length; > + AVBufferRef *data_ref; > + } other; > + } payload; > +} H265RawSEIPayload; > + > +typedef struct H265RawSEI { > + H265RawNALUnitHeader nal_unit_header; > + > + H265RawSEIPayload payload[H265_MAX_SEI_PAYLOADS]; > + uint8_t payload_count; > +} H265RawSEI; > > typedef struct CodedBitstreamH265Context { > // Reader/writer context in common with the H.264 implementation. > diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c > index 140c827c9d..cbe8f30be0 100644 > --- a/libavcodec/cbs_h265_syntax_template.c > +++ b/libavcodec/cbs_h265_syntax_template.c > @@ -1501,3 +1501,153 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw, > > return 0; > } > + > +static int FUNC(sei_mastering_display)(CodedBitstreamContext *ctx, RWContext *rw, > + H265RawSEIMasteringDiplayColorVolume *current) > +{ > + int err, i; > + > + for (i = 0; i < 3; i++) { This variable is called 'c' in the standard (which matters when it ends up in the trace name). > + xu(16, display_primaries_x, current->display_primaries[i].x, 0, 50000); > + xu(16, display_primaries_y, current->display_primaries[i].y, 0, 50000); > + } > + > + xu(16, white_point_x, current->white_point_x, 0, 50000); > + xu(16, white_point_y, current->white_point_y, 0, 50000); > + > + xu(32, max_display_mastering_luminance, current->max_display_mastering_luminance, 0, 0xFFFFFFFF); MAX_UINT_BITS(32) Also break the overlong line. > + xu(32, min_display_mastering_luminance, current->min_display_mastering_luminance, 0, 0xFFFFFFFF); You could use "min_display_mastering_luminance shall be less than max_display_mastering_luminance" as a bound here? All of these should use the u() macro rather than xu(), since they aren't writing to a different variable. > + > + return 0; > +} > + > +static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, > + H265RawSEIPayload *current) > +{ > + int err, i; > + int start_position, end_position; > + > +#ifdef READ > + start_position = get_bits_count(rw); > +#else > + start_position = put_bits_count(rw); > +#endif > + > + switch (current->payload_type) { > + case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: > + CHECK(FUNC(sei_mastering_display) > + (ctx, rw, ¤t->payload.mastering_display)); > + > + break; > + > + default: > + allocate(current->payload.other.data, current->payload_size); > + > + for (i = 0; i < current->payload_size; i++) > + xu(8, payload_byte, current->payload.other.data[i], 0, 255); > + } > + > + if (byte_alignment(rw)) { > + av_unused int one = 1, zero = 0; > + xu(1, bit_equal_to_one, one, 1, 1); > + while (byte_alignment(rw)) > + xu(1, bit_equal_to_zero, zero, 0, 0); > + } > + > +#ifdef READ > + end_position = get_bits_count(rw); > + if (end_position < start_position + 8 * current->payload_size) { > + av_log(ctx->log_ctx, AV_LOG_ERROR, "Incorrect SEI payload length: " > + "header %"PRIu32" bits, actually %d bits.\n", > + 8 * current->payload_size, > + end_position - start_position); > + return AVERROR_INVALIDDATA; > + } > +#else > + end_position = put_bits_count(rw); > + current->payload_size = (end_position - start_position) >> 3; > +#endif > + > + return 0; > +} > + > +static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, > + H265RawSEI *current) > +{ > + int err, k; > + > + HEADER("Supplemental Enhancement Information"); > + > + CHECK(FUNC(nal_unit_header)(ctx, rw, ¤t->nal_unit_header, HEVC_NAL_SEI_PREFIX)); > + > +#ifdef READ > + for (k = 0; k < H265_MAX_SEI_PAYLOADS; k++) { > + uint32_t payload_type = 0; > + uint32_t payload_size = 0; > + uint32_t tmp; > + > + while (show_bits(rw, 8) == 0xff) { > + xu(8, ff_byte, tmp, 0xff, 0xff); > + payload_type += 255; > + } > + xu(8, last_payload_type_byte, tmp, 0, 254); > + payload_type += tmp; > + > + while (show_bits(rw, 8) == 0xff) { > + xu(8, ff_byte, tmp, 0xff, 0xff); > + payload_size += 255; > + } > + xu(8, last_payload_size_byte, tmp, 0, 254); > + payload_size += tmp; > + > + current->payload[k].payload_type = payload_type; > + current->payload[k].payload_size = payload_size; > + > + CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); > + > + if (!cbs_h2645_read_more_rbsp_data(rw)) > + break; > + } > + if (k >= H265_MAX_SEI_PAYLOADS) { > + av_log(ctx->log_ctx, AV_LOG_ERROR, "Too many payloads in " > + "SEI message: found %d.\n", k); > + return AVERROR_INVALIDDATA; > + } > + current->payload_count = k + 1; > +#else > + for (k = 0; k < current->payload_count; k++) { > + PutBitContext start_state; > + uint32_t tmp; > + int need_size, i; > + > + // Somewhat clumsy: we write the payload twice when > + // we don't know the size in advance. This will mess > + // with trace output, but is otherwise harmless. > + start_state = *rw; > + need_size = !current->payload[k].payload_size; > + for (i = 0; i < 1 + need_size; i++) { > + *rw = start_state; > + > + tmp = current->payload[k].payload_type; > + while (tmp >= 255) { > + xu(8, ff_byte, 0xff, 0xff, 0xff); > + tmp -= 255; > + } > + xu(8, last_payload_type_byte, tmp, 0, 254); > + > + tmp = current->payload[k].payload_size; > + while (tmp >= 255) { > + xu(8, ff_byte, 0xff, 0xff, 0xff); > + tmp -= 255; > + } > + xu(8, last_payload_size_byte, tmp, 0, 254); > + > + CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); > + } > + } > +#endif I admit you're copying what I wrote in the equivalent code for H.264, but this is still kindof horrible. Any thoughts on how to do this more nicely are welcome... > + > + CHECK(FUNC(rbsp_trailing_bits)(ctx, rw)); > + > + return 0; > +} > Looks generally ok, thank you for working on this! - Mark
> On 20/04/18 08:27, Haihao Xiang wrote: > > Similar to cbs_h264, cbs_h265_{read, write}_nal_unit() can handle HEVC > > prefix SEI NAL units now. Currently mastering display colour volume SEI > > message is added only, we may add more SEI message if needed later > > > > Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> > > --- > > libavcodec/cbs_h2645.c | 43 ++++++++++ > > libavcodec/cbs_h265.h | 38 +++++++++ > > libavcodec/cbs_h265_syntax_template.c | 150 > > ++++++++++++++++++++++++++++++++++ > > 3 files changed, 231 insertions(+) > > > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > > index 5585831cf6..6fc5832966 100644 > > --- a/libavcodec/cbs_h2645.c > > +++ b/libavcodec/cbs_h2645.c > > @@ -29,6 +29,7 @@ > > #include "h264_sei.h" > > #include "h2645_parse.h" > > #include "hevc.h" > > +#include "hevc_sei.h" > > > > > > static int cbs_read_ue_golomb(CodedBitstreamContext *ctx, GetBitContext > > *gbc, > > @@ -465,6 +466,26 @@ static void cbs_h265_free_slice(void *unit, uint8_t > > *content) > > av_freep(&content); > > } > > > > +static void cbs_h265_free_sei_payload(H265RawSEIPayload *payload) > > +{ > > + switch (payload->payload_type) { > > + case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: > > + break; > > + default: > > + av_buffer_unref(&payload->payload.other.data_ref); > > + break; > > + } > > +} > > + > > +static void cbs_h265_free_sei(void *unit, uint8_t *content) > > +{ > > + H265RawSEI *sei = (H265RawSEI*)content; > > + int i; > > + for (i = 0; i < sei->payload_count; i++) > > + cbs_h265_free_sei_payload(&sei->payload[i]); > > + av_freep(&content); > > +} > > + > > static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx, > > CodedBitstreamFragment *frag, > > const H2645Packet *packet) > > @@ -972,6 +993,20 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext > > *ctx, > > } > > break; > > > > + case HEVC_NAL_SEI_PREFIX: > > I don't think it would be too hard to handling SEI_SUFFIX here too? (The > different set of valid payloads doesn't matter when we aren't checking that > below.) > I also don't think so. But it seems most pre-defined SEI types in hevc_sei.h are SEI_PREFIX, hence my thought is we may add support for SEI_SUFFIX in the future if SEI_SUFFIX is really needed. > > + err = ff_cbs_alloc_unit_content(ctx, unit, sizeof(H265RawSEI), > > + &cbs_h265_free_sei); > > + > > + if (err < 0) > > + return err; > > + > > + err = cbs_h265_read_sei(ctx, &gbc, unit->content); > > + > > + if (err < 0) > > + return err; > > + > > + break; > > + > > default: > > return AVERROR(ENOSYS); > > } > > @@ -1212,6 +1247,14 @@ static int > > cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, > > } > > break; > > > > + case HEVC_NAL_SEI_PREFIX: > > + err = cbs_h265_write_sei(ctx, pbc, unit->content); > > + > > + if (err < 0) > > + return err; > > + > > + break; > > + > > Please make these cases match the styling of the others. > My original patch used the same coding style of the others, but patcheck in FFmpeg reported a warning below: { should be on the same line as the related previous statement 0001-vaapi_encode_h265-Insert-SEI-if-needed.patch:24:+ { I will update it if you prefer the same coding style. > > default: > > av_log(ctx->log_ctx, AV_LOG_ERROR, "Write unimplemented for " > > "NAL unit type %"PRIu32".\n", unit->type); > > diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h > > index 33e71fc234..e37c1b8d94 100644 > > --- a/libavcodec/cbs_h265.h > > +++ b/libavcodec/cbs_h265.h > > @@ -25,6 +25,14 @@ > > #include "cbs_h2645.h" > > #include "hevc.h" > > > > +enum { > > + // This limit is arbitrary - it is sufficient for one message of each > > + // type plus some repeats, and will therefore easily cover all sane > > + // streams. However, it is possible to make technically-valid streams > > + // for which it will fail (for example, by including a large number of > > + // user-data-unregistered messages). > > + H265_MAX_SEI_PAYLOADS = 64, > > +}; > > > > typedef struct H265RawNALUnitHeader { > > uint8_t forbidden_zero_bit; > > @@ -516,6 +524,36 @@ typedef struct H265RawSlice { > > AVBufferRef *data_ref; > > } H265RawSlice; > > > > +typedef struct H265RawSEIMasteringDiplayColorVolume { > > The H.265 standard uses English spelling of colour, not USAian. Thanks, I will update it. > > > + struct { > > + uint16_t x; > > + uint16_t y; > > + } display_primaries[3]; > > Make it two arrays, display_primaries_x and display_primaries_y, so that it > matches the standard. Thanks, I will update it. > > > + uint16_t white_point_x; > > + uint16_t white_point_y; > > + uint32_t max_display_mastering_luminance; > > + uint32_t min_display_mastering_luminance; > > +} H265RawSEIMasteringDiplayColorVolume; > > + > > +typedef struct H265RawSEIPayload { > > + uint32_t payload_type; > > + uint32_t payload_size; > > + union { > > + H265RawSEIMasteringDiplayColorVolume mastering_display; > > + struct { > > + uint8_t *data; > > + size_t data_length; > > + AVBufferRef *data_ref; > > + } other; > > + } payload; > > +} H265RawSEIPayload; > > + > > +typedef struct H265RawSEI { > > + H265RawNALUnitHeader nal_unit_header; > > + > > + H265RawSEIPayload payload[H265_MAX_SEI_PAYLOADS]; > > + uint8_t payload_count; > > +} H265RawSEI; > > > > typedef struct CodedBitstreamH265Context { > > // Reader/writer context in common with the H.264 implementation. > > diff --git a/libavcodec/cbs_h265_syntax_template.c > > b/libavcodec/cbs_h265_syntax_template.c > > index 140c827c9d..cbe8f30be0 100644 > > --- a/libavcodec/cbs_h265_syntax_template.c > > +++ b/libavcodec/cbs_h265_syntax_template.c > > @@ -1501,3 +1501,153 @@ static int > > FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw, > > > > return 0; > > } > > + > > +static int FUNC(sei_mastering_display)(CodedBitstreamContext *ctx, > > RWContext *rw, > > + H265RawSEIMasteringDiplayColorVolume > > *current) > > +{ > > + int err, i; > > + > > + for (i = 0; i < 3; i++) { > > This variable is called 'c' in the standard (which matters when it ends up in > the trace name). Thanks, I will update it. > > > + xu(16, display_primaries_x, current->display_primaries[i].x, 0, > > 50000); > > + xu(16, display_primaries_y, current->display_primaries[i].y, 0, > > 50000); > > + } > > + > > + xu(16, white_point_x, current->white_point_x, 0, 50000); > > + xu(16, white_point_y, current->white_point_y, 0, 50000); > > + > > + xu(32, max_display_mastering_luminance, current- > > >max_display_mastering_luminance, 0, 0xFFFFFFFF); > > MAX_UINT_BITS(32) > > Also break the overlong line. Thanks, I will update it. > > > + xu(32, min_display_mastering_luminance, current- > > >min_display_mastering_luminance, 0, 0xFFFFFFFF); > > You could use "min_display_mastering_luminance shall be less than > max_display_mastering_luminance" as a bound here? yes, i will update it. > > All of these should use the u() macro rather than xu(), since they aren't > writing to a different variable. > OK, I will use the u() macro in the new version of the patch. > > + > > + return 0; > > +} > > + > > +static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, > > + H265RawSEIPayload *current) > > +{ > > + int err, i; > > + int start_position, end_position; > > + > > +#ifdef READ > > + start_position = get_bits_count(rw); > > +#else > > + start_position = put_bits_count(rw); > > +#endif > > + > > + switch (current->payload_type) { > > + case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: > > + CHECK(FUNC(sei_mastering_display) > > + (ctx, rw, ¤t->payload.mastering_display)); > > + > > + break; > > + > > + default: > > + allocate(current->payload.other.data, current->payload_size); > > + > > + for (i = 0; i < current->payload_size; i++) > > + xu(8, payload_byte, current->payload.other.data[i], 0, 255); > > + } > > + > > + if (byte_alignment(rw)) { > > + av_unused int one = 1, zero = 0; > > + xu(1, bit_equal_to_one, one, 1, 1); > > + while (byte_alignment(rw)) > > + xu(1, bit_equal_to_zero, zero, 0, 0); > > + } > > + > > +#ifdef READ > > + end_position = get_bits_count(rw); > > + if (end_position < start_position + 8 * current->payload_size) { > > + av_log(ctx->log_ctx, AV_LOG_ERROR, "Incorrect SEI payload length: " > > + "header %"PRIu32" bits, actually %d bits.\n", > > + 8 * current->payload_size, > > + end_position - start_position); > > + return AVERROR_INVALIDDATA; > > + } > > +#else > > + end_position = put_bits_count(rw); > > + current->payload_size = (end_position - start_position) >> 3; > > +#endif > > + > > + return 0; > > +} > > + > > +static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, > > + H265RawSEI *current) > > +{ > > + int err, k; > > + > > + HEADER("Supplemental Enhancement Information"); > > + > > + CHECK(FUNC(nal_unit_header)(ctx, rw, ¤t->nal_unit_header, > > HEVC_NAL_SEI_PREFIX)); > > + > > +#ifdef READ > > + for (k = 0; k < H265_MAX_SEI_PAYLOADS; k++) { > > + uint32_t payload_type = 0; > > + uint32_t payload_size = 0; > > + uint32_t tmp; > > + > > + while (show_bits(rw, 8) == 0xff) { > > + xu(8, ff_byte, tmp, 0xff, 0xff); > > + payload_type += 255; > > + } > > + xu(8, last_payload_type_byte, tmp, 0, 254); > > + payload_type += tmp; > > + > > + while (show_bits(rw, 8) == 0xff) { > > + xu(8, ff_byte, tmp, 0xff, 0xff); > > + payload_size += 255; > > + } > > + xu(8, last_payload_size_byte, tmp, 0, 254); > > + payload_size += tmp; > > + > > + current->payload[k].payload_type = payload_type; > > + current->payload[k].payload_size = payload_size; > > + > > + CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); > > + > > + if (!cbs_h2645_read_more_rbsp_data(rw)) > > + break; > > + } > > + if (k >= H265_MAX_SEI_PAYLOADS) { > > + av_log(ctx->log_ctx, AV_LOG_ERROR, "Too many payloads in " > > + "SEI message: found %d.\n", k); > > + return AVERROR_INVALIDDATA; > > + } > > + current->payload_count = k + 1; > > +#else > > + for (k = 0; k < current->payload_count; k++) { > > + PutBitContext start_state; > > + uint32_t tmp; > > + int need_size, i; > > + > > + // Somewhat clumsy: we write the payload twice when > > + // we don't know the size in advance. This will mess > > + // with trace output, but is otherwise harmless. > > + start_state = *rw; > > + need_size = !current->payload[k].payload_size; > > + for (i = 0; i < 1 + need_size; i++) { > > + *rw = start_state; > > + > > + tmp = current->payload[k].payload_type; > > + while (tmp >= 255) { > > + xu(8, ff_byte, 0xff, 0xff, 0xff); > > + tmp -= 255; > > + } > > + xu(8, last_payload_type_byte, tmp, 0, 254); > > + > > + tmp = current->payload[k].payload_size; > > + while (tmp >= 255) { > > + xu(8, ff_byte, 0xff, 0xff, 0xff); > > + tmp -= 255; > > + } > > + xu(8, last_payload_size_byte, tmp, 0, 254); > > + > > + CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); > > + } > > + } > > +#endif > > I admit you're copying what I wrote in the equivalent code for H.264, but this > is still kindof horrible. Any thoughts on how to do this more nicely are > welcome... Do you mean writing the payload twice in the code or re-using the code in this way? > > + > > + CHECK(FUNC(rbsp_trailing_bits)(ctx, rw)); > > + > > + return 0; > > +} > > > > Looks generally ok, thank you for working on this! Thanks for your review, I will update the patch. > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 23/04/18 06:55, Xiang, Haihao wrote: > >> On 20/04/18 08:27, Haihao Xiang wrote: >>> Similar to cbs_h264, cbs_h265_{read, write}_nal_unit() can handle HEVC >>> prefix SEI NAL units now. Currently mastering display colour volume SEI >>> message is added only, we may add more SEI message if needed later >>> >>> Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> >>> --- >>> libavcodec/cbs_h2645.c | 43 ++++++++++ >>> libavcodec/cbs_h265.h | 38 +++++++++ >>> libavcodec/cbs_h265_syntax_template.c | 150 >>> ++++++++++++++++++++++++++++++++++ >>> 3 files changed, 231 insertions(+) >>> >>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c >>> index 5585831cf6..6fc5832966 100644 >>> --- a/libavcodec/cbs_h2645.c >>> +++ b/libavcodec/cbs_h2645.c >>> @@ -29,6 +29,7 @@ >>> #include "h264_sei.h" >>> #include "h2645_parse.h" >>> #include "hevc.h" >>> +#include "hevc_sei.h" >>> >>> >>> static int cbs_read_ue_golomb(CodedBitstreamContext *ctx, GetBitContext >>> *gbc, >>> @@ -465,6 +466,26 @@ static void cbs_h265_free_slice(void *unit, uint8_t >>> *content) >>> av_freep(&content); >>> } >>> >>> +static void cbs_h265_free_sei_payload(H265RawSEIPayload *payload) >>> +{ >>> + switch (payload->payload_type) { >>> + case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: >>> + break; >>> + default: >>> + av_buffer_unref(&payload->payload.other.data_ref); >>> + break; >>> + } >>> +} >>> + >>> +static void cbs_h265_free_sei(void *unit, uint8_t *content) >>> +{ >>> + H265RawSEI *sei = (H265RawSEI*)content; >>> + int i; >>> + for (i = 0; i < sei->payload_count; i++) >>> + cbs_h265_free_sei_payload(&sei->payload[i]); >>> + av_freep(&content); >>> +} >>> + >>> static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx, >>> CodedBitstreamFragment *frag, >>> const H2645Packet *packet) >>> @@ -972,6 +993,20 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext >>> *ctx, >>> } >>> break; >>> >>> + case HEVC_NAL_SEI_PREFIX: >> >> I don't think it would be too hard to handling SEI_SUFFIX here too? (The >> different set of valid payloads doesn't matter when we aren't checking that >> below.) >> > > I also don't think so. But it seems most pre-defined SEI types in hevc_sei.h are > SEI_PREFIX, hence my thought is we may add support for SEI_SUFFIX in the future > if SEI_SUFFIX is really needed. Might be nice for trace_headers output? But yes, it's probably easier not too for now; so, sure, stay with what you wrote initially. >>> + err = ff_cbs_alloc_unit_content(ctx, unit, sizeof(H265RawSEI), >>> + &cbs_h265_free_sei); >>> + >>> + if (err < 0) >>> + return err; >>> + >>> + err = cbs_h265_read_sei(ctx, &gbc, unit->content); >>> + >>> + if (err < 0) >>> + return err; >>> + >>> + break; >>> + >>> default: >>> return AVERROR(ENOSYS); >>> } >>> @@ -1212,6 +1247,14 @@ static int >>> cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, >>> } >>> break; >>> >>> + case HEVC_NAL_SEI_PREFIX: >>> + err = cbs_h265_write_sei(ctx, pbc, unit->content); >>> + >>> + if (err < 0) >>> + return err; >>> + >>> + break; >>> + >> >> Please make these cases match the styling of the others. >> > > My original patch used the same coding style of the others, but patcheck in > FFmpeg reported a warning below: > > { should be on the same line as the related previous statement > 0001-vaapi_encode_h265-Insert-SEI-if-needed.patch:24:+ { > > I will update it if you prefer the same coding style. I think it's more important to match what's already there. (Also I don't agree with that test, isolated blocks are useful...) >>> default: >>> av_log(ctx->log_ctx, AV_LOG_ERROR, "Write unimplemented for " >>> "NAL unit type %"PRIu32".\n", unit->type); >>> diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h >>> index 33e71fc234..e37c1b8d94 100644 >>> --- a/libavcodec/cbs_h265.h >>> +++ b/libavcodec/cbs_h265.h >>> @@ -25,6 +25,14 @@ >>> #include "cbs_h2645.h" >>> #include "hevc.h" >>> >>> +enum { >>> + // This limit is arbitrary - it is sufficient for one message of each >>> + // type plus some repeats, and will therefore easily cover all sane >>> + // streams. However, it is possible to make technically-valid streams >>> + // for which it will fail (for example, by including a large number of >>> + // user-data-unregistered messages). >>> + H265_MAX_SEI_PAYLOADS = 64, >>> +}; >>> >>> typedef struct H265RawNALUnitHeader { >>> uint8_t forbidden_zero_bit; >>> @@ -516,6 +524,36 @@ typedef struct H265RawSlice { >>> AVBufferRef *data_ref; >>> } H265RawSlice; >>> >>> +typedef struct H265RawSEIMasteringDiplayColorVolume { >> >> The H.265 standard uses English spelling of colour, not USAian. > > Thanks, I will update it. > >> >>> + struct { >>> + uint16_t x; >>> + uint16_t y; >>> + } display_primaries[3]; >> >> Make it two arrays, display_primaries_x and display_primaries_y, so that it >> matches the standard. > > Thanks, I will update it. > >> >>> + uint16_t white_point_x; >>> + uint16_t white_point_y; >>> + uint32_t max_display_mastering_luminance; >>> + uint32_t min_display_mastering_luminance; >>> +} H265RawSEIMasteringDiplayColorVolume; >>> + >>> +typedef struct H265RawSEIPayload { >>> + uint32_t payload_type; >>> + uint32_t payload_size; >>> + union { >>> + H265RawSEIMasteringDiplayColorVolume mastering_display; >>> + struct { >>> + uint8_t *data; >>> + size_t data_length; >>> + AVBufferRef *data_ref; >>> + } other; >>> + } payload; >>> +} H265RawSEIPayload; >>> + >>> +typedef struct H265RawSEI { >>> + H265RawNALUnitHeader nal_unit_header; >>> + >>> + H265RawSEIPayload payload[H265_MAX_SEI_PAYLOADS]; >>> + uint8_t payload_count; >>> +} H265RawSEI; >>> >>> typedef struct CodedBitstreamH265Context { >>> // Reader/writer context in common with the H.264 implementation. >>> diff --git a/libavcodec/cbs_h265_syntax_template.c >>> b/libavcodec/cbs_h265_syntax_template.c >>> index 140c827c9d..cbe8f30be0 100644 >>> --- a/libavcodec/cbs_h265_syntax_template.c >>> +++ b/libavcodec/cbs_h265_syntax_template.c >>> @@ -1501,3 +1501,153 @@ static int >>> FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw, >>> >>> return 0; >>> } >>> + >>> +static int FUNC(sei_mastering_display)(CodedBitstreamContext *ctx, >>> RWContext *rw, >>> + H265RawSEIMasteringDiplayColorVolume >>> *current) >>> +{ >>> + int err, i; >>> + >>> + for (i = 0; i < 3; i++) { >> >> This variable is called 'c' in the standard (which matters when it ends up in >> the trace name). > > Thanks, I will update it. > >> >>> + xu(16, display_primaries_x, current->display_primaries[i].x, 0, >>> 50000); >>> + xu(16, display_primaries_y, current->display_primaries[i].y, 0, >>> 50000); >>> + } >>> + >>> + xu(16, white_point_x, current->white_point_x, 0, 50000); >>> + xu(16, white_point_y, current->white_point_y, 0, 50000); >>> + >>> + xu(32, max_display_mastering_luminance, current- >>>> max_display_mastering_luminance, 0, 0xFFFFFFFF); >> >> MAX_UINT_BITS(32) >> >> Also break the overlong line. > > Thanks, I will update it. > >> >>> + xu(32, min_display_mastering_luminance, current- >>>> min_display_mastering_luminance, 0, 0xFFFFFFFF); >> >> You could use "min_display_mastering_luminance shall be less than >> max_display_mastering_luminance" as a bound here? > > yes, i will update it. > >> >> All of these should use the u() macro rather than xu(), since they aren't >> writing to a different variable. >> > > OK, I will use the u() macro in the new version of the patch. > >>> + >>> + return 0; >>> +} >>> + >>> +static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, >>> + H265RawSEIPayload *current) >>> +{ >>> + int err, i; >>> + int start_position, end_position; >>> + >>> +#ifdef READ >>> + start_position = get_bits_count(rw); >>> +#else >>> + start_position = put_bits_count(rw); >>> +#endif >>> + >>> + switch (current->payload_type) { >>> + case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: >>> + CHECK(FUNC(sei_mastering_display) >>> + (ctx, rw, ¤t->payload.mastering_display)); >>> + >>> + break; >>> + >>> + default: >>> + allocate(current->payload.other.data, current->payload_size); >>> + >>> + for (i = 0; i < current->payload_size; i++) >>> + xu(8, payload_byte, current->payload.other.data[i], 0, 255); >>> + } >>> + >>> + if (byte_alignment(rw)) { >>> + av_unused int one = 1, zero = 0; >>> + xu(1, bit_equal_to_one, one, 1, 1); >>> + while (byte_alignment(rw)) >>> + xu(1, bit_equal_to_zero, zero, 0, 0); >>> + } >>> + >>> +#ifdef READ >>> + end_position = get_bits_count(rw); >>> + if (end_position < start_position + 8 * current->payload_size) { >>> + av_log(ctx->log_ctx, AV_LOG_ERROR, "Incorrect SEI payload length: " >>> + "header %"PRIu32" bits, actually %d bits.\n", >>> + 8 * current->payload_size, >>> + end_position - start_position); >>> + return AVERROR_INVALIDDATA; >>> + } >>> +#else >>> + end_position = put_bits_count(rw); >>> + current->payload_size = (end_position - start_position) >> 3; >>> +#endif >>> + >>> + return 0; >>> +} >>> + >>> +static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, >>> + H265RawSEI *current) >>> +{ >>> + int err, k; >>> + >>> + HEADER("Supplemental Enhancement Information"); >>> + >>> + CHECK(FUNC(nal_unit_header)(ctx, rw, ¤t->nal_unit_header, >>> HEVC_NAL_SEI_PREFIX)); >>> + >>> +#ifdef READ >>> + for (k = 0; k < H265_MAX_SEI_PAYLOADS; k++) { >>> + uint32_t payload_type = 0; >>> + uint32_t payload_size = 0; >>> + uint32_t tmp; >>> + >>> + while (show_bits(rw, 8) == 0xff) { >>> + xu(8, ff_byte, tmp, 0xff, 0xff); >>> + payload_type += 255; >>> + } >>> + xu(8, last_payload_type_byte, tmp, 0, 254); >>> + payload_type += tmp; >>> + >>> + while (show_bits(rw, 8) == 0xff) { >>> + xu(8, ff_byte, tmp, 0xff, 0xff); >>> + payload_size += 255; >>> + } >>> + xu(8, last_payload_size_byte, tmp, 0, 254); >>> + payload_size += tmp; >>> + >>> + current->payload[k].payload_type = payload_type; >>> + current->payload[k].payload_size = payload_size; >>> + >>> + CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); >>> + >>> + if (!cbs_h2645_read_more_rbsp_data(rw)) >>> + break; >>> + } >>> + if (k >= H265_MAX_SEI_PAYLOADS) { >>> + av_log(ctx->log_ctx, AV_LOG_ERROR, "Too many payloads in " >>> + "SEI message: found %d.\n", k); >>> + return AVERROR_INVALIDDATA; >>> + } >>> + current->payload_count = k + 1; >>> +#else >>> + for (k = 0; k < current->payload_count; k++) { >>> + PutBitContext start_state; >>> + uint32_t tmp; >>> + int need_size, i; >>> + >>> + // Somewhat clumsy: we write the payload twice when >>> + // we don't know the size in advance. This will mess >>> + // with trace output, but is otherwise harmless. >>> + start_state = *rw; >>> + need_size = !current->payload[k].payload_size; >>> + for (i = 0; i < 1 + need_size; i++) { >>> + *rw = start_state; >>> + >>> + tmp = current->payload[k].payload_type; >>> + while (tmp >= 255) { >>> + xu(8, ff_byte, 0xff, 0xff, 0xff); >>> + tmp -= 255; >>> + } >>> + xu(8, last_payload_type_byte, tmp, 0, 254); >>> + >>> + tmp = current->payload[k].payload_size; >>> + while (tmp >= 255) { >>> + xu(8, ff_byte, 0xff, 0xff, 0xff); >>> + tmp -= 255; >>> + } >>> + xu(8, last_payload_size_byte, tmp, 0, 254); >>> + >>> + CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); >>> + } >>> + } >>> +#endif >> >> I admit you're copying what I wrote in the equivalent code for H.264, but this >> is still kindof horrible. Any thoughts on how to do this more nicely are >> welcome... > > Do you mean writing the payload twice in the code or re-using the code in this > way? I meant how the original code worked, the writing twice in particular. I guess just ignore this - I'm complaining about ugliness in what I wrote before. (The re-use is of course correct because the behaviour between H.264 and H.265 is identical.) >>> + >>> + CHECK(FUNC(rbsp_trailing_bits)(ctx, rw)); >>> + >>> + return 0; >>> +} >>> >> >> Looks generally ok, thank you for working on this! > > Thanks for your review, I will update the patch. - Mark
diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c index 5585831cf6..6fc5832966 100644 --- a/libavcodec/cbs_h2645.c +++ b/libavcodec/cbs_h2645.c @@ -29,6 +29,7 @@ #include "h264_sei.h" #include "h2645_parse.h" #include "hevc.h" +#include "hevc_sei.h" static int cbs_read_ue_golomb(CodedBitstreamContext *ctx, GetBitContext *gbc, @@ -465,6 +466,26 @@ static void cbs_h265_free_slice(void *unit, uint8_t *content) av_freep(&content); } +static void cbs_h265_free_sei_payload(H265RawSEIPayload *payload) +{ + switch (payload->payload_type) { + case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: + break; + default: + av_buffer_unref(&payload->payload.other.data_ref); + break; + } +} + +static void cbs_h265_free_sei(void *unit, uint8_t *content) +{ + H265RawSEI *sei = (H265RawSEI*)content; + int i; + for (i = 0; i < sei->payload_count; i++) + cbs_h265_free_sei_payload(&sei->payload[i]); + av_freep(&content); +} + static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, const H2645Packet *packet) @@ -972,6 +993,20 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx, } break; + case HEVC_NAL_SEI_PREFIX: + err = ff_cbs_alloc_unit_content(ctx, unit, sizeof(H265RawSEI), + &cbs_h265_free_sei); + + if (err < 0) + return err; + + err = cbs_h265_read_sei(ctx, &gbc, unit->content); + + if (err < 0) + return err; + + break; + default: return AVERROR(ENOSYS); } @@ -1212,6 +1247,14 @@ static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx, } break; + case HEVC_NAL_SEI_PREFIX: + err = cbs_h265_write_sei(ctx, pbc, unit->content); + + if (err < 0) + return err; + + break; + default: av_log(ctx->log_ctx, AV_LOG_ERROR, "Write unimplemented for " "NAL unit type %"PRIu32".\n", unit->type); diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h index 33e71fc234..e37c1b8d94 100644 --- a/libavcodec/cbs_h265.h +++ b/libavcodec/cbs_h265.h @@ -25,6 +25,14 @@ #include "cbs_h2645.h" #include "hevc.h" +enum { + // This limit is arbitrary - it is sufficient for one message of each + // type plus some repeats, and will therefore easily cover all sane + // streams. However, it is possible to make technically-valid streams + // for which it will fail (for example, by including a large number of + // user-data-unregistered messages). + H265_MAX_SEI_PAYLOADS = 64, +}; typedef struct H265RawNALUnitHeader { uint8_t forbidden_zero_bit; @@ -516,6 +524,36 @@ typedef struct H265RawSlice { AVBufferRef *data_ref; } H265RawSlice; +typedef struct H265RawSEIMasteringDiplayColorVolume { + struct { + uint16_t x; + uint16_t y; + } display_primaries[3]; + uint16_t white_point_x; + uint16_t white_point_y; + uint32_t max_display_mastering_luminance; + uint32_t min_display_mastering_luminance; +} H265RawSEIMasteringDiplayColorVolume; + +typedef struct H265RawSEIPayload { + uint32_t payload_type; + uint32_t payload_size; + union { + H265RawSEIMasteringDiplayColorVolume mastering_display; + struct { + uint8_t *data; + size_t data_length; + AVBufferRef *data_ref; + } other; + } payload; +} H265RawSEIPayload; + +typedef struct H265RawSEI { + H265RawNALUnitHeader nal_unit_header; + + H265RawSEIPayload payload[H265_MAX_SEI_PAYLOADS]; + uint8_t payload_count; +} H265RawSEI; typedef struct CodedBitstreamH265Context { // Reader/writer context in common with the H.264 implementation. diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c index 140c827c9d..cbe8f30be0 100644 --- a/libavcodec/cbs_h265_syntax_template.c +++ b/libavcodec/cbs_h265_syntax_template.c @@ -1501,3 +1501,153 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw, return 0; } + +static int FUNC(sei_mastering_display)(CodedBitstreamContext *ctx, RWContext *rw, + H265RawSEIMasteringDiplayColorVolume *current) +{ + int err, i; + + for (i = 0; i < 3; i++) { + xu(16, display_primaries_x, current->display_primaries[i].x, 0, 50000); + xu(16, display_primaries_y, current->display_primaries[i].y, 0, 50000); + } + + xu(16, white_point_x, current->white_point_x, 0, 50000); + xu(16, white_point_y, current->white_point_y, 0, 50000); + + xu(32, max_display_mastering_luminance, current->max_display_mastering_luminance, 0, 0xFFFFFFFF); + xu(32, min_display_mastering_luminance, current->min_display_mastering_luminance, 0, 0xFFFFFFFF); + + return 0; +} + +static int FUNC(sei_payload)(CodedBitstreamContext *ctx, RWContext *rw, + H265RawSEIPayload *current) +{ + int err, i; + int start_position, end_position; + +#ifdef READ + start_position = get_bits_count(rw); +#else + start_position = put_bits_count(rw); +#endif + + switch (current->payload_type) { + case HEVC_SEI_TYPE_MASTERING_DISPLAY_INFO: + CHECK(FUNC(sei_mastering_display) + (ctx, rw, ¤t->payload.mastering_display)); + + break; + + default: + allocate(current->payload.other.data, current->payload_size); + + for (i = 0; i < current->payload_size; i++) + xu(8, payload_byte, current->payload.other.data[i], 0, 255); + } + + if (byte_alignment(rw)) { + av_unused int one = 1, zero = 0; + xu(1, bit_equal_to_one, one, 1, 1); + while (byte_alignment(rw)) + xu(1, bit_equal_to_zero, zero, 0, 0); + } + +#ifdef READ + end_position = get_bits_count(rw); + if (end_position < start_position + 8 * current->payload_size) { + av_log(ctx->log_ctx, AV_LOG_ERROR, "Incorrect SEI payload length: " + "header %"PRIu32" bits, actually %d bits.\n", + 8 * current->payload_size, + end_position - start_position); + return AVERROR_INVALIDDATA; + } +#else + end_position = put_bits_count(rw); + current->payload_size = (end_position - start_position) >> 3; +#endif + + return 0; +} + +static int FUNC(sei)(CodedBitstreamContext *ctx, RWContext *rw, + H265RawSEI *current) +{ + int err, k; + + HEADER("Supplemental Enhancement Information"); + + CHECK(FUNC(nal_unit_header)(ctx, rw, ¤t->nal_unit_header, HEVC_NAL_SEI_PREFIX)); + +#ifdef READ + for (k = 0; k < H265_MAX_SEI_PAYLOADS; k++) { + uint32_t payload_type = 0; + uint32_t payload_size = 0; + uint32_t tmp; + + while (show_bits(rw, 8) == 0xff) { + xu(8, ff_byte, tmp, 0xff, 0xff); + payload_type += 255; + } + xu(8, last_payload_type_byte, tmp, 0, 254); + payload_type += tmp; + + while (show_bits(rw, 8) == 0xff) { + xu(8, ff_byte, tmp, 0xff, 0xff); + payload_size += 255; + } + xu(8, last_payload_size_byte, tmp, 0, 254); + payload_size += tmp; + + current->payload[k].payload_type = payload_type; + current->payload[k].payload_size = payload_size; + + CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); + + if (!cbs_h2645_read_more_rbsp_data(rw)) + break; + } + if (k >= H265_MAX_SEI_PAYLOADS) { + av_log(ctx->log_ctx, AV_LOG_ERROR, "Too many payloads in " + "SEI message: found %d.\n", k); + return AVERROR_INVALIDDATA; + } + current->payload_count = k + 1; +#else + for (k = 0; k < current->payload_count; k++) { + PutBitContext start_state; + uint32_t tmp; + int need_size, i; + + // Somewhat clumsy: we write the payload twice when + // we don't know the size in advance. This will mess + // with trace output, but is otherwise harmless. + start_state = *rw; + need_size = !current->payload[k].payload_size; + for (i = 0; i < 1 + need_size; i++) { + *rw = start_state; + + tmp = current->payload[k].payload_type; + while (tmp >= 255) { + xu(8, ff_byte, 0xff, 0xff, 0xff); + tmp -= 255; + } + xu(8, last_payload_type_byte, tmp, 0, 254); + + tmp = current->payload[k].payload_size; + while (tmp >= 255) { + xu(8, ff_byte, 0xff, 0xff, 0xff); + tmp -= 255; + } + xu(8, last_payload_size_byte, tmp, 0, 254); + + CHECK(FUNC(sei_payload)(ctx, rw, ¤t->payload[k])); + } + } +#endif + + CHECK(FUNC(rbsp_trailing_bits)(ctx, rw)); + + return 0; +}
Similar to cbs_h264, cbs_h265_{read, write}_nal_unit() can handle HEVC prefix SEI NAL units now. Currently mastering display colour volume SEI message is added only, we may add more SEI message if needed later Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> --- libavcodec/cbs_h2645.c | 43 ++++++++++ libavcodec/cbs_h265.h | 38 +++++++++ libavcodec/cbs_h265_syntax_template.c | 150 ++++++++++++++++++++++++++++++++++ 3 files changed, 231 insertions(+)