Open
Conversation
peaBerberian
force-pushed
the
misc/more-tests
branch
3 times, most recently
from
e668c92 to
40e6c45
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
peaBerberian
force-pushed
the
misc/more-tests
branch
2 times, most recently
from
1231f7e to
28a358a
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
peaBerberian
added
the
work-in-progress
peaBerberian
force-pushed
the
misc/more-tests
branch
from
83ff3f3 to
f0092ad
Compare
peaBerberian
force-pushed
the
misc/more-tests
branch
from
f0092ad to
425ffb8
Compare
github-actions
bot
force-pushed
the
dev
branch
from
a42734c to
29372ac
Compare
New integration tests I've added recently led to a curious behavior: for multithreaded contents, we had two initial `textTrackChange` (with the payload `null`) in a row instead of just one. This exact behavior does not break the API, but it was nonetheless not normal, so I looked at it. --- Turns out it is linked to a bad ordering in core between the first "PeriodChange" Core message (indicating a Period is currently playing, thus implies that we already chose the audio+video+text tracks) and the last "AdaptationChange" (for audio or video or text type) for that first period. What happened: 1. The content begins to be loaded, the RxPlayer setup everything, and start choosing the initial Adaptations (tracks) and Representations. 2. Once all Adaptation are setup, Core send a `PeriodChange` message 3. Then Core sent an `AdaptationChange` message for the last Adaptation set-up (the text one here) 4. Main thread receives the `PeriodChange` message and bubble it to the public API module. 5. The public API on this event looks at the current audio / video / text tracks for that new Period and send the right `...TrackChange` events, then send a `periodChange` event, as expected 6. Main thread then receives the `AdaptationChange` message and also bubble it. 7. The Public API reacts on this event by sending again `textTrackChange` for the same text track, because it thinks from the `AdaptationChange` message that the track just changed, after the PeriodChange --- Reversing the ordering (the last `AdaptationChange`, then `PeriodChange`) in Core makes more sense (messages are the Core's API, and advertising for a Period change should be done after advertising for all tracks initially choosen for that Period) fixes the issue. It's very unclear to me if this led to actual bugs later in the RxPlayer. For now the only seen impact was a failing integration tests which was voluntarily less "tolerant" than our advertised API.
peaBerberian
force-pushed
the
misc/more-tests
branch
from
425ffb8 to
111bff2
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
peaBerberian
force-pushed
the
dev
branch
2 times, most recently
from
d4be192 to
9ad6758
Compare
peaBerberian
force-pushed
the
dev
branch
7 times, most recently
from
270a289 to
6bb7e85
Compare
peaBerberian
force-pushed
the
dev
branch
2 times, most recently
from
0142e34 to
1fd9df3
Compare
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.
STATUS: Still adding some tests over time in this branch
This just adds tests for cases that are easy to break: an application performing an action with large side-effects (e.g.
stop(),loadVideo(,reload(),setAudioTrack()etc.) right when receiving an event.For now I just added tests checking that when
stop()is called on any event, the RxPlayer does stop synchronously after that and cancel all that it was doing, including potential future events it would have sent.