[FFmpeg-devel] [PATCH 1/2] avcodec/avutil: move dynamic HDR metadata parsing to libavutil

[Raphaël Zumer]

On 3/13/23 13:10, James Almer wrote:
> On 3/13/2023 2:05 PM, Raphaël Zumer wrote:
>> On 3/13/23 12:58, James Almer wrote:
>>> On 3/13/2023 1:56 PM, Raphaël Zumer wrote:
>>>> On 3/13/23 12:09, Andreas Rheinhardt wrote:
>>>>>>    
>>>>>> +/**
>>>>>> + * Parse the user data registered ITU-T T.35 to AVbuffer (AVDynamicHDRVivid).
>>>>>> + * @param s A pointer containing the decoded AVDynamicHDRVivid structure.
>>>>>> + * @param data The byte array containing the raw ITU-T T.35 data.
>>>>>> + * @param size Size of the data array in bytes.
>>>>>> + *
>>>>>> + * @return 0 if succeed. Otherwise, returns the appropriate AVERROR.
>>>>>> + */
>>>>>> +int av_dynamic_hdr_vivid_from_t35(AVDynamicHDRVivid *s, const uint8_t *data,
>>>>>> +                                  int size);
>>>>> Who has an interest in this function being public?
>>>>>
>>>>> - Andreas
>>>> I have no need for it so can change it to avpriv_ considering there's no serialization function available for it, if there's no objection to that.
>>>>
>>>> Raphaël Zumer
>>> No, just don't move it out of libavcodec. Unless it's needed elsewhere,
>>> it can stay there as is.
>> The inconsistency between HDR10+ and Vivid will be confusing IMO if one of them is left in libavcodec and the other is moved to libavutil.
> Why? The HDR10+ one is useful for libraries like lavf and external 
> container parsers, the Vivid one isn't.

That is obvious in this specific context, but for someone simply looking at the internals and dynamic HDR metadata handling, it would be odd to find a lone function for Vivid HDR parsing sitting in libavcodec while everything else (including the definition for the parsed Vivid metadata structure) is in one place in libavutil.

>> What are the specific concerns with making it public (or avpriv), aside from it not being useful without a corresponding serialization function?
> You said it, it serves no purpose. Making something public (or exposed 
> internally as avpriv_) is done only when it will be used by code outside 
> the library where it resides.

If it is standard practice in the FFmpeg codebase then I won't argue further, I just think that avpriv accomplishes the desired outcome (keep that function private) without violating best practices (keeping logically-related code together).

RZ