From 23c9481ee9d6b1da9c263bb55eb2f535f52807f6 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Thu, 16 Jul 2020 10:28:08 +0100 Subject: [PATCH 1/4] Rename function-level variable for clarity - removes potential for ambiguity between liveDelay variable in startPlaybackCatchUp function and PlaybackController module when reasoning about the code - new name intended to convey that the player's liveDelay value is used throughout this function --- src/streaming/controllers/PlaybackController.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/streaming/controllers/PlaybackController.js b/src/streaming/controllers/PlaybackController.js index 221b31dc4f..1f0f83838f 100644 --- a/src/streaming/controllers/PlaybackController.js +++ b/src/streaming/controllers/PlaybackController.js @@ -629,8 +629,8 @@ function PlaybackController() { function startPlaybackCatchUp() { if (videoModel) { const cpr = settings.get().streaming.liveCatchUpPlaybackRate; - const liveDelay = mediaPlayerModel.getLiveDelay(); - const deltaLatency = getCurrentLiveLatency() - liveDelay; + const playerLiveDelay = mediaPlayerModel.getLiveDelay(); + const deltaLatency = getCurrentLiveLatency() - playerLiveDelay; const d = deltaLatency * 5; // Playback rate must be between (1 - cpr) - (1 + cpr) // ex: if cpr is 0.5, it can have values between 0.5 - 1.5 @@ -641,7 +641,7 @@ function PlaybackController() { // just cause more and more stall situations if (playbackStalled) { const bufferLevel = getBufferLevel(); - if (bufferLevel > liveDelay / 2) { + if (bufferLevel > playerLiveDelay / 2) { playbackStalled = false; } else if (deltaLatency > 0) { newRate = 1.0; From 43e2f43ffbfe30a2e6de24ae130c077e8cc07606 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Thu, 16 Jul 2020 17:24:07 +0100 Subject: [PATCH 2/4] Fix: exclude text processors when text is disabled This fixes a bug observed with low-latency streams containing one or more text representations. The bug prevented the liveCatchUpPlaybackRate from being correctly applied following a buffer stall, leading to unconstrained drift from the user-specified target latency. The bug manifested as follows: 1. a buffer stall event occurs, the 'playbackStalled' field on StreamController is set to 'true' 2. 'StreamController.startPlaybackCatchUp' is called, it uses 'StreamController.getBufferLevel' to obtain the lowest current audio, video text or fragmented text buffer level 3. in streams with a text or fragmented text representation, the recent change to disable text by default resulted in an empty buffer being maintained for that representation _unless_ text had been explicitly enabled for the player 4. the empty buffer prevents 'StreamController.startPlaybackCatchUp' from resetting the 'playbackStalled' field of 'StreamController' to 'false', further, it assumes a buffer stall is ongoing, and resets the 'newRate' to '1.0', preventing latency from being reduced This change then checks whether text has been enabled and only adds the stream processors for textual representations to those returned by the 'Stream.getProcessors' if it is. Tested on Firefox 78.02 & Chrome 84.0.414.79 on Ubuntu 20.04. --- src/streaming/Stream.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/streaming/Stream.js b/src/streaming/Stream.js index 985bafd837..85471ae1b5 100644 --- a/src/streaming/Stream.js +++ b/src/streaming/Stream.js @@ -714,6 +714,8 @@ function Stream(config) { } function getProcessors() { + const isTextEnabled = textController.isTextEnabled(); + let arr = []; let type, @@ -723,7 +725,9 @@ function Stream(config) { streamProcessor = streamProcessors[i]; type = streamProcessor.getType(); - if (type === Constants.AUDIO || type === Constants.VIDEO || type === Constants.FRAGMENTED_TEXT || type === Constants.TEXT) { + if (type === Constants.AUDIO || type === Constants.VIDEO) { + arr.push(streamProcessor); + } else if ((type === Constants.FRAGMENTED_TEXT || type == Constants.TEXT) && isTextEnabled) { arr.push(streamProcessor); } } From 1cb20c6b4cee150a8a651151eee00916e930aab3 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Tue, 21 Jul 2020 11:41:24 +0100 Subject: [PATCH 3/4] Tests: verify behaviour of Stream.getProcessors - existing test verifies that empty array of stream processors exists when stream is not activated - additional tests check that text and fragmented text processors are returned by Stream.getProcessors _only_ when text is enabled via the TextController --- test/unit/helpers/ObjectsHelper.js | 3 +- test/unit/mocks/AbrControllerMock.js | 4 +- test/unit/mocks/AdapterMock.js | 3 +- test/unit/mocks/MediaControllerMock.js | 4 +- test/unit/mocks/TextControllerMock.js | 2 +- test/unit/streaming.Stream.js | 57 ++++++++++++++++++++++++++ 6 files changed, 67 insertions(+), 6 deletions(-) diff --git a/test/unit/helpers/ObjectsHelper.js b/test/unit/helpers/ObjectsHelper.js index 65e31684ea..388c28b578 100644 --- a/test/unit/helpers/ObjectsHelper.js +++ b/test/unit/helpers/ObjectsHelper.js @@ -44,7 +44,8 @@ class ObjectsHelper { calcSegmentAvailabilityRange: () => { return {start: start, end: end};}, isTimeSyncCompleted: () => {return true;}, setExpectedLiveEdge: () => {}, - setRange: (range) => {start = range.start; end = range.end;} + setRange: (range) => {start = range.start; end = range.end;}, + setTimeSyncCompleted: () => {} }; } diff --git a/test/unit/mocks/AbrControllerMock.js b/test/unit/mocks/AbrControllerMock.js index 04df135bf3..eebfce2bb0 100644 --- a/test/unit/mocks/AbrControllerMock.js +++ b/test/unit/mocks/AbrControllerMock.js @@ -43,7 +43,9 @@ function AbrControllerMock () { this.getAbandonmentStateFor = function () {}; - this.getQualityForBitrate = function () {}; + this.getQualityForBitrate = function () { + return this.QUALITY_DEFAULT(); + }; this.getBitrateList = function () { return []; diff --git a/test/unit/mocks/AdapterMock.js b/test/unit/mocks/AdapterMock.js index 2caed88dc4..84151d3a55 100644 --- a/test/unit/mocks/AdapterMock.js +++ b/test/unit/mocks/AdapterMock.js @@ -21,7 +21,8 @@ function AdapterMock () { this.getAllMediaInfoForType = function () { return [{codec: 'audio/mp4;codecs="mp4a.40.2"', id: undefined, index: 0, isText: false, lang: 'eng',mimeType: 'audio/mp4', roles: ['main']}, - {codec: 'audio/mp4;codecs="mp4a.40.2"', id: undefined, index: 1, isText: false, lang: 'deu',mimeType: 'audio/mp4', roles: ['main']}]; + {codec: 'audio/mp4;codecs="mp4a.40.2"', id: undefined, index: 1, isText: false, lang: 'deu',mimeType: 'audio/mp4', roles: ['main']}, + {codec: 'audio/mp4;codecs="mp4a.40.2"', id: undefined, index: 2, isText: true, lang: 'eng',mimeType: 'audio/mp4', roles: ['main']}]; }; this.getDataForMedia = function () { diff --git a/test/unit/mocks/MediaControllerMock.js b/test/unit/mocks/MediaControllerMock.js index f0aa5a97e1..0366224973 100644 --- a/test/unit/mocks/MediaControllerMock.js +++ b/test/unit/mocks/MediaControllerMock.js @@ -22,8 +22,8 @@ class MediaControllerMock { return this.tracks; } - getCurrentTrackFor() { - return this.track; + getCurrentTrackFor(type) { + return this.track === undefined || this.track === null ? { codec: '', type } : this.track; } /** diff --git a/test/unit/mocks/TextControllerMock.js b/test/unit/mocks/TextControllerMock.js index 3a5d567ecc..402cf3f606 100644 --- a/test/unit/mocks/TextControllerMock.js +++ b/test/unit/mocks/TextControllerMock.js @@ -15,7 +15,7 @@ class TextControllerMock { this.textEnabled = state; } getTextDefaultEnabled() { - return true; + return false; } addEmbeddedTrack() {} } diff --git a/test/unit/streaming.Stream.js b/test/unit/streaming.Stream.js index c85010479e..b2e1380526 100644 --- a/test/unit/streaming.Stream.js +++ b/test/unit/streaming.Stream.js @@ -9,6 +9,7 @@ import Errors from '../../src/core/errors/Errors'; import Settings from '../../src/core/Settings'; import AdapterMock from './mocks/AdapterMock'; +import BaseURLControllerMock from './mocks/BaseURLControllerMock'; import ManifestModelMock from './mocks/ManifestModelMock'; import ErrorHandlerMock from './mocks/ErrorHandlerMock'; import AbrControllerMock from './mocks/AbrControllerMock'; @@ -45,6 +46,7 @@ describe('Stream', function () { const dashMetricsMock = new DashMetricsMock(); const textControllerMock = new TextControllerMock(); const videoModelMock = new VideoModelMock(); + const baseURLControllerMock = new BaseURLControllerMock(); const timelineConverter = objectsHelper.getDummyTimelineConverter(); const streamInfo = { id: 'id', @@ -70,6 +72,7 @@ describe('Stream', function () { dashMetrics: dashMetricsMock, textController: textControllerMock, videoModel: videoModelMock, + baseURLController: baseURLControllerMock, settings: settings}); }); @@ -90,6 +93,60 @@ describe('Stream', function () { expect(processors).to.be.empty; // jshint ignore:line }); + it('should return an array that does not include the streamProcessors for text and fragmented text when getProcessors is called and text is disabled', () => { + // test-specific setup + adapterMock.setRepresentation({ + adaptation: { period: { mpd: { manifest: { type: 'bar' } } } }, + hasInitialization: () => false, + hasSegments: () => true, + id: 'foo' + }); + + stream.initialize(streamInfo, {}); + stream.activate( + null, + null + ); + + // check textControllerMock is in correct state + expect(textControllerMock.isTextEnabled()).to.be.false; // jshint ignore:line + + // check assertions + const processors = stream.getProcessors(); + expect(processors.length).to.be.equal(2); // jshint ignore:line + expect(processors[0].getType()).to.be.equal('video'); // jshint ignore:line + expect(processors[1].getType()).to.be.equal('audio'); // jshint ignore:line + }); + + it('should return an array that includes the streamProcessors for text and fragmented text when getProcessors is called and text is enabled', () => { + // test-specific setup + adapterMock.setRepresentation({ + adaptation: { period: { mpd: { manifest: { type: 'bar' } } } }, + hasInitialization: () => false, + hasSegments: () => true, + id: 'foo' + }); + + textControllerMock.enableText(true); + + stream.initialize(streamInfo, {}); + stream.activate( + null, + null + ); + + // check textControllerMock is in correct state + expect(textControllerMock.isTextEnabled()).to.be.true; // jshint ignore:line + + // check assertions + const processors = stream.getProcessors(); + expect(processors.length).to.be.equal(4); // jshint ignore:line + expect(processors[0].getType()).to.be.equal('video'); // jshint ignore:line + expect(processors[1].getType()).to.be.equal('audio'); // jshint ignore:line + expect(processors[2].getType()).to.be.equal('text'); // jshint ignore:line + expect(processors[3].getType()).to.be.equal('fragmentedText'); // jshint ignore:line + }); + it('should trigger MANIFEST_ERROR_ID_NOSTREAMS_CODE error when setMediaSource is called but streamProcessors array is empty', () => { stream.setMediaSource(); expect(errHandlerMock.errorCode).to.be.equal(Errors.MANIFEST_ERROR_ID_NOSTREAMS_CODE); // jshint ignore:line From 8353fbeb1467c34720a00c30e5b3a055a34c7e75 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Wed, 12 Aug 2020 12:35:22 +0100 Subject: [PATCH 4/4] Fix: exclude text buffer level if text is disabled This fix ensure the correct minimum buffer level is reported when text is disabled. Previously a buffer level of 0 is reported at all times if a text track is present in the manifest but text is disabled in the player. This fix checks whether text is enabled and only includes text buffer metrics in buffer level reporting if it is. --- .../metrics/metrics/handlers/BufferLevelHandler.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/streaming/metrics/metrics/handlers/BufferLevelHandler.js b/src/streaming/metrics/metrics/handlers/BufferLevelHandler.js index df06c6db15..de3a940493 100644 --- a/src/streaming/metrics/metrics/handlers/BufferLevelHandler.js +++ b/src/streaming/metrics/metrics/handlers/BufferLevelHandler.js @@ -29,6 +29,7 @@ * POSSIBILITY OF SUCH DAMAGE. */ +import Constants from '../../../constants/Constants'; import HandlerHelpers from '../../utils/HandlerHelpers'; function BufferLevelHandler(config) { @@ -47,6 +48,7 @@ function BufferLevelHandler(config) { let storedVOs = []; const metricsConstants = config.metricsConstants; + const textController = config.textController || {}; function getLowestBufferLevelVO() { try { @@ -93,8 +95,14 @@ function BufferLevelHandler(config) { } function handleNewMetric(metric, vo, type) { + if (metric === metricsConstants.BUFFER_LEVEL) { - storedVOs[type] = vo; + const isTextVo = type === Constants.FRAGMENTED_TEXT || type === Constants.TEXT; + const storeTextVo = isTextVo && (textController.isTextEnabled() || false); + + if (!isTextVo || storeTextVo) { + storedVOs[type] = vo; + } } }