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

Add an extension that provide DataSource using OkHttp #735

Merged
merged 1 commit into from
Sep 29, 2015

Conversation

b95505017
Copy link
Contributor

@b95505017
Copy link
Contributor Author

Not sure where should I write a demo for it, cause I need to rewrite all render builder in demo app.
Or maybe we should make the data source is configurable in demo app?

@ojw28
Copy link
Contributor

ojw28 commented Aug 23, 2015

I don't think it needs a demo app. It would be trivial for someone to swap it into the normal demo app in a couple of minutes, if they want to experiment with it, so I don't think it needs a demo of its own.

@talklittle
Copy link
Contributor

👍

Tried this, swapping into the ExtractorRendererBuilder in r1.4.2 demo, works great with ExtractorSampleSource and mp4 files.

I love that it can integrate with the OkHttp's Cache to avoid redownloading the mp4 file when looping. So for me, this serves as a pretty good solution to #420

Thank you @b95505017 !

@b95505017
Copy link
Contributor Author

@talklittle You could also integrate Stetho via OkHttp's Interceptor mechanism, then all request could monitor via chrome dev tool.
@ojw28 any review progress update?

@ojw28
Copy link
Contributor

ojw28 commented Sep 25, 2015

We're finalizing a 1.5.x release at the moment. We'll be merging it after that's done. Sorry for the long delay; perhaps we need more branches :). Thanks!

@ojw28
Copy link
Contributor

ojw28 commented Sep 29, 2015

1.5.0 is out, so we're pretty much ready to merge this. Our plan is to merge it as-is, since it looks in pretty good shape already and works well. Just FYI, we then plan to make some follow-up modifications, including:

  • Fixing code style to be consistent with the rest of the project.
  • Force application to inject OkHttpClient instance through all constructors, and remove allowCrossProtocolRedirect/connectTimeout/readTimeoutMillis parameters. Right now if you inject a client, the OkHttpDataSource will change the default properties of that client, which is confusing given it might be used elsewhere. It's probably simplest just to require the application to instantiate, configure and inject a client as appropriate.

ojw28 added a commit that referenced this pull request Sep 29, 2015
Add an extension that provide DataSource using OkHttp
@ojw28 ojw28 merged commit 2112bc2 into google:dev Sep 29, 2015
@ojw28
Copy link
Contributor

ojw28 commented Sep 29, 2015

This is now merged, with the modifications described above applied. Please take a look and check that it still needs the original needs. Thanks!

@b95505017
Copy link
Contributor Author

LGTM 👍 :)

@TommyVisic
Copy link

@b95505017, thanks for this OkHttp version. @talklittle can you share how you set this up to get caching working with mp4s? Are you working with partial responses at all?

@talklittle
Copy link
Contributor

@TommyVisic Nothing fancy at all, just setting a Cache on the OkHttpClient.

EDIT: Depending on server configuration, specifically if the server supports HTTP 206 partial responses, then OkHttp cache won't handle it. The solution I came up with was:

Another way is by forcing OkHttp to request the full file each time using a network interceptor and removing the Range header that ExoPlayer uses. Then you'll get 200 responses instead of 206's, which OkHttp's Cache will handle.

Or see comment by @econnelly for a couple other potential solutions.

@b95505017
Copy link
Contributor Author

@TommyVisic take a look with OkHttp recipes

@kwelsh
Copy link

kwelsh commented Oct 29, 2015

The ReadMe for the OkHttp Extension doesn't have any build instructions. I'm assuming this is similar to how other extensions are built? Anyone with more experience care to comment on the process of including it in a project? Any info would greatly be appreciated!

Would be nice to have some way of just adding a single compile line to your Gradle file add these extensions...

Oh, awesome extension by the way! Really helps considering that caching still seems to be quite a bit of work to set up.

@b95505017
Copy link
Contributor Author

Yes just using compile ':extension-okhttp' in your submodule or some another path if you have another settings.gradle.

Just like the usage of DefaultHttpDataSource.java, if you really need an example then I could provide it. :)

@brianchu
Copy link

Adding a line to the settings.gradle will get it compile into the build, but we still need to program the app to use okhttp instead default one. There is no magic (like reflection) to pick up the okhttp.

@b95505017
Copy link
Contributor Author

OK I'll try to write an example AOAP.

@econnelly
Copy link

@talklittle OkHttp doesn't cache partial requests. The OkHttp cach may work on a fast connection where the entire file can be downloaded in one shot, but it's not reliable. I just went through the exact scenario where a looping mp4 was redownloading every time it started over.

There are two ways to prevent this.

  1. Download the file somewhere else using OkHttp directly and use FileDataSource for playback
  2. Modify OkHttpDataSource and CacheDataSource to produce and accept the final file size before starting playback

@talklittle
Copy link
Contributor

@econnelly Hey thanks for pointing that out.

Another way is by forcing OkHttp to request the full file each time using a network interceptor and removing the Range header that ExoPlayer uses. Then you'll get 200 responses instead of 206's, which OkHttp's Cache will handle.

I'll update my other comment for posterity.

@kwelsh
Copy link

kwelsh commented Oct 30, 2015

Thanks, @talklittle, @econnelly, @b95505017, and @brianchu for the extra info. It's been very insightful.

I feel like I'm getting tripped up in the set up here, as my IDE (Android Studio) doesn't seem to auto-complete OkHttpDataSource. I added compile 'com.google.android.exoplayer:exoplayer:extension-okhttp' in my dependencies and received no errors/warnings, but this doesn't seem to work.

Does someone mind sharing their Gradle?

@talklittle
Copy link
Contributor

@kwelsh Copy the file into your project. It's not in the Maven artifact on JCenter.

You can verify this in Android Studio in the Project view: External Libraries -> exoplayer-r1.5.2 -> classes.jar. The extensions are not included. The Maven artifact corresponds to the library module of this repo only.

@b95505017
Copy link
Contributor Author

Here is my example which replace demo data source with OkHttp, default is no cache.
https://github.com/b95505017/ExoPlayer/tree/okhttp_http_data_source
Also I connect with Facebook Stetho, so you could monitor HTTP behavior in Chrome Dev Tool via chrome://inspect

@kwelsh
Copy link

kwelsh commented Nov 2, 2015

@talklittle Ah I see. Thanks for clarifying this.

@b95505017 That is awesome! It seems a lot simpler than I envisioned in my head. I'm gonna give this another go.

@kwelsh
Copy link

kwelsh commented Nov 2, 2015

I got this thing up and running. Just need to configure it to cache now. Thanks again, everyone.

@laurencedawson
Copy link

@kwelsh just a heads up, partial requests are not cached.

If you modify the method makeRequest in OkHttpDataSource you can get caching to work correctly.

@welshk91
Copy link

welshk91 commented Jan 4, 2016

@laurencedawson Thanks for the heads up.

I'm still struggling with getting caching to work on the demo for ExoPlayer. I've added in OkHttp to handle the networking requests, but it still seems to re-download the video when you rotate the screen or power off (anything that recreates the fragment again).

I'd be extremely interested in your solution, if just to see a different perspective on the matter! :)

@b95505017
Copy link
Contributor Author

You could inject your own CacheControl (or just give it null) into OkHttpDataSource, or using Interceptor skill to monitor/override each request & response.

@b95505017
Copy link
Contributor Author

I'v upgrade the extension to supporting okhttp3 #1119
One thing should notice that OkHttp now using its own CookieJar to handle cookie stuff, checkout JavaNetCookieJar to pass original java.net.CookieHandler into it.

@laminsk
Copy link

laminsk commented Jan 29, 2016

Thank you b95505017 and everyone for valuable inputs. I've been trying to download audio stream from server to disk using okhttpdatasource and still no luck so far. Has anyone manage to do this yet? Any help would be greatly appreciated. Thanks in advance.

@b95505017
Copy link
Contributor Author

What do you mean about "no luck"?

@mariuspena
Copy link

Hi

I want to cache an mp4.
I also wrote about it here:
#420 (comment)
@laurencedawson can you tell me what am I missing there?

it seems that in OkHttpDataSource on makeRequest the DataSpec has an unbounded length and this makes it to download the file every time because it cannot cache it.
How can I cache it ? Is there a way?

@mariuspena
Copy link

@talklittle so you made cache work for mp4s ? can you please elaborate?
I am trying to achieve that and nothing seems to work. see #420 (comment)

@google google locked and limited conversation to collaborators Jun 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.