On 3/2/2018 1:47 PM, wm4 wrote:
> On Fri, 2 Mar 2018 13:11:35 -0300
> James Almer <jamr...@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.
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 {
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
