[Bug 785684] rtspconnection: Add API for sending multiple messages at once, and for having the message body consist of multiple chunks of memory

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785684

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #373681|none                        |reviewed
             status|                            |

--- Comment #30 from Tim-Philipp Müller <t.i.m at 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?



>+/**
>+ * gst_rtsp_message_set_body_buffer:
>+ * @msg: a #GstRTSPMessage
>+ * @buffer: a #GstBuffer
>+ *
>+ * Set the body of @msg to @buffer.
>+ *
>+ * 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.