After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 333657 - Replacing icy demuxing in gnomevfssrc
Replacing icy demuxing in gnomevfssrc
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal enhancement
: 0.10.7
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 311343 325847 328689 338148 338980 340466 345269 (view as bug list)
Depends on: 341432
Blocks:
 
 
Reported: 2006-03-06 20:27 UTC by Michael Smith
Modified: 2006-06-18 23:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Icydemux plugin. (17.36 KB, text/plain)
2006-03-06 20:29 UTC, Michael Smith
  Details
Icydemux header (2.38 KB, text/plain)
2006-03-06 20:30 UTC, Michael Smith
  Details
Hack to gnomevfssrc (2.09 KB, patch)
2006-03-06 20:32 UTC, Michael Smith
none Details | Review
typefind patch. Kinda hacky. (1.67 KB, patch)
2006-03-06 20:34 UTC, Michael Smith
none Details | Review
Updated typefind patch (1.29 KB, patch)
2006-03-24 15:29 UTC, Michael Smith
none Details | Review
proper patch against gst-plugins-good, adding icydemux (21.71 KB, patch)
2006-03-27 19:54 UTC, Tim-Philipp Müller
committed Details | Review
remove icy handling to the extent possible from gnomevfssrc (15.68 KB, patch)
2006-03-31 14:42 UTC, Michael Smith
committed Details | Review
Final typefind patch (1.02 KB, patch)
2006-03-31 14:46 UTC, Michael Smith
committed Details | Review
gstneon iradio-mode patch (8.66 KB, patch)
2006-04-24 19:40 UTC, Sebastien Moutte
committed Details | Review
gst-plugins-good [icydemux] - split StreamTitle to Artist, Title if possible (1.11 KB, patch)
2006-05-05 16:01 UTC, Martin Szulecki
none Details | Review

Description Michael Smith 2006-03-06 20:27:31 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.
Comment 1 Michael Smith 2006-03-06 20:29:46 UTC
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.
Comment 2 Michael Smith 2006-03-06 20:30:05 UTC
Created attachment 60793 [details]
Icydemux header

And the header file.
Comment 3 Michael Smith 2006-03-06 20:32:39 UTC
Created attachment 60794 [details] [review]
Hack to gnomevfssrc
Comment 4 Michael Smith 2006-03-06 20:34:11 UTC
Created attachment 60795 [details] [review]
typefind patch. Kinda hacky.
Comment 5 Jonathan Matthew 2006-03-22 21:46:51 UTC
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.
Comment 6 Michael Smith 2006-03-24 12:06:41 UTC
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? 
Comment 7 Jonathan Matthew 2006-03-24 13:46:59 UTC
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.
Comment 8 Michael Smith 2006-03-24 15:29:46 UTC
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.
Comment 9 Tim-Philipp Müller 2006-03-27 18:03:53 UTC
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?)
Comment 10 Tim-Philipp Müller 2006-03-27 18:14:43 UTC
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.
Comment 11 Tim-Philipp Müller 2006-03-27 19:16:48 UTC
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.
Comment 12 Tim-Philipp Müller 2006-03-27 19:54:56 UTC
Created attachment 62153 [details] [review]
proper patch against gst-plugins-good, adding icydemux
Comment 13 Michael Smith 2006-03-28 08:58:56 UTC
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.
Comment 14 Michael Smith 2006-03-31 14:42:50 UTC
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.
Comment 15 Michael Smith 2006-03-31 14:46:07 UTC
Created attachment 62460 [details] [review]
Final typefind patch

Fixed problem with probability argument noted by tim. Final patch unless I hear otherwise.
Comment 16 Tim-Philipp Müller 2006-03-31 15:11:54 UTC
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?
Comment 17 Tim-Philipp Müller 2006-04-06 09:04:57 UTC
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.
Comment 18 Jonathan Matthew 2006-04-06 11:35:01 UTC
*** Bug 311343 has been marked as a duplicate of this bug. ***
Comment 19 Alex Lancaster 2006-04-12 01:25:33 UTC
*** Bug 338148 has been marked as a duplicate of this bug. ***
Comment 20 Sebastien Moutte 2006-04-17 13:55:50 UTC
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 ?
Comment 21 Michael Smith 2006-04-17 14:01:06 UTC
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.
Comment 22 Alex Lancaster 2006-04-19 09:10:35 UTC
*** Bug 338980 has been marked as a duplicate of this bug. ***
Comment 23 Alex Lancaster 2006-04-19 09:12:27 UTC
Should bug #325847 be closed as a duplicate of this one, or are they mutually exclusive?
Comment 24 Michael Smith 2006-04-21 09:31:57 UTC
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.
Comment 25 Michael Smith 2006-04-21 09:34:54 UTC
*** Bug 325847 has been marked as a duplicate of this bug. ***
Comment 26 Alex Lancaster 2006-04-21 13:14:41 UTC
*** Bug 328689 has been marked as a duplicate of this bug. ***
Comment 27 Sebastien Moutte 2006-04-21 14:39:08 UTC
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
Comment 28 Sebastien Moutte 2006-04-24 19:40:38 UTC
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
Comment 29 Wim Taymans 2006-04-27 08:56:49 UTC
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.
Comment 30 Michael Smith 2006-04-27 09:17:37 UTC
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.
Comment 31 Wim Taymans 2006-04-27 09:36:45 UTC
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.
Comment 32 Michael Smith 2006-04-28 15:02:37 UTC
Improved event handling in icydemux committed.

gnomevfs icy handling removed; committed now that we're unfrozen.
Comment 33 Michael Smith 2006-04-28 15:09:25 UTC
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;
 }
 
Comment 34 Sebastien Moutte 2006-04-29 15:50:35 UTC
i've commited the patch for neonhttpsrc.
Comment 35 Jonathan Matthew 2006-05-02 22:16:00 UTC
*** Bug 340466 has been marked as a duplicate of this bug. ***
Comment 36 Martin Szulecki 2006-05-05 11:32:57 UTC
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 :)
Comment 37 Martin Szulecki 2006-05-05 12:00:08 UTC
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.
Comment 38 Martin Szulecki 2006-05-05 13:01:08 UTC
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.
Comment 39 Alex Lancaster 2006-05-05 13:46:18 UTC
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!
Comment 40 Martin Szulecki 2006-05-05 16:01:15 UTC
Created attachment 64870 [details] [review]
gst-plugins-good [icydemux] - split StreamTitle to Artist, Title if possible
Comment 41 Michael Smith 2006-05-11 16:11:24 UTC
ok. Tim's committed the last stage of this. Closing.

Martin: no, that infers semantics that don't exist.
Comment 42 Jonathan Matthew 2006-06-18 23:55:30 UTC
*** Bug 345269 has been marked as a duplicate of this bug. ***