[FFmpeg-devel] [PATCH] Update HDR10+ metadata structure.

[James Almer]

On 2/20/2020 5:02 PM, Vittorio Giovara wrote:
> On Mon, Feb 10, 2020 at 3:14 PM Mohammad Izadi <moh.izadi at gmail.com> wrote:
> 
>> From: Mohammad Izadi <izadi at google.com>
>>
>> Trying to read HDR10+ metadata from HEVC/SEI and pass it to packet side
>> data in the follow-up CLs.
>> ---
>>  libavutil/hdr_dynamic_metadata.c | 165 +++++++++++++++++++++++++++++++
>>  libavutil/hdr_dynamic_metadata.h |  13 ++-
>>  libavutil/version.h              |   2 +-
>>  3 files changed, 178 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavutil/hdr_dynamic_metadata.c
>> b/libavutil/hdr_dynamic_metadata.c
>> index 0fa1ee82de..f24bcb40f5 100644
>> --- a/libavutil/hdr_dynamic_metadata.c
>> +++ b/libavutil/hdr_dynamic_metadata.c
>> @@ -19,8 +19,17 @@
>>   */
>>
>>  #include "hdr_dynamic_metadata.h"
>> +#include "libavcodec/get_bits.h"
>>
> 
> wait is it ok to use libavcodec headers in libavutil? while it's fine at
> compilation time since it is an inlined header, I don't think it's a good
> idea to freely use such functions in this way: what I would do is rather
> implement the parsing in libavcodec, store the fields in a structure
> defined in libavutil and then use this new structure in here

This seems excessive only to use a bitstream reader to read a bunch of
fields in a buffer.

get_bits.h is included in lavf, so i don't see why it couldn't be used
in lavu. As you said it's inlined.

> 
> 
>>  #include "mem.h"
>>
>> +static const int32_t luminance_den = 1;
>> +static const int32_t peak_luminance_den = 15;
>> +static const int32_t rgb_den = 100000;
>> +static const int32_t fraction_pixel_den = 1000;
>> +static const int32_t knee_point_den = 4095;
>> +static const int32_t bezier_anchor_den = 1023;
>> +static const int32_t saturation_weight_den = 8;
>>
> 
> These fields could also be stored in the structure i mentioned, rather then
> having them declared as static here.
>