Fix async tracks callback

PR #2387 introduced a bug. It awaited the track selection callback,
but that was done in a loop, the results of which were not awaited.
This would cause storage to continue before the complete list of
tracks was available.

Closes #2383

Change-Id: I18a429cf0f40b829020c520c622ffdae8b12622e

Author: joeyparrish

lib/offline/storage.js

@@ -356,7 +356,7 @@ shaka.offline.Storage = class {
         throw drmError;
       }
 
-      this.filterManifest_(manifest, drmEngine);
+      await this.filterManifest_(manifest, drmEngine);
 
       await muxer.init();
       this.ensureNotDestroyed_();
@@ -413,9 +413,10 @@ shaka.offline.Storage = class {
    *
    * @param {shaka.extern.Manifest} manifest
    * @param {!shaka.media.DrmEngine} drmEngine
+   * @return {!Promise}
    * @private
    */
-  filterManifest_(manifest, drmEngine) {
+  async filterManifest_(manifest, drmEngine) {
     // Filter the manifest based on the restrictions given in the player
     // configuration.
     const maxHwRes = {width: Infinity, height: Infinity};
@@ -437,7 +438,7 @@ shaka.offline.Storage = class {
     // Filter each variant based on what the app says they want to store. The
     // app will only be given variants that are compatible with all previous
     // post-filtered periods.
-    shaka.util.ManifestFilter.rollingFilter(manifest, async (period) => {
+    await shaka.util.ManifestFilter.rollingFilter(manifest, async (period) => {
       const StreamUtils = shaka.util.StreamUtils;
       const allTracks = [];
 

lib/util/manifest_filter.js

@@ -118,11 +118,10 @@ shaka.util.ManifestFilter = class {
    * are compatible with at least one variant in the previous period.
    *
    * @param {shaka.extern.Manifest} manifest
-   * @param {function(shaka.extern.Period)} filter
+   * @param {function(shaka.extern.Period):!Promise} filter
+   * @return {!Promise}
    */
-  static rollingFilter(manifest, filter) {
-    const ManifestFilter = shaka.util.ManifestFilter;
-
+  static async rollingFilter(manifest, filter) {
     // Store a reference to the variants so that the next period can easily
     // reference them too.
     /** @type {shaka.util.ManifestFilter.VariantCodecSummarySet} */
@@ -135,17 +134,19 @@ shaka.util.ManifestFilter = class {
       // path across all periods.
       if (previous) {
         period.variants = period.variants.filter((variant) => {
-          const summary = new ManifestFilter.VariantCodecSummary(variant);
+          const summary =
+              new shaka.util.ManifestFilter.VariantCodecSummary(variant);
           return previous.contains(summary);
         });
       }
 
-      filter(period);
+      // eslint-disable-next-line no-await-in-loop
+      await filter(period);
 
       // Use the results of filtering this period as the "previous" for the
       // next period.
-      previous =
-          ManifestFilter.VariantCodecSummarySet.fromVariants(period.variants);
+      previous = shaka.util.ManifestFilter.VariantCodecSummarySet.fromVariants(
+          period.variants);
     }
   }
 };

test/offline/storage_integration.js

@@ -724,32 +724,29 @@ filterDescribe('Storage', storageSupport, () => {
 
     it('only stores chosen tracks', async () => {
       // Change storage to only store one track so that it will be easy
-      // for use to ensure that only the one track was stored.
-      const selectTrack = (tracks) => {
+      // for us to ensure that only the one track was stored.
+      const selectTracks = (tracks) => {
         const selected = tracks.filter((t) => t.language == frenchCanadian);
         expect(selected.length).toBe(1);
         return selected;
       };
       storage.configure({
         offline: {
-          trackSelectionCallback: selectTrack,
+          trackSelectionCallback: selectTracks,
         },
       });
 
       // Stored content should reflect the tracks in the first period, so we
       // should only find track there.
       const stored = await storage.store(
           manifestWithPerStreamBandwidthUri, noMetadata, FakeManifestParser);
-      expect(stored).toBeTruthy();
-      expect(stored.tracks).toBeTruthy();
       expect(stored.tracks.length).toBe(1);
       expect(stored.tracks[0].language).toBe(frenchCanadian);
 
       // Pull the manifest out of storage so that we can ensure that it only
       // has one variant.
       /** @type {shaka.offline.OfflineUri} */
       const uri = shaka.offline.OfflineUri.parse(stored.offlineUri);
-      expect(uri).toBeTruthy();
 
       /** @type {!shaka.offline.StorageMuxer} */
       const muxer = new shaka.offline.StorageMuxer();
@@ -758,28 +755,40 @@ filterDescribe('Storage', storageSupport, () => {
         await muxer.init();
         const cell = await muxer.getCell(uri.mechanism(), uri.cell());
         const manifests = await cell.getManifests([uri.key()]);
-        expect(manifests).toBeTruthy();
         expect(manifests.length).toBe(1);
 
         const manifest = manifests[0];
-        expect(manifest).toBeTruthy();
-        expect(manifest.periods).toBeTruthy();
         expect(manifest.periods.length).toBe(1);
 
         const period = manifest.periods[0];
-        expect(period).toBeTruthy();
-        expect(period.streams).toBeTruthy();
         // There should be 2 streams, an audio and a video stream.
         expect(period.streams.length).toBe(2);
 
         const audio = period.streams.filter((s) => s.contentType == 'audio')[0];
-        expect(audio).toBeTruthy();
         expect(audio.language).toBe(frenchCanadian);
       } finally {
         await muxer.destroy();
       }
     });
 
+    it('can choose tracks asynchronously', async () => {
+      storage.configure({
+        offline: {
+          trackSelectionCallback: async (tracks) => {
+            await Util.delay(0.1);
+            const selected = tracks.filter((t) => t.language == frenchCanadian);
+            expect(selected.length).toBe(1);
+            return selected;
+          },
+        },
+      });
+
+      const stored = await storage.store(
+          manifestWithPerStreamBandwidthUri, noMetadata, FakeManifestParser);
+      expect(stored.tracks.length).toBe(1);
+      expect(stored.tracks[0].language).toBe(frenchCanadian);
+    });
+
     it('stores drm info without license', async () => {
       const drmInfo = makeDrmInfo();
       const session1 = 'session-1';