[Bug 792131] New: rtspsrc: add an action signal to send SET_PARAMETER

["GStreamer" (GNOME Bugzilla)]

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

Bug ID: 792131
Summary: rtspsrc: add an action signal to send SET_PARAMETER
Classification: Platform
Product: GStreamer
Version: 1.x
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gst-plugins-good
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@svrinformatica.it
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

Created attachment 366207
--> https://bugzilla.gnome.org/attachment.cgi?id=366207&action=edit
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?

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #1 from Nicola <***@svrinformatica.it> ---
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.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #2 from Nicola <***@svrinformatica.it> ---
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

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #3 from Nicola <***@svrinformatica.it> ---
(In reply to Nicola from comment #2)

Post by "GStreamer" (GNOME Bugzilla)
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!

["GStreamer" (GNOME Bugzilla)]

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

Tim-Philipp Müller <***@zen.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@zen.co.uk
Severity|normal |enhancement

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net

--- Comment #4 from Sebastian Dröge (slomo) <***@coaxion.net> ---
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.

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #366207|none |needs-work
status| |

["GStreamer" (GNOME Bugzilla)]

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

***@axis.com changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@axis.com

--- Comment #5 from ***@axis.com ---
Hi,

I will provide a patch next week with GET_PARAMETER and SET_PARAMETER using
GstPromise during next week

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #6 from ***@axis.com ---
Created attachment 372950
--> https://bugzilla.gnome.org/attachment.cgi?id=372950&action=edit
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

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #366207|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #7 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to ulfo from comment #6)

Post by "GStreamer" (GNOME Bugzilla)
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
:)

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372950|none |needs-work
status| |

--- Comment #8 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 372950:
--> (https://bugzilla.gnome.org/review?bug=792131&attachment=372950)

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?

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #9 from ***@axis.com ---
Created attachment 372988
--> https://bugzilla.gnome.org/attachment.cgi?id=372988&action=edit
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.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #10 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to ulfo from comment #9)

Post by "GStreamer" (GNOME Bugzilla)
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?

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #11 from ***@axis.com ---
(In reply to Sebastian Dröge (slomo) from comment #10)

Post by "GStreamer" (GNOME Bugzilla)
(In reply to ulfo from comment #9)

Post by "GStreamer" (GNOME Bugzilla)
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

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #12 from Sebastian Dröge (slomo) <***@coaxion.net> ---
I'm not sure, does it work and is it done like this elsewhere? :)

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #13 from ***@axis.com ---
(In reply to Sebastian Dröge (slomo) from comment #12)

Post by "GStreamer" (GNOME Bugzilla)
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

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #14 from Sebastian Dröge (slomo) <***@coaxion.net> ---
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.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #15 from ***@axis.com ---
(In reply to Sebastian Dröge (slomo) from comment #14)

Post by "GStreamer" (GNOME Bugzilla)
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

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #16 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Do you have any updates on this?

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #17 from ***@axis.com ---
(In reply to Sebastian Dröge (slomo) from comment #16)

Post by "GStreamer" (GNOME Bugzilla)
Do you have any updates on this?

I'm back from vacation today so there will be an update to this issue later
today

["GStreamer" (GNOME Bugzilla)]

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

***@axis.com changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #372950|needs-work |none
status| |
Attachment #372950|0 |1
is obsolete| |
Attachment #372988|0 |1
is obsolete| |

--- Comment #18 from ***@axis.com ---
Created attachment 373308
--> https://bugzilla.gnome.org/attachment.cgi?id=373308&action=edit
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver3

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #373308|none |needs-work
status| |

--- Comment #19 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Review of attachment 373308:
--> (https://bugzilla.gnome.org/review?bug=792131&attachment=373308)

::: 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 :)

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #20 from ***@axis.com ---
(In reply to Sebastian Dröge (slomo) from comment #19)

Post by "GStreamer" (GNOME Bugzilla)
::: 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?

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #21 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to ulfo from comment #20)

Post by "GStreamer" (GNOME Bugzilla)
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.

Post by "GStreamer" (GNOME Bugzilla)
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.

Post by "GStreamer" (GNOME Bugzilla)
Plus, should we limit the number of SET/GET requests in the queue?

No limit seems fine, let the application handle that

["GStreamer" (GNOME Bugzilla)]

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

***@axis.com changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #373308|needs-work |none
status| |
Attachment #373308|0 |1
is obsolete| |

--- Comment #22 from ***@axis.com ---
Created attachment 373347
--> https://bugzilla.gnome.org/attachment.cgi?id=373347&action=edit
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver4

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

--- Comment #23 from Sebastian Dröge (slomo) <***@coaxion.net> ---
commit 0f6a4e7c9893fb81b6888b4332deaf2db921d634 (HEAD -> master)
Author: Ulf Olsson <***@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

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #373347|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|git master |1.15.1