[FFmpeg-devel] [PATCH] Support HDR10+ metadata for HEVC.

[James Almer]

On 12/6/2020 8:13 AM, Hendrik Leppkes wrote:
> On Sat, Dec 5, 2020 at 11:28 PM James Almer <jamrial at gmail.com> wrote:
>>
>> On 12/1/2020 3:09 PM, Mohammad Izadi wrote:
>>> Ian, can you please take a look into it? And if it's fine to push it.
>>>
>>> Thanks,
>>> Mohammad
>>
>> I removed the packet side data addition since it was not used by any
>> module. It can be added when a demuxer needs to propagate it.
>>
>> Also removed the entry for dynamic_hdr10_plus.h in the installed
>> libraries list as it's internal, simplified the test (cut the sample
>> into a single frame, 70kb vs 1mb sample) and split the patch in four
>> commits before pushing.
>>
> 
> I looked through the committed patch as I was curious how it compares
> to the code i've been using for a bit, and I noticed some concerns
> with the GetBitContext handling.
> 
> - All SEI parsing uses one shared "gb", not only for one SEI message,
> but the entire SEI NALU (this is fine, but important to know for the
> other points)

Apparently, h264 creates standalone GetBitContexts for each SEI message, 
whereas hevc doesn't. This was probably written with the former in mind.

> - ff_parse_itu_t_t35_to_dynamic_hdr10_plus will always consume the
> *full* GetBitContext with a "skip_bits(gb, get_bits_left(gb));" at its
> end, so it may skip over a SEI message following the HDR10+ data?
> - The skip in decode_nal_sei_user_data_registered_itu_t_t35 was moved
> out of any conditions, so in case of decoding HDR10+ data, it would
> skip data despite it already being consumed by parsing?
> It seems like the intention of the code was to pass a copy of the
> GetBitContex (and not the original) to
> ff_parse_itu_t_t35_to_dynamic_hdr10_plus, as it would avoid any of
> these issues, but then I don't know what the skip at the end of the
> hdr10+ function is for?

It may be better to make ff_parse_itu_t_t35_to_dynamic_hdr10_plus() 
behave like ff_parse_a53_cc(), taking a buffer as argument and creating 
a GetBitContext internally instead. I'll send a patch for that.