Fixed adding stream to playlist

[andersbll]

Hi there, great work porting Sonata to Python 3. It seems to be pretty solid by now!

I recently noticed that I was unable to add music streams to the playlist. The problem is caused by urllib returning bytes instead of a string. Here is a quick fix that seems to work for links to binary streams as well as playlist files.

[multani]

Hi Anders, thanks for the fix!

Actually, there's already a pull request opened to fix this, have a look at #34. It seems you went for the same kind of solution, and I already raised some concerns there, can you have a look? I believe @Mic92 is not working on this at the moment, if you can get something to work, that would be awesome!

[Mic92]

I began to write a playlist parser, but have never actually finish it. Maybe it helps a little. You might want write unittests for the various playlist formats... https://gist.github.com/Mic92/5902534 Good luck!

[andersbll]

Oops! :)

The fix proposed in PR #34 does not work as intended because urllib.request always returns bytes. Therefore type(f) is bytes will always return true.

@multani: regarding your concerns: I'm a bit skeptical about the MIME type detection approach. Even with MIME type detection we would not be sure to get it right because the servers might return incorrect types. E.g. one of my URL points to a .pls playlist which is detected as application/octet-stream. I have an alternative proposal: Try decoding the response from urllib.response as a string. If the decoding fails, the data is binary and the original URL should be added to the playlist. If the decoding succeeds, search for occurrences of 'http://' and 'ftp://'. If these strings occur, parse the data as a playlist as we currently do.

@Mic92: Thanks for gist! When I first thought I had fixed the problem I did not imagine it being so complex! In fact I was pretty delighted by the b'\0' search hack! :)

[multani]

Yes, this can be a bit tricky.

• urllib.request does indeed always return bytes; your`b'\0'`` is a nice hack, although there can be cases where it fails because a text file (a playlist) will contain this character. Maybe it is good enough anyway?
• can you elaborate how and in which case you get and application/octet-stream? Are you using @Mic92's code to detect this or something else? Where is this MIME type coming from?
• as for decoding the input stream, for it to work, you will need to have a look at the HTTP header ("Content-Type"?) sent by the server (if it's an HTTP request) to know which encoding to decode the data from.

@Mic92: how do you think your approache in your Gist is going to be?

(On a side note, the current code is currently broken with playlists longer than 4000 bytes I believe... -_- )

On Mon, Jul 01, 2013 at 11:09:34AM -0700, Anders Boesen Lindbo Larsen wrote:

Oops! :)

The fix proposed in PR #34 does not work as intended because urllib.request always returns bytes. Therefore type(f) is bytes will always return true.

@multani: regarding your concerns: I'm a bit skeptical about the MIME type detection approach. Even with MIME type detection we would not be sure to get it right because the servers might return incorrect types. E.g. one of my URL points to a .pls playlist which is detected as application/octet-stream. I have an alternative proposal: Try decoding the response from urllib.response as a string. If the decoding fails, the data is binary and the original URL should be added to the playlist. If the decoding succeeds, search for occurrences of 'http://' and 'ftp://'. If these strings occur, parse the data as a playlist as we currently do.

@Mic92: Thanks for gist! When I first thought I had fixed the problem I did not imagine it being so complex! In fact I was pretty delighted by the b'\0' search hack! :)

---

Reply to this email directly or view it on GitHub:
#41 (comment)

[andersbll]

I think the current solution is good enough; it is short and has worked fine for me so far. However, if this is not the case for everyone it should definitely be fixed!

I have looked into the MIME type detection problem I mentioned earlier: The problem is caused by the HTTP server that reports a wrong MIME type in the header. If I use mimetypes.guess_type() it works fine. This does not mean that mimetypes.guess_type() is flawless as it fails for URLs without file extensions. In these cases we would have to fall back to the current solution.

[multani]

Hum, fair enough.

Let's do something about this fix then. Would you considerate doing something slightly different, as I udnerstood you mentionned above: I think we can probably get rid of misc.is_binary() and try as you said to decode the value received. Then, if it succeed, try to guess the content as a playlist, and if it failed, supposed it is a binary file and add it to MPD. Basically, the code would remove the condition on misc.is_binary() and your decode, and replace this by a try/except around f.decode() on UnicodeDecodeError, with the current "binary" code path in the except block, and the alternative "playlist" code path in the else block. Do you think that would be OK?

Also, I need some information on how to reproduce this issue in the first place. Can you describe me which steps you are doing to face this bug? I tried several things and I'm hitting other bugs in the meanwhile -_- So I can test your fix after.

Thanks!

[andersbll]

Ok, I will revise the PR over the weekend!

Regarding the bug: I add a new stream in Sonata and enter "http://kcrw.ic.llnwd.net/stream/kcrw_music" as Stream URL. mimetypes.guess_type returns None for this URL.

[Mic92]

Maybe the solution the original authors chosen, wasn't that bad. This mimetype things just seems to breaks to often on the internet. The only thing I have to note, that you shouldn't add all songs, you find in a radio playlist. Radio stations often put multiple formats of the same stream into 1 playlist. Instead you should add the first playable stream to mpd. This behaviour different from what a user expect, if he/she add local playlist.

[multani]

Hi Anders,

any progress on this? If not and you think it's acceptable, I'll probably just implement what I said in #41 (comment) and propose it as a fix.

[multani]

@andersbll @Mic92 I implemented in 60cd4c3 a fix, inspired by what as been done by @andersbll in this pull request, and what we discussed here. I tested it with a few use cases, and it seems to work.
Can you have a look at the branch fix/41-add-stream to see if it works OK?

(On a side note, I also did after some cleanups of this portion of the code, hopefully it's a bit better now. See branch refactor/stream-parsing)