From e49ff5c496c8ae90b2f1b77017f471ffe340c077 Mon Sep 17 00:00:00 2001 From: Rob Walch Date: Sat, 18 Feb 2023 12:03:50 -0800 Subject: [PATCH] Do not reload level after calling stopLoad() and startLoad() Warn on currentFrag context reference change and use currentFrag to maintain correct stats ref Fixes #5230 --- api-extractor/report/hls.js.api.md | 2 +- src/controller/audio-track-controller.ts | 2 +- src/controller/base-playlist-controller.ts | 15 ++++-- src/controller/base-stream-controller.ts | 8 +++- src/controller/level-controller.ts | 2 +- src/controller/subtitle-track-controller.ts | 2 +- .../unit/controller/audio-track-controller.ts | 47 ++++++++++--------- 7 files changed, 44 insertions(+), 34 deletions(-) diff --git a/api-extractor/report/hls.js.api.md b/api-extractor/report/hls.js.api.md index 91fd4302827..7688af69b51 100644 --- a/api-extractor/report/hls.js.api.md +++ b/api-extractor/report/hls.js.api.md @@ -234,7 +234,7 @@ export class BasePlaylistController implements NetworkComponentAPI { // (undocumented) protected retryCount: number; // (undocumented) - protected shouldLoadTrack(track: MediaPlaylist): boolean; + protected shouldLoadPlaylist(playlist: Level | MediaPlaylist): boolean; // (undocumented) startLoad(): void; // (undocumented) diff --git a/src/controller/audio-track-controller.ts b/src/controller/audio-track-controller.ts index ee3682e39a8..20ff2cfd22f 100644 --- a/src/controller/audio-track-controller.ts +++ b/src/controller/audio-track-controller.ts @@ -252,7 +252,7 @@ class AudioTrackController extends BasePlaylistController { protected loadPlaylist(hlsUrlParameters?: HlsUrlParameters): void { super.loadPlaylist(); const audioTrack = this.tracksInGroup[this.trackId]; - if (this.shouldLoadTrack(audioTrack)) { + if (this.shouldLoadPlaylist(audioTrack)) { const id = audioTrack.id; const groupId = audioTrack.groupId as string; let url = audioTrack.url; diff --git a/src/controller/base-playlist-controller.ts b/src/controller/base-playlist-controller.ts index 7b5589c903f..681810b2818 100644 --- a/src/controller/base-playlist-controller.ts +++ b/src/controller/base-playlist-controller.ts @@ -1,6 +1,11 @@ import type Hls from '../hls'; import type { NetworkComponentAPI } from '../types/component-api'; -import { getSkipValue, HlsSkip, HlsUrlParameters } from '../types/level'; +import { + getSkipValue, + HlsSkip, + HlsUrlParameters, + Level, +} from '../types/level'; import { computeReloadInterval, mergeDetails } from './level-helper'; import { logger } from '../utils/logger'; import type { LevelDetails } from '../loader/level-details'; @@ -107,12 +112,12 @@ export default class BasePlaylistController implements NetworkComponentAPI { // Loading is handled by the subclasses } - protected shouldLoadTrack(track: MediaPlaylist): boolean { + protected shouldLoadPlaylist(playlist: Level | MediaPlaylist): boolean { return ( this.canLoad && - track && - !!track.url && - (!track.details || track.details.live) + playlist && + !!playlist.url && + (!playlist.details || playlist.details.live) ); } diff --git a/src/controller/base-stream-controller.ts b/src/controller/base-stream-controller.ts index 91700690cb0..fb2e0f55862 100644 --- a/src/controller/base-stream-controller.ts +++ b/src/controller/base-stream-controller.ts @@ -807,7 +807,7 @@ export default class BaseStreamController protected getCurrentContext( chunkMeta: ChunkMetadata ): { frag: Fragment; part: Part | null; level: Level } | null { - const { levels } = this; + const { levels, fragCurrent } = this; const { level: levelIndex, sn, part: partIndex } = chunkMeta; if (!levels || !levels[levelIndex]) { this.warn( @@ -819,10 +819,14 @@ export default class BaseStreamController const part = partIndex > -1 ? getPartWith(level, sn, partIndex) : null; const frag = part ? part.fragment - : getFragmentWithSN(level, sn, this.fragCurrent); + : getFragmentWithSN(level, sn, fragCurrent); if (!frag) { return null; } + if (fragCurrent && fragCurrent !== frag) { + logger.warn(`Expected current context to match fragCurrent`); + return { frag: fragCurrent, part, level }; + } return { frag, part, level }; } diff --git a/src/controller/level-controller.ts b/src/controller/level-controller.ts index 01ff6d316d4..1bdc052c183 100644 --- a/src/controller/level-controller.ts +++ b/src/controller/level-controller.ts @@ -512,7 +512,7 @@ export default class LevelController extends BasePlaylistController { const currentLevelIndex = this.currentLevelIndex; const currentLevel = this.currentLevel; - if (this.canLoad && currentLevel && currentLevel.url.length > 0) { + if (currentLevel && this.shouldLoadPlaylist(currentLevel)) { const id = currentLevel.urlId; let url = currentLevel.uri; if (hlsUrlParameters) { diff --git a/src/controller/subtitle-track-controller.ts b/src/controller/subtitle-track-controller.ts index 4faeb9b5048..458d9999289 100644 --- a/src/controller/subtitle-track-controller.ts +++ b/src/controller/subtitle-track-controller.ts @@ -276,7 +276,7 @@ class SubtitleTrackController extends BasePlaylistController { protected loadPlaylist(hlsUrlParameters?: HlsUrlParameters): void { super.loadPlaylist(); const currentTrack = this.tracksInGroup[this.trackId]; - if (this.shouldLoadTrack(currentTrack)) { + if (this.shouldLoadPlaylist(currentTrack)) { const id = currentTrack.id; const groupId = currentTrack.groupId as string; let url = currentTrack.url; diff --git a/tests/unit/controller/audio-track-controller.ts b/tests/unit/controller/audio-track-controller.ts index a2376634a0e..45a5e887d41 100644 --- a/tests/unit/controller/audio-track-controller.ts +++ b/tests/unit/controller/audio-track-controller.ts @@ -43,7 +43,7 @@ type AudioTrackControllerTestable = Omit< | 'groupId' | 'trackId' | 'canLoad' - | 'shouldLoadTrack' + | 'shouldLoadPlaylist' | 'timer' | 'onManifestLoading' | 'onManifestParsed' @@ -57,7 +57,7 @@ type AudioTrackControllerTestable = Omit< trackId: number; canLoad: boolean; timer: number; - shouldLoadTrack: (track: Object) => boolean; + shouldLoadPlaylist: (track: Object) => boolean; onManifestLoading: () => void; onManifestParsed: ( type: string, @@ -284,19 +284,20 @@ describe('AudioTrackController', function () { .to.have.been.calledThrice; }); - describe('shouldLoadTrack', function () { + describe('shouldLoadPlaylist', function () { it('should not need loading because the audioTrack is embedded in the main playlist', function () { audioTrackController.canLoad = true; - expect(audioTrackController.shouldLoadTrack({ details: { live: true } })) - .to.be.false; - expect(audioTrackController.shouldLoadTrack({ details: undefined })).to.be - .false; + expect( + audioTrackController.shouldLoadPlaylist({ details: { live: true } }) + ).to.be.false; + expect(audioTrackController.shouldLoadPlaylist({ details: undefined })).to + .be.false; }); it('should need loading because the track has not been loaded yet', function () { audioTrackController.canLoad = true; expect( - audioTrackController.shouldLoadTrack({ + audioTrackController.shouldLoadPlaylist({ details: { live: true }, url: 'http://example.com/manifest.m3u8', }), @@ -304,7 +305,7 @@ describe('AudioTrackController', function () { ).to.be.true; expect( - audioTrackController.shouldLoadTrack({ + audioTrackController.shouldLoadPlaylist({ details: null, url: 'http://example.com/manifest.m3u8', }), @@ -349,9 +350,9 @@ describe('AudioTrackController', function () { }); it('should load audio tracks with a url', function () { - const shouldLoadTrack = sinon.spy( + const shouldLoadPlaylist = sinon.spy( audioTrackController, - 'shouldLoadTrack' + 'shouldLoadPlaylist' ); const audioTrackLoadingCallback = sinon.spy(); const trackWithUrl: MediaPlaylist = { @@ -385,24 +386,24 @@ describe('AudioTrackController', function () { }); audioTrackController.startLoad(); - expect(shouldLoadTrack).to.have.been.calledTwice; - expect(shouldLoadTrack).to.have.been.calledWith(trackWithUrl); + expect(shouldLoadPlaylist).to.have.been.calledTwice; + expect(shouldLoadPlaylist).to.have.been.calledWith(trackWithUrl); expect( - shouldLoadTrack.firstCall.returnValue, - 'expected shouldLoadTrack to return false before startLoad() is called' + shouldLoadPlaylist.firstCall.returnValue, + 'expected shouldLoadPlaylist to return false before startLoad() is called' ).to.be.false; expect( - shouldLoadTrack.secondCall.returnValue, - 'expected shouldLoadTrack to return true after startLoad() is called' + shouldLoadPlaylist.secondCall.returnValue, + 'expected shouldLoadPlaylist to return true after startLoad() is called' ).to.be.true; expect(audioTrackLoadingCallback).to.have.been.calledOnce; }); it('should not attempt to load audio tracks without a url', function () { - const shouldLoadTrack = sinon.spy( + const shouldLoadPlaylist = sinon.spy( audioTrackController, - 'shouldLoadTrack' + 'shouldLoadPlaylist' ); const audioTrackLoadingCallback = sinon.spy(); const trackWithOutUrl = tracks[0]; @@ -425,10 +426,10 @@ describe('AudioTrackController', function () { }); audioTrackController.startLoad(); - expect(shouldLoadTrack).to.have.been.calledTwice; - expect(shouldLoadTrack).to.have.been.calledWith(trackWithOutUrl); - expect(shouldLoadTrack.firstCall.returnValue).to.be.false; - expect(shouldLoadTrack.secondCall.returnValue).to.be.false; + expect(shouldLoadPlaylist).to.have.been.calledTwice; + expect(shouldLoadPlaylist).to.have.been.calledWith(trackWithOutUrl); + expect(shouldLoadPlaylist.firstCall.returnValue).to.be.false; + expect(shouldLoadPlaylist.secondCall.returnValue).to.be.false; expect(audioTrackLoadingCallback).to.not.have.been.called; }); });