Re: [FFmpeg-devel] [PATCH 2/2] avutil: add HDR10+ dynamic metadata serialization function

["Raphaël Zumer" <raphael.zumer@vimeo.com>]

On 3/2/23 15:24, Leo Izen wrote:
> On 3/2/23 14:25, Raphaël Zumer wrote:
>> Signed-off-by: Raphaël Zumer <rzumer@tebako.net>
>> ---
>>   libavutil/hdr_dynamic_metadata.c | 146 +++++++++++++++++++++++++++++++
>>   libavutil/hdr_dynamic_metadata.h |  11 +++
>>   libavutil/version.h              |   2 +-
>>   3 files changed, 158 insertions(+), 1 deletion(-)
>>
> Why not put this in avcodec/dynamic_hdr10_plus.c? You reference 
> put_bits.h which is in avcodec, so that can possibly be an issue, even 
> though it is inlined (i.e. it sends the wrong message since avutil is 
> supposed to not depend on avcodec).

I agree it is somewhat awkward to introduce a circular dependency (albeit to header-only files). On the other hand, I think those functions make more sense in libavutil than libavcodec, and it improves readability by not splitting files that are logically connected between libraries. If there is a general consensus that it is better to keep them in libavcodec, I don't mind reverting that change, or moving get_bits and put_bits to libavutil if that is doable.

>> diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c
>> index 98f399b032..39a7886a2e 100644
>> --- a/libavutil/hdr_dynamic_metadata.c
>> +++ b/libavutil/hdr_dynamic_metadata.c
>> @@ -225,3 +225,149 @@ int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data,
>>   
>>       return 0;
>>   }
>> +
>> +AVBufferRef *av_dynamic_hdr_plus_to_t35(AVDynamicHDRPlus *s)
>> +{
> av_dynamic_hdr_plus_from_t35 returns an int status code and takes a 
> pointer as an argument, is there any particular reason you didn't mirror 
> user interface here?

Mainly the added complexity of buffer size calculation. I think it would be doable by adding an additional function such as av_dynamic_hdr_plus_to_t35_size() that would return the serialized buffer size, which could be then used by the user to allocate a buffer to be written by av_dynamic_hdr_plus_to_t35(). But adding an additional function just to make the function signatures consistent feels contrived to me, and there aren't several errors that could happen in that function that would need to be disambiguated by the user.

>> +    AVBufferRef *buf;
>> +    size_t size_bits, size_bytes;
>> +    PutBitContext pbc, *pb = &pbc;
>> +
>> +    if (!s)
>> +        return NULL;
>> +
>> +    // Buffer size per CTA-861-H p.253-254:
>> +    size_bits =
>> +    // 56 bits for the header, minus 8-bit excluded country code
>> +    48 +
>> +    // 2 bits for num_windows
>> +    2 +
>> +    // 937 bits for window geometry for each window above 1
>> +    FFMAX((s->num_windows - 1), 0) * 937 +
>> +    // 27 bits for targeted_system_display_maximum_luminance
>> +    27 +
>> +    // 1-3855 bits for targeted system display peak luminance information
>> +    1 + (s->targeted_system_display_actual_peak_luminance_flag ? 10 +
>> +        s->num_rows_targeted_system_display_actual_peak_luminance *
>> +        s->num_cols_targeted_system_display_actual_peak_luminance * 4 : 0) +
>> +    // 0-442 bits for intra-window pixel distribution information
>> +    s->num_windows * 82;
> This sequence above is difficult to read due to the inline // comments. 
> It should be more readable to just have the entire expression be 
> contiguous with a /* */ multiline block comment above it explaining each 
> item.
>> +    for (int w = 0; w < s->num_windows; w++) {
>> +        size_bits += s->params[w].num_distribution_maxrgb_percentiles * 24;
>> +    }
> Likewise, another code style issue, don't use {} to enclose a single 
> line unless it's unavoidable. This occurs in several places in this patch.

OK, will correct.


Thanks
Raphaël Zumer

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".