Fix ordering of a PeriodChange/AdaptationChange couple#1764
Fix ordering of a PeriodChange/AdaptationChange couple#1764peaBerberian wants to merge 1 commit intodevfrom
Conversation
| value.type, | ||
| value.period, | ||
| value.adaptation, | ||
| ); |
There was a problem hiding this comment.
NOTE: I guessed that it was not necessary to check currentLoadCanceller in between the two because sendMessage implies only asynchronous state changes as per our current Core API. Thus a sendMessage call cannot change the world.
If later it turns out that we change the behavior of sendMessage to potentially lead to synchronous change (e.g. if we find out that it improves the perf of a monothreaded scenario), I trust due to the importance of that change that we will think about checking each of its calls and add the check at that point.
|
I'm now wondering if that simple fix is the right way. |
7a7c44f to
1ae7f29
Compare
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.
1ae7f29 to
f9b6ccd
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
d4be192 to
9ad6758
Compare
0142e34 to
1fd9df3
Compare
New integration tests I've added recently (#1763) led me to a curious RxPlayer behavior: for multithreaded contents, we had two initial
textTrackChange(with the payloadnull) in a row instead of just one.This exact behavior does not break the API (so it's not a bug), 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:
PeriodChangemessageAdaptationChangemessage for the last Adaptation set-up (the text one here)PeriodChangemessage and bubble it to the public API module....TrackChangeevents, then send aperiodChangeevent, as expectedAdaptationChangemessage and also bubble it.textTrackChangefor the same text track, because it thinks from theAdaptationChangemessage that the track just changed, after the PeriodChangeReversing the ordering (the last
AdaptationChange, thenPeriodChange) 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 test which was voluntarily less "tolerant" than our advertised API.