Created attachment 370311 [details] [review]
rtsp-server: add API to enable retransmission requests

Created attachment 370315 [details] [review]
rtsp-server: add API to enable retransmission requests

"do-retransmission" was previously set when rtx-time != 0,
which made no sense as do-retransmission is used to enable
the sending of retransmission requests, where as rtx-time
is used by the peer to enable storing of buffers in order
to respond to retransmission requests.

rtsp-media now also provides a callback for the
request-aux-receiver signal.

Review of attachment 370315 [details] [review]:

Looks mostly good to me

::: gst/rtsp-server/rtsp-media.c
@@ +163,3 @@
 #define DEFAULT_STOP_ON_DISCONNECT TRUE
 
+#define DEFAULT_DO_RETRANSMISSION FALSE

Also needs a GObject property for consistency I guess

::: gst/rtsp-server/rtsp-stream.c
@@ +2369,3 @@
+  stream->priv->rtxreceive = gst_element_factory_make ("rtprtxreceive", NULL);
+  pt_map = gst_structure_new_empty ("application/x-rtp-pt-map");
+  g_hash_table_foreach (stream->priv->ptmap, (GHFunc) add_rtx_pt, pt_map);

This might have to be updated whenever the ptmap changes, or do we just not support that?

Review of attachment 370315 [details] [review]:

::: gst/rtsp-server/rtsp-media.c
@@ +3076,3 @@
+  g_mutex_unlock (&priv->lock);
+
+  return gst_rtsp_stream_request_aux_receiver (stream, sessid);

Maybe should handle the case when the stream is not found (return NULL)? And we find the stream by comparing the sessid with its index, but then we still pass the sessid into this function? The stream already knows about it :)

(In reply to Sebastian Dröge (slomo) from comment #3)
> Review of attachment 370315 [details] [review] [review]:
> 
> Looks mostly good to me
> 
> ::: gst/rtsp-server/rtsp-media.c
> @@ +163,3 @@
>  #define DEFAULT_STOP_ON_DISCONNECT TRUE
>  
> +#define DEFAULT_DO_RETRANSMISSION FALSE
> 
> Also needs a GObject property for consistency I guess

rtx-time doesn't expose a property, I can add one but then one will also be needed in media-factory

> 
> ::: gst/rtsp-server/rtsp-stream.c
> @@ +2369,3 @@
> +  stream->priv->rtxreceive = gst_element_factory_make ("rtprtxreceive",
> NULL);
> +  pt_map = gst_structure_new_empty ("application/x-rtp-pt-map");
> +  g_hash_table_foreach (stream->priv->ptmap, (GHFunc) add_rtx_pt, pt_map);
> 
> This might have to be updated whenever the ptmap changes, or do we just not
> support that?

I'm not sure whether a new ANNOUNCE in the RECORD case is supported by rtsp-server, but I'll update stream_set_pt_map anyway :)

(In reply to Sebastian Dröge (slomo) from comment #4)
> Review of attachment 370315 [details] [review] [review]:
> 
> ::: gst/rtsp-server/rtsp-media.c
> @@ +3076,3 @@
> +  g_mutex_unlock (&priv->lock);
> +
> +  return gst_rtsp_stream_request_aux_receiver (stream, sessid);
> 
> Maybe should handle the case when the stream is not found (return NULL)?

Indeed, that is also a bug in the function that was copied (request_aux_sender), I'll fix both

> And
> we find the stream by comparing the sessid with its index, but then we still
> pass the sessid into this function? The stream already knows about it :)

this also was copied from stream_request_aux_sender, as this is "API" we cannot change that prototype, and I'd rather have both functions share the same prototype?

Fine with me :)

Created attachment 370339 [details] [review]
rtsp-server: add API to enable retransmission requests

"do-retransmission" was previously set when rtx-time != 0,
which made no sense as do-retransmission is used to enable
the sending of retransmission requests, where as rtx-time
is used by the peer to enable storing of buffers in order
to respond to retransmission requests.

rtsp-media now also provides a callback for the
request-aux-receiver signal.

Attachment 370339 [details] pushed as 988db52 - rtsp-server: add API to enable retransmission requests