Avoid destroying PeriodStream if one is re-created for the same period#1232
Merged
peaBerberian merged 1 commit intomasterfrom Apr 4, 2023
Merged
Avoid destroying PeriodStream if one is re-created for the same period#1232peaBerberian merged 1 commit intomasterfrom
peaBerberian merged 1 commit intomasterfrom
Conversation
peaBerberian
added
bug
peaBerberian
force-pushed
the
fix/do-not-destroy-period-stream
branch
from
fe2e73f to
d685c5f
Compare
While doing some debugging on multi-Period contents, we saw some situations where at some Period's boundary, no segment would be loaded, since the `v3.30.0`. Though we are still in the process of testing it, I relatedly found what seems like an issue where we could be in some kind of (slow) loop creating and destroying `PeriodStream`, which is the module allowing to load segment corresponding to a particular DASH period (or related in other `transport`s) when we approach from the end of the previous Period. To try to fix this logic, I added a check to see if the next `PeriodStream` wanted was linked to the same `Period` than the next `PeriodStream` which was already created, in which case we won't be destroying it immediately, just to re-creating it like we did before. For the source of this behavior, I think that re-checking that the right `PeriodStream` is created regularly (more specifically: each time the previous chronological `PeriodStream` signals that is has no segment to load) is not that bad of an idea, as a security, so I did not change it.
peaBerberian
force-pushed
the
fix/do-not-destroy-period-stream
branch
from
d685c5f to
b576e92
Compare
Merged
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.
While doing some debugging on multi-Period contents, we saw some situations where at some Period's boundary, no segment would be loaded, since the
v3.30.0.Though we are still in the process of testing it, I relatedly found what seems like an issue where we could be in some kind of (slow) loop creating and destroying
PeriodStream, which is the module allowing to load segment corresponding to a particular DASH period (or related in othertransports) when we approach from the end of the previous Period.To try to fix this logic, I added a check to see if the next
PeriodStreamwanted was linked to the samePeriodthan the nextPeriodStreamwhich was already created, in which case we won't be destroying it immediately, just to re-creating it like we did before.For the source of this behavior, I think that re-checking that the right
PeriodStreamis created regularly (more specifically: each time the previous chronologicalPeriodStreamsignals that is has no segment to load) is not that bad of an idea, as a security, so I did not change it.