✨ [Story video] Run sortSources/load only if hasLowerBitrate (#33065)

* Hide sortSources behind hasLowerBitrate * Fix call * Added test * Run flexible-bitrate * Updated codestyle for comments * Added test for not loading * Linted
ampproject · Mar 25, 2021 · 74f2878 · 74f2878
1 parent 038cd10
commit 74f2878
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 11 deletions.
diff --git a/examples/amp-story/videos-cdn.html b/examples/amp-story/videos-cdn.html
@@ -5,7 +5,7 @@
   <link rel="canonical" href="ampconf.html">
   <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
   <script async src="https://cdn.ampproject.org/v0.js"></script>
-  <title>Example story with 10 videos</title>
+  <title>10 videos with flexible bitrate</title>
 
   <script async custom-element="amp-story" src="https://cdn.ampproject.org/v0/amp-story-1.0.js"></script>
   <script async custom-element="amp-video" src="https://cdn.ampproject.org/v0/amp-video-0.1.js"></script>
@@ -24,6 +24,11 @@
 		border-radius: 50%;
 	}
 	</style>
+  <script>
+    (self.AMP = self.AMP || []).push(function(AMP) {
+      AMP.toggleExperiment('flexible-bitrate', true);
+    });
+</script>
 </head>
   <body>
     <amp-story standalone id="cats"

diff --git a/extensions/amp-video/0.1/flexible-bitrate.js b/extensions/amp-video/0.1/flexible-bitrate.js
@@ -104,7 +104,11 @@ export class BitrateManager {
     }
     onNontrivialWait(video, () => {
       const current = currentSource(video);
-      this.acceptableBitrate_ = current.bitrate_ - 1;
+      const newBitrate = current.bitrate_ - 1;
+      if (newBitrate >= this.acceptableBitrate_) {
+        return;
+      }
+      this.acceptableBitrate_ = newBitrate;
       this.switchToLowerBitrate_(video, current.bitrate_);
       this.updateOtherManagedAndPausedVideos_();
     });
@@ -150,7 +154,9 @@ export class BitrateManager {
   /**
    * Sorts the sources of the given video element by their bitrates such that
    * the sources closest matching the acceptable bitrate are in front.
+   * Returns true if the sorting changed the order of sources.
    * @param {!Element} video
+   * @return {boolean}
    */
   sortSources_(video) {
     const sources = toArray(childElementsByTag(video, 'source'));
@@ -164,15 +170,23 @@ export class BitrateManager {
         ? parseInt(bitrate, 10)
         : Number.POSITIVE_INFINITY;
     });
+    let hasChanges = false;
     sources.sort((a, b) => {
       // Biggest first, bitrates above threshold to the back
-      return (
-        this.getBitrateForComparison_(b) - this.getBitrateForComparison_(a)
-      );
-    });
-    sources.forEach((source) => {
-      video.appendChild(source);
+      const value =
+        this.getBitrateForComparison_(b) - this.getBitrateForComparison_(a);
+      if (value < 0) {
+        hasChanges = true;
+      }
+      return value;
     });
+
+    if (hasChanges) {
+      sources.forEach((source) => {
+        video.appendChild(source);
+      });
+    }
+    return hasChanges;
   }
 
   /**
@@ -221,7 +235,11 @@ export class BitrateManager {
     }
     const {currentTime} = video;
     video.pause();
-    this.sortSources_(video);
+    const hasChanges = this.sortSources_(video);
+    if (!hasChanges) {
+      video.play();
+      return;
+    }
     video.load();
     listenOnce(video, 'loadedmetadata', () => {
       // Restore currentTime after loading new source.
@@ -251,8 +269,10 @@ export class BitrateManager {
       ) {
         return;
       }
-      this.sortSources_(video);
-      video.load();
+      const hasChanges = this.sortSources_(video);
+      if (hasChanges) {
+        video.load();
+      }
     }
   }
 }

diff --git a/extensions/amp-video/0.1/test/test-flexible-bitrate.js b/extensions/amp-video/0.1/test/test-flexible-bitrate.js
@@ -226,6 +226,26 @@ describes.fakeWin('amp-video flexible-bitrate', {}, (env) => {
         })
       ).to.jsonEqual([null, null, null, null, null, null, 'CACHE', null, null]);
     });
+
+    it("should sort sources only when it's not already sorted", () => {
+      const m = getManager('4g');
+      const v0 = getVideo([4000, 1000, 3000, 2000]);
+      m.manage(v0);
+
+      // Sorting once should work, but second time it should be a noop.
+      expect(m.sortSources_(v0)).to.be.true;
+      expect(m.sortSources_(v0)).to.be.false;
+    });
+
+    it('should not call load if there are no lower bitrates', () => {
+      const m = getManager('4g');
+      const v0 = getVideo([4000, 1000, 3000, 2000]);
+      m.manage(v0);
+      m.sortSources_(v0);
+      v0.load = env.sandbox.spy();
+      m.switchToLowerBitrate_(v0, m.acceptableBitrate_);
+      expect(v0.load).to.not.have.been.called;
+    });
   });
 
   function currentBitrates(video) {