Comment on attachment 305079 [details] [review]
rtsp-media: always use real payloader when creating streams

In the interest of keeping the patch to a minimum, please leave the find_payload_element() function where it is now and simply add a declaration to the top of the file.

Also, it would be great if you could provide some context for this change, like what does it fix exactly?

I should probably describe the use case first, I am adding the onviftimestamp element (https://bugzilla.gnome.org/show_bug.cgi?id=731769) to the pipeline after the payloader. So the payloader and the onviftimestamp are inside an inner GstBin called "pay%d".
What happens than is that GstRTSPMedia will crate the stream with the GstBin instead of using the real payloader. Which will then be a problem when GstRTSPStream tries to use the payloader, e.g. from gst_rtsp_stream_set_mtu(). 
AFAIAA the only limitation of using an inner GstBin which contains the actual payloader is this.

Created attachment 305277 [details] [review]
rtsp-media: always use real payloader when creating streams

Definitely a good idea! I'm going to need this very soon too

Review of attachment 305277 [details] [review]:

::: gst/rtsp-server/rtsp-media.c
@@ +1457,3 @@
+      if (pay == NULL) {
+        GST_WARNING ("could not find real payloader, using bin");
+        gst_rtsp_media_create_stream (media, elem, pad);

Why is this a warning? Should only be one if GST_IS_BIN(elem), right? Otherwise we just have the plain payloader as elem?

> I should probably describe the use case first, (snip)

Thanks, could you also put some sort of explanation into the commit message please :)

(In reply to Sebastian Dröge (slomo) from comment #5)
> Review of attachment 305277 [details] [review] [review]:
> 
> ::: gst/rtsp-server/rtsp-media.c
> @@ +1457,3 @@
> +      if (pay == NULL) {
> +        GST_WARNING ("could not find real payloader, using bin");
> +        gst_rtsp_media_create_stream (media, elem, pad);
> 
> Why is this a warning? Should only be one if GST_IS_BIN(elem), right?
> Otherwise we just have the plain payloader as elem?

find_payload_element() can only return NULL when elem is a GstBin and there is no element which inherits from GstBaseRTPPayload in it.
May be there should be a g_assert instead before the warning: g_assert (GST_IS_BIN (elem))?

Comment on attachment 305277 [details] [review]
rtsp-media: always use real payloader when creating streams

Ah no, that's ok then

commit fb71b9c4e9bb66784491df81537d4865b99480f1
Author: Ognyan Tonchev <ognyan@axis.com>
Date:   Thu Jun 11 17:39:00 2015 +0200

    rtsp-media: Always use real payloader when creating streams
    
    A bin that contains the real payloader might be used as payloader. In this
    case we have to get the real payloader for the various properties it provides.
    
    Example use cases for this are bins that payload some media and then have
    additional elements that add metadata or RTP extension headers to the stream.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=750800