Created attachment 270544 [details] [review]
uridecodebin: "iradio-mode" property is G_TYPE_BOOLEAN

When selecting a source for a stream uri, decodebin sets an existent
"iradio-mode" property in the source to TRUE. Unfortunately, we were
checking wheter the property was G_TYPE_STRING instead of
G_TYPE_BOOLEAN.

uridecodebin is supposed to automatically set the "iradio-mode" property
when the uri is a "http://" stream and the source element supports this
property but it is actually not doing it.

This problem is probably affecting several versions

What is the actual problem though?

This could should probably just be removed, since we default to iradio-mode=true these days.

(In reply to comment #4)
> What is the actual problem though?
> 
> This could should probably just be removed, since we default to
> iradio-mode=true these days.

mmm, this could be true in souphttpsrc (?), but not in a custom source that may be used with playbin/uridecodebin

This problem raised in the source element used in WebKit. Do you think we should default to "true" always there too or do you think we should keep the current behavior and change this in uridecodebin?

It's only used in WebKit with GStreamer decodebin/playbin, right? If so, yes, I think it should default to true. In fact, you probably don't even need a property.

It should just send the request headers, and when the server doesn't understand them it will ignore them, otherwise you get back some icy headers in the response.

Originally we didn't have that property in souphttpsrc and just defaulted to true, but that was a problem for people who wanted to do souphttpsrc ! mpegaudioparse ! filesink, because they got glitches in the stream, so we added the property to allow them to switch it off.

In your case that's not a problem, so I'd just always send it and get rid of the property.

Do you agree with this approach? Can I just remove this from uridecodebin?

Yes, I think it is OK.

I've checked other source elements:
 * gst-plugins-bad: neonhttpsrc defaults to TRUE
 * gst-plugins-bad: wininetsrc defaults to FALSE

Hence, I think I will open a bug and upload a patch for wininetsrc.

In the meanwhile, I'm changing the description of this one and updating the patch.

Created attachment 270908 [details] [review]
uridecodebin: Removed setting "iradio-mode" property in the source element

The "iradio-mode" property used to have a default FALSE value in HTTP
source elements but now it should default to TRUE or just do not exist
as a property so it is not really needed to set it any more in
uridecodebin.

Fixes

Review of attachment 270908 [details] [review]:

::: gst/playback/gsturidecodebin.c
@@ -1318,3 @@
-    pspec = g_object_class_find_property (source_class, "iradio-mode");
-
-    if (pspec && G_PARAM_SPEC_VALUE_TYPE (pspec) == G_TYPE_STRING) {

Huh? STRING?

Normally I would say that we can only do this change in 2.0 as it might break existing code... but considering that the code you remove with this patch could've never worked that should be fine.

Any objections?

OK with us. We have already removed the property in the WebKit source:
http://trac.webkit.org/changeset/165106

Any update on this? The patch has a negative "accepted" flag.

commit 09872442f88286198883fee5e60c377ef0832a46
Author: Andres Gomez <agomez@igalia.com>
Date:   Tue Mar 4 16:51:11 2014 +0200

    uridecodebin: Removed setting "iradio-mode" property in the source element
    
    The "iradio-mode" property used to have a default FALSE value in HTTP
    source elements but now it should default to TRUE or just do not exist
    as a property so it is not really needed to set it any more in
    uridecodebin.
    
    Apart from that this code could've never worked as uridecodebin looks for a
    string-typed iradio-mode property, but it's a boolean in all sources.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=725383

Thanks for taking care of this!