refactor: Add a better distinction between master and child dash loaders #992
Conversation
This file contains 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
brandonocasey
force-pushed
the
refactor/distinctive-master
branch
from
1cdbf5f to
1ece93f
Compare
gkatsev
reviewed
Nov 17, 2020
Comment on lines
-280
to
+285
| if (typeof srcUrlOrPlaylist === 'string') { | ||
| if (this.isMaster_) { |
There was a problem hiding this comment.
should we verify that srcUrlOrPlaylist is a string or is it basically guaranteed to be one if we're a master manifest?
There was a problem hiding this comment.
For master it will always be a string, for others it can be a playlist or string. So yes it would seem so.
gesinger
reviewed
Nov 18, 2020
| @@ -528,7 +518,7 @@ export default class DashPlaylistLoader extends EventTarget { | |||
|
|
|||
| // request the specified URL | |||
| this.request = this.vhs_.xhr({ | |||
| uri: this.srcUrl, | |||
| uri: this.masterPlaylistLoader_.srcUrl, | |||
There was a problem hiding this comment.
Since we return early above for non master loaders, would it be clearer to use just this?
There was a problem hiding this comment.
I think we want it to be like this because:
- The distinction will help us migrate master playlist loader code into a new file after these refactors.
- It's much easier to know that you are accessing the correct object if you always access it the same way.
| this.srcUrl = srcUrlOrPlaylist; | ||
| // TODO: reset sidxMapping between period changes | ||
| // once multi-period is refactored | ||
| this.sidxMapping_ = {}; | ||
| return; | ||
| this.masterPlaylistLoader_.sidxMapping_ = {}; |
There was a problem hiding this comment.
Since we have the conditional above for master loaders, would it be clearer to use just this?
Comment on lines
+626
to
+629
| masterXml: this.masterPlaylistLoader_.masterXml_, | ||
| srcUrl: this.masterPlaylistLoader_.srcUrl, | ||
| clientOffset: this.masterPlaylistLoader_.clientOffset_, | ||
| sidxMapping: this.masterPlaylistLoader_.sidxMapping_ |
Co-authored-by: Garrett Singer <[email protected]>
gesinger
approved these changes
Nov 18, 2020
evanfarina
pushed a commit
to evanfarina/http-streaming
that referenced
this pull request
Nov 26, 2020
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.
Description
I am working on refactoring the parsing logic in dash-playlist-loader. This is difficult because the logic is almost impossible to follow in the current state of the code. I tried a grand over-arching refactor and ultimately wasn't happy with it. Instead I am going to opt for smaller changes to get us towards what we want.
Steps
this.masterPlaylistLoadertothisfor master so that operations that update master always happen onthis.masterPlaylistLoader. This simplifies a lot of the code and makes it much easier to understand.filterChangedSidxMappingsuse this logic rather than parsing on it's own.