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 792131 - rtspsrc: add an action signal to send SET_PARAMETER
rtspsrc: add an action signal to send SET_PARAMETER
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other Linux
: Normal enhancement
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-01-02 16:42 UTC by Nicola
Modified: 2018-08-16 06:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed fix (4.54 KB, patch)
2018-01-02 16:42 UTC, Nicola
needs-work Details | Review
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals (21.33 KB, patch)
2018-07-05 11:06 UTC, ulfo
none Details | Review
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver2 (23.33 KB, patch)
2018-07-10 13:26 UTC, ulfo
none Details | Review
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver3 (22.30 KB, patch)
2018-08-13 12:41 UTC, ulfo
none Details | Review
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver4 (21.47 KB, patch)
2018-08-15 13:48 UTC, ulfo
committed Details | Review

Description Nicola 2018-01-02 16:42:10 UTC
Created attachment 366207 [details] [review]
proposed fix

initial implementation, I'm a bit uncertain about the return value, actually it is void and so the server response is ignored, suggestions are welcome.

In my test I see something like this:

rtspsrc request:


SET_PARAMETER rtsp://192.168.3.123/axis-media/media.amp RTSP/1.0
CSeq: 7
User-Agent: GStreamer/1.13.0.1
Content-Type: text/parameters
Session: IemB45YYdQRExppD
Authorization: Digest username="root", realm="AXIS_ACCC8E5CE596", nonce="00055658Y507659aff44419369eb4f0ff9704f52cff924", uri="rtsp://192.168.3.123/axis-media/media.amp", response="f14b02119d0fc9aa02a848e42e3de04f"
Date: Tue, 02 Jan 2018 16:38:42 GMT
Content-Length: 19

Renew-Stream: yes

camera response:

RTSP/1.0 200 OK
CSeq: 7
Server: GStreamer RTSP server
Session: IemB45YYdQRExppD; timeout=60
Date: Tue, 02 Jan 2018 16:38:32 GMT

or:

RTSP/1.0 451 Parameter Not Understood
CSeq: 7
Server: GStreamer RTSP server
Session: aN1ZL3+SFs+iEA4O; timeout=60
Date: Tue, 02 Jan 2018 16:06:37 GMT

if a set an unsupported parameter,

we should return a GstRTSPMessage response and let the user free it?
Comment 1 Nicola 2018-01-02 16:46:08 UTC
or maybe the response code is enough? 

Actually I don't need GET_PARAMETER but maybe could be useful to have a way to send this request too.
Comment 2 Nicola 2018-01-03 09:11:17 UTC
well, the problem in returning response code or message body is that actually rtspsrc ignore response message, I need an advice on how to handle this in the proper way, thanks
Comment 3 Nicola 2018-01-03 16:26:30 UTC
(In reply to Nicola from comment #2)
> well, the problem in returning response code or message body is that
> actually rtspsrc ignore response message, I need an advice on how to handle
> this in the proper way, thanks

the problem here is that if we use for example gst_rtspsrc_send to send the SET_PARAMETER request it will internally call gst_rtspsrc_connection_send to send the request and then gst_rtsp_src_receive_response and then gst_rtspsrc_connection_receive to receive the response and populate the passed response object.

If udp or tcp receive loop are running they also call gst_rtspsrc_connection_receive that will receive the response to the SET_PARAMETER request. 

So basically, for what I understand, actually rtspsrc does not have a method that allow to send a request and receive a response while udp or tcp loop are running. 

For my use case send the SET_PARAMETER request is enough, the response is not so important. Eventually I can work on this but I need some advices to produce an acceptable patch, thanks!
Comment 4 Sebastian Dröge (slomo) 2018-06-27 10:35:38 UTC
A bit more is needed here, and we should also add GET_PARAMETER support. We a) need to keep track of multiple requests that are pending and b) handle them async (the GstRTSPWatch is async for sending and the response can come at some point later) for which I'd propose to use GstPromise to get the actual response.
Comment 5 ulfo 2018-06-28 09:26:15 UTC
Hi,

I will provide a patch next week with GET_PARAMETER and SET_PARAMETER using GstPromise during next week
Comment 6 ulfo 2018-07-05 11:06:22 UTC
Created attachment 372950 [details] [review]
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals

The error handling is a bit basic, maybe should the RTSP code and reason be included in the response as well
Comment 7 Sebastian Dröge (slomo) 2018-07-05 18:48:15 UTC
(In reply to ulfo from comment #6)

> The error handling is a bit basic, maybe should the RTSP code and reason be
> included in the response as well

That would make sense. After all we have a GstStructure and can store everything, and we have this information available anyway. Just put it in there :)
Comment 8 Sebastian Dröge (slomo) 2018-07-09 07:54:50 UTC
Review of attachment 372950 [details] [review]:

Looks generally good, thanks!

::: gst/rtsp/gstrtspsrc.c
@@ +1143,3 @@
+  }
+
+  parameters = g_malloc0 (2 * sizeof (gchar *));

This can be stack allocated :)

@@ +1175,3 @@
+  req->body = g_string_new (NULL);
+  while (*parameters) {
+    g_string_append_printf (req->body, "%s:\r\n", *parameters);

Should we do some sanity-checks for parameters here? They should probably not contain \n or \r at least, or stuff will fail in interesting ways

@@ +1227,3 @@
+  if (gst_rtspsrc_loop_send_cmd (src, CMD_WAIT, CMD_LOOP)) {
+    GST_RTSP_STREAM_LOCK (src);
+    GST_RTSP_STREAM_UNLOCK (src);

Why do we block here (and above)? The response comes async anyway

@@ +1294,3 @@
 
+  src->set_param_q = g_queue_new ();
+  src->get_param_q = g_queue_new ();

If you use a GQueue, just store it directly inside the instance struct and use g_queue_init() here

@@ +1311,3 @@
+  ParameterRequest *req = data;
+
+  gst_promise_unref (req->promise);

Should probably get gst_promise_expire() here if we're freeing and will never ever reply to it again

@@ +5796,3 @@
+      break;
+    case CMD_SET_PARAMETER:
+      GST_ELEMENT_PROGRESS (src, ERROR, "request", ("SET_PARAMETER failed"));

And these should probably get some kind of error response on the promise?

@@ +8997,3 @@
+
+  res = gst_rtsp_message_add_header (&request, GST_RTSP_HDR_CONTENT_TYPE,
+      req->content_type == NULL ? "text/parameters" : req->content_type->str);

Why use a GString for the content_type btw? Could be a plain gchar* or not?
Comment 9 ulfo 2018-07-10 13:26:27 UTC
Created attachment 372988 [details] [review]
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver2

Thank you for your feedback. I have fixed most of the comments but I'm still a 
bit uncertain how to best handle gst_rtspsrc_loop_end_cmd in 
gst_rtspsrc_get_parameter and gst_rtspsrc_set_parameter.
Comment 10 Sebastian Dröge (slomo) 2018-07-11 09:21:11 UTC
(In reply to ulfo from comment #9)

> Thank you for your feedback. I have fixed most of the comments but I'm still
> a  bit uncertain how to best handle gst_rtspsrc_loop_end_cmd in 
> gst_rtspsrc_get_parameter and gst_rtspsrc_set_parameter.

What do you mean exactly?
Comment 11 ulfo 2018-07-11 11:33:16 UTC
(In reply to Sebastian Dröge (slomo) from comment #10)
> (In reply to ulfo from comment #9)
> 
> > Thank you for your feedback. I have fixed most of the comments but I'm still
> > a  bit uncertain how to best handle gst_rtspsrc_loop_end_cmd in 
> > gst_rtspsrc_get_parameter and gst_rtspsrc_set_parameter.
> 
> What do you mean exactly?

<@@ +5796,3 @@
<+      break;
<+    case CMD_SET_PARAMETER:
<+      GST_ELEMENT_PROGRESS (src, ERROR, "request", ("SET_PARAMETER failed"));
<
<And these should probably get some kind of error response on the promise?

It was this comment that triggered it. For an example the call sequence to
gst_rtspsrc_loop_error_cmd is

gst_rtspsrc_set_parameter -> gst_rtspsrc_loop_end_cmd -> gst_rtspsrc_loop_error_cmd

and I have already an active request so then I have to pass the request to
gst_rtspsrc_loop_end_cmd like

if (async && res != GST_RTSP_OK)
      gst_rtspsrc_loop_end_cmd (src, CMD_SET_PARAMETER, res, req);

Well that is one way to do it
Comment 12 Sebastian Dröge (slomo) 2018-07-11 13:16:03 UTC
I'm not sure, does it work and is it done like this elsewhere? :)
Comment 13 ulfo 2018-07-11 14:00:32 UTC
(In reply to Sebastian Dröge (slomo) from comment #12)
> I'm not sure, does it work and is it done like this elsewhere? :)

Maybe I misunderstood the code a bit. Is the sole purpose with the gst_rtspsrc_loop_xxx_cmd functions to post a message on the bus? If that is case shouldn't gst_promise_reply be enough in gst_rtspsrc_set_parameter/gst_rtspsrc_get_parameter
Comment 14 Sebastian Dröge (slomo) 2018-07-12 09:00:02 UTC
Yes, it's for updating the progress messages. You don't want progress messages for any of this and if something indirectly posts those messages already for the SET/GET_PARAMETER code then that's a bug. I guess that means the calls to send_cmd() are wrong as-is.
Comment 15 ulfo 2018-07-12 09:23:49 UTC
(In reply to Sebastian Dröge (slomo) from comment #14)
> Yes, it's for updating the progress messages. You don't want progress
> messages for any of this and if something indirectly posts those messages
> already for the SET/GET_PARAMETER code then that's a bug. I guess that means
> the calls to send_cmd() are wrong as-is.

OK, great. I will update the patch according to this
Comment 16 Sebastian Dröge (slomo) 2018-08-13 07:52:46 UTC
Do you have any updates on this?
Comment 17 ulfo 2018-08-13 08:08:57 UTC
(In reply to Sebastian Dröge (slomo) from comment #16)
> Do you have any updates on this?

I'm back from vacation today so there will be an update to this issue later today
Comment 18 ulfo 2018-08-13 12:41:40 UTC
Created attachment 373308 [details] [review]
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver3
Comment 19 Sebastian Dröge (slomo) 2018-08-14 06:48:07 UTC
Review of attachment 373308 [details] [review]:

::: gst/rtsp/gstrtspsrc.c
@@ +1073,3 @@
+      G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION, G_STRUCT_OFFSET (GstRTSPSrcClass,
+          get_parameter), NULL, NULL, g_cclosure_marshal_generic,
+      G_TYPE_BOOLEAN, 3, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_POINTER);

This should be GST_TYPE_PROMISE, not GST_TYPE_POINTER

@@ +1091,3 @@
+      G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION, G_STRUCT_OFFSET (GstRTSPSrcClass,
+          get_parameters), NULL, NULL, g_cclosure_marshal_generic,
+      G_TYPE_BOOLEAN, 3, G_TYPE_STRV, G_TYPE_STRING, G_TYPE_POINTER);

And here

@@ +1111,3 @@
+          set_parameter), NULL, NULL, g_cclosure_marshal_generic,
+      G_TYPE_BOOLEAN, 4, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING,
+      G_TYPE_POINTER);

And here

@@ +9036,3 @@
+  {
+
+    if (req) {

Shouldn't we expire/fail the promise in all other cases here or does that automatically happen later?

Same about the other functions below

@@ +9074,3 @@
+
+    GST_ELEMENT_ERROR (src, LIBRARY, INIT, (NULL),
+        ("Could not create request. (%s)", str));

All the errors here, do they have to be fatal (error messages are fatal)? Or could we instead just continue normally and only signal an error via the promise?

@@ +9083,3 @@
+
+    GST_ELEMENT_ERROR (src, LIBRARY, INIT, (NULL),
+        ("Could add content header. (%s)", str));

"Could not" instead of "Could" I assume? Also above :)
Comment 20 ulfo 2018-08-14 07:28:22 UTC
(In reply to Sebastian Dröge (slomo) from comment #19)
> Review of attachment 373308 [details] [review] [review]:
> 
> ::: gst/rtsp/gstrtspsrc.c
> @@ +1073,3 @@
> +      G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION, G_STRUCT_OFFSET (GstRTSPSrcClass,
> +          get_parameter), NULL, NULL, g_cclosure_marshal_generic,
> +      G_TYPE_BOOLEAN, 3, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_POINTER);
> 
> This should be GST_TYPE_PROMISE, not GST_TYPE_POINTER
> 
> @@ +1091,3 @@
> +      G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION, G_STRUCT_OFFSET (GstRTSPSrcClass,
> +          get_parameters), NULL, NULL, g_cclosure_marshal_generic,
> +      G_TYPE_BOOLEAN, 3, G_TYPE_STRV, G_TYPE_STRING, G_TYPE_POINTER);
> 
> And here
> 
> @@ +1111,3 @@
> +          set_parameter), NULL, NULL, g_cclosure_marshal_generic,
> +      G_TYPE_BOOLEAN, 4, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING,
> +      G_TYPE_POINTER);
> 
> And here
> 
> @@ +9036,3 @@
> +  {
> +
> +    if (req) {
> 
> Shouldn't we expire/fail the promise in all other cases here or does that
> automatically happen later?
> 
> Same about the other functions below
> 
> @@ +9074,3 @@
> +
> +    GST_ELEMENT_ERROR (src, LIBRARY, INIT, (NULL),
> +        ("Could not create request. (%s)", str));
> 
> All the errors here, do they have to be fatal (error messages are fatal)? Or
> could we instead just continue normally and only signal an error via the
> promise?
> 
> @@ +9083,3 @@
> +
> +    GST_ELEMENT_ERROR (src, LIBRARY, INIT, (NULL),
> +        ("Could add content header. (%s)", str));
> 
> "Could not" instead of "Could" I assume? Also above :)

Yes, it is better to pass on the errors via the promise. Should we do that for all potential errors in gst_rtspsrc_set_parameter/gst_rtspsrc_get_parameter?

if (req) { 

is unnecessary (req == NULL is a major bug) but currently "promises" can only expire when gst_rtspsrc_cleanup is called. So the question is, should a promise just live in the queue for a limited period of time? Plus, should we limit the number of SET/GET requests in the queue?
Comment 21 Sebastian Dröge (slomo) 2018-08-14 07:36:27 UTC
(In reply to ulfo from comment #20)
> 
> Yes, it is better to pass on the errors via the promise. Should we do that
> for all potential errors in
> gst_rtspsrc_set_parameter/gst_rtspsrc_get_parameter?

All errors that don't mean that the element will stop functioning afterwards. If the server closes the connection for example then that's an error.

> if (req) { 
> 
> is unnecessary (req == NULL is a major bug) but currently "promises" can
> only expire when gst_rtspsrc_cleanup is called. So the question is, should a
> promise just live in the queue for a limited period of time?

I wouldn't time them out, but if a promise is in the queue and the corresponding message is sent to the server then it should be handled on response. Either by a successful or error reply.

> Plus, should we limit the number of SET/GET requests in the queue?

No limit seems fine, let the application handle that
Comment 22 ulfo 2018-08-15 13:48:41 UTC
Created attachment 373347 [details] [review]
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver4
Comment 23 Sebastian Dröge (slomo) 2018-08-16 06:04:03 UTC
commit 0f6a4e7c9893fb81b6888b4332deaf2db921d634 (HEAD -> master)
Author: Ulf Olsson <ulfo@axis.com>
Date:   Wed Aug 15 13:43:53 2018 +0200

    rtspsrc: Add support for SET_PARAMETER and GET_PARAMETER using signals
    
    https://bugzilla.gnome.org/show_bug.cgi?id=792131