New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASX playlists are not correctly typefound #687

Closed
kingosticks opened this Issue Feb 10, 2014 · 34 comments

Comments

3 participants
@kingosticks
Member

kingosticks commented Feb 10, 2014

For some radio stations the Tunein API provides a URL to a playlist file which needs to be downloaded and parsed in order to extract the actual stream URIs (e.g. http://www.bbc.co.uk/radio/listen/live/r2.asx). The code to do this already exists in https://github.com/mopidy/mopidy/tree/develop/mopidy/audio for scanning local playlists but I wanted to reuse this for these remote playlists. I know the stream extension just provides Scanner.scan() with a playable stream URI and it's happy, but I when I try to give it a remote playlist URI (like the BBC one above) it can't handle it. Unfortunately I don't have the exact exception message to hand.

I don't understand what goes on inside gstreamer than doesn't differentiate between playing local and remote music files, but does have a problem with remote vs local playlist files. Why can't it just use the various playlist handlers that are registered?

I could fetch the playlist data myself, wrap it in a file like object and give that to Scanner.scan() - similar to what the tests do - or call the parse_asx|pls|m3u functions directly. Failing that I could just duplicate the playlist parsing code in the extension but that's horrible. What's the best thing to do here?

@jodal

This comment has been minimized.

Member

jodal commented Feb 14, 2014

@adamcik surely has opinions on this...

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 16, 2014

If memory serves I dropped adding asx support as there issues with the mime type for it and asf all being video/x-ms-asf. So for asx to work we would need to tag it with a custom mime and take it from there.

As for the rest of them, currently I believe that scanner should expand things correctly and inspect the first underlying track:

In [9]: scanner = scan.Scanner(timeout=5000, min_duration=None)

In [10]: scanner.scan('http://somafm.com/groovesalad.pls')
Out[10]: 
{u'duration': -1L,
 u'mtime': None,
 u'tags': {'audio-codec': [u'MPEG-1 layer 3'],
  'bitrate': [128000],
  'channel-mode': [u'stereo'],
  'emphasis': [u'none'],
  'genre': [u'Ambient Chill'],
  'has-crc': [False],
  'layer': [3],
  'location': [u'http://somafm.com'],
  'mode': [u'stereo'],
  'nominal-bitrate': [128000],
  'organization': [u'Groove Salad: a nicely chilled plate of ambient beats and grooves. [SomaFM]']},
 u'uri': 'http://somafm.com/groovesalad.pls'}
@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 16, 2014

Thanks for the reply @adamcik. I'm confused why I had no luck with using the scanner but since posting this I've come across some malformed playlists (mostly missing headers) so maybe this is what has been the problem. I will try this again.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 17, 2014

I've tried it again and it's now working just fine, except for the malformed and .asx playlists in which case I'm falling back to using my hacky type_findesque function.

What's the problem with registering the video/x-ms-asf mime type? Does receiving this make gstreamer try to output a video to a non-existent video pipeline, or something?

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 17, 2014

Huh, https://github.com/mopidy/mopidy/blob/develop/mopidy/audio/playlists.py#L128 turns out I did add it. In that case it's simply broken in some way that I don't have time to debug today. Running with a high enough GST_DEBUG level should be enough to figure it out.

As for the malformed playlists, my typefinder and parser is currently strict for at least m3u, and for pls I respect the number of entries entry. In other words we can/should probably be less strict and start adding test cases for malformed playlists as we come across them.

On a final side note, ideally we should really just be using totems playlist libraries which have been split out of totem for this exact reason, saving us from having the "badly" re-implement playlist parsing.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 17, 2014

Yes, you did, and with tests too. It works a treat.

I came across some at least one .m3u that had no header (so it was just a list of urls - how lazy can you get?!).

And thanks for the totem-pl-parser hint, it looks great!

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 17, 2014

p.s. sorry for originally saying this didn't work. I can only assume I managed to test with a bunch of malformed playlists.

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 17, 2014

Problem is actually that the ASX typefinder doesn't handle all caps elements. I.e. this needs to be fixed. Same error could also happen with the xspf finder.

@adamcik adamcik reopened this Feb 17, 2014

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 17, 2014

Though, with a quick fix it seems we hang on the mms connection instead. So warrants more investigation. Other thing that seems to come to mind is that only way to get metadata might have been to start playing and wait for the next time it is emitted, though I need to double check this.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 17, 2014

You've lost me on caps elements, is it that it should be 'video/x-ms-asx' and audio/x-ms-asx' ?

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 17, 2014

OK, so after having some fun in the interpreter I now find I can't use my code in the extension. When using from gi.repository import TotemPlParser I get:

ImportError: could not import gobject (error was: ImportError('When using gi.repository you must not import static modules like "gobject". Please change all occurrences of "import gobject" to "from gi.repository import GObject".',))

This is all very foreign to me. Is this because the rest of mopidy uses the old gobject stuff for gst and they are incompatible? Is there any solution to this or am I buggered?

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 17, 2014

Won't work until #225 is done.

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 17, 2014

And there is also the question about adding new deps, and an other issue with if this type of magic with custom elements+typefinders is even possible using gi. I've so far run into a lot of issues experimenting with custom python gstreamer elements and gi :( So currently I'm actually wondering if writing gstreamer elements in C that use totem-pl-parser is the way to go in the longer term...

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 18, 2014

Depending on higher quality code seems reasonable. But it's moot thanks to #225 anyway. Thanks for all your help.

p.s. type_finders as in http://lazka.github.io/pgi-docs/api/Gst_1.0/functions.html?highlight=type_find_register#Gst.type_find_register ?

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 18, 2014

We've become dependency adverse at some point, but yes it's probably preferable to depend on that lib once more things fall into place.

As for the file without a header case, depending on how it is formed it might be identified as a text/uri-list, in which case https://github.com/mopidy/mopidy/blob/develop/mopidy/audio/playlists.py#L293 would handle it.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 18, 2014

So I understand (and help), in terms of currently used custom elements, it's just these playlist ('decoder'?) ones, correct? The appsrc spotify playback stuff is part of the regular playbin2 element?

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 18, 2014

Mhm, I have a custom typefind elements to identify playlist data and set caps identifying it as such. Once these have been typefound the custom parser elements get plugged by the uridecodebin inside the playbin. The parsers convert the playlist to the text/uri-list format, which then gets auto plugged with the UriListElement. The UriListElement takes the first uri once we get an EOS, and creates its own internal uridecodebin, swallows the EOS and connects the new uridecodebin to the rest of the pipeline.

http://dev.1024.no/tmp/debug4.svg shows this in action, also see the comments in #460

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 18, 2014

As for the spotify bit, currently we set the URI to appsrc:// in the playbin and then have hooks to inject data into the source that gets setup. Ideally I would like this to be setting the URI to spotify:track:... and have a spotify element handle it, but this is impossible in GStreamer 1.x from python :(

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 18, 2014

Thanks a lot, that's clearer now. I think that diagram might be the best explanation of gstreamer I've ever seen.

And this ideal way is currently impossible in 1.x due the lack of urihandler interface (#152).

But otherwise, hypothetically and assuming the custom typefind and parser elements can be registered/used etc, 1.x would be ok to use?

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 18, 2014

Correct. As for the diagram, this is simply using GST_DEBUG_DUMP_DOT_DIR + a function call to trigger this dump for given element/pipeline. Extremely useful for debugging when deep in GStreamer (to the point that I have a /dump handler on a HTTP streaming gstreamer side project I'm playing with).

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 18, 2014

#701 has a new plan for how to improve the playlist handling. Though it won't fix the problem I'm having with the bbc asx as that is likely a libmms bug.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Feb 18, 2014

My test of BBC Radio 2 (only testing!!) worked fine when I experimented with using video/x-ms-asf instead of audio/x-ms-asf in the typefinder (and parser) code. I think the BBC change their mms stream urls all the time, it is possible your playlist is out of date?

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 18, 2014

Nope, pulled http://www.bbc.co.uk/radio/listen/live/r2.asx and did GST_DEBUG=mmssrc:5,4 gst-launch-1.0 -v -t playbin audio-sink=fakesink 'uri=mms://...' with one of the hrefs. On Ubuntu 13.04 with libmms=0.6.2-3 it fails, while on 13.10 with 0.6.2-3ubuntu1 it works.

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 20, 2014

Have a audio-playlist-improvement branch which fixes the handling of all caps tags and plays with some other things. Though I should probably just get that fix pushed for this, and leave the rest as a separate branch.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Apr 14, 2014

In terms of malformed asx playlists I keep coming across ones where the start and end tags use different capitalisations i.e. <ARTIST>some artist</artist>.

Another one used a <repeat> tag without a count parameter which should apparently mean infinite repeating. I'm not even sure if we support the presence of <repeat> tags at all, I must admit I didn't spend any time debugging this further.

@adamcik adamcik added this to the v0.19 - MPD playlist mgmt and other MPD improvements milestone Jun 11, 2014

@jodal jodal modified the milestones: v0.20, v0.19 - MPD playlist mgmt and other MPD improvements Jun 13, 2014

@jodal jodal closed this in #750 Jun 20, 2014

jodal added a commit that referenced this issue Jun 20, 2014

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jul 11, 2014

Do we handle the old ini format style ASX playlists? i.e.

[Reference]
Ref1=mms://streaming.radionz.co.nz/national-mbr?MSWMExt=.asf
Ref2=mms://103.14.3.137:80/national-mbr?MSWMExt=.asf
@kingosticks

This comment has been minimized.

Member

kingosticks commented Jul 11, 2014

And could we look at handling this https://bugzilla.gnome.org/show_bug.cgi?id=668575

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 11, 2014

Nope, problem that came up the other day was someone having a new one, point at the old ini style one and then we couldn't make it past that point.

I tried playing what Ref1 points at and that worked for me at least.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jul 11, 2014

But in the general case it should be possible to peek into the file, look for the header line and then select between ini and xml formats for asx content type?

I ran into new style pointing to old style with a mailing list user (maybe it's the same guy). What a terrible format!

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 11, 2014

Mhm, we just need to add another typefinder, and an element to handle the mime we claim it is. And yes same guy dropped by on IRC.

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jul 11, 2014

I didn't realise you could have two type finders trying to grab the same
mime type.

The bugzilla link was quite interesting I thought. Not looked how hard that
would be to implement.

I spent a fair while looking at it earlier but even though you've already
sorted it for him I did find a bug in tunein extension so wasn't a waste of
time. Plus it's motivated me to actually write some tests.
On 11 Jul 2014 19:34, "Thomas Adamcik" notifications@github.com wrote:

Mhm, we just need to add another typefinder, and an element to handle the
mime we claim it is. And yes same guy dropped by on IRC.


Reply to this email directly or view it on GitHub
#687 (comment).

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 11, 2014

Either we announce both under the same MIME and have the handler manage both, or we expose a second MIME. Either should work just fine.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 26, 2014

We never added support for the old style ASX types, reopening.

@adamcik adamcik reopened this Jul 26, 2014

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 26, 2014

Or, ehm, creating a new issue so this one can stay in 0.19....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment