[FFmpeg-devel] [PATCH 3/4] vf_tonemap_opencl: Move update_metadata() to a shared file

[Mark Thompson]

On 06/08/18 20:21, James Almer wrote:
> On 7/25/2018 12:46 PM, Vittorio Giovara wrote:
>> ---
>>  libavfilter/colorspace.c        | 17 +++++++++++++++++
>>  libavfilter/colorspace.h        |  1 +
>>  libavfilter/vf_tonemap_opencl.c | 19 +------------------
>>  3 files changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/libavfilter/colorspace.c b/libavfilter/colorspace.c
>> index d6f6055401..c6682216d6 100644
>> --- a/libavfilter/colorspace.c
>> +++ b/libavfilter/colorspace.c
>> @@ -118,3 +118,20 @@ double ff_determine_signal_peak(AVFrame *in)
>>  
>>      return peak;
>>  }
>> +
>> +void ff_update_hdr_metadata(AVFrame *in, double peak)
>> +{
>> +    AVFrameSideData *sd = av_frame_get_side_data(in, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
>> +
>> +    if (sd) {
>> +        AVContentLightMetadata *clm = (AVContentLightMetadata *)sd->data;
>> +        clm->MaxCLL = (unsigned)(peak * REFERENCE_WHITE);
>> +    }
>> +
>> +    sd = av_frame_get_side_data(in, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
>> +    if (sd) {
>> +        AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)sd->data;
>> +        if (metadata->has_luminance)
>> +            metadata->max_luminance = av_d2q(peak * REFERENCE_WHITE, 10000);
>> +    }
> Frame side data is refcounted, so this is not correct. I'm not sure if a
> side data element is ever modified in-place long after being created to
> being with, aside from this function here.
> 
> Easiest option is to call av_buffer_make_writable() on sd->buf and
> updating the sd->data pointer before modifying the fields in question,
> but such manual handling of side data seems improper and fragile.
> I guess another option is av_mastering_display_metadata_alloc(), copy
> the values, av_frame_remove_side_data(), av_buffer_create(), then
> av_frame_new_side_data_from_buf().
> 
> Honestly, i think we may have to add a make_writable() function for side
> data for this.

The frame side-data was already copied for the output frame by av_frame_copy_props(), so modifying it here is fine.  (I had exactly the same thought when this code appeared in tonemap_opencl.)

- Mark