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 shaka-project#3383

Change-Id: I067de59c222a165d58926646f53fa61862853377

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,20 +1341,28 @@ 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.
     if (outputStream.id == candidate.id) {
       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;
   }
 

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.<shaka.util.PeriodCombiner.Period>} */
+    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.<shaka.util.PeriodCombiner.Period>} */
     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