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.

Sign up for GitHub

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

Jump to bottom

[DRAFT] Demo: store the settings in the url hash #1349

Open
wants to merge 103 commits into
base: dev
Choose a base branch
from
Open

[DRAFT] Demo: store the settings in the url hash #1349

wants to merge 103 commits into from

Conversation

Florent-Bouisset
Copy link
Collaborator

@Florent-Bouisset Florent-Bouisset commented Jan 5, 2024 •
edited
Loading

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:

  • Add a switch/checkbox to disable/enable the storing in url OR let the RxPlayer logo be a tag with href with an empty hash allowing to reset.
  • Reloading a preset content will display it as a custom content, we can eventually improve that

@peaBerberian
Copy link
Collaborator

Add a switch/checkbox to disable/enable the storing in url OR let the RxPlayer logo be a tag with href with an empty hash allowing to reset.

I would advocate for both:

  • the switch would stop the URL from updating but keep it where it was. This would be useful when debugging settings you know are problematic, then playing with them, while being able to reset to the problematic scenario by just refreshing the page
  • the link would just reset to the default

peaBerberian
peaBerberian reviewed Jan 8, 2024
View reviewed changes
demo/full/scripts/controllers/ContentList.tsx Outdated
const loadContent = React.useCallback((content: IPlayableContent) => {
getLoadVideoOptions(content).then(
(loadVideoOptions) => {
loadVideo(loadVideoOptions);
Copy link
Collaborator

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

Copy link
Collaborator

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)

Copy link
Collaborator Author

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.

@peaBerberian peaBerberian changed the base branch from next-v4 to dev January 25, 2024 11:38
@peaBerberian peaBerberian added Demo Relative to the RxPlayer's demo page Priority: 2 (Medium) This issue or PR has a medium priority. labels Feb 5, 2024
@peaBerberian peaBerberian added this to the 4.1.0 milestone Feb 5, 2024
Florent-Bouisset and others added 14 commits June 17, 2024 18:45
@Florent-Bouisset
api: throw an error if an application use a video element in multiple…
8a6a31c 
… 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
@peaBerberian @Florent-Bouisset
DRM: Cleaner implementation for setMediaKeys
9bf4194 
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.
@peaBerberian @Florent-Bouisset
Throw CREATE_MEDIA_KEYS_ERROR when MediaKeys attachment fails
1eeb547
@peaBerberian @Florent-Bouisset
Add MEDIA_KEYS_ATTACHMENT_ERROR
2fbdfc0
@peaBerberian @Florent-Bouisset
Do not rely on performance.now if unavailable
416a99c 
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.
@peaBerberian @Florent-Bouisset
Fix Worker usage on PS4 browser
fda9867 
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.
@peaBerberian @Florent-Bouisset
Update changelog
37f74b3
@peaBerberian @Florent-Bouisset
On the playstation 5, reload ASAP if a key become unusable
4de5ee7 
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.
@peaBerberian @Florent-Bouisset
Add ps5 fix to the CHANGELOG file
7f59c45
@peaBerberian @Florent-Bouisset
Provide ES5 version of the worker file
e07bdbc 
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.
@peaBerberian @Florent-Bouisset
Fix typo in changelog and run prettier on it
7836eb6
@Florent-Bouisset
fix(safari): autoplay was not working on Safari
e2e98fc 
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.
@Florent-Bouisset
fix(safari): no longer stuck in buffering on safari iOS when video ha…
c1d0575 
…s attribute "playsinline"
@Florent-Bouisset
refactor: factorize keyId comparison for blacklisting
f4b647f
@peaBerberian peaBerberian modified the milestones: 4.1.0, 4.2.0 Jun 27, 2024
@peaBerberian peaBerberian modified the milestones: 4.2.0, 4.3.0 Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peaBerberian peaBerberian peaBerberian left review comments

Assignees
No one assigned
Labels
Demo Relative to the RxPlayer's demo page Priority: 2 (Medium) This issue or PR has a medium priority.
Projects
None yet
Milestone
4.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Florent-Bouisset @peaBerberian