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.

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

(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!

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.

Hi,

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

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

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

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?

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.

(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?

(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

I'm not sure, does it work and is it done like this elsewhere? :)

(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

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.

(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

Do you have any updates on this?

(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

Created attachment 373308 [details] [review]
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver3

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

(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?

(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

Created attachment 373347 [details] [review]
Support RTSP SET_PARAMETER and GET_PARAMETER using action signals ver4

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