feat: Create PlaylistLoader classes that share more code and do less #1208
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1208 +/- ##
==========================================
+ Coverage 86.42% 86.97% +0.54%
==========================================
Files 39 46 +7
Lines 9743 10177 +434
Branches 2263 2373 +110
==========================================
+ Hits 8420 8851 +431
- Misses 1323 1326 +3
Continue to review full report at Codecov.
|
brandonocasey
changed the title
| * | ||
| * @extends PlaylistLoader | ||
| */ | ||
| class DashMainPlaylistLoader extends PlaylistLoader { |
There was a problem hiding this comment.
This class deals with getting the sever clock offset and merging the old main manifest into the new one.
| parseManifest_(manifestString, callback) { | ||
| this.syncClientServerClock_(manifestString, (clientOffset) => { | ||
| const parsedManifest = parseMpd(manifestString, { | ||
| manifestUri: this.uri_, |
There was a problem hiding this comment.
Note that sidxMapping is not done her at all. Instead each individual DashMediaPlaylistLoader handles all of that logic on it's own.
| }); | ||
|
|
||
| // merge everything except for playlists, they will merge themselves | ||
| const mergeResult = mergeManifest(this.manifest_, parsedManifest, ['playlists', 'mediaGroups']); |
There was a problem hiding this comment.
Not also that we do not merge the playlists or mediaGroups here, which means only the new unmerged playlists will exist after this point. We do this because each individual DashMediaPlaylistLoader merges it's own playlist (if the playlist loader is started).
| const mergeResult = mergeManifest(this.manifest_, parsedManifest, ['playlists', 'mediaGroups']); | ||
|
|
||
| // always trigger updated, as playlists will have to update themselves | ||
| callback(mergeResult.manifest, true); |
There was a problem hiding this comment.
We always report updated here as we want DashMediaPlaylistLoaders to be the ones that check for stale playlist updates. The DashMainPlaylistLoader will hardly ever change substantially.
| // MPD has no future validity, so a new one will need to be acquired when new | ||
| // media segments are to be made available. Thus, we use the target duration | ||
| // in this case | ||
| // TODO: can we do this in a better way? It would be much better |
There was a problem hiding this comment.
If anyone has a better idea for this it would make the separation between DashMediaPlaylistLoader and DashMainPlaylistLoader much more complete. It would also prevent the scenario that we currently have where an audio playlist target duration could take precedence over a video playlist.
|
|
||
| // noop, as media playlists in dash do not have | ||
| // a uri to refresh or a manifest string | ||
| refreshManifest_() {} |
There was a problem hiding this comment.
Since dash media playlists don't refresh, parse, or have a manifest string we noop all of these.
| * | ||
| * @extends PlaylistLoader | ||
| */ | ||
| class DashMediaPlaylistLoader extends PlaylistLoader { |
There was a problem hiding this comment.
This class deals:
- finding it's playlist whenever
DashMainPlaylistLoadertriggersupdated - Requesting sidx and adding sidx segments
- Setting the media refresh time on
DashMainPlaylistLoaderso that it can refresh using target duration.
| * @listens {PlaylistLoader#updated} | ||
| * @private | ||
| */ | ||
| refreshManifest_() { |
There was a problem hiding this comment.
The main function of most playlist loaders, called when started or when updated is triggered on itself.
| * @listens {DashMainPlaylistLoader#updated} | ||
| * @private | ||
| */ | ||
| onMainUpdated_() { |
There was a problem hiding this comment.
This is the main function for this file. It finds the playlist this loader is associated with. Adds sidx segments (if needed) and merges with it's reference of it's old playlist.
| * | ||
| * @private | ||
| */ | ||
| makeRequest_(options, callback) { |
There was a problem hiding this comment.
A generic request function with shared error handling. This allows stop to cancel any pending request.
| * @private | ||
| */ | ||
| setMediaRefreshTimeout_() { | ||
| // do nothing if disposed |
There was a problem hiding this comment.
loops media refresh timeout while getMediaRefreshTime is a valid number. called when this playlist loader triggers updated.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
A cleaner playlist loader refactor that is in the works