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

[James Almer]

On 3/13/2023 2:34 PM, Raphaël Zumer wrote:
> 
> 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.

The metadata structures for both HDR10+ and Vivid are in lavu and public 
because they are used by AVFrame side data types. The parsing code that 
fills the Vivid struct is in lavc and internal because nothing outside 
of it needs it. Meanwhile, the function filling the HDR10+ struct can 
and will be used by lavf, so moving it to lavu and making it public like 
you're doing here makes sense and is useful.

If at some point the Vivid parsing code is deemed useful outside of 
lavc, it can be moved and made public.

> 
>>> 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).

Using avpriv_ here would make even less sense. It'd be exposed (And thus 
tied to the ABI) yet not public, which is for even less gain since lavc 
will still be its sole user.