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

Partial offline support #873

Closed
gscragg opened this issue Jun 13, 2017 · 10 comments
Closed

Partial offline support #873

gscragg opened this issue Jun 13, 2017 · 10 comments
Assignees
Labels
flag: seeking PR We are actively seeking PRs for this; we do not currently expect the core team will resolve this status: archived Archived and locked; will not be updated type: enhancement New feature or request
Milestone

Comments

@gscragg
Copy link
Contributor

gscragg commented Jun 13, 2017

I have had a look into offline support in shaka player 2.1.3 and noticed that it really only applies (currently) in ChromeOS.

I have had a tinker in storage.js and with a custom ManifestParser to get a partial offline scenario working where the content is downloaded but the license check to the remote Uri still occurs.

This would help out people who can pre download the content before traveling to a place with terrible/metered internet (hotels).

Would you accept a PR or discussion for this? I am in the middle of a POC for it, but the main chokepoints of change are:

  • storage.js:301 (where storage initialises the drmengine with a hardcoded isOffline=true parameter)
  • 'storage:425 (same as above)
  • 'drm_engine.js:232` (which is called from the above methods, does some checks against the media keys available)

I think what I want is to tell the storage api that I still want a temporary license (instead of it forcing a persistent-license (which has the flow on effect in drm_engine of checking the persistent-license session type).

Good idea / bad idea?

@joeyparrish
Copy link
Member

At a glance, it seems like a great idea! In particular because of the lack of availability of persistent license support and because of the incredibly low bandwidth requirement for licensing.

Let us know if you have any questions as you work on the PR. My gut instinct is to either add a parameter to store or to add an option to storage.configure.

Thanks for the suggestion, and for offering to contribute! We really appreciate it.

@joeyparrish joeyparrish added type: enhancement New feature or request flag: seeking PR We are actively seeking PRs for this; we do not currently expect the core team will resolve this labels Jun 13, 2017
@joeyparrish joeyparrish added this to the v2.3.0 milestone Jun 13, 2017
@gscragg
Copy link
Contributor Author

gscragg commented Jun 13, 2017

Cool!

My current work adds a persistLicense flag to storage.configure.

The default being true.

@gscragg
Copy link
Contributor Author

gscragg commented Jun 13, 2017

I changed my approach to an extra param for store as I figured it may be desirable to have the storage logic change per manifest. Although I am not sure how common it is to use a single storage instance to store/remove multiple manifests in the same session.

Here is what the changes currently look like, along with current naming scheme: https://github.com/google/shaka-player/compare/master...indiereign:offline-data-online-license?expand=1

I am about to look into updating and adding tests but would appreciate feedback on current approach as well.
Thanks.

@joeyparrish
Copy link
Member

I took a look and left some comments. In short, I think configuration is better in this case. I can't see any reason you would need to change that flag per-asset, and if you need to, you still can through configure().

Thanks again for working on this!

@gscragg
Copy link
Contributor Author

gscragg commented Jun 13, 2017

Yep, got a barrage of emails :).
If its ok with you, I will submit a PR with the changes so I can keep all feedback in one place.

@joeyparrish
Copy link
Member

Okay, sounds good. We can continue discussion on the PR.

@joeyparrish
Copy link
Member

Reopening. Just noticed that we didn't update the configuration structure definition to include the new flag. I'll fix it shortly.

@joeyparrish
Copy link
Member

I am also renaming the option from isPersistentLicense to usePersistentLicense, which makes more sense to me as a directive for the library's behavior.

@gscragg
Copy link
Contributor Author

gscragg commented Jun 21, 2017

OK, are you doing that, or do you want me to submit another PR?

@joeyparrish
Copy link
Member

No, I'm taking care of it real quick. Sorry about that. I should have noticed these things before merging. :-)

@joeyparrish joeyparrish modified the milestones: v2.3.0, v2.2.0 Jun 22, 2017
@shaka-project shaka-project locked and limited conversation to collaborators Mar 22, 2018
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
flag: seeking PR We are actively seeking PRs for this; we do not currently expect the core team will resolve this status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants