-
Notifications
You must be signed in to change notification settings - Fork 133
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
[DRAFT] Demo: store the settings in the url hash #1349
base: dev
Are you sure you want to change the base?
Conversation
I would advocate for both:
|
0b408c2
to
e7c95fc
Compare
const loadContent = React.useCallback((content: IPlayableContent) => { | ||
getLoadVideoOptions(content).then( | ||
(loadVideoOptions) => { | ||
loadVideo(loadVideoOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look too much into it yet but have you verified that this code is not sensible to race conditions?
For example, if loadContent
is called multiple consecutive times, are we sure that the last call will lead to playback in the end?
Will look more into it, but that's a question I was asking myself reading this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the issue may already have been there before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to provoke a race condition but didn't succeed in doing so.
I still have added the code to prevent any race condition, let me know what you think of that because I'm not 100% sure that this is the correct way of doing so.
90fbc13
to
cd9ff08
Compare
e7c95fc
to
b66c200
Compare
1de6ca5
to
a18689b
Compare
b66c200
to
7731780
Compare
1d13bfb
to
1a7da65
Compare
… instance of the RxPlayer (#1394) * api: throw and error if an application use a video element in multiple instance of the RxPlayer The RxPlayer was not designed to run multiple instance of RxPlayer with the same videoElement. Having multiple instance sharing the same video element would result in various issues and unexpected behavior. This commit adds a check to throw an error if a videoElement is shared between RxPlayer instances. This will provide better guidance to application developers regarding any errors in their implementation. * demo: dispose the previous RxPlayer instance before creating a new one in the demo. Commit 57fd4b9 added an error message if the video element is reused between RxPlayer instance. This was the case on the demo, and this commit ensure to dispose the previous instance before creating a new one. * add unit test and console.warn instead of throwing to prevent breaking change in API * lint * PR feedback * format
While doing heavy testing for the upcoming `v4.0.0-rc1` (which if everything goes well, should be released next week), we noticed of an issue on LG TVs linked to the fact that we were setting a `MediaKeys` instance too close to the time when we removed it on an `HTMLMediaElement`. Because we were doing heavy testing and that was the first time we've seen that issue, we decided to just put an ugly exception for LG in that release, and to postpone a more global solution. This is a proposal for a more global solution (so aimed to be merged for something like a `4.0.1` release in many months potentially). The idea is that the `setMediaKeys` typically returns a Promise, that we were not awaiting because we've seen that on some targets, that Promise may hang for a very long time (e.g. until segments are pushed). Yet in the Encrypted Media Extensions (EME) W3C recommendation, it is indicated that doing concurrent `setMediaKeys` calls may fail. So here, we're racing with a timeout of 1 second. If it elapsed first, we consider the MediaKeys as set. A better solution may be to directly detect on which devices an issue appears and on which one it does not, and returns an appropriate logic accordingly, but this commit just propose an initial solution. Note that this solution comes with risk: we may be adding a length of up to 1 second to the loading time of contents on devices where `setMediaKeys` is long. What I propose when we merge this is to identify those devices at testing time and decide of a solution for them.
After #1401 (fix about the fact that `typeof Worker === "object"` on the PlayStation 4 for some reason), I finally succeeded to run the `MULTI_THREAD` experimental feature on a PlayStation 4 by adding one supplementary fix. Turns out that the `performance.now` web API, that we use to measure time differences (Date.now() being clock-related, it's not always appropriate for that) was not available in a Worker environment on the PlayStation 4. I have no idea why, as it is available in main thread, a random guess would be that this is a remnant of mitigations against something like spectre/meltdown but all I could find is that [they reduced its precision](https://trac.webkit.org/changeset/226495/webkit), not removed it from worker. Anyway, we thankfully only relied on this API at two places, as we voluntarily reduced those places previously to allow a synchronization mechanism between main thread and worker. This means that we can now easily use `Date.now()` in its place when `performance.now()` is unavailable without much fear of the issue coming back again in the future (our linter already ensure the API isn't used elsewhere). The main issue would be that timing measures would now become time-dependant on the PlayStation 4 and thus may be affected by clock changes, or subtle unix timestamp quirks. This should very rarely be problematic though, and it will just happen when the experimental `MULTI_THREAD` feature is explicitely relied on on the PlayStation 4 for now.
We noticed that ps4 applications were not relying on the `MULTI_THREAD` despite being theoretically compatible with it. Turns out that `typeof Worker` does not return `"function"` as expected with such objects but `"object"` instead on that browser. Thus, the RxPlayer believes that it hasn't access to the Worker API, the necessary requirement for the `MULTI_THREAD` feature. I suppose that this may also be true for other old Webkit-based browsers, though it isn't for recent-ish webkitgtk-based browsers on my PC, so for now I only enforce it for the PlayStation 4.
Issue ----- We noticed that the PlayStation 5 may have the HTMLMediaElement on which the content is played stop on a `MEDIA_ERR_DECODE` error if it encounters encrypted media data whose key is not usable due to policy restrictions (the most usual issue being non-respect of HDCP restrictions). This is not an usual behavior, other platforms just do not attempt to decode the encrypted media data and stall the playback instead (which is a much preferable behavior for us as we have some advanced mechanism to restart playback when this happens). Consequently, we have to specifically consider platforms with that fail-on-undecipherable-data issue, to perform a work-around in that case. Work-around ----------- The work-around found for now is to just directly cancel all streaming logic linked to a `MediaSource` as soon as the RxPlayer's `ContentDecryptor` (its module handling content decryption) signals about unusable keys, which should normally happen synchronously after the corresponding event is triggered by the browser. To ensure this is done as soon as possible (because this is a race with the browser), this is performed in the `ContentInitializer` RxPlayer module, which is the module directly interfacing with the `ContentDecryptor`, with media element errors, and with the logic of creating `MediaSource` instances. If we handled this in the `StreamOrchestrator` for example (which is better able to handle this scenario because it knows which segments have been buffered and are going to be buffered), we would have much more risk to "lose" the race with the browser here, as more RxPlayer modules will be involved as well as two threads and message-passing in a multithreaded scenario. I'm not that satisfied with this solution though. Because a race is still going on between the browser and the RxPlayer, it may break in the future if we bring an asynchronicity (e.g. a round trip to the event loop) between the moment a decryption key becomes unusable and the moment we're reloading the `MediaSource`. Perhaps a better solution would be to just let the corresponding `MEDIA_ERR_DECODE` happen, then determine heuristically if it had a good chance to happen due to the aforementioned decryption issue and reload in that case, but this is also difficult to implement.
The `MULTI_THREAD` experimental feature rely on a Worker file which use ES2017 features. The most relied on way to import this file is through its "embedded version", which is usually not processed by bundlers/compilers of the application so it could be translated to a more compatible (usually older) version of JavaScript. Yet, we're encountering on the PlayStation 4 a case where the `MULTI_THREAD` feature may be used, the device is compatible to the WebWorker web API, but does not understand all of ES2017 features (let alone ES2015 features). For those devices, and because we could still want to be able to rely on ES2017 for the vast majority of devices that do support it, I propose a solution where we provide a supplementary Worker file that is compiled down to ES5. I rel on webpack instead of esbuild for this because esbuild does not target es5 easily. I'm not very confident of my webpack skills - even more considering that Worker environments have some constraints and I'm not sure of every web+JS features that babel + webpack might use, but hopefully, this should work (I'll test it). There are several downsides coming with this commit: 1. Webpack takes several seconds on my PC to compile the worker files. As our building process was before relatively fast, this "slowness" is now visible during build, where "Bundling worker files..." is the slowest step on my PC. 2. Because I'm not that confident on the file outputted, I decided to re-run all integration tests running in `MULTI_THREAD` mode both on the regular and the ES5 version of that script. This means that integration tests will take much more time to complete.
When seeking on the current position the video element trigger the "seeking" event, but the seeking attribute of the video element itself is either not updated or is already reset to `false` at the moment the event "seeking" is received. video.onseeking = () => { // video.seeking attribute can be false console.log("on seeking!", video.seeking) } Due to a recent change we are waiting for the attribute "video.seeking" to be true. Checking that the event type correspond to a seeking event fix the issue.
…s attribute "playsinline"
Storing the player settings in the url hash let us refresh the demo page while keeping all the settings previously set.
It's more efficient and comfortable when debugging to not have to reset them manually after each page refresh.
To Do: