GNOME Bugzilla – Bug 333657
Replacing icy demuxing in gnomevfssrc
Last modified: 2006-06-18 23:55:30 UTC
Currently, there's a lot of cruft in gnomevfssrc for doing icy (i.e. shoutcast, also implemented in icecast and probably others) style metadata. A minimal amount of that has to remain, but most of the actual demuxing should be elsewhere. I'm going to attach a series of patches here. They're not ready to go in yet. 1) The actual icydemux element. Mostly working, but some lurking problems when used in decodebin? 2) A hack to gnomevfssrc to make this usable. Very much a hack; a proper patch would be written to actually remove the icy code properly, and clean things up. 3) A change to typefind to make it not override things that already have caps - upstream elements are assumed to know better. This probably requires more discussion... Without this, typefind still thinks mp3 streams are mp3 streams, even though they have icy stuff at intervals; icydemux doesn't get autoplugged, and mp3 decoders make nasty sounding blips. This works if you do gst-launch gnomevfssrc iradio-mode=true location=http://wherever/ ! icydemux ! mad ! audioconvert ! alsasink Currently it fails if you try to use decodebin instead of icydemux ! mad (though those two elements get appropriately plugged), and sounds bad. Haven't found out why yet.
Created attachment 60792 [details] Icydemux plugin. Actual plugin. Today, I can't convince cvs to diff my tree, so I'll just attach the two files without the autotools cruft, which is very simple anyway.
Created attachment 60793 [details] Icydemux header And the header file.
Created attachment 60794 [details] [review] Hack to gnomevfssrc
Created attachment 60795 [details] [review] typefind patch. Kinda hacky.
I have no idea what changed, but this now works in decodebin. It even works in playbin. I'm using current or recent cvs head for everything, with the change to gstplaybasebin.c from the patch in bug 325847 to set iradio-mode on the source element for http streams.
Yep, this works well for me now too - maybe I just had some stale bits in my tree that were causing it to fail. Anyway, I'd like some review of these changes, particularly the typefind one and the new element (the gnomevfssrc changes are a hack, and I'd clean those up before committing). One other thing: would people be happy with completely dropping the audiocast protocol support? I don't know of any servers that ever use it (and even those that are capable tend not to, as clients implement shoutcast's 'icy' protocol), and it's a lot of complex/yucky code in gnomevfssrc, that really doesn't belong there at all. I'd much rather just delete it all. Also, this sort of breaks API: now, with iradio-mode, gnomevfssrc doesn't demux things itself; it outputs different caps instead. Using it in playbin ends up with the same effect (because we autoplug icydemux), so does this matter?
I noticed that the typefind change causes assertion failures like this: (gst-launch-0.10:1357): GStreamer-CRITICAL **: gst_caps_is_any: assertion `GST_IS_CAPS (caps)' failed ** ERROR **: file gsttypefindelement.c: line 156 (gst_type_find_element_have_type): assertion failed: (caps != NULL) aborting... when trying to play this stream: mmsh://bdcast-jeffersonpilot-kbzt-fm.wm.llnwd.net/bdcast_jeffersonpilot_kbzt-fm%3FMSWMExt%3D.asf Adding a check for caps == NULL before the gst_caps_is_any() call on line 529 fixes it. This may be just hiding a separate bug somewhere else, of course.
Created attachment 61920 [details] [review] Updated typefind patch Yes, thanks for noticing that. Checking for caps == NULL is indeed the right fix for this. Amended patch attached.
I like this approach, it's much cleaner than doing all this inside gnomevfssrc IMHO. Two minor issues with the typefind patch: - the int argument before the caps in the g_signal_emit() should be a typefind probability like GST_TYPE_FIND_MAXIMUM, not a size - we should probably set typefind->caps to the caps from the buffer (or alternatively change the gst_buffer_set_caps (store, typefind->caps) to gst_buffer_set_caps (store, GST_BUFFER_CAPS (buffer)); This only triggers in the extremely unlikely case that upstream sends a very small/too small to typefind first buffer without caps and then a second one with caps, so it's not that important I guess. other than that I think the typefind bit should go in, I've wanted to add something like that a while ago as well, but then forgot. icydemux looks good to me as well, haven't tried it yet though. One thing I've noticed though: can icecast streams ever contain ID3v2 tags in front of the actual stream data? if yes, we probably need to think about that a bit more, because - id3demux looks at buffer offsets in push mode IIRC (can be changed though) - id3demux can only parse one ID3v2 tag per stream at the moment (can ID3v2 tags be embedded as additional metadata in icecast streams?)
Also - just so we don't forget it later: src->icy_caps needs to be freed and set back to NULL in ::stop() and finalize() so the source can be re-used.
Ignore the second comment about the typefind patch, I forgot typefind->caps is being set in the vfunc triggered from the signal emission, so that's all fine.
Created attachment 62153 [details] [review] proper patch against gst-plugins-good, adding icydemux
Thanks for the review, Tim. I've never seen any evidence of any streaming servers allowing ID3 data in the stream (nor of any client using such data), so I think that bit's ok. If one turns up in the future, I'll add the neccesary bits. I'll fix up the things you mentioned, add tests, and submit a finished version of this patch later this week.
Created attachment 62459 [details] [review] remove icy handling to the extent possible from gnomevfssrc gnomevfssrc patch. Mostly deletes code - Yay! I consider this ready to go in, pending approval from someone else.
Created attachment 62460 [details] [review] Final typefind patch Fixed problem with probability argument noted by tim. Final patch unless I hear otherwise.
Looks good to me. I'd say the sooner we commit, the more testing we get before the release. In the typefind patch, there should now also be a _send_cached_events() call right before the gst_pad_push(), just added event caching the other day. Dunno where icydemux should go, I think -good, what do others think?
Current status (as I understand it): in order to avoid regressions in functionality, we need icydemux to get into a -good release before we can apply the changes to gnomevfssrc.
*** Bug 311343 has been marked as a duplicate of this bug. ***
*** Bug 338148 has been marked as a duplicate of this bug. ***
Hi all ! I would like to use this new icydemux on Windows. The problem is i don't have gnomevfs (i'm using neonhttpscr)and icydemux seems to need it to work as gnomevfs extracts metadata-interval for icydemux's pad sink caps. May be icydemux can have a generic sink caps like "ANY", and extract metadata-interval itself so it could work with all http sources (neon, fakesrc, gnomevfs, ...) what do you think ?
It isn't possible to extract metadata-interval - that's why it HAS to be provided by an upstream element. This is because it's a dumb-ass protocol (blame nullsoft!) neonhttpsrc should have the appropriate bits added to work with this; doing so shouldn't be too hard.
*** Bug 338980 has been marked as a duplicate of this bug. ***
Should bug #325847 be closed as a duplicate of this one, or are they mutually exclusive?
icydemux has been committed. Of course, it'll never be autoplugged right now; that needs the gnomevfssrc changes, which can't go in until AFTER a new release of -good has happened. Leaving the bug open to remind us of that. It'd be nice if someone made the appropriate changes now for neonhttpsrc, though (add iradio-mode property, send appropriate header, parse response headers, set icy headers when appropriate). Alex: bug #325847 is probably best treated as a duplicate.
*** Bug 325847 has been marked as a duplicate of this bug. ***
*** Bug 328689 has been marked as a duplicate of this bug. ***
Hi All, I've already changed neonhttpsrc. I've added iradio-mode property, send the header, parse reponse ... :) (it was really easier than in gnomevfs thanks to libneon) I've also added a user-agent property because neon wasn't using a user-agent before and some shoutcast streams were not sending response when the GET request didn't have a user-agent. Then i have tested icydemux, it works very well !! :) I need to make other investigations because i've seen neonhttpsrc doesn't support http redirection (code 302). I'll try commit it this week-end
Created attachment 64229 [details] [review] gstneon iradio-mode patch Hi all, Can someone test it on linux before committing ? It works fine for me on Windows ... Thanks, Sebastien
the neonhttpsrc patch seems to work fine for me on Linux. icydemux seems to ignore and throw away segment events and timestamps, which is not very nice.
Hrm... not sure what icydemux can do with timestamps. In practice, I wouldn't expect useful/meaningful timestamps on the input anyway. I think I can guess what the segment problem is: probably we need to cache a new-segment even on our sink pad, then when we create the src pad we can forward it.
Storing and forwarding the segment would be enough since you don't generate or modify timestamps. For the timestamp you could store the first valid timestamp you see (which will be 0 for all these httpsrc elements) and put that on the first buffer you output. Been thinking about adding simple support for timestamps to adapter as well which could help.
Improved event handling in icydemux committed. gnomevfs icy handling removed; committed now that we're unfrozen.
So, remaining things for this bug: someone who uses/knows neonhttpsrc should commit the patch there, and we should decide what to do about this chunk of the patch from bug #325847: It just automatically sets iradio-mode for on sources in playbin for http streams. Thoughts? Index: gst/playback/gstplaybasebin.c =================================================================== RCS file: /cvs/gstreamer/gst-plugins-base/gst/playback/gstplaybasebin.c,v retrieving revision 1.94 diff -u -p -u -p -r1.94 gstplaybasebin.c --- gst/playback/gstplaybasebin.c 15 Dec 2005 09:48:19 -0000 1.94 +++ gst/playback/gstplaybasebin.c 5 Jan 2006 10:57:42 -0000 @@ -1091,6 +1091,11 @@ gen_source_element (GstPlayBaseBin * pla !strncmp (play_base_bin->uri, "rtp://", 6) || !strncmp (play_base_bin->uri, "rtsp://", 7); + /* enable iradio mode for HTTP streams to get icecast metadata */ + if (!strncmp (play_base_bin->uri, "http://", 7) && + g_object_class_find_property (G_OBJECT_GET_CLASS (source), "iradio-mode")) + g_object_set (source, "iradio-mode", TRUE, NULL); + return source; }
i've commited the patch for neonhttpsrc.
*** Bug 340466 has been marked as a duplicate of this bug. ***
So I was eager to try it out and test with HEAD. However most of my iradio shoutcast stations work but do not show the metadata (in rhythmbox). Despite rhythmbox still plays the stream (i guess it moves to a different pipeline then), I fail with this: "gst-launch gnomevfssrc location=http://www.di.fm/mp3/house.pls ! icydemux ! fakesink -t" Result: ** (gst-launch-0.10:19394): CRITICAL **: gst_icydemux_chain: assertion `icydemux->meta_interval >= 0' failed ERROR: from element /pipeline0/gnomevfssrc0: Internal data flow error. --- Checking the HTTP return headers yielded a permanent "icy-metaint: 0" which is for sure the cause of the error. However since I know the stations supply metadata I did some testing. Appearently adding "Icy-MetaData:1" to the HTTP headers requesting the stream made the station contain correct "icy-metaint: 8192" data along with the "StreamTitle=" and "StreamURL=" pairs and thus I suppose would work with icydemux. "curl -s http://64.236.34.196:80/stream/1007 -H "Icy-MetaData:1" -I" I have no idea though how to implement it (not into the whole source structure) but maybe this info will help. gj anyways :)
Ups, well I checked the sources and this is already considered. This works as intended: "gst-launch gnomevfssrc iradio-mode=true location=http://64.236.34.196:80/stream/1007 ! icydemux ! fakesink -t" I "think" the problem here is the url forwarding. So the "icy-metadata:1" header info is added to the initial request, but not to the final resolved forwarded from the ".pls" playlist.
Wrong again. "playbin" was not setting "iradio-mode" for gnomevfssrc which therefore did not add the required additional header. Patch in #33 fixed it. So basicly ignore my last two comments. :E However this sets iradio-mode for anything starting with "http://" I guess. The ideal would be to only set it if the stream header would contain "icy" information.
The gstneonhttpsrc.c patch appears to be committed now, so changing patch status. I can verify comment #38. I applied the fragment of the patch in comment #33 to gst-plugins-base and restarted rhythmbox and streaming titles work again!
Created attachment 64870 [details] [review] gst-plugins-good [icydemux] - split StreamTitle to Artist, Title if possible
ok. Tim's committed the last stage of this. Closing. Martin: no, that infers semantics that don't exist.
*** Bug 345269 has been marked as a duplicate of this bug. ***