[FFmpeg-devel] [PATCH v2] lavu/frame: add QP side data

[James Almer]

On 3/2/2018 1:47 PM, wm4 wrote:
> On Fri, 2 Mar 2018 13:11:35 -0300
> James Almer <jamrial at gmail.com> wrote:
> 
>> On 3/2/2018 8:16 AM, wm4 wrote:
>>> This adds a way for an API user to transfer QP data and metadata without
>>> having to keep the reference to AVFrame, and without having to
>>> explicitly care about QP APIs. It might also provide a way to finally
>>> remove the deprecated QP related fields. In the end, the QP table should
>>> be handled in a very similar way to e.g. AV_FRAME_DATA_MOTION_VECTORS.
>>>
>>> There are two side data types, because I didn't care about having to
>>> repack the QP data so the table and the metadata are in a single
>>> AVBufferRef. Otherwise it would have either required a copy on decoding
>>> (extra slowdown for something as obscure as the QP data), or would have
>>> required making intrusive changes to the codecs which support export of
>>> this data.  
>>
>> Why not add an AVBufferRef pointer to the qp_properties struct instead?
>> You don't need to merge the properties fields into the buffer data.
> 
> Not sure what you mean. The whole purpose of this is _not_ to add new
> pointers because it'd require an API user to deal with extra fields
> just for QP. I want to pretend that QP doesn't exist.

I mean keep the buffer and the int fields all in a single opaque (for
the user) struct handled by a single side data type. The user still only
needs to worry about using the get/set functions and nothing else.

See the attached, untested PoC to get an idea of what i mean.

If I'm really missing the entire point of this patch (Which i don't
discard may be the case), then ignore this.
-------------- next part --------------
diff --git a/libavutil/frame.c b/libavutil/frame.c
index 0db2a2d57b..b349ff84fe 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -46,8 +46,28 @@ MAKE_ACCESSORS(AVFrame, frame, enum AVColorRange, color_range)
                av_get_channel_layout_nb_channels((frame)->channel_layout))
 
 #if FF_API_FRAME_QP
+struct qp_properties {
+    AVBufferRef *buf;
+    int stride;
+    int type;
+};
+
+// Free the qp_properties struct and the QP data buffer
+// To be used as the free() function for the side data buffer.
+static void frame_qp_free(void *opaque, uint8_t *data)
+{
+    struct qp_properties *p = (struct qp_properties *)data;
+
+    av_buffer_unref(&p->buf);
+    av_free(data);
+}
+
 int av_frame_set_qp_table(AVFrame *f, AVBufferRef *buf, int stride, int qp_type)
 {
+    struct qp_properties *p = NULL;
+    AVFrameSideData *sd;
+    AVBufferRef *sd_buf = NULL, *ref = NULL;
+
 FF_DISABLE_DEPRECATION_WARNINGS
     av_buffer_unref(&f->qp_table_buf);
 
@@ -57,20 +77,69 @@ FF_DISABLE_DEPRECATION_WARNINGS
     f->qscale_type  = qp_type;
 FF_ENABLE_DEPRECATION_WARNINGS
 
+    av_frame_remove_side_data(f, AV_FRAME_DATA_QP_TABLE);
+
+    // Create a new QP data table ref for the side data
+    ref = av_buffer_ref(buf);
+    if (!ref)
+        return AVERROR(ENOMEM);
+
+    // Alloc the qp_properties struct
+    p = av_malloc(sizeof(struct qp_properties));
+    if (!p)
+        goto fail;
+
+    // Create a buffer to be used in side data, containing the qp_properties struct.
+    // Use a custom free() function to properly unref the QP table buffer when the side data
+    // is removed.
+    sd_buf = av_buffer_create((uint8_t *)p, sizeof(struct qp_properties), frame_qp_free, NULL, 0);
+    if (!sd_buf)
+        goto fail;
+
+    // Add the buffer containing the qp_properties struct as side data
+    sd = av_frame_new_side_data_from_buf(f, AV_FRAME_DATA_QP_TABLE, sd_buf);
+    if (!sd)
+        goto fail;
+
+    // Fill the prop fields and the QP table buffer.
+    p->stride = stride;
+    p->type = qp_type;
+    p->buf = ref;
+
     return 0;
+fail:
+    av_buffer_unref(&ref);
+    av_buffer_unref(&sd_buf);
+    av_free(p);
+    return AVERROR(ENOMEM);
 }
 
 int8_t *av_frame_get_qp_table(AVFrame *f, int *stride, int *type)
 {
-FF_DISABLE_DEPRECATION_WARNINGS
-    *stride = f->qstride;
-    *type   = f->qscale_type;
+    AVBufferRef *buf = NULL;
 
-    if (!f->qp_table_buf)
-        return NULL;
+    *stride = 0;
+    *type   = 0;
 
-    return f->qp_table_buf->data;
+FF_DISABLE_DEPRECATION_WARNINGS
+    if (f->qp_table_buf) {
+        *stride = f->qstride;
+        *type   = f->qscale_type;
+        buf     = f->qp_table_buf;
 FF_ENABLE_DEPRECATION_WARNINGS
+    } else {
+        AVFrameSideData *sd;
+        struct qp_properties *p;
+        sd = av_frame_get_side_data(f, AV_FRAME_DATA_QP_TABLE);
+        if (!sd)
+            return NULL;
+        p = (struct qp_properties *)sd->data;
+        *stride = p->stride;
+        *type   = p->type;
+        buf     = p->buf;
+    }
+
+    return buf ? buf->data : NULL;
 }
 #endif
 
@@ -787,6 +856,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
     case AV_FRAME_DATA_CONTENT_LIGHT_LEVEL:         return "Content light level metadata";
     case AV_FRAME_DATA_GOP_TIMECODE:                return "GOP timecode";
     case AV_FRAME_DATA_ICC_PROFILE:                 return "ICC profile";
+    case AV_FRAME_DATA_QP_TABLE:                    return "QP table";
     }
     return NULL;
 }
diff --git a/libavutil/frame.h b/libavutil/frame.h
index ddbac3156d..dd1917882b 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -141,6 +141,16 @@ enum AVFrameSideDataType {
      * metadata key entry "name".
      */
     AV_FRAME_DATA_ICC_PROFILE,
+
+#if FF_API_FRAME_QP
+    /**
+     * QP table data.
+     * The contents of this side data are undocumented and internal; use
+     * av_frame_set_qp_table() and av_frame_get_qp_table() to access this in a
+     * meaningful way instead.
+     */
+    AV_FRAME_DATA_QP_TABLE,
+#endif
 };
 
 enum AVActiveFormatDescription {