After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762884 - rtspsrc: allow to control headers via properties
rtspsrc: allow to control headers via properties
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-02-29 18:03 UTC by Nicola
Modified: 2017-12-06 08:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (7.45 KB, patch)
2016-02-29 18:03 UTC, Nicola
none Details | Review
add handle-message signal (4.83 KB, patch)
2016-12-19 10:57 UTC, Nicola
needs-work Details | Review
register GstRTSPMessage type (gst-plugins-base) (2.80 KB, patch)
2017-01-16 18:06 UTC, Matt Staples
none Details | Review
Adds before-send signal to rtspsrc (3.29 KB, patch)
2017-01-16 18:12 UTC, Matt Staples
none Details | Review
register GstRTSPMessage as a boxed type (gst-plugins-base) (6.04 KB, patch)
2017-01-30 14:22 UTC, Matt Staples
none Details | Review
Adds before-send-signal to rtspsrc (3.29 KB, patch)
2017-01-31 17:31 UTC, Matt Staples
needs-work Details | Review
register GstRTSPMessage as a boxed type (gst-plugins-base) (5.77 KB, patch)
2017-01-31 17:35 UTC, Matt Staples
accepted-commit_now Details | Review
register GstRTSPMessage as a boxed type (gst-plugins-base) (3.23 KB, patch)
2017-11-01 20:00 UTC, Matt Staples
none Details | Review
Adds before-send-signal to rtspsrc (3.23 KB, patch)
2017-11-01 20:03 UTC, Matt Staples
none Details | Review
Adds a signal to rtspsrc, allowing RTSP messages to be modified before being sent (3.30 KB, patch)
2017-11-27 14:54 UTC, Matt Staples
committed Details | Review
register GstRTSPMessage as a boxed type (gst-plugins-base) (4.91 KB, application/mbox)
2017-11-27 14:54 UTC, Matt Staples
  Details
register GstRTSPMessage as a boxed type (gst-plugins-base) (4.91 KB, patch)
2017-11-27 14:56 UTC, Matt Staples
committed Details | Review

Description Nicola 2016-02-29 18:03:37 UTC
Created attachment 322686 [details] [review]
patch

This is useful for example to implement onvif playback that require custom headers for SETUP and PLAY requests

http://www.onvif.org/specs/stream/ONVIF-Streaming-Spec-v220.pdf

see 6.3 and 6.4
Comment 1 Tim-Philipp Müller 2016-02-29 18:07:29 UTC
I think you can already do that by hooking up the right signals/vfuncs, not sure it's important enough to add properties for this. And then we'd also need to add them for all other requests really..
Comment 2 Nicola 2016-02-29 18:12:09 UTC
thanks, 

if you like this way I can add equivalent properties for other rtsp methods too,

regarding signals/vfuncs can you please point to relevant docs and or signal to react to? (handle-request?)
Comment 3 Tim-Philipp Müller 2016-02-29 18:30:34 UTC
Erm, I may just be confused, this is rtsp*src* after all. I was thinking of rtsp-server.
Comment 4 Nicola 2016-03-21 11:34:35 UTC
the patch is missing a 

g_free (field_content);

in _append_extra_header function let me known if this is the accepted way and so you want an updated patch
Comment 5 Sebastian Dröge (slomo) 2016-03-21 11:44:13 UTC
Independent of that, we should make onvif generally work out of the box with rtspsrc. See bug #762910.
Comment 6 Sebastian Dröge (slomo) 2016-03-21 11:46:00 UTC
Review of attachment 322686 [details] [review]:

Maybe instead a signal would be useful that allows the application to do whatever it wants with the GstRTSPMessage before it is sent?

::: gst/rtsp/gstrtspsrc.c
@@ +4253,3 @@
+  GST_DEBUG ("Appending extra header: \"%s: %s\"", field_name, field_content);
+  gst_rtsp_message_remove_header_by_name (msg, field_name, -1);
+  gst_rtsp_message_add_header_by_name (msg, field_name, field_content);

What if you want to add the same header multiple times with different content?
Comment 7 Nicola 2016-03-21 11:58:44 UTC
(In reply to Sebastian Dröge (slomo) from comment #6)
> Review of attachment 322686 [details] [review] [review]:
> 
> Maybe instead a signal would be useful that allows the application to do
> whatever it wants with the GstRTSPMessage before it is sent?

OK, I'll see if I can find some free time, for my actual needs this patch is ok as is

> 
> ::: gst/rtsp/gstrtspsrc.c
> @@ +4253,3 @@
> +  GST_DEBUG ("Appending extra header: \"%s: %s\"", field_name,
> field_content);
> +  gst_rtsp_message_remove_header_by_name (msg, field_name, -1);
> +  gst_rtsp_message_add_header_by_name (msg, field_name, field_content);
> 
> What if you want to add the same header multiple times with different
> content?

it is done in this way since for onvif we need to set a Range header and override the one that gstreamer could set. Maybe this can be configurable. Not sure if is still needed if we use a signal: if I understand correctly with a signal an user can inspect and modify the header as he wants and so decide if replace some specific field or set multiple times with different content
Comment 8 Sebastian Dröge (slomo) 2016-03-21 12:19:24 UTC
(In reply to Nicola from comment #7)

> if I understand correctly with a signal an user can inspect and modify the
> header as he wants and so decide if replace some specific field or set
> multiple times with different content

Correct. It would be much more flexible.
Comment 9 dashesy 2016-12-05 21:12:10 UTC
Adding a signal for extra headers will make it flexible for plugins (including future plugins), but creating pipeline through `gst-launch` will not be as easy. 
Isn't it better to set individual extra headers, also a `is_onvif` boolean property to set `range: clock` instead of `range: npt` that is required for `onvif` playback?
That boolean may also help in auto-plugging the source.

Any thoughts?
Comment 10 Nicola 2016-12-19 10:57:00 UTC
Created attachment 342197 [details] [review]
add handle-message signal

please review
Comment 11 dashesy 2016-12-29 23:03:42 UTC
The patch to augment headers (in different stages of PLAY, SETUP, ...) is very useful to deal with special cases when using gstreamer as a library. That would make it much easier to deal with *any* special requirement also in the future.

I worked on a small and special case that is relevant to this issue (Bug 776601)
Comment 12 Matt Staples 2017-01-16 18:06:33 UTC
Created attachment 343581 [details] [review]
register GstRTSPMessage type (gst-plugins-base)

The new signal would be easier to work with if the GstRTSPMessage were registered as a type.  The attached patch follows the pattern used by GstSDPMessage to do that.
Comment 13 Matt Staples 2017-01-16 18:12:12 UTC
Created attachment 343582 [details] [review]
Adds before-send signal to rtspsrc

For reference, here's my rtspsrc changes that use the newly registered GstRTSPMessage type.  It's similar to the previously proposed "handle-message" signal, but it's called from the more central, try_send method, and it adds the ability to drop the message entirely if the application needs to.
Comment 14 Sebastian Dröge (slomo) 2017-01-26 13:26:26 UTC
Review of attachment 343581 [details] [review]:

::: gst-libs/gst/rtsp/gstrtspmessage.c
@@ +75,3 @@
 }
 
+G_DEFINE_POINTER_TYPE (GstRTSPMessage, gst_rtsp_msg);

Should be a boxed type just like GstSDPMessage. Pointer types are mostly useless
Comment 15 Sebastian Dröge (slomo) 2017-01-26 13:28:03 UTC
Review of attachment 343582 [details] [review]:

Sounds good to me, and better than lots of properties.

::: gst/rtsp/gstrtspsrc.c
@@ +869,3 @@
+   * command should be dropped.
+   *
+   * Since: TBD

1.12
Comment 16 Sebastian Dröge (slomo) 2017-01-26 13:30:29 UTC
Comment on attachment 342197 [details] [review]
add handle-message signal

How is this different from Matt's "before-send" patch? Also "handle-message" seems misleading, this is actually a "before-send". "handle-message" sounds like something that parses the messages received from the server.
Comment 17 Nicola 2017-01-26 13:43:31 UTC
(In reply to Sebastian Dröge (slomo) from comment #16)
> Comment on attachment 342197 [details] [review] [review]
> add handle-message signal
> 
> How is this different from Matt's "before-send" patch? Also "handle-message"
> seems misleading, this is actually a "before-send". "handle-message" sounds
> like something that parses the messages received from the server.

before-send allows the same thing in a better way
Comment 18 Matt Staples 2017-01-27 19:57:40 UTC
Review of attachment 343581 [details] [review]:

::: gst-libs/gst/rtsp/gstrtspmessage.c
@@ +75,3 @@
 }
 
+G_DEFINE_POINTER_TYPE (GstRTSPMessage, gst_rtsp_msg);

Makes sense. I originally had trouble getting that to work, but I believe I've got it figured out now. 
So now that I've locally made the suggested change and tested it out, what's the preferred way to post an update?  Just add a follow-on patch with the diffs, or upload a complete replacement for this one with the two commits squashed together?
Comment 19 Sebastian Dröge (slomo) 2017-01-27 21:11:52 UTC
Just a new patch that includes the old one and your new changes, thanks :)
Comment 20 Matt Staples 2017-01-30 14:22:25 UTC
Created attachment 344556 [details] [review]
register GstRTSPMessage as a boxed type (gst-plugins-base)
Comment 21 Sebastian Dröge (slomo) 2017-01-31 11:59:39 UTC
Review of attachment 344556 [details] [review]:

::: gst-libs/gst/rtsp/gstrtspmessage.c
@@ +98,3 @@
+
+static GstRTSPMessage *gst_rtsp_message_boxed_copy (GstRTSPMessage * orig);
+static void gst_rtsp_message_boxed_free (GstRTSPMessage * msg);

Instead of forward declarations, just move the function definitions above G_DEFINE_BOXED_TYPE() :)

@@ +115,3 @@
+
+static void
+gst_rtsp_message_boxed_free (GstRTSPMessage * msg)

Why a new function? Can't you just use gst_rtsp_message_free() here?

@@ +554,3 @@
+ * Returns: a #GstRTSPResult
+ *
+ * Since: ???

Since: 1.12

(also in the other patch, then I can merge)

@@ +589,3 @@
+      break;
+    default:
+      return GST_RTSP_OK;

This should probably fail. For any other kind of message, we don't properly copy, or do we? If so add all here for which we properly copy explicitly so that whenever someone adds a new type it will be noticed that this code here has to be extended
Comment 22 Matt Staples 2017-01-31 16:29:37 UTC
Review of attachment 344556 [details] [review]:

::: gst-libs/gst/rtsp/gstrtspmessage.c
@@ +83,3 @@
+  copy->field = kv->field;
+  copy->value = g_strdup (kv->value);
+  g_return_if_fail (kv != NULL);

As long as I'm making another round of changes...  The new function, key_value_copy isn't handling the case that copy already has strings assigned.  It arguably doesn't need to since this function is static and isn't used that way.  But in hindsight, it's probably better to eliminate this and do this work in key_value_append.

@@ +98,3 @@
+
+static GstRTSPMessage *gst_rtsp_message_boxed_copy (GstRTSPMessage * orig);
+static void

Agreed. (I was just blindly following the GstSDPMessage pattern. :-) )

@@ +589,3 @@
+      break;
+    default:
+  if (ret != GST_RTSP_OK)

Ok, I'll add a (success) case for the remaining enum value, GST_RTSP_MESSAGE_INVALID, and make default return failure.
Comment 23 Matt Staples 2017-01-31 17:15:21 UTC
Review of attachment 344556 [details] [review]:

::: gst-libs/gst/rtsp/gstrtspmessage.c
@@ +115,3 @@
+
+static void
+  g_array_append_val (array, kvcopy);

Another case of just following the GstSDPMessage pattern.  In this case though, It think the wrapper function is actually necessary since gst_rtsp_message_free returns a value while GBoxedFreeFunc is defined as void.  I could get it to compile by using ((GBoxedFreeFunc)gst_rtsp_messageFree) in the call to G_DEFINE_BOXED_TYPE, but I think that could result in stack corruption at runtime since the caller wouldn't reserve the necessary stack space for the return value.
Comment 24 Matt Staples 2017-01-31 17:31:53 UTC
Created attachment 344659 [details] [review]
Adds before-send-signal to rtspsrc

Updated previous patch with "Since 1.12"
Comment 25 Matt Staples 2017-01-31 17:35:22 UTC
Created attachment 344660 [details] [review]
register GstRTSPMessage as a boxed type (gst-plugins-base)

Updates based on code review.
Comment 26 Matt Staples 2017-03-24 13:42:30 UTC
Is this one still slated for 1.12?  Or maybe there was a problem with my last commit?
Also, since this change adds a feature, does that mean it would need to go out in a 1.11 release before being allowed into the 1.12 (stable) release?
Comment 27 Matt Staples 2017-09-06 14:35:08 UTC
Has this bug slipped through the cracks?  I thought it was approved (for 1.12) pending some suggested changes, but there was never any feedback on that last round of changes.
Comment 28 Sebastian Dröge (slomo) 2017-10-13 16:05:31 UTC
Comment on attachment 344660 [details] [review]
register GstRTSPMessage as a boxed type (gst-plugins-base)

(needs to be updated to "Since: 1.14" before merging)
Comment 29 Sebastian Dröge (slomo) 2017-10-13 16:07:18 UTC
Review of attachment 344659 [details] [review]:

Looks good generally

::: gst/rtsp/gstrtspsrc.c
@@ +431,3 @@
+
+  myboolean = g_value_get_boolean (handler_return);
+  GST_DEBUG ("accum %d", myboolean);

This is not very descriptive :) Please output something more meaningful

@@ +5379,3 @@
+  if (!allow_send) {
+    GST_DEBUG_OBJECT (src, "skipping message, disabled by signal");
+    return GST_RTSP_OK;

Or maybe it should also be possible to make it an error?
Comment 30 Sebastian Dröge (slomo) 2017-10-13 16:08:43 UTC
Sorry for the long delay, this is indeed more or less ready to go. I'll make sure to get it into 1.14 if you fix the last remaining issues
Comment 31 Sebastian Dröge (slomo) 2017-10-31 17:06:48 UTC
Matt?
Comment 32 Matt Staples 2017-10-31 19:36:01 UTC
(In reply to Sebastian Dröge (slomo) from comment #31)
> Matt?

Sorry, yes, I can make those changes.  (I'm just now getting some time to get back to that.)
Comment 33 Matt Staples 2017-10-31 20:44:36 UTC
Review of attachment 344659 [details] [review]:

::: gst/rtsp/gstrtspsrc.c
@@ +431,3 @@
+
+  myboolean = g_value_get_boolean (handler_return);
+

This was just a cut/paste from select_stream_accum, following the same pattern.  I'm not sure the debug message is needed here at all.  Ok to just remove it?

@@ +5379,3 @@
+  if (!allow_send) {
+    GST_DEBUG_OBJECT (src, "skipping message, disabled by signal");
+    return GST_RTSP_OK;

I'm not sure what you mean.  Should the signal handler take something other than the allow_send boolean?  like, (send, skip or error)?
Comment 34 Sebastian Dröge (slomo) 2017-11-01 09:02:23 UTC
(In reply to Matt Staples from comment #33)
> Review of attachment 344659 [details] [review] [review]:
> 
> ::: gst/rtsp/gstrtspsrc.c
> @@ +431,3 @@
> +
> +  myboolean = g_value_get_boolean (handler_return);
> +
> 
> This was just a cut/paste from select_stream_accum, following the same
> pattern.  I'm not sure the debug message is needed here at all.  Ok to just
> remove it?

Sure

> @@ +5379,3 @@
> +  if (!allow_send) {
> +    GST_DEBUG_OBJECT (src, "skipping message, disabled by signal");
> +    return GST_RTSP_OK;
> 
> I'm not sure what you mean.  Should the signal handler take something other
> than the allow_send boolean?  like, (send, skip or error)?

Maybe let the signal return a GstRTSPResult
Comment 35 Matt Staples 2017-11-01 15:31:38 UTC
(In reply to Sebastian Dröge (slomo) from comment #34)
> (In reply to Matt Staples from comment #33)
> > Review of attachment 344659 [details] [review] [review] [review]:
> > 
> > ::: gst/rtsp/gstrtspsrc.c
> > @@ +431,3 @@
> > +
> > +  myboolean = g_value_get_boolean (handler_return);
> > +
> > 
> > This was just a cut/paste from select_stream_accum, following the same
> > pattern.  I'm not sure the debug message is needed here at all.  Ok to just
> > remove it?
> 
> Sure
> 
> > @@ +5379,3 @@
> > +  if (!allow_send) {
> > +    GST_DEBUG_OBJECT (src, "skipping message, disabled by signal");
> > +    return GST_RTSP_OK;
> > 
> > I'm not sure what you mean.  Should the signal handler take something other
> > than the allow_send boolean?  like, (send, skip or error)?
> 
> Maybe let the signal return a GstRTSPResult

The original idea of the boolean return value was to allow the signal handler to indicate that the given RTSP message (e.g., PAUSE) should be skipped entirely (i.e., not sent to the RTSP server) without otherwise causing an error.  Returning something other than GST_RTSP_OK from here would cause the whole protocol to end in error as if there had a problem communicating with the RTSP server, and that wasn't the intended behavior.  I'm not sure why an application would want to force the protocol to error out here, but it seems like there are other ways it could achieve that.
Comment 36 Sebastian Dröge (slomo) 2017-11-01 16:23:45 UTC
Ok :)
Comment 37 Matt Staples 2017-11-01 20:00:45 UTC
Created attachment 362779 [details] [review]
register GstRTSPMessage as a boxed type (gst-plugins-base)

Updates patch 344660 with "Since 1.14" instead of 1.12.
Comment 38 Matt Staples 2017-11-01 20:03:44 UTC
Created attachment 362780 [details] [review]
Adds before-send-signal to rtspsrc

Updates patch 344659 with changes requested in the review. Also changed "Since: 1.12" to "Since: 1.14", and re-based the diff on the latest code from master.
Comment 39 Sebastian Dröge (slomo) 2017-11-22 15:27:25 UTC
Comment on attachment 362779 [details] [review]
register GstRTSPMessage as a boxed type (gst-plugins-base)

You attached the same patch twice, the one for registering the boxed type is missing :(
Comment 40 Sebastian Dröge (slomo) 2017-11-22 15:29:10 UTC
Comment on attachment 362780 [details] [review]
Adds before-send-signal to rtspsrc

Can you update the commit message? It should be:

rtspsrc: One line summary of the change
[empty line]
Long description of what was changed and why.
Can be multiple lines.
[empty line]
link to this bugzilla ticket
Comment 41 Matt Staples 2017-11-27 14:54:08 UTC
Created attachment 364505 [details] [review]
Adds a signal to rtspsrc, allowing RTSP messages to be modified before being sent
Comment 42 Matt Staples 2017-11-27 14:54:48 UTC
Created attachment 364506 [details]
register GstRTSPMessage as a boxed type (gst-plugins-base)
Comment 43 Matt Staples 2017-11-27 14:56:19 UTC
Created attachment 364507 [details] [review]
register GstRTSPMessage as a boxed type (gst-plugins-base)
Comment 44 Sebastian Dröge (slomo) 2017-12-06 08:42:02 UTC
commit 4b4f8d1a05e2ca5e5fbd4a33d36424104ce8a86e (HEAD -> master)
Author: Matt Staples <staples255@gmail.com>
Date:   Tue Oct 31 16:10:19 2017 -0600

    rtsp: Register GstRTSPMessage as a boxed type
    
    Registering GstRTSPMessage as a boxed type allows it to be conveniently
    used as an argument to signals, a-la GstSDPMessage, and general usage
    from bindings.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=762884
Comment 45 Sebastian Dröge (slomo) 2017-12-06 08:46:34 UTC
commit ea1b10e4cacfdb3f9205a4a849bdbf6653ece92e (HEAD -> master)
Author: Matt Staples <staples255@gmail.com>
Date:   Wed Nov 1 08:21:37 2017 -0600

    rtspsrc: Add a signal to allow outgoing messages to be modified or dropped
    
    This feature allows applications to implement extensions to the RTSP
    protocol, such as those defined in the ONVIF Streaming Specification.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=762884