[FFmpeg-devel] [PATCH 1/2] cbs_h265: read/write HEVC PREFIX SEI

Mark Thompson sw at jkqxz.net
Tue Apr 24 01:35:18 EEST 2018


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 at 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, &current->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, &current->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, &current->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, &current->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


More information about the ffmpeg-devel mailing list