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

Standalone offline Storage (no Player, no video) #1297

Closed
OrenMe opened this issue Feb 12, 2018 · 11 comments
Closed

Standalone offline Storage (no Player, no video) #1297

OrenMe opened this issue Feb 12, 2018 · 11 comments
Assignees
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Milestone

Comments

@OrenMe
Copy link
Contributor

OrenMe commented Feb 12, 2018

I see that the only thing required by the offline class from the player instance is getConfiguration and getNetworkingEngine.

ThegetNetworkingEngine in the player class gets a callback for on segment download
https://github.com/google/shaka-player/blob/35d8838ed38ea798cb3b63065d1ce05e364b3bab/lib/player.js#L805-L807

But it is not used in offline mode as there's no ABR(right? I don't see anyone calling player.load which initialises the abrManager_ member):
https://github.com/google/shaka-player/blob/35d8838ed38ea798cb3b63065d1ce05e364b3bab/lib/player.js#L2422-L2427

So it actually can be exposed and used directly, no?

I'm asking this as I'm trying to understand how can I relate to the following use case:
Usually for offline an app will showcase a gallery window where potentially no player is initialised in it yet.
The user will see a mosaic of videos and might want to download from this screen.
If I follow the Shaka offline guide I see that a player instance needs to be created, and I would like to avoid this.
To add to this - an app might want to create the logic of a queue manager and download simultaneously a few entries and for this it would have to initialise a couple of players, each one with it's own video tag.
So my question - is there any necessity for the player instance to be passed to the offline class?

Maybe I'm oversimplifying things and there are other considerations you took into account.
Maybe creating the offline as a separate component will yield a smaller lib that takes care of download, that as I mentioned above, can happen without any player being loaded, then this can help an app load less code for as it would only have to load the offline download library for just downloading.

The offline requires mainly classes from the offline directory and utils.
The two main classes it requires from the player is the ManifestParser and DrmEngine, both requiring the NetworkingEngine, which is also required by the offline(as mentioned in the start here above).
In addition there's the configuration management but this can also be separated to its own class and then used by the offline class directly.
I think this can lead to major decrease in size of the offline and create an offline manager which can be used in tandem with the player but serve the purpose of actual offline download use case.

WDYT?

Thanks,

Oren M.

@OrenMe OrenMe changed the title question: why is a player instance required for offline? suggestion: create standalone offline download manager Feb 12, 2018
@theodab
Copy link
Collaborator

theodab commented Feb 16, 2018

It's possible.

If I recall, we have previously gotten requests to break Shaka Player into multiple modules that can be optionally loaded depending on a user's needs. This could be part of a larger effort like that.

On a smaller scale, it might be easier to make it so that you can make a Storage instance without a player. In that case, we disable methods that cannot be done without a player like loadInternal. And then, perhaps, you could associate the offline with a Player so that it can play, and unlink it later? You would still have to download the entirety of Shaka Player ahead of time, but it would be fairly simple to implement.

@theodab theodab added the type: enhancement New feature or request label Feb 16, 2018
@OrenMe
Copy link
Contributor Author

OrenMe commented Feb 18, 2018

Starting with not having to associate a player with storage would be good.
Why wouldn’t you be able to call loadInternal? Because of the getMetworkingEngine call? Cause that’s exactly what I was wondering about in the original question - is it mandatory to get networking engine from the player instance? I don’t see any reason to not use the network engine directly. Maybe it is just not exposed as public in the lib?

@theodab
Copy link
Collaborator

theodab commented Feb 20, 2018

I misread the function. I was thinking it was part of the loading process, not the storing process. No, you're right, it'd be possible to call loadInternal without a player, we could totally use a local networking engine. The only link the NetworkingEngine has to the Player is the onSegmentDownloaded_ callback, which we use for ABR. There's no need for ABR when downloading samples for offline, so we could just not provide a callback.

@OrenMe
Copy link
Contributor Author

OrenMe commented Feb 20, 2018

So in general the only issue I see is the coupling to the getConfiguration call on the player.
The entire configuration method and logic can be exported to it's own module and then can be used with both player and storage.
Maybe not even that far - the config used is pretty limited, I think it's DRM and network config, so this can just be set via the storage config itself.
WDYT?

@theodab
Copy link
Collaborator

theodab commented Feb 20, 2018

From config, it uses streaming.retryParameters, restrictions, drm, and manifest.retryParameters. That's disparate enough that making a offline store its own configuration would be messy. So breaking configuration off from Player is probably a good idea.

@TheModMaker
Copy link
Contributor

The reason we use a Player instance in Storage is to share the configuration and networking engine. We wanted to avoid requiring the app to configure both the Player and the Storage with their retry parameters, restrictions, and request filters. We could have Storage use its own networking engine, but the app will need to register its request/response filters with Storage in addition to the Player. But we could add a convenience method to copy the settings from the given Player so the app doesn't need to copy everything.

As for the configuration, maybe we should move the Storage-specific settings into a sub-section. Like streaming, we could have a storage group. Then the Storage.configure method would accept the same thing as Player. The downside is that it will be complicated to implement with reverse-compatibility.

@TheModMaker TheModMaker added this to the Backlog milestone Feb 20, 2018
@OrenMe
Copy link
Contributor Author

OrenMe commented Feb 20, 2018 via email

@joeyparrish joeyparrish modified the milestones: Backlog, v2.5 Mar 5, 2018
@joeyparrish
Copy link
Member

I am making plans to remove the requirement that Player have a video element upfront. This would mean Storage depending on Player would no longer imply the need for a video element.

Storage still uses Player for a NetworkingEngine, which has the advantage of an application only needing one set of networking filters for both storage and playback.

Storage also uses Player for DRM, manifest, and retry configs, which has a similar advantage. Only one config needs to be given for both storage and playback.

We could go further and allow Storage to create/own its own NetworkingEngine and own its own copy of the complete player config, but I'm reluctant to do that because of the complexity of the configs. We would need to manage a (nearly) complete copy of the Player config structure, possibly with different values, and we might also need to merge that with the existing offline config owned by Storage today.

If we did all of that, we could still have an optional link to a Player, which would provide a default config and shared NetworkingEngine for convenience.

@joeyparrish joeyparrish changed the title suggestion: create standalone offline download manager Standalone offline Storage (no Player, no video) Mar 5, 2018
@joeyparrish joeyparrish moved this from Ready to Blocked in Media & streaming refactor Mar 6, 2018
@joeyparrish joeyparrish moved this from Blocked to Blocked 2 in Media & streaming refactor Mar 6, 2018
@joeyparrish joeyparrish moved this from Blocked to Ready in Media & streaming refactor Mar 6, 2018
@vaage vaage self-assigned this Jul 2, 2018
@chrisfillmore
Copy link
Contributor

Could the client app instantiate a NetworkingEngine and pass it to Player and Storage via a new method setNetworkingEngine? This would make it easy to share, it seems.

@joeyparrish
Copy link
Member

Good idea. I'll let @vaage have the final say, though, as he is already deep in the implementation.

@vaage
Copy link
Contributor

vaage commented Jul 9, 2018

@chrisfillmore Thank you for the suggestion - I will add that to my list of possible solutions to explore. Right now the greatest challenge we are facing with this issue is how to maintain backward-compatibility while not inhibiting the new designs.

shaka-bot pushed a commit that referenced this issue Jul 31, 2018
As part of the effort to allow storage to be initialized with or
without a player instance, this change moves the offline config
into the player config and changes storage to share a configuration
object with a player instance.

We opted to share the instance between the two classes because if
storage was initialized with player, the initializer is implying a
relationship between the two objects and therefore configuration
changes should be shared.

Issue #1297

Change-Id: I991365255e63c284fbfcf147cf63c9588dd764ab
shaka-bot pushed a commit that referenced this issue Aug 1, 2018
Allow users to get access to the networking engine that storage
instances are using.

This is part of allowing storage to be used without a player instance
as it will allow users to configure the networking engine when they
create a storage instance without a player instance.

The code still assumes that Storage should never destroy the networking
engine. This is something that will need to change when storage can
be initialized without player.

Issue #1297

Change-Id: If8d1642259e0cf24cb43f2ca7936227e8d939260
shaka-bot pushed a commit that referenced this issue Aug 2, 2018
There was a cast in Storage.deleteAll that really should not
have been there. This changes it to use an assert to ensure
that the case is exposed if done wrong.

Issue #1297

Change-Id: I21f9737c0fc79c2b4324a7385a2814f94f40f884
shaka-bot pushed a commit that referenced this issue Aug 8, 2018
We store the references to player's networking engine and
configuration in Storage, so there is no reason to call player
anymore. This change removes those lingering calls.

Issue #1297

Change-Id: I3486ffcb60cd80fd765259d690091cc3267bb8ba
shaka-bot pushed a commit that referenced this issue Aug 14, 2018
This changes Storage to use the destroyer object to manage
clean-up. This will later be used to change how we destroy
storage depending on how it was initialized.

Issue #1297

Change-Id: I7e8c4f959a6764859504b0259bd5e815e5fd3dc8
Media & streaming refactor automation moved this from Actionable to Done Aug 14, 2018
@shaka-project shaka-project locked and limited conversation to collaborators Oct 13, 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
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
No open projects
Development

No branches or pull requests

7 participants