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.xi...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel