Fix getPeriodForTime util
#1738
Merged
Conversation
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
An issue, #1736, noticed that we have an issue with the `getPeriodForTime` util which links time and Period. The way the util was written means it had to be looped over from the last to the first, but I don't know why I decided to replace it with a fancier `for...of` (which goes from first to last) in a totally unrelated refacto (#1507). This broke the logic and lead in conditions that are hard to predict infinite rebuffering issues. I also made some other code more resilient here, because I thought that I saw another issue first. I don't really know if that other case is possible but I thought that it didn't cost anything to be resilient there.
peaBerberian
added
the
bug
peaBerberian
added
the
Priority: 0 (Very high)
peaBerberian
commented
Sep 3, 2025
| // Last check just for resilience reasons that the wanted Period is | ||
| // not one of the already-handled ones | ||
| return; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
NOTE: This was the part just for resilience, as I was under the impression that the edge case of:
timeis exactly at a Period's end- next Period is later (there's a discontinuity currently)
Was not properly considered here, but I never reproduced an issue with it.
Only risk that I can think of from that update is that we now compute nextPeriod before the periodStreamCleared() callback and currentCanceller.cancel() both which may perform unknown side-effects.
We could also just re-compute nextPeriod there - at the expense of code readability - as you wish.
Collaborator
Author
There was a problem hiding this comment.
I finally chose to just re-compute it.
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
Florent-Bouisset
approved these changes
Sep 3, 2025
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.
An issue, #1736, noticed that we have a problem with the
getPeriodForTimeutil which links the current playback timestamp and the Period to which it is associated to.The way the util was written means it had to be looped over from the last to the first, but I don't know why I decided to replace it with a fancier
for...of(which goes from first to last) in a totally unrelated refacto (#1507).This broke the logic and lead, with conditions that are hard to predict, to some infinite rebuffering issues.
I also made some other code more resilient here, because I thought that I saw another issue first. I don't really know if that other case is possible but I thought that it didn't cost anything to be resilient there.