Skip to content
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

Support ICY metadata #3735

Closed
saschpe opened this issue Jan 21, 2018 · 15 comments
Closed

Support ICY metadata #3735

saschpe opened this issue Jan 21, 2018 · 15 comments
Assignees

Comments

@saschpe
Copy link
Contributor

saschpe commented Jan 21, 2018

Hi,

there are plenty of (closed) issues asking for Shoutcast / ICY support. Since I had the same challenge I came up with an extension. It's in use already but I'd like to know if it makese sense to be mentioned in the extension directory or if there's interest in merging it into ExoPlayer's code-base. I guess it needs more documentation and tests but I'd be curious.

@ojw28
Copy link
Contributor

ojw28 commented Jan 22, 2018

My understanding is that on some versions of Android DefaultHttpDataSource wont work at all, failing with:

java.net.ProtocolException: Unexpected status line: ICY 200 OK

(here are some relevant issues). This is unfortunately working as intended at the platform level, which is where the failure comes from, because ICY deviates from HTTP protocol. Does your solution do anything to address this issue, or does it just add metadata support where DefaultHttpDataSource works?

I think it's a great idea to provide support, but if we're to do so then I think we need a solution that works correctly across all devices. You might want to do this by implementing a solution that extends OkHttpDataSource instead, since that'll give you consistent behavior across all Android version, meaning that if it works, it'll work everywhere. It also means you'll be using a network stack that's just "better", according to most people.

@ojw28 ojw28 added the question label Jan 22, 2018
@ojw28 ojw28 self-assigned this Jan 22, 2018
@saschpe
Copy link
Contributor Author

saschpe commented Jan 22, 2018

Well, OkHttp is quite popular in the Android community but it's also already part ot Android itself, hidden below HttpConnection (AFAIK). On the other hand, it adds a lot of additional size to any APK.
I'm sure things can be implemented on top of OkHttp but I'd rather expect someone with the ICY 200 Ok issue to submit a patch. I don't have a strong opinion though but I just didn't see the need for OkHttp.

This extension is more focussed on really parsing and returning the ICY metadata. At least there is now a place where people could contribute that previously had to be told it's beyond scope for exoplayer.

@ojw28
Copy link
Contributor

ojw28 commented Jan 22, 2018

As far as the ICY protocol is concerned, I think the ICY 200 OK response is correct. The network stack underneath HttpURLConnection doesn't handle the response on certain versions of Android (if I remember correctly, I think KitKat is affected). This is also working as intended, because the HTTP stack isn't designed to handle the ICY protocol.

So I'm somewhat unclear on where you would expect someone with the ICY 200 issue to submit a patch? If I've understood correctly, the extension you've provided will basically not work for ICY streams on some versions of Android, and I'm not aware of anything you can do at the application level to fix it. Except use a bundled network stack like OkHttp directly, so as to remove the dependency on the network stack under HttpURLConnection.

Please clarify. Thanks!

@saschpe
Copy link
Contributor Author

saschpe commented Jan 22, 2018

What I meant is that the underlying implementation could still be changed given there are some more tests in place. It's nothing I'd do immediately though.

@ojw28
Copy link
Contributor

ojw28 commented Jan 22, 2018

The underlying implementation of what? Are you suggesting the solution is to patch KitKat devices at the platform level? That's not feasible in practice; many such devices are no longer receiving regular updates from the device OEMs and/or carriers.

@saschpe
Copy link
Contributor Author

saschpe commented Jan 24, 2018

All I can say is that the implementation works for me on API level 19. Different Shoutcast server implementations do different things, the servers I care about work. My point was that if someone needs a library for shoutcast metadata but doesn't get it to work with the server he uses, I'd be more than happy to take patches to port to OkHttp. At the moment I just lack a reason to do it myself.

More importantly, the idea of this issue is to have some list of third-party extensions that might be useful to Exoplayer users.

@ojw28
Copy link
Contributor

ojw28 commented Mar 28, 2018

I think we're interested in merging this in some form, possibly directly into the existing OkHttp extension as we'd like it to be based on OkHttpDataSource rather than DefaultHttpDataSource.

It's fine if you don't want to spend time making changes yourself, but in order to look at merging we'd need you to contribute the extension in its current form by opening a pull request to our dev-v2 branch, and signing the corresponding CLA as described here. Please do that if you're able to do so, and we'll take a more detailed look at the best way to merge it. Thanks!

@saschpe
Copy link
Contributor Author

saschpe commented Apr 22, 2018

Sorry for the delay. I'll look into it.

@chriscoderdr
Copy link

Any update into this?

@saschpe
Copy link
Contributor Author

saschpe commented Sep 13, 2018

Not yet, but I didn't forget about it

saschpe added a commit to saschpe/android-exoplayer2-ext-icy that referenced this issue Sep 25, 2018
As requested in ExoPlayer issue 3735[0].

[0] google/ExoPlayer#3735
saschpe added a commit to saschpe/android-exoplayer2-ext-icy that referenced this issue Sep 25, 2018
As requested in ExoPlayer issue 3735[0].

[0] google/ExoPlayer#3735
saschpe added a commit to saschpe/android-exoplayer2-ext-icy that referenced this issue Sep 26, 2018
As requested in ExoPlayer issue 3735 [0]. Changes exoplayer dependencies
to 'api' to ensure library users automatically get it.

[0] google/ExoPlayer#3735
@saschpe
Copy link
Contributor Author

saschpe commented Sep 26, 2018

Porting to OkHttp is done, CLA is signed. I'll start a pull-request against dev-v2...

@ojw28
Copy link
Contributor

ojw28 commented Sep 26, 2018

It might be worth revisiting how this works, given we now have DataSource.getResponseHeaders. It may well be possible to do it in a much more consistent way (e.g. similar to how ID3 metadata works) than was previously the case. And also directly in the core library, rather than as an extension.

Are streams in this use case always in a particular format (e.g. mp3?), or can they be in multiple formats (e.g. aac)? Given that information, I can probably suggest an approach for evaluation.

saschpe added a commit to saschpe/ExoPlayer that referenced this issue Oct 22, 2018
Based on the OkHttp extension.

Resolves google#3735
@saschpe
Copy link
Contributor Author

saschpe commented Oct 26, 2018

@ojw28 I'm fine simplifying the code and therefore prepared a pull as you proposed. I don't think the stream format is really fixed but ICY isn't exactly a standard. It's more established practice with a couple well-known headers and a couple more obscure ones ;-)

@ojw28
Copy link
Contributor

ojw28 commented Dec 14, 2018

I've been taking a look at this! I've managed to take the change in the pull request and do most of the simplification work mentioned on top. Hopefully we'll be able to get it merged in sometime in the next week or two.

@ojw28
Copy link
Contributor

ojw28 commented Dec 18, 2018

We've merged a heavily modified version of the pull request into dev-v2. Thanks for the contribution! In the modified version, metadata is enabled by default and does not require an extension. You may need to use OkHttpDataSource to avoid the Unexpected status line: ICY 200 OK issue on some versions of Android, but we do not require that you do so. The metadata is reported in the same way we report ID3 metadata:

  • ICY headers are reported as metadata in the track format.
  • ICY in-stream metadata is reported in a metadata track (use player.addMetadataOutput to register an output to receive it).

In the demo app you'll see this metadata arriving in logcat. Header metadata arrives when the tracks become known at the start of playback:

EventLogger: tracksChanged [0.85, 0.00, window=0, period=0, 
EventLogger:   Renderer:1 [
EventLogger:     Group:0, adaptive_supported=N/A [
EventLogger:       [X] Track:0, id=null, mimeType=audio/mpeg, bitrate=128000, channels=2, sample_rate=44100, supported=YES
EventLogger:     ]
EventLogger:     Metadata [
EventLogger:       IcyHeaders: name="SomaFM presents: Indie Pop Rocks! [SomaFM]", genre="Indie LoFi College", bitrate=128000, metadataInterval=45000
EventLogger:     ]
EventLogger:   ]
EventLogger:   Renderer:3 [
EventLogger:     Group:0, adaptive_supported=N/A [
EventLogger:       [X] Track:0, id=icy, mimeType=application/x-icy, supported=YES
EventLogger:     ]
EventLogger:     Metadata [
EventLogger:       IcyHeaders: name="SomaFM presents: Indie Pop Rocks! [SomaFM]", genre="Indie LoFi College", bitrate=128000, metadataInterval=45000
EventLogger:     ]
EventLogger:   ]
EventLogger: ]

Note that the ICY metadata track may or may not be present, depending on whether icy-metaint is declared in the response headers. The ICY header metadata will be attached to the audio track, and also to the ICY metadata track if present (as is the case above).

If the ICY metadata track is present, in-stream metadata arrives at its corresponding time:

EventLogger: metadata [1.13, 0.05, window=0, period=0, 
EventLogger:   ICY: title="Deerhunter - Death In Midsummer", url="null"
EventLogger: ]

The modified version parses known fields only (StreamTitle and StreamUrl for now), rather than arbitrary key/value pairs. We can consider adding additional fields or reverting back to arbitrary key/value pairs if we need to, but it's unclear whether there really are lots of custom implementations out there all using different keys (or what that would be useful for, given players wouldn't know how to present the data to the user).

Please give it a try and let us know how you get on.

@ojw28 ojw28 changed the title Add third-party Shoutcast extension to the extension directory Support ICY metadata Dec 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants