-
Notifications
You must be signed in to change notification settings - Fork 136
api: throw an error if an application use a video element in multiple instance of the RxPlayer #1394
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
Merged
Merged
api: throw an error if an application use a video element in multiple instance of the RxPlayer #1394
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
57fd4b9
api: throw and error if an application use a video
Florent-Bouisset 8de7315
demo: dispose the previous RxPlayer instance before creating a new one
Florent-Bouisset c298e91
add unit test and console.warn instead of throwing to prevent breakin…
Florent-Bouisset 1d246eb
lint
Florent-Bouisset e32b5a2
PR feedback
Florent-Bouisset c8e4173
format
Florent-Bouisset File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,6 +162,13 @@ class Player extends EventEmitter<IPublicAPIEvent> { | |
| /** Current version of the RxPlayer. */ | ||
| public readonly version: string; | ||
|
|
||
| /** | ||
| * Store all video elements currently in use by an RxPlayer instance. | ||
| * This is used to check that a video element is not shared between multiple instances. | ||
| * Use of a WeakSet ensure the object is garbage collected if it's not used anymore. | ||
| */ | ||
| private static _priv_currentlyUsedVideoElements = new WeakSet<HTMLMediaElement>(); | ||
|
|
||
| /** | ||
| * Media element attached to the RxPlayer. | ||
| * Set to `null` when the RxPlayer is disposed. | ||
|
|
@@ -326,6 +333,39 @@ class Player extends EventEmitter<IPublicAPIEvent> { | |
| addFeatures(featureList); | ||
| } | ||
|
|
||
| /** | ||
| * Register the video element to the set of elements currently in use. | ||
| * @param videoElement the video element to register. | ||
| * @throws Error - Throws if the element is already used by another player instance. | ||
| */ | ||
| private static _priv_registerVideoElement(videoElement: HTMLMediaElement) { | ||
| const errorMessage = | ||
|
||
| "The video element is already attached to another RxPlayer instance." + | ||
| "\nMake sure to dispose the previous instance with player.dispose() before creating" + | ||
| " a new player instance attaching that video element."; | ||
| if (Player._priv_currentlyUsedVideoElements.has(videoElement)) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn(errorMessage); | ||
| /* | ||
| * TODO: for next major version 5.0: this need to throw an error instead of just logging | ||
| * this was not done for minor version as it could be considerated a breaking change. | ||
| * | ||
| * throw new Error(errorMessage); | ||
| */ | ||
| } | ||
| Player._priv_currentlyUsedVideoElements.add(videoElement); | ||
| } | ||
|
|
||
| /** | ||
| * Deregister the video element of the set of elements currently in use. | ||
| * @param videoElement the video element to deregister. | ||
| */ | ||
| static _priv_deregisterVideoElement(videoElement: HTMLMediaElement) { | ||
| if (Player._priv_currentlyUsedVideoElements.has(videoElement)) { | ||
| Player._priv_currentlyUsedVideoElements.delete(videoElement); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @constructor | ||
| * @param {Object} options | ||
|
|
@@ -350,7 +390,7 @@ class Player extends EventEmitter<IPublicAPIEvent> { | |
| this.log = log; | ||
| this.state = "STOPPED"; | ||
| this.videoElement = videoElement; | ||
|
|
||
| Player._priv_registerVideoElement(this.videoElement); | ||
| const destroyCanceller = new TaskCanceller(); | ||
| this._destroyCanceller = destroyCanceller; | ||
|
|
||
|
|
@@ -567,6 +607,7 @@ class Player extends EventEmitter<IPublicAPIEvent> { | |
| this.stop(); | ||
|
|
||
| if (this.videoElement !== null) { | ||
| Player._priv_deregisterVideoElement(this.videoElement); | ||
| // free resources used for decryption management | ||
| disposeDecryptionResources(this.videoElement).catch((err: unknown) => { | ||
| const message = err instanceof Error ? err.message : "Unknown error"; | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Wording of the test case to update.
Same for the next one