diff mbox series

[FFmpeg-devel,1/3] avutil/frame: add av_frame_replace

Message ID 20210810220001.13494-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/3] avutil/frame: add av_frame_replace
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer Aug. 10, 2021, 9:59 p.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
Not going to bother with implementing replace for side data, since it's IMO not
worth it, but patches are welcome of course.

Missing version bump and APIchanges entry.

 libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++
 libavutil/frame.h | 18 +++++++++
 2 files changed, 116 insertions(+)

Comments

James Almer Aug. 13, 2021, 3:19 p.m. UTC | #1
On 8/10/2021 6:59 PM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Not going to bother with implementing replace for side data, since it's IMO not
> worth it, but patches are welcome of course.
> 
> Missing version bump and APIchanges entry.
> 
>   libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++
>   libavutil/frame.h | 18 +++++++++
>   2 files changed, 116 insertions(+)

Will apply the set if there are no objections.
Andreas Rheinhardt Aug. 13, 2021, 3:22 p.m. UTC | #2
James Almer:
> On 8/10/2021 6:59 PM, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Not going to bother with implementing replace for side data, since
>> it's IMO not
>> worth it, but patches are welcome of course.
>>
>> Missing version bump and APIchanges entry.
>>
>>   libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++
>>   libavutil/frame.h | 18 +++++++++
>>   2 files changed, 116 insertions(+)
> 
> Will apply the set if there are no objections.

How many allocations are saved by this normally? (I.e. with a random
H.264 file you have)?

- Andreas
James Almer Aug. 13, 2021, 3:25 p.m. UTC | #3
On 8/13/2021 12:22 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 8/10/2021 6:59 PM, James Almer wrote:
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> Not going to bother with implementing replace for side data, since
>>> it's IMO not
>>> worth it, but patches are welcome of course.
>>>
>>> Missing version bump and APIchanges entry.
>>>
>>>    libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++
>>>    libavutil/frame.h | 18 +++++++++
>>>    2 files changed, 116 insertions(+)
>>
>> Will apply the set if there are no objections.
> 
> How many allocations are saved by this normally? (I.e. with a random
> H.264 file you have)?

I looked at a VP9 file with patch 4/4, and it seemed to remove about a 
third of the overall allocations.

> 
> - Andreas
> _______________________________________________
> 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".
>
Andreas Rheinhardt Aug. 13, 2021, 3:27 p.m. UTC | #4
James Almer:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Not going to bother with implementing replace for side data, since it's IMO not
> worth it, but patches are welcome of course.
> 
> Missing version bump and APIchanges entry.
> 
>  libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/frame.h | 18 +++++++++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index ff2540a20f..36ed128886 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -695,6 +695,24 @@ void av_frame_free(AVFrame **frame);
>   */
>  int av_frame_ref(AVFrame *dst, const AVFrame *src);
>  
> +/**
> + * Ensure the destination frame refers to the same data described by the source
> + * frame.
> + *
> + * Copy frame properties from src to dst and create a new reference for each
> + * AVBufferRef from src if they differ from those in dst.
> + *
> + * src must be reference counted.
> + *
> + * @param src The source frame. If there's data described in it, it must be
> + *            reference counted.
> + * @param dst The destination frame.
> + *
> + * @return 0 on success, a negative AVERROR on error. On error, dst is
> + *         unreferenced.
> + */
> +int av_frame_replace(AVFrame *dst, const AVFrame *src);
> +
>  /**
>   * Create a new frame that references the same data as src.
>   *
> 
av_buffer_replace() is a no-op if src == dst (and documented to be so);
this is not true for this function where you instantly wipe side data.

- Andreas
James Almer Aug. 13, 2021, 3:31 p.m. UTC | #5
On 8/13/2021 12:27 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Not going to bother with implementing replace for side data, since it's IMO not
>> worth it, but patches are welcome of course.
>>
>> Missing version bump and APIchanges entry.
>>
>>   libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++
>>   libavutil/frame.h | 18 +++++++++
>>   2 files changed, 116 insertions(+)
>>
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index ff2540a20f..36ed128886 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -695,6 +695,24 @@ void av_frame_free(AVFrame **frame);
>>    */
>>   int av_frame_ref(AVFrame *dst, const AVFrame *src);
>>   
>> +/**
>> + * Ensure the destination frame refers to the same data described by the source
>> + * frame.
>> + *
>> + * Copy frame properties from src to dst and create a new reference for each
>> + * AVBufferRef from src if they differ from those in dst.
>> + *
>> + * src must be reference counted.
>> + *
>> + * @param src The source frame. If there's data described in it, it must be
>> + *            reference counted.
>> + * @param dst The destination frame.
>> + *
>> + * @return 0 on success, a negative AVERROR on error. On error, dst is
>> + *         unreferenced.
>> + */
>> +int av_frame_replace(AVFrame *dst, const AVFrame *src);
>> +
>>   /**
>>    * Create a new frame that references the same data as src.
>>    *
>>
> av_buffer_replace() is a no-op if src == dst (and documented to be so);
> this is not true for this function where you instantly wipe side data.

Like i said above, I'm not going to try and implement replace for side 
data. It's just not worth the extra complexity.

> 
> - Andreas
> _______________________________________________
> 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".
>
Andreas Rheinhardt Aug. 13, 2021, 3:50 p.m. UTC | #6
James Almer:
> On 8/13/2021 12:27 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> Not going to bother with implementing replace for side data, since
>>> it's IMO not
>>> worth it, but patches are welcome of course.
>>>
>>> Missing version bump and APIchanges entry.
>>>
>>>   libavutil/frame.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++
>>>   libavutil/frame.h | 18 +++++++++
>>>   2 files changed, 116 insertions(+)
>>>
>>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>>> index ff2540a20f..36ed128886 100644
>>> --- a/libavutil/frame.h
>>> +++ b/libavutil/frame.h
>>> @@ -695,6 +695,24 @@ void av_frame_free(AVFrame **frame);
>>>    */
>>>   int av_frame_ref(AVFrame *dst, const AVFrame *src);
>>>   +/**
>>> + * Ensure the destination frame refers to the same data described by
>>> the source
>>> + * frame.
>>> + *
>>> + * Copy frame properties from src to dst and create a new reference
>>> for each
>>> + * AVBufferRef from src if they differ from those in dst.
>>> + *
>>> + * src must be reference counted.
>>> + *
>>> + * @param src The source frame. If there's data described in it, it
>>> must be
>>> + *            reference counted.
>>> + * @param dst The destination frame.
>>> + *
>>> + * @return 0 on success, a negative AVERROR on error. On error, dst is
>>> + *         unreferenced.
>>> + */
>>> +int av_frame_replace(AVFrame *dst, const AVFrame *src);
>>> +
>>>   /**
>>>    * Create a new frame that references the same data as src.
>>>    *
>>>
>> av_buffer_replace() is a no-op if src == dst (and documented to be so);
>> this is not true for this function where you instantly wipe side data.
> 
> Like i said above, I'm not going to try and implement replace for side
> data. It's just not worth the extra complexity.
> 
I don't think you got my comment: It was not about saving allocations
for side data; it is about what happens when src and dst coincide.
According to the documentation, you should do nothing, because src and
dst already refer to the same data. Instead you wipe the side data and
modify the frame.

- Andreas
diff mbox series

Patch

diff --git a/libavutil/frame.c b/libavutil/frame.c
index b0ceaf7145..035a48eb48 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -421,6 +421,104 @@  fail:
     return ret;
 }
 
+int av_frame_replace(AVFrame *dst, const AVFrame *src)
+{
+    int i, ret = 0;
+
+    if (!src->buf[0]) {
+        if ((src->extended_data && src->extended_data[0]) || src->data[0])
+            ret = AVERROR(EINVAL);
+
+        goto fail;
+    }
+
+    dst->format         = src->format;
+    dst->width          = src->width;
+    dst->height         = src->height;
+    dst->channels       = src->channels;
+    dst->channel_layout = src->channel_layout;
+    dst->nb_samples     = src->nb_samples;
+
+    wipe_side_data(dst);
+    av_dict_free(&dst->metadata);
+    ret = frame_copy_props(dst, src, 0);
+    if (ret < 0)
+        goto fail;
+
+    /* replace the buffers */
+    for (i = 0; i < FF_ARRAY_ELEMS(src->buf); i++) {
+        ret = av_buffer_replace(&dst->buf[i], src->buf[i]);
+        if (ret < 0)
+            goto fail;
+    }
+
+    if (src->extended_buf) {
+        if (dst->nb_extended_buf != src->nb_extended_buf) {
+            int nb_extended_buf = FFMIN(dst->nb_extended_buf, src->nb_extended_buf);
+            void *tmp;
+
+            for (i = nb_extended_buf; i < dst->nb_extended_buf; i++)
+                av_buffer_unref(&dst->extended_buf[i]);
+
+            tmp = av_realloc_array(dst->extended_buf, sizeof(*dst->extended_buf),
+                                   src->nb_extended_buf);
+            if (!tmp) {
+                ret = AVERROR(ENOMEM);
+                goto fail;
+            }
+            dst->extended_buf = tmp;
+            dst->nb_extended_buf = src->nb_extended_buf;
+
+            memset(&dst->extended_buf[nb_extended_buf], 0,
+                   (src->nb_extended_buf - nb_extended_buf) * sizeof(*dst->extended_buf));
+        }
+
+        for (i = 0; i < src->nb_extended_buf; i++) {
+            ret = av_buffer_replace(&dst->extended_buf[i], src->extended_buf[i]);
+            if (ret < 0)
+                goto fail;
+        }
+    } else if (dst->extended_buf) {
+        for (i = 0; i < dst->nb_extended_buf; i++)
+            av_buffer_unref(&dst->extended_buf[i]);
+        av_freep(&dst->extended_buf);
+    }
+
+    ret = av_buffer_replace(&dst->hw_frames_ctx, src->hw_frames_ctx);
+    if (ret < 0)
+        goto fail;
+
+    if (dst->extended_data != dst->data)
+        av_freep(&dst->extended_data);
+
+    if (src->extended_data != src->data) {
+        int ch = src->channels;
+
+        if (!ch) {
+            ret = AVERROR(EINVAL);
+            goto fail;
+        }
+        CHECK_CHANNELS_CONSISTENCY(src);
+
+        dst->extended_data = av_malloc_array(sizeof(*dst->extended_data), ch);
+        if (!dst->extended_data) {
+            ret = AVERROR(ENOMEM);
+            goto fail;
+        }
+        memcpy(dst->extended_data, src->extended_data, sizeof(*src->extended_data) * ch);
+    } else
+        dst->extended_data = dst->data;
+
+    memcpy(dst->data,     src->data,     sizeof(src->data));
+    memcpy(dst->linesize, src->linesize, sizeof(src->linesize));
+
+    return 0;
+
+fail:
+    av_frame_unref(dst);
+    return ret;
+}
+
 AVFrame *av_frame_clone(const AVFrame *src)
 {
     AVFrame *ret = av_frame_alloc();
diff --git a/libavutil/frame.h b/libavutil/frame.h
index ff2540a20f..36ed128886 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -695,6 +695,24 @@  void av_frame_free(AVFrame **frame);
  */
 int av_frame_ref(AVFrame *dst, const AVFrame *src);
 
+/**
+ * Ensure the destination frame refers to the same data described by the source
+ * frame.
+ *
+ * Copy frame properties from src to dst and create a new reference for each
+ * AVBufferRef from src if they differ from those in dst.
+ *
+ * src must be reference counted.
+ *
+ * @param src The source frame. If there's data described in it, it must be
+ *            reference counted.
+ * @param dst The destination frame.
+ *
+ * @return 0 on success, a negative AVERROR on error. On error, dst is
+ *         unreferenced.
+ */
+int av_frame_replace(AVFrame *dst, const AVFrame *src);
+
 /**
  * Create a new frame that references the same data as src.
  *