Fix story stretched poster images by removing media pool default sour…

…ces. (#30652) * Fixing stretched poster images by removing default sources. * Reset HTMLMediaElement state, including readyState, when removing the sources.
ampproject · Oct 15, 2020 · c64b8d2 · c64b8d2
1 parent c15a28f
commit c64b8d2
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 132 deletions.
diff --git a/extensions/amp-story/1.0/default-media.js b/extensions/amp-story/1.0/default-media.js
diff --git a/extensions/amp-story/1.0/media-pool.js b/extensions/amp-story/1.0/media-pool.js
@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-import {BLANK_AUDIO_SRC, BLANK_VIDEO_SRC} from './default-media';
 import {
   BlessTask,
   ELEMENT_BLESSED_PROPERTY_NAME,
@@ -35,7 +34,6 @@ import {ampMediaElementFor} from './utils';
 import {dev, devAssert} from '../../../src/log';
 import {findIndex} from '../../../src/utils/array';
 import {isConnectedNode, matches} from '../../../src/dom';
-import {isExperimentOn} from '../../../src/experiments';
 import {toWin} from '../../../src/types';
 import {userInteractedWith} from '../../../src/video-interface';
 
@@ -194,19 +192,6 @@ export class MediaPool {
      */
     this.blessed_ = false;
 
-    /**
-     * The default source to use for pool-created audio sources,
-     * @private @const {!Object<!MediaType, string>}
-     */
-    this.defaultSources_ = {
-      [MediaType.AUDIO]: isExperimentOn(win, 'disable-amp-story-default-media')
-        ? ''
-        : BLANK_AUDIO_SRC,
-      [MediaType.VIDEO]: isExperimentOn(win, 'disable-amp-story-default-media')
-        ? ''
-        : BLANK_VIDEO_SRC,
-    };
-
     /** @private {?Array<!AmpElement>} */
     this.ampElementsToBless_ = null;
 
@@ -275,19 +260,11 @@ export class MediaPool {
           ? mediaElSeed
           : mediaElSeed.cloneNode(/* deep */ true));
         mediaEl.addEventListener('error', this.onMediaError_, {capture: true});
-        const sources = this.getDefaultSource_(type);
         mediaEl.id = POOL_ELEMENT_ID_PREFIX + poolIdCounter++;
         // In Firefox, cloneNode() does not properly copy the muted property
         // that was set in the seed. We need to set it again here.
         mediaEl.muted = true;
         mediaEl[MEDIA_ELEMENT_ORIGIN_PROPERTY_NAME] = MediaElementOrigin.POOL;
-        this.enqueueMediaElementTask_(
-          mediaEl,
-          new UpdateSourcesTask(this.win_, sources)
-        );
-        // TODO(newmuis): Check the 'error' field to see if MEDIA_ERR_DECODE
-        // is returned.  If so, we should adjust the pool size/distribution
-        // between media types.
         this.unallocated[type].push(mediaEl);
       }
     });
@@ -310,18 +287,11 @@ export class MediaPool {
   }
 
   /**
-   * @param {!MediaType} mediaType The media type whose source should be
-   *     retrieved.
-   * @return {!Sources} The default source for the specified type of media.
+   * @return {!Sources} The default source, empty.
+   * @private
    */
-  getDefaultSource_(mediaType) {
-    const sourceStr = this.defaultSources_[mediaType];
-    if (sourceStr === undefined) {
-      dev().error('AMP-STORY', `No default media for type ${mediaType}.`);
-      return new Sources();
-    }
-
-    return new Sources(sourceStr);
+  getDefaultSource_() {
+    return new Sources();
   }
 
   /**
@@ -546,13 +516,14 @@ export class MediaPool {
       new SwapIntoDomTask(placeholderEl)
     ).then(
       () => {
-        this.maybeResetAmpMedia_(ampMediaForPoolEl);
-        this.maybeResetAmpMedia_(ampMediaForDomEl);
         this.enqueueMediaElementTask_(
           poolMediaEl,
           new UpdateSourcesTask(this.win_, sources)
         );
-        this.enqueueMediaElementTask_(poolMediaEl, new LoadTask());
+        this.enqueueMediaElementTask_(poolMediaEl, new LoadTask()).then(() => {
+          this.maybeResetAmpMedia_(ampMediaForPoolEl);
+          this.maybeResetAmpMedia_(ampMediaForDomEl);
+        });
       },
       () => {
         this.forceDeallocateMediaElement_(poolMediaEl);
@@ -588,13 +559,12 @@ export class MediaPool {
    *     has been reset.
    */
   resetPoolMediaElementSource_(poolMediaEl) {
-    const mediaType = this.getMediaType_(poolMediaEl);
-    const defaultSources = this.getDefaultSource_(mediaType);
+    const defaultSources = this.getDefaultSource_();
 
     return this.enqueueMediaElementTask_(
       poolMediaEl,
       new UpdateSourcesTask(this.win_, defaultSources)
-    );
+    ).then(() => this.enqueueMediaElementTask_(poolMediaEl, new LoadTask()));
   }
 
   /**

diff --git a/extensions/amp-story/1.0/media-tasks.js b/extensions/amp-story/1.0/media-tasks.js
@@ -373,6 +373,11 @@ export class UpdateSourcesTask extends MediaTask {
     this.newSources_.applyToElement(this.win_, mediaEl);
     return Promise.resolve();
   }
+
+  /** @override */
+  requiresSynchronousExecution() {
+    return true;
+  }
 }
 
 /**