[WIP] v4 code reorganization #1365
Merged
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
force-pushed
the
misc/v4-dir-refacto
branch
4 times, most recently
from
58c80da
to
1341878
Compare
peaBerberian
force-pushed
the
misc/v4-dir-refacto
branch
from
1341878
to
1b74f29
Compare
peaBerberian
force-pushed
the
misc/v4-dir-refacto
branch
from
075c73a
to
92e3396
Compare
peaBerberian
added a commit
that referenced
this pull request
Jan 26, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
peaBerberian
added a commit
that referenced
this pull request
Jan 26, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is the second solution (after #1366), which only moves utils common to both the WebWorker's PlaybackObserver and the main thread's PlaybackObserver into the `src/core/main/common/` directory - just like `ContentInitializer` utils in monothreaded mode. In a way, this solution repeats the same solution found for `ContentInitializer` utils - which are in a different place when ran in monothreaded mode (where the utils is imported by the `src/main_thread/init` logic) or ran in multithreaded mode (where the utils imported by the `src/core/main/worker` logic). Result ------ I will admit that I'm less a fan of this solution than #1366, as the `src/core/main/common` feels like a hack in any scenario it is used. There is also the fact that types exported through that path need to be re-exported through a more accessible path (usually src/main_thread/types`) to simplify import paths in the project. Thoughts?
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
This was referenced Jan 26, 2024
Florent-Bouisset
approved these changes
Jan 31, 2024
peaBerberian
added a commit
that referenced
this pull request
Jan 31, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
peaBerberian
added a commit
that referenced
this pull request
Feb 5, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
peaBerberian
added a commit
that referenced
this pull request
Feb 5, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
peaBerberian
added a commit
that referenced
this pull request
Feb 5, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
peaBerberian
added a commit
that referenced
this pull request
Feb 5, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
peaBerberian
added a commit
that referenced
this pull request
Feb 20, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
peaBerberian
added a commit
that referenced
this pull request
Feb 20, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
Merged
peaBerberian
added a commit
that referenced
this pull request
Jun 13, 2024
Root issue ---------- While working on code refactoring for better taking into account the new potentially multithread nature of the RxPlayer code (with some files only running in a WebWorker environment and others only running in a main thread environment), we recently refactored our file hierarchy (#1365) to better reflect that new situation. The idea is to make RxPlayer developpers more aware of what code is intended to run where. In that work, we had a remaining issue concerning the `PlaybackObserver`. This is the part of the code that is monitoring and advertising playback conditions to the rest of the RxPlayer code. Most core RxPlayer modules rely on it, in both main thread and WebWorker environments. The root issue is that the `PlaybackObserver` is a class and thus cannot be easily transmitted in-between environments (as the main thread and WebWorker only exchanges between one another through `postMessage` calls, which follows some rules preventing from doing that). As such, and only on a multithreaded scenario, the RxPlayer is serializing in some way the constructed source PlaybackObserver in the main thread to reconstruct one (the `WorkerPlaybackObserver`) on the worker. Because a whole complex PlaybackObserver-compatible structure has to both be constructed in the main thread and in the worker (yet only the main thread can act as the "true" source one, because the media element can only be accessed in main thread), we were asking ourselves where should we put the common utils (required by both the main thread and worker) needed to construct one (like the `ObservationPosition` class and the `generateReadOnlyObserver` util). Solution 1 ---------- This is a first solution proposal, which moves the `PlaybackObserver` directory outside of the `main_thread` and `core` directories. Instead, its code is now directly in `src/playback_observer`. This solution takes inspiration from the `src/mse` directory, which also exports both: 1. files intended to be imported when MSE API are present in the current environment (when in main thread or when in a WebWorker with the MSE-in-worker feature) and 2. files intended to be imported in environments without MSE. For example the `src/mse/main_media_source_interface.ts` should __ONLY__ be imported in environments with MSE capabilities. If you do not, you should import `src/mse/worker_media_source_interface.ts`. Likewise, I here added a `src/playback_observer/media_element_playback_observer.ts` file exporting a `MediaElementPlaybackObserver` structure (new name of the `PlaybackObserver`) which can only be imported in environements where the media element is available (so, only on main thread) and a `src/playback_observer/worker_playback_observer.ts` file which should only be imported in other environments. Doing this allows to easily share common utils, in a `src/playback_observer/utils` directory, without involving the rest of the RxPlayer code. Result ------ I'm quite happy with the result, though I get it may seem weird to have a complex core frequently-running logic part directly in `src` (and not in `main_thread` or `core` as was the norm) - though it is also the case of directories like `mse` or `transports`. RxPlayer code very rarely imported files inside other directories (which it does here with paths like `../playback_observer/media_element_playback_observer`) as a way to push better modularization. Though here it may seem like a feature because its unusual-ness forces the developper to double check if this is the right file that is imported.
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.
As discussed (with the core RxPlayer team), now is one of the very rare occasions in the project life where we have few long-lived branch to regularly rebase (or at least not in the scale of the enormous v4 or worker branches).
This is because if the
v4.0.0-rc1is the confirmedv4.0.0, we won't rebasev4branches on thev3anymore, both will evolve separately).So we had several discussions on what heavy code updates we can now try and came up with several.
Remaining steps:
src/manifestusage is still weird due to the Object/class schismCore code division for webworker vs main thread
The
MULTI_THREADexperimental feature required some extensive changes that may profit from reorganizing directories. For example, there's no clear division between files intended to run in main thread and those intended to run in a WebWorker environment, and one file could easily import the wrong kind of file, making the bundle size bigger or worse, leading to runtime errors.I thus splitted-up the
src/coredirectory of the RxPlayer into two directories:src/main_threadwhich contains the part of the core that always runs in main thread (i.e. even in MultiThreading mode).It contains today:
src/main_tread/api: the RxPlayer public API. Previously insrc/core/apisrc/main_tread/decrypt: theContentDecryptormodule previously insrc/core/decryptsrc/main_tread/init: Only theContentInitializerinterface implementations found previously insrc/core/initand the utils that always have to be performed in main thread (init utils which may now be performed on a webworker are insrc/core/main/common)src/main_tread/text_displayer: theTextDisplayerdefinitions previously insrc/text_displayersrc/main_tread/tracks_store: theTracksStoreclasses previously insrc/core/api/track_managementA
types.tsfile which should generally be all the external code may need to importsrc/corewhich contains the part of the core that may run in a WebWorkerIt contains today:
src/core/adaptive: unchangedsrc/core/main: the part of the previoussrc/core/initdirectory which may now run in a WebWorker. Utils that may be run in a monothreading mode are in ancommonsubdirectory (which may be directly imported by thesrc/main_thread/initcode - which doesn't lead to pretty import paths so we may change that in the future). Files only intended to run in a WebWorker are in aworkersubdirectory.src/core/fetchers: unchangedsrc/core/segment_sinks: the new name of thesrc/core/segment_buffersdirectory (see SegmentBuffers renaming chapter below)src/core/stream: unchangedA
types.tsfile which should generally be all the external code may need to importThis new file organization make it clearer by looking at imported paths when something that shouldn't be imported is imported (
coreshouldn't rely onmain_threadbesides type definitions, basically).It may also be an important step to rely on TypeScript to enforce that WebWorker files do not relies on main-thread-only types (not done for now).
SegmentBuffersare renamed toSegmentSinksSegmentBuffersare far from being the actual media buffers but can more be seen as the destination of loaded segments. Moreover both thesrc/mseandsrc/main_thread/text_displayerdirectories (added since the worker branch) may fill more the role of what could be called "media buffers". Consequenly I chose to renameSegmentBuffersand theSegmentBuffersStoreclass toSegmentSinksStore.src/manifestdirectory changesThe MultiThread work set an awkward separation between the
Manifestclass, constructed optionally in a WebWorker in which case it is never used in main thread, and theIManifestMetadataobject that is used in its place on the main thread when a WebWorker environment is set-up.To make it clearer that the
Manifestclass is not to be expected everywhere, I moved class definitions deeper insrc/manifest/classes(instead of just accessible insrc/manifest) and only now exports types (named like the class but prefixed byI, likeIManifest,IPeriodand so on) throughsrc/manifest. This last part was added for places where classes where imported just to define a type.All this was added to better communicate through import paths that the actual class should very rarely be relied on, though it is still awkward and not hyper clear either so I'm not too happy with the result.
import typeare enforced through our linterWe now enforce through the linter the mention of
typewhen importing TypeScripttypes andinterfaces.This is just to make it clearer to both humans and tools of which import is going to have an influence into the final bundle, and which are "only" here for checks at compile-time.
Enforcing eqeqeq linter rule
Previously, we had an exception to allow the usage of double equals when it was for
null(e.g.a == null/a != null) as it was a shortcut for checking the equality to bothnullandundefinedwhich is a generally frequent need in JavaScript.However, we found that the trick was not known by everyone and thus we could raise code readability by checking more explicitly for both situations, by for example relying on our
isNullOrUndefinedutil instead.I now chose to completely enforce the linting rule which forbids double equals, even for that case.
PlaybackObservermoved tosrc/playback_observerThe
PlaybackObserver, now namedMediaElementPlaybackObserver, has been moved fromsrc/core/api/playback_observertosrc/playback_observer. This is because the concept of playback observations need to be performed by both themain_threadand thecorecode.TODO: Rely on prettier
We've finally decided to take this occasion to rely on prettier for all compatible code in the project (demo, tests, source code, documentation, scripts). I will do it at the end once we are ready with everything else in this PR.