fix: Fix skip interstitials with another ID but same URL (#7050)

shaka-project · Jul 17, 2024 · 8b70bb6 · 8b70bb6
1 parent fcd87aa
commit 8b70bb6
Show file tree

Hide file tree

Showing 5 changed files with 93 additions and 29 deletions.
diff --git a/externs/shaka/ads.js b/externs/shaka/ads.js
@@ -59,6 +59,7 @@ shaka.extern.AdCuePoint;
 
 /**
  * @typedef {{
+ *   id: ?string,
  *   startTime: number,
  *   endTime: ?number,
  *   uri: string,
@@ -75,6 +76,8 @@ shaka.extern.AdCuePoint;
  * @description
  * Contains the ad interstitial info.
  *
+ * @property {?string} id
+ *   The id of the interstitial.
  * @property {number} startTime
  *   The start time of the interstitial.
  * @property {?number} endTime

diff --git a/lib/ads/interstitial_ad_manager.js b/lib/ads/interstitial_ad_manager.js
@@ -584,6 +584,11 @@ shaka.ads.InterstitialAdManager = class {
     if (!assetUri && !assetList) {
       return interstitialsAd;
     }
+    let id = null;
+    const hlsInterstitialId = hlsInterstitial.values.find((v) => v.key == 'ID');
+    if (hlsInterstitialId) {
+      id = /** @type {string} */(hlsInterstitialId.data);
+    }
     const restrict = hlsInterstitial.values.find((v) => v.key == 'X-RESTRICT');
     let isSkippable = true;
     let canJump = true;
@@ -637,6 +642,7 @@ shaka.ads.InterstitialAdManager = class {
         return interstitialsAd;
       }
       interstitialsAd.push({
+        id,
         startTime: hlsInterstitial.startTime,
         endTime: hlsInterstitial.endTime,
         uri,
@@ -668,6 +674,7 @@ shaka.ads.InterstitialAdManager = class {
         for (const asset of dataAsJson['ASSETS']) {
           if (asset['URI']) {
             interstitialsAd.push({
+              id,
               startTime: hlsInterstitial.startTime,
               endTime: hlsInterstitial.endTime,
               uri: asset['URI'],

diff --git a/lib/hls/hls_parser.js b/lib/hls/hls_parser.js
@@ -3931,7 +3931,6 @@ shaka.hls.HlsParser = class {
       // Exclude these attributes from the metadata since they already go into
       // other fields (eg: startTime or endTime) or are not necessary..
       const excludedAttributes = [
-        'ID',
         'CLASS',
         'START-DATE',
         'END-DATE',
@@ -3961,7 +3960,8 @@ shaka.hls.HlsParser = class {
         values.push(metadataFrame);
       }
 
-      if (values.length) {
+      // ID is always required. So we need more than 1 value.
+      if (values.length > 1) {
         this.playerInterface_.onMetadata(type, startTime, endTime, values);
       }
 

diff --git a/lib/player.js b/lib/player.js
@@ -2123,6 +2123,9 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
           }
         }
         for (const payload of values) {
+          if (payload.name == 'ID') {
+            continue;
+          }
           preloadManager.addQueuedOperation(false, () => {
             this.dispatchMetadataEvent_(
                 startTime, endTime, metadataType, payload);

diff --git a/test/hls/hls_parser_unit.js b/test/hls/hls_parser_unit.js
@@ -5603,26 +5603,59 @@ describe('HlsParser', () => {
       await parser.start('test:/master', playerInterface);
 
       const metadataType = 'com.apple.quicktime.HLS';
-      const value = {
-        key: 'X-SHAKA',
-        data: 'FOREVER',
-      };
-      const plannedDuration = {
-        key: 'PLANNED-DURATION',
-        data: '1',
-      };
+      const firstValues = [
+        jasmine.objectContaining({
+          key: 'ID',
+          data: '0',
+        }),
+        jasmine.objectContaining({
+          key: 'X-SHAKA',
+          data: 'FOREVER',
+        }),
+      ];
+      const secondValues = [
+        jasmine.objectContaining({
+          key: 'ID',
+          data: '1',
+        }),
+        jasmine.objectContaining({
+          key: 'X-SHAKA',
+          data: 'FOREVER',
+        }),
+      ];
+      const thirdValues = [
+        jasmine.objectContaining({
+          key: 'ID',
+          data: '2',
+        }),
+        jasmine.objectContaining({
+          key: 'PLANNED-DURATION',
+          data: '1',
+        }),
+        jasmine.objectContaining({
+          key: 'X-SHAKA',
+          data: 'FOREVER',
+        }),
+      ];
+      const forthValues = [
+        jasmine.objectContaining({
+          key: 'ID',
+          data: '3',
+        }),
+        jasmine.objectContaining({
+          key: 'X-SHAKA',
+          data: 'FOREVER',
+        }),
+      ];
       expect(onMetadataSpy).toHaveBeenCalledTimes(4);
       expect(onMetadataSpy).toHaveBeenCalledWith(metadataType, 0, 1,
-          [jasmine.objectContaining(value)]);
+          firstValues);
       expect(onMetadataSpy).toHaveBeenCalledWith(metadataType, 5, 6,
-          [jasmine.objectContaining(value)]);
+          secondValues);
       expect(onMetadataSpy).toHaveBeenCalledWith(metadataType, 10, 11,
-          [
-            jasmine.objectContaining(plannedDuration),
-            jasmine.objectContaining(value),
-          ]);
+          thirdValues);
       expect(onMetadataSpy).toHaveBeenCalledWith(metadataType, 15, null,
-          [jasmine.objectContaining(value)]);
+          forthValues);
     });
 
     it('supports END-ON-NEXT', async () => {
@@ -5645,13 +5678,18 @@ describe('HlsParser', () => {
       await parser.start('test:/master', playerInterface);
 
       const metadataType = 'com.apple.quicktime.HLS';
-      const value = {
-        key: 'X-SHAKA',
-        data: 'FOREVER',
-      };
+      const values = [
+        jasmine.objectContaining({
+          key: 'ID',
+          data: '0',
+        }),
+        jasmine.objectContaining({
+          key: 'X-SHAKA',
+          data: 'FOREVER',
+        }),
+      ];
       expect(onMetadataSpy).toHaveBeenCalledTimes(1);
-      expect(onMetadataSpy).toHaveBeenCalledWith(metadataType, 0, 5,
-          [jasmine.objectContaining(value)]);
+      expect(onMetadataSpy).toHaveBeenCalledWith(metadataType, 0, 5, values);
     });
 
     it('skip duplicate IDs', async () => {
@@ -5676,13 +5714,18 @@ describe('HlsParser', () => {
       await parser.start('test:/master', playerInterface);
 
       const metadataType = 'com.apple.quicktime.HLS';
-      const value = {
-        key: 'X-SHAKA',
-        data: 'FOREVER',
-      };
+      const values = [
+        jasmine.objectContaining({
+          key: 'ID',
+          data: '0',
+        }),
+        jasmine.objectContaining({
+          key: 'X-SHAKA',
+          data: 'FOREVER',
+        }),
+      ];
       expect(onMetadataSpy).toHaveBeenCalledTimes(1);
-      expect(onMetadataSpy).toHaveBeenCalledWith(metadataType, 0, 1,
-          [jasmine.objectContaining(value)]);
+      expect(onMetadataSpy).toHaveBeenCalledWith(metadataType, 0, 1, values);
     });
 
     it('with no EXT-X-PROGRAM-DATE-TIME', async () => {
@@ -5767,6 +5810,10 @@ describe('HlsParser', () => {
 
       const metadataType = 'com.apple.hls.interstitial';
       const values = [
+        jasmine.objectContaining({
+          key: 'ID',
+          data: '1',
+        }),
         jasmine.objectContaining({
           key: 'X-ASSET-URI',
           data: 'test:/fake',
@@ -5809,6 +5856,10 @@ describe('HlsParser', () => {
 
       const metadataType = 'com.apple.hls.interstitial';
       const values = [
+        jasmine.objectContaining({
+          key: 'ID',
+          data: '1',
+        }),
         jasmine.objectContaining({
           key: 'X-ASSET-LIST',
           data: 'test:/fake',