[FFmpeg-devel] [PATCH 03/11] avutil/frame: add functions to check and ensure a side data entry is writable

[James Almer]

On 2/21/2025 8:44 AM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>   libavutil/frame.h     | 19 +++++++++++++++++++
>>   libavutil/side_data.c | 25 +++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index 49260ae2dd..a707b35087 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -1211,6 +1211,25 @@ void av_frame_side_data_remove(AVFrameSideData ***sd, int *nb_sd,
>>   void av_frame_side_data_remove_by_props(AVFrameSideData ***sd, int *nb_sd,
>>                                           int props);
>>   
>> +/**
>> + * Check whether the data described by a given AVFrameSideData can be
>> + * written to.
>> + *
>> + * @return 1 if the caller may write to the data, 0 otherwise.
>> + */
>> +int av_frame_side_data_is_writable(const AVFrameSideData *sd);
>> +
>> +/**
>> + * Create a writable reference for the data described by a given
>> + * AVFrameSideData, avoiding data copy if possible.
>> + *
>> + * @param sd Side data whose data should be made writable.
>> + *
>> + * @return 0 on success, a negative AVERROR on failure. On failure, the
>> + *         side data is unchanged.
>> + */
>> +int av_frame_side_data_make_writable(AVFrameSideData *sd);
> 
> I don't see the point of either of them: The former is just a wrapper
> around av_buffer_is_writable() and the latter is basically the same as
> av_buffer_make_writable() (and could actually use it).
> 
> If this is designed with using something else but flat buffers in the
> future in mind, then av_frame_side_data_make_writable() is misdesigned,
> because making a non-flat object writable is ambiguous: It is not clear
> whether making something writable is meant in a shallow or a deep sense
> which may become an issue if future side-data contains references of its
> own.

In the previous iteration of this set, where i added RefCount as backing 
for the side data, av_frame_side_data_make_writable() included per type 
callbacks in the descriptors that would take care of any kind of 
refcounted buffer or object within the side data.

I can skip this patch in any case, since you were against adding the 
RefCount backing implementation right now, its usefulness is limited.

> 
>> +
>>   /**
>>    * @}
>>    */
>> diff --git a/libavutil/side_data.c b/libavutil/side_data.c
>> index 8c57fd838a..beb8ea3212 100644
>> --- a/libavutil/side_data.c
>> +++ b/libavutil/side_data.c
>> @@ -315,3 +315,28 @@ const AVFrameSideData *av_frame_side_data_get_c(const AVFrameSideData * const *s
>>       }
>>       return NULL;
>>   }
>> +
>> +int av_frame_side_data_is_writable(const AVFrameSideData *sd)
>> +{
>> +    return !!av_buffer_is_writable(sd->buf);
> 
> av_buffer_is_writable() is documented to only return 0 or 1, so the
> binarization is unnecessary.
> 
>> +}
>> +
>> +int av_frame_side_data_make_writable(AVFrameSideData *sd)
>> +{
>> +    AVBufferRef *buf = NULL;
>> +
>> +    if (av_buffer_is_writable(sd->buf))
>> +        return 0;
>> +
>> +    buf = av_buffer_alloc(sd->size);
>> +    if (!buf)
>> +        return AVERROR(ENOMEM);
>> +
>> +    if (sd->size)
>> +        memcpy(buf->data, sd->data, sd->size);
>> +    av_buffer_unref(&sd->buf);
>> +    sd->buf  = buf;
>> +    sd->data = buf->data;
>> +
>> +    return 0;
>> +}
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250221/693611b3/attachment.sig>