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

Do not setup up MediaSource in advance of load() #1087

Closed
peterdotjs opened this issue Oct 25, 2017 · 14 comments
Closed

Do not setup up MediaSource in advance of load() #1087

peterdotjs opened this issue Oct 25, 2017 · 14 comments
Assignees
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Milestone

Comments

@peterdotjs
Copy link

Have you read the FAQ and checked for duplicate issues:
yes

What version of Shaka Player are you using:
2.2.1

What browser and OS are you using:
Chrome 62, OSX

What did you do?
Force an error when player loads manifest.

What did you expect to happen?
I expect that we should gracefully fall back to behavior prior to using shaka. Possibly expose an API to update video tag back to original state since the player updates the src on initialization.

What actually happened?
Example: https://jsfiddle.net/az7q2x85/3/

Current workaround is to manually update the video src url back to the original.

@chrisfillmore
Copy link
Contributor

Possibly expose an API to update video tag back to original state since the player updates the src on initialization.

You can call Player#unload to do this.

@peterdotjs
Copy link
Author

peterdotjs commented Oct 26, 2017

@chrisfillmore if you see the fiddle that I linked I am already calling player.unload() on error.

You can see that the src of the video element has been modified to blob: - this doesn't match the original src of the video.

@TheModMaker
Copy link
Contributor

We assume that the <video> is under our control for the lifetime of the Player object. If you call load() again, we will restart and load that content instead. You shouldn't change the video.src while the Player is in use. To indicate the Player is no longer being used, call player.destroy(). Also note that both destroy() and unload() return a Promise which you should wait to complete before doing other work.

We will continue to keep the MediaSource attached to the video even after unload() (and also before load()) because some browsers take a bit to setup the media stack. To reduce startup latency, we keep the MediaSource attached (video.src is set to a special blob string) for the duration of the Player instance.

We will not restore the video.src to the original string, but we will reset the <video> element so that it is ready to have your app set the source to whatever you want.

@theodab theodab added status: working as intended The behavior is intended; this is not a bug and removed needs triage labels Oct 26, 2017
@joeyparrish
Copy link
Member

It may also be worth considering why you want a fallback from Shaka Player to setting the video src. If the load() failure is caused by a lack of browser support, you should call shaka.Player.isBrowserSupported() ahead of time to avoid this scenario. If the browser is not supported, you can go directly to video.src.

For a more detailed probe of what the browser does and does not support, you can use shaka.Player.probeSupport() to get a look at what media types, manifest types, and DRM systems will be usable on this particular browser. Note, though, that this method may result in a user permission prompt (from EME) on certain platforms.

You can read more about these methods in the API docs here: https://shaka-player-demo.appspot.com/docs/api/shaka.Player.html

@peterdotjs
Copy link
Author

peterdotjs commented Oct 26, 2017

Thanks for the quick response!

@TheModMaker yup as of now that's the approach - using destroy and then resetting the src, but I figured I can't be the only one that needs to rely on a fallback approach. If you think it's a corner case then it doesn't deserve any API support. I'm fine with setting the src myself but interested to hear what the community uses as fallbacks as when this happens it's basically the worst experience of a broken video.

@joeyparrish we're already using shaka.Player.isBrowserSupport prior to this point but still seeing some issues in the wild hence this fallback approach. I still need to dig deeper to why it's happening in these cases. If I find that they are due to cases not handled in isBrowserSupported i'll reach out again.

@joeyparrish
Copy link
Member

Thanks for the feedback, Peter.

A few things worth noting here:

  1. We set up MediaSource in advance, as @TheModMaker says, to speed up load() later. This was based on feedback we got from one of your colleagues (Daniel Baulig) when we were designing v2.
  2. This design decision is in conflict with three upcoming features:
    a. Preload API #880, an API to preload content without a video tag
    b. Support HLS through src= on iOS #997, HLS support on iOS through src=
    c. Allow playback of single files through src= #816, support of progressive, non-adaptive media through src=

In all likelihood, we will have to scrap advanced MediaSource setup at some point for those features. Since we originally did it for the sake of your application, and since you don't seem to want it any more, it seems like it won't be missed if we remove it in v2.4.

For now, I'll mark this as an enhancement for v2.4, rename to reflect the removal of advanced MediaSource setup, and note that this is a prerequisite for the other three features mentioned above.

@joeyparrish joeyparrish added type: enhancement New feature or request and removed status: working as intended The behavior is intended; this is not a bug labels Oct 26, 2017
@joeyparrish joeyparrish added this to the v2.4.0 milestone Oct 26, 2017
@joeyparrish joeyparrish changed the title Gracefully recover from .load() error Do not setup up MediaSource in advance of load() Oct 26, 2017
@peterdotjs
Copy link
Author

peterdotjs commented Oct 26, 2017

Thanks for providing more context, Joey. Where did you get the sense that we didn't want MediaSource set up in advance anymore?

@joeyparrish
Copy link
Member

I'm sorry, maybe I misunderstood.

Setting up MediaSource in advance of load() means setting it up in both the Player constructor and in unload(). You can't set up MediaSource without attaching it to the src attribute of a video element.

So we can't give you control of src= after unload and set up MediaSource in advance of load().

What would make the most sense from your perspective?

@peterdotjs
Copy link
Author

@joeyparrish apologies for the miscommunication - I wasn't aware of the perf implications until you posted the link to the feedback from 2015. Perf I think would be the highest pri but it seems unfortunate that this design decision impacts the list of features you pointed out. From my perspective perf would be more important than getting control over the src to handle the fallback case.

@joeyparrish
Copy link
Member

Okay, then in that case, I'll try to make sure our design changes offer some way to explicitly setup MediaSource in advance, rather than implicitly, so that applications can take control of the flow. How does that sound?

@peterdotjs
Copy link
Author

Sounds like a perfect balance. Thanks Joey!

@joeyparrish
Copy link
Member

No problem. Sorry for the misunderstanding.

@joeyparrish joeyparrish modified the milestones: v2.4.0, Backlog Dec 4, 2017
@joeyparrish joeyparrish self-assigned this Feb 5, 2018
shaka-bot pushed a commit that referenced this issue Feb 20, 2018
This isolates MediaSource code without changing the early
initialization of MediaSource, which was designed for low-latency
startup.

Prerequisite for #1087, #880, #997, #816

Change-Id: I61acedd95d73610d3e67436d9c7767d643fe2d29
@joeyparrish
Copy link
Member

Here's what I'm building at the moment:

  • New method attach(video, initalizeMediaSource) to take ownership of a video element, where initalizeMediaSource decides if we set up MediaSource in advance or not, and defaults to true
  • video argument in Player constructor becomes optional, and if provided, is equivalent to calling attach(video, true)
  • New method detach() to release ownership of the media element
  • New argument reinitializeMediaSource in unload(), decides if we should set up MediaSource in advance of a subsequent load() or not, and defaults to true
  • attach() and detach() are unavailable while casting

@peterdotjs, what do you think? The default behavior will stay the same, but you will have options to explicitly decide when we take over a video element and when we set up MediaSource.

@shaka-project shaka-project locked and limited conversation to collaborators May 5, 2018
@joeyparrish
Copy link
Member

This feature will be out in v2.4 this week.

@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
None yet
Development

No branches or pull requests

6 participants