Skip to content

Commit

Permalink
Do not reload level after calling stopLoad() and startLoad()
Browse files Browse the repository at this point in the history
Warn on currentFrag context reference change and use currentFrag to maintain correct stats ref
Fixes #5230
  • Loading branch information
robwalch committed Feb 22, 2023
1 parent 4e62f3e commit 9bb6060
Show file tree
Hide file tree
Showing 7 changed files with 39 additions and 34 deletions.
2 changes: 1 addition & 1 deletion api-extractor/report/hls.js.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/controller/audio-track-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 5 additions & 5 deletions src/controller/base-playlist-controller.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
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';
Expand Down Expand Up @@ -107,12 +107,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)
);
}

Expand Down
8 changes: 6 additions & 2 deletions src/controller/base-stream-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,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?.[levelIndex]) {
this.warn(
Expand All @@ -820,10 +820,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 };
}

Expand Down
2 changes: 1 addition & 1 deletion src/controller/level-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/controller/subtitle-track-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
47 changes: 24 additions & 23 deletions tests/unit/controller/audio-track-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type AudioTrackControllerTestable = Omit<
| 'groupId'
| 'trackId'
| 'canLoad'
| 'shouldLoadTrack'
| 'shouldLoadPlaylist'
| 'timer'
| 'onManifestLoading'
| 'onManifestParsed'
Expand All @@ -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,
Expand Down Expand Up @@ -284,27 +284,28 @@ 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',
}),
'track 1'
).to.be.true;

expect(
audioTrackController.shouldLoadTrack({
audioTrackController.shouldLoadPlaylist({
details: null,
url: 'http://example.com/manifest.m3u8',
}),
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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];
Expand All @@ -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;
});
});
Expand Down

0 comments on commit 9bb6060

Please sign in to comment.