From 052e7943b191973776aeab899ac97a21b09dc819 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Fri, 9 Jul 2021 11:12:19 -0700 Subject: [PATCH] fix: Fix failure with multiple thumbnails per period Matching image streams across periods was not working correctly with multiple streams per period. Matching was done on MIME type only, meaning there was no way to differentiate multiple streams. Resolution is a better signal, so this uses the same logic as for video resolution in thumbnail resolution matching. This also adds extra error logs that helped track down the issue, and fixes some comment typos. Issue #3383 Change-Id: I067de59c222a165d58926646f53fa61862853377 --- lib/util/periods.js | 29 +++++++++++++----- test/util/periods_unit.js | 64 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/lib/util/periods.js b/lib/util/periods.js index 2472d1f1d9..812ae09ca1 100644 --- a/lib/util/periods.js +++ b/lib/util/periods.js @@ -525,6 +525,8 @@ shaka.util.PeriodCombiner = class { // Any other unused stream is likely a bug in our algorithm, so throw // an error. + shaka.log.error('Unused stream in period-flattening!', + stream, outputStreams); throw new shaka.util.Error( shaka.util.Error.Severity.CRITICAL, shaka.util.Error.Category.MANIFEST, @@ -563,6 +565,8 @@ shaka.util.PeriodCombiner = class { if (!matches) { // We were unable to extend this output stream. + shaka.log.error('No matches extending output stream!', + outputStream, streamsPerPeriod); return false; } @@ -1066,7 +1070,7 @@ shaka.util.PeriodCombiner = class { return true; } - // Otherwise, compare the streams' characteristics to detecrmine the best + // Otherwise, compare the streams' characteristics to determine the best // match. // The most important thing is language. In some cases, we will accept a @@ -1164,6 +1168,7 @@ shaka.util.PeriodCombiner = class { } } + // If the result of each comparison was inconclusive, default to false. return false; } @@ -1188,7 +1193,7 @@ shaka.util.PeriodCombiner = class { return true; } - // Otherwise, compare the streams' characteristics to detecrmine the best + // Otherwise, compare the streams' characteristics to determine the best // match. // Take the video with the closest resolution to the output. @@ -1234,6 +1239,7 @@ shaka.util.PeriodCombiner = class { } } + // If the result of each comparison was inconclusive, default to false. return false; } @@ -1258,7 +1264,7 @@ shaka.util.PeriodCombiner = class { return true; } - // Otherwise, compare the streams' characteristics to detecrmine the best + // Otherwise, compare the streams' characteristics to determine the best // match. // The most important thing is language. In some cases, we will accept a @@ -1319,6 +1325,7 @@ shaka.util.PeriodCombiner = class { return true; } + // If the result of each comparison was inconclusive, default to false. return false; } @@ -1334,6 +1341,8 @@ shaka.util.PeriodCombiner = class { * @private */ static isImageStreamBetterMatch_(outputStream, best, candidate) { + const {BETTER, EQUAL, WORSE} = shaka.util.PeriodCombiner.BetterOrWorse; + // If the output stream was based on the candidate stream, the candidate // stream should be considered a better match. We can check this by // comparing their ids. @@ -1341,13 +1350,19 @@ shaka.util.PeriodCombiner = class { return true; } - // If the candidate has the same MIME type, upgrade to the - // candidate. It's not required that image streams use the same format - // across periods, but it's a helpful signal. - if (candidate.mimeType == outputStream.mimeType) { + // Take the image with the closest resolution to the output. + const resolutionBetterOrWorse = + shaka.util.PeriodCombiner.compareClosestPreferLower( + outputStream.width * outputStream.height, + best.width * best.height, + candidate.width * candidate.height); + if (resolutionBetterOrWorse == BETTER) { return true; + } else if (resolutionBetterOrWorse == WORSE) { + return false; } + // If the result of each comparison was inconclusive, default to false. return false; } diff --git a/test/util/periods_unit.js b/test/util/periods_unit.js index d53d636e02..a2411d9075 100644 --- a/test/util/periods_unit.js +++ b/test/util/periods_unit.js @@ -429,7 +429,6 @@ describe('PeriodCombiner', () => { expect(highBandwidth.video.originalId).toBe('480'); }); - it('Filters out duplicate streams', async () => { // v1 and v3 are duplicates const v1 = makeVideoStream(1280); @@ -546,6 +545,49 @@ describe('PeriodCombiner', () => { } }); + // Regression test for #3383, where we failed on multi-period content with + // multiple image streams per period. + it('Can handle multiple image streams', async () => { + /** @type {!Array.} */ + const periods = [ + { + id: '1', + videoStreams: [ + makeVideoStream(1280), + ], + audioStreams: [], + textStreams: [], + imageStreams: [ + makeImageStream(240), + makeImageStream(480), + ], + }, + { + id: '2', + videoStreams: [ + makeVideoStream(1280), + ], + audioStreams: [], + textStreams: [], + imageStreams: [ + makeImageStream(240), + makeImageStream(480), + ], + }, + ]; + + await combiner.combinePeriods(periods, /* isDynamic= */ true); + + const imageStreams = combiner.getImageStreams(); + expect(imageStreams.length).toBe(2); + + const imageIds = imageStreams.map((i) => i.originalId); + expect(imageIds).toEqual([ + '240,240', + '480,480', + ]); + }); + it('Text track gaps', async () => { /** @type {!Array.} */ const periods = [ @@ -1181,6 +1223,26 @@ describe('PeriodCombiner', () => { return streamGenerator.build_(); } + /** + * @param {number} height + * @return {shaka.extern.Stream} + * @suppress {accessControls} + */ + function makeImageStream(height) { + const width = height * 4 / 3; + const streamGenerator = new shaka.test.ManifestGenerator.Stream( + /* manifest= */ null, + /* isPartial= */ false, + /* id= */ nextId++, + /* type= */ shaka.util.ManifestParserUtils.ContentType.IMAGE, + /* lang= */ 'und'); + streamGenerator.size(width, height); + streamGenerator.originalId = height.toString(); + streamGenerator.mime('image/jpeg'); + streamGenerator.tilesLayout = '1x1'; + return streamGenerator.build_(); + } + /** * @param {number} height * @param {string} language