https://bugzilla.gnome.org/show_bug.cgi?id=785684
Tim-Philipp Müller <***@zen.co.uk> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #373681|none |reviewed
status| |
--- Comment #30 from Tim-Philipp Müller <***@zen.co.uk> ---
Comment on attachment 373681
--> https://bugzilla.gnome.org/attachment.cgi?id=373681
rtsp: Add support for storing GstBuffers directly as body payload of messages
This patch is a bit unwieldy, maybe you could split out the "add new API to
GstRTSPMessage" bit from the "make use of it in RtspConnection" bit?
Subject: [PATCH] rtsp: Add support for storing GstBuffers directly as body
payload of messages
This makes it unnecessary for callers to first merge together all
memories. They are now written out one-by-one instead.
At a later time this can make use of a g_output_stream_writev() and
could also be extended with new API for sending multiple messages at
once.
Might be worth explaining the background / scenario / reason you want to do
this, i.e. in what cases will this be helpful later (tcp interleaved?) or is it
a general optimisation?
Looking first at the RtspMessage part:
- new API looks fine
- The ABI compat union in the header is not really needed here if it's just a
pointer, is it? Not that it matters much, but it does make the code a bit
uglier, would be nicer not to do that :)
--- a/gst-libs/gst/rtsp/gstrtspmessage.c
+++ b/gst-libs/gst/rtsp/gstrtspmessage.c
@@ -985,6 +990,13 @@ gst_rtsp_message_get_body (const GstRTSPMessage * msg, guint8 ** data,
g_return_val_if_fail (data != NULL, GST_RTSP_EINVAL);
g_return_val_if_fail (size != NULL, GST_RTSP_EINVAL);
+ if (msg->ABI.abi.body_buffer) {
+ guint8 *data = g_malloc (gst_buffer_get_size (msg->ABI.abi.body_buffer));
+ gst_buffer_extract (msg->ABI.abi.body_buffer, 0, data, gst_buffer_get_size (msg->ABI.abi.body_buffer));
+ gst_buffer_replace (&((GstRTSPMessage *)msg)->ABI.abi.body_buffer, NULL);
+ ((GstRTSPMessage *)msg)->body = data;
+ }
Can we use gst_buffer_extract_dup() here?
Should we add a performance logging/warning or is it expected?
@@ -1009,6 +1021,13 @@ gst_rtsp_message_steal_body (GstRTSPMessage * msg, guint8 ** data, guint * size)
g_return_val_if_fail (data != NULL, GST_RTSP_EINVAL);
g_return_val_if_fail (size != NULL, GST_RTSP_EINVAL);
+ if (msg->ABI.abi.body_buffer) {
+ guint8 *data = g_malloc (gst_buffer_get_size (msg->ABI.abi.body_buffer));
+ gst_buffer_extract (msg->ABI.abi.body_buffer, 0, data, gst_buffer_get_size (msg->ABI.abi.body_buffer));
+ gst_buffer_replace (&msg->ABI.abi.body_buffer, NULL);
+ msg->body = data;
+ }
Can we use gst_buffer_extract_dup() here?
+/**
+ *
+ *
+ * Returns: #GST_RTSP_OK.
+ */
Please add a "Since: 1.16" marker here (also to the other functions).
+GstRTSPResult
+gst_rtsp_message_get_body_buffer (const GstRTSPMessage * msg, GstBuffer ** buffer)
+{
+ g_return_val_if_fail (msg != NULL, GST_RTSP_EINVAL);
+ g_return_val_if_fail (buffer != NULL, GST_RTSP_EINVAL);
+
+ *buffer = msg->ABI.abi.body_buffer;
+
+ return GST_RTSP_OK;
+}
Is it an error to call this if only a normal data body was set but not a
buffer? Or is it expected that in this case NULL will be returned to indicate
that a buffer was not set and perhaps the caller should try the old method? Or
should we create a fallback buffer? Should document what's expected/allowed in
the docs.
@@ -1045,6 +1154,7 @@ gst_rtsp_message_dump (GstRTSPMessage * msg)
{
guint8 *data;
guint size;
+ GstBuffer *body_buffer;
I would suggest initialising this to NULL since we don't check return values
below and that will make coverity and such unhappy.
RtspConnection in separate comment.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.