Deploy of AdaptationStream leak fix - #37
Open
peaBerberian wants to merge 2 commits into
Open
Conversation
|
A built version of the branch has been deployed to the following URL: You can import it by changing your "rx-player": "https://github.com/peaBerberian/rx-player/releases/download/autobuild.abfe41a45958879cdd9984b83014d0a6311298a4/rx-player.tgz"Then run: |
The canalplus#1778 and canalplus#1779 issues / PR noticed a leak that seem to arise when multiple quality switches happen. I'm still unsure of the severity (looking at it what this fixes seems very minimal, and we did not notice this yet on production at Canal+ including on low-memory devices for what seems to be a change that has been here for 2 years - but external contributors actually did notice a leak so maybe a set of conditions amplify the issue), but looking closely at the code in question, there does seem to be an improper event listener clean-up on a quality switch. The issue is rooted in the complexity behind how quality switch happen: - depending on heuristics, we may either perform an "urgent" quality switch (where we directly cancel the requests linked to the older quality) or a non-urgent one (where we will wait for the current requests to finish and only after load the new quality). - If non-urgent, we want to still do the requests for the new quality as soon as we can, thus we parallelize it with the pushing operations of the segments we just loaded from the previous quality. Thus when a "non-urgent" quality switch happen, there might be a short time where several quality-linked modules are running at the same time (the old one to push segments, the new one to load them), whereas at first glance they seemed conflicting (one loads and push one quality, the other loads and push another quality of the same thing). This lead to an awkward architecture where the clean-up process of those modules is subtly different than in other RxPlayer modules - this one has actually 2 means to terminate: - its `terminate` parameter, kind of like a SIGTERM: just finish what you're doing (e.g. finish loading segments and/or pushing them then stop). Once the `RepresentationStream` (the module in question) has finished loading segments, it sends a `terminating` event - but it might still be pushing segments. It however has no event to indicate that segments have been pushed, for now. - its `cancelSignal` parameter, more akin to a SIGKILL: terminate everything now without delay. This one is e.g. triggered when stopping the content, changing the track etc. The leaking event listener was wrongly linked to that "SIGKILL" signal, even if it was intended to be cleaned up when the module is not needed anymore. When the module was only "SIGTERMed", it was not cleaned up. --- I chose to clean it up not right when "SIGTERMed", but when the module itself anounced that it is "terminating" (it is done loading and is now pushing segments). I found it to be more appropriate for the logic in question and a corresponding `CancellationSignal` was already used for other similar logic linked to the same lifetime.
peaBerberian
force-pushed
the
fix/apap-stream-listener-leak
branch
from
January 19, 2026 13:55
0797303 to
405b338
Compare
|
A built version of the branch has been deployed to the following URL: You can import it by changing your "rx-player": "https://github.com/peaBerberian/rx-player/releases/download/autobuild.a8898927222197f8c157a06e19320e007a0ae384/rx-player.tgz"Then run: |
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
|
A built version of the branch has been deployed to the following URL: You can import it by changing your "rx-player": "https://github.com/peaBerberian/rx-player/releases/download/autobuild.eb574a3a7288ebe348ecc39e4fbcdfa8ac7c175b/rx-player.tgz"Then run: |
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Deploy of canalplus#1781