[POC] [WIP] Remove MediaSourceContentInitializer, add CoreInterface #1627
Conversation
This file contains 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
peaBerberian
added
work-in-progress
peaBerberian
force-pushed
the
poc/common-init
branch
5 times, most recently
from
7569943 to
47b4bf7
Compare
peaBerberian
force-pushed
the
poc/common-init
branch
3 times, most recently
from
a203984 to
d97c89e
Compare
peaBerberian
added
the
skip-performance-checks
peaBerberian
force-pushed
the
poc/common-init
branch
from
d97c89e to
809f8e8
Compare
peaBerberian
added
skip-performance-checks
peaBerberian
force-pushed
the
poc/common-init
branch
3 times, most recently
from
c632c93 to
33ce62e
Compare
peaBerberian
removed
skip-performance-checks
peaBerberian
force-pushed
the
poc/common-init
branch
3 times, most recently
from
b0b30e6 to
f480912
Compare
This is a proof-of-concept where I try to put in common the "content
initialization" logic for our "multithread" mode and our monothreaded
mode.
The idea is to replace that mode-specific code to a very thin layer
(here called `CoreInterface`) between our "init" code (always running in
main thread) and our "core" code (in a WebWorker in "multithread" mode)
which would handle both modes:
- in multithreaded mode, it would be the part doing `postmessage` calls
and `onmessage` registering for thread communication
- in monothreaded mode, it would just do the same thing through a very
simple EventEmitter-like approach
Or written another way: "multithread" and single-threaded mode would now
share the same logic beside the communication system used at the frontier
between the main-thread and potential worker (respectively
`src/main_thread` code and `src/core` code).
The end goal is to remove a lot of code, and to reduce the difference
between the multithreaded and monothreaded logic, so our tests
(integration, manual tests etc.) actually almost test the two in one go.
There might be some performance lost due to steps we are now performing
unnecessarily when in monothreaded mode (e.g. we serialize the Manifest
structure even though it's not needed when a single thread is used, we
create another PlaybackObserver on the core even though the main thread
one could be re-used etc.). To see if that lead to a visible difference
and if it does, it shouldn't be that hard to work-around.
peaBerberian
force-pushed
the
poc/common-init
branch
from
f480912 to
aa614e4
Compare
|
Automated performance checks have been performed on commit Tests results✅ Tests have passed. Performance tests 1st run outputNo significative change in performance for tests:
If you want to skip performance checks for latter commits, add the |
peaBerberian
added
low-latency
peaBerberian
added a commit
that referenced
this pull request
Feb 27, 2025
I propose here to make our `MULTI_THREAD` feature, one the main feature brought in our `v4.0.0`, a stable (i.e. not `experimental` anymore) one. At Canal+ Group, it has been the main way to rely on the RxPlayer for more than a year (with gains in terms of adaptive stability and performances), and with #1627, we're in the process of making the multi-thread code path the main one that will be relied on (with an adaptation to support the default monothread one). Removing the `experimental` label would encourage applications to rely on this feature more, which would bring both its advantages to them and align them more with what we're used to internally at Canal+. We've never changed its API but still kept its "experimental" label mainly to let us update its API, especially for the following issues: 1. For now it only supports DASH streaming, which is directily included with the worker code. The issue with supporting more streaming protocols is that we don't want to include those other rarely-used ones in every applications using the feature, and that we're still unsure how a feature switching concept could be relied on together with `MULTI_THREAD` (as the feature code to inject should be sometimes injected to the worker which runs in another thread - adding communication issues). 2. For a long time, I was not sure if we should directly include the "embedded worker" file in the `MULTI_THREAD` feature (as 95% of applications should want that) or if we let this as a supplementary step to let the application do more advanced things if it wants (worker transpiled to ES5, stored on a CDN somewhere and loaded lazily etc.). For now I chose the more flexible one as I think that this supplementary step is not a huge added complexity to its API. 3. It has some limitations (DASH-only, no `manifestLoader` or `segmentLoader`, [platform compatible to WebWorkers](https://caniuse.com/?search=webworker)), which is why we often advised people to rely on both `MULTI_THREAD` and on the main-thread `DASH` feature as a security fallback if those limitations aren't respected. This means more complexity in how the player may run and much more code being loaded (for what is most times unnecessarily duplicated code). Keeping the API experimental would let us maybe find a way to fix or reduce those issues at some point, but I think the gain of making it stable (encouraging more people to use it and to report issues/needs on this) make it up for the issues of doing it (less freedom to update its API).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Based on #1630 as performance regression is the main risk here
This is a proof-of-concept where I try to put in common the "content initialization" logic for our "multithread" mode and our monothreaded mode.
The idea is to replace that mode-specific code to a very thin layer (here called
CoreInterface) between our "init" code (always running in main thread) and our "core" code (in a WebWorker in "multithread" mode) which would handle both modes:in multithreaded mode, it would be the part doing
postmessagecalls andonmessageregistering for thread communicationin monothreaded mode, it would just do the same thing through a very simple EventEmitter-like approach
Or written another way: "multithread" and single-threaded mode would now share the same logic beside the communication system used at the frontier between the main-thread and potential worker.
The end goal is to remove a lot of code, and to reduce the difference between the multithreaded and monothreaded logic, so our tests (integration, manual tests etc.) actually almost test the two in one go.
There might be some performance lost due to steps we are now performing unnecessarily when in monothreaded mode (e.g. we serialize the Manifest structure even though it's not needed when a single thread is used, we create another PlaybackObserver on the core even though the main thread one could be re-used etc.). To see if that lead to a visible difference and if it does, it shouldn't be that hard to work-around.