GNOME Bugzilla – Bug 762884
rtspsrc: allow to control headers via properties
Last modified: 2017-12-06 08:47:11 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
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..
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?)
Erm, I may just be confused, this is rtsp*src* after all. I was thinking of rtsp-server.
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
Independent of that, we should make onvif generally work out of the box with rtspsrc. See bug #762910.
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?
(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
(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.
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?
Created attachment 342197 [details] [review] add handle-message signal please review
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)
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.
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.
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
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 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.
(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
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?
Just a new patch that includes the old one and your new changes, thanks :)
Created attachment 344556 [details] [review] register GstRTSPMessage as a boxed type (gst-plugins-base)
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
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.
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.
Created attachment 344659 [details] [review] Adds before-send-signal to rtspsrc Updated previous patch with "Since 1.12"
Created attachment 344660 [details] [review] register GstRTSPMessage as a boxed type (gst-plugins-base) Updates based on code review.
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?
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 on attachment 344660 [details] [review] register GstRTSPMessage as a boxed type (gst-plugins-base) (needs to be updated to "Since: 1.14" before merging)
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?
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
Matt?
(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.)
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)?
(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
(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.
Ok :)
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.
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 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 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
Created attachment 364505 [details] [review] Adds a signal to rtspsrc, allowing RTSP messages to be modified before being sent
Created attachment 364506 [details] register GstRTSPMessage as a boxed type (gst-plugins-base)
Created attachment 364507 [details] [review] register GstRTSPMessage as a boxed type (gst-plugins-base)
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
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