refactor: unify sidx/master/error request logic #998
Conversation
| @@ -295,6 +243,111 @@ export default class DashPlaylistLoader extends EventTarget { | |||
| } | |||
| } | |||
|
|
|||
| requestErrored_(err, request, startingState) { | |||
There was a problem hiding this comment.
triggers a request error if it finds an error and returns true if there was an error so request functions can exit early. We used this logic in many places in the original code and we use it in two places now.
| * Verify that the container of the sidx segment can be parsed | ||
| * and if it can, get and parse that segment. | ||
| */ | ||
| addSidxSegments_(playlist, startingState, cb) { |
There was a problem hiding this comment.
All of the logic from sidxRequestFinished and requestSidx merged into one because they were really doing the same things anyway. I also added some extra logic here for things that we were doing in 3+ places outside of this function, but might as well have shared.
| const uri = resolveManifestRedirect(this.handleManifestRedirects, playlist.sidx.resolvedUri); | ||
| const sidxMapping = this.masterPlaylistLoader_.sidxMapping_; | ||
|
|
||
| sidxMapping[sidxKey] = { |
There was a problem hiding this comment.
This is an addition, every function that requests sidx must also update the sidx mapping.
|
|
||
| const sidx = parseSidx(toUint8(request.response).subarray(8)); | ||
|
|
||
| sidxMapping[sidxKey].sidx = sidx; |
There was a problem hiding this comment.
This line and the line below were previously done after the sidx request, but it just didn't make sense to do it everywhere we call this function. We will always want to add sidx segments to the playlist that we request sidx for, and we will always update our sidx mapping cache.
| }); | ||
| }) | ||
| ); | ||
| this.addSidxSegments_(playlist, startingState, (sidxChanged) => { |
There was a problem hiding this comment.
Since this function now calls back for playlists that don't have sidx segments, we don't have to separate the logic.
|
|
||
| this.addSidxSegments_(this.media(), this.state, (sidxChanged) => { |
There was a problem hiding this comment.
We don't have to check this.media() has sidx as addSidxSegments_ will do that now, and the callback will be called.
brandonocasey
force-pushed
the
refactor/3-unify-logic
branch
2 times, most recently
from
4b34420 to
bf6b9a6
Compare
brandonocasey
force-pushed
the
refactor/sidx-add-segments
branch
from
4650469 to
82828d7
Compare
brandonocasey
force-pushed
the
refactor/3-unify-logic
branch
from
66162dc to
9f4f6da
Compare
| // playlist lacks sidx or sidx segments were added to this playlist already. | ||
| if (!playlist.sidx || !sidxKey || this.masterPlaylistLoader_.sidxMapping_[sidxKey]) { | ||
| // keep this function async | ||
| this.mediaRequest_ = window.setTimeout(() => cb(false), 0); |
There was a problem hiding this comment.
I was looking through the code to ensure that we cleared the mediaRequest_ for all callers, and we do, but it's in nested function calls after the callback. I wonder if it would be clearer to either null it before the callback, or to have the callbacks manage nulling mediaRequest_ after receiving the callbacks.
There was a problem hiding this comment.
I honestly wonder why we even need mediaRequest_. It seems to me like we only use it one place in the code to verify that a media change is pending. What if we just set pentingMedia_ in media() and removed mediaRequest?
There was a problem hiding this comment.
I'm all for removing more things. Sounds good to me 👍
There was a problem hiding this comment.
After making the change it causes a lot of tests to fail. Seems like we should do one more refactor pull request for this change to keep the size of the pull request down.
There was a problem hiding this comment.
Was there any way to centralize the mediaRequest_ clearing (rather than removing it) without as many test failures?
There was a problem hiding this comment.
Not really as clearing it out in a central location and switching to pendingMedia_ required similar changes, and had similar test failures.
brandonocasey
force-pushed
the
refactor/3-unify-logic
branch
from
9f4f6da to
23871ac
Compare
brandonocasey
changed the base branch from
refactor/sidx-add-segments
to
main
Description
Third and final dash-playlist-loader refactor.
Based on #994
Specific Changes proposed
requestMaster_addSidxSegments_requestErrored_