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

[James Almer]

On 8/6/2018 4:52 PM, Mark Thompson wrote:
> 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

Ah, good. I guess the ff_update_hdr_metadata() doxy should in any case
point that the frame or at least its side data must be writable to
prevent other uses in the future where this may not be taken into
account, now that it's shared.

And an actual, generic solution like a make_writable function is imo
still a good idea, unless the user is expected to use
av_frame_make_writable() if they want to do *any* kind of in-place
change to the frame.