Create task queue to synchronize function calls to media pool elements #13259

newmuis · 2018-02-04T09:07:25Z

Fixes #13232. Also includes changes from #13245.

This PR creates a task queue on each media element in the MediaPool, implemented as an array of strings. The array stores the next functions to be invoked for that element (e.g. `['bless', 'load', 'play', 'unmute']), and ensures that they are each only called after the previous one has finished executing.

This is necessary due to the async nature of the amp-story code, as well as the async nature of the media element's play() function. In some cases, play() is invoked but then other code is executed before any callbacks on play()'s promise chain are invoked. In our case, the "other code" that gets executed is sometimes also media-related code, calling load() or play() again, both of which can cause the media in question to cease playback.

See #13232 for more details on the issue.

newmuis · 2018-02-05T13:47:00Z

The play() promise (at least the one used for the bless task) seems to hang indefinitely in a pending state sometimes, on iOS. I'm not sure what is causing this, as it meets all of the criteria stated in the WebKit .

I will continue investigating the cause there. Additionally, this seems to make the overall navigation slower, as these operations are all blocking on one another. I can't think of a good way to feature detect whether the browser can handle allowing these to be completely async as it was before, but I will also at least try using vsync in the cases where we swap elements in/out of the DOM to ensure that the swap is done within a single frame.

gmajoulet

Great! Do you think we could improve this queue, later, to bless several elements in parallel? From what I understand they're blessed one by one here

gmajoulet · 2018-02-05T17:19:41Z

extensions/amp-story/0.1/media-pool.js

+  blessPromise.catch(reason => {
+    dev().expectedError('AMP-STORY', 'Blessing media element failed:',
+        reason, mediaEl);
+  }).then(() => dispatch(mediaEl, 'bless', false));


Might be worth adding a comment to mention this line works as a finally and is triggered on both success or failure of the promise

refactored this

gmajoulet · 2018-02-05T17:23:09Z

extensions/amp-story/0.1/media-pool.js

+/**
+ * @const {!ElementTaskDef}
+ */
+const NOOP_ELEMENT_TASK = () => Promise.resolve();


Should this method store the expected taskName and log an error message saying this task does not exist?

refactored this

gmajoulet · 2018-02-05T17:25:06Z

extensions/amp-story/0.1/media-pool.js

+
+  task(mediaEl)
+      .catch(reason => dev().error('AMP-STORY', reason))
+      .then(() => {


Cool pattern, but unless it's widely used, I think it's worth adding a // Finally comment

gmajoulet · 2018-02-05T17:29:32Z

extensions/amp-story/0.1/media-pool.js

-      dev().expectedError('AMP-STORY', 'Blessing media element failed:',
-          reason);
-    });
+   * @param {!HTMLMediaElement} poolMediaEl The media element to bless.


Nit: s/poolMediaEl/mediaEl

done (moved to media-tasks.js)

gmajoulet · 2018-02-05T17:35:42Z

extensions/amp-story/0.1/media-pool.js

+  const queue = mediaEl[ELEMENT_TASK_QUEUE_PROPERTY_NAME];
+  queue.push(taskName);
+
+  if (queue.length === 1) {


Nit: might be worth implementing a isQueueRunning() or shouldExecuteQueue or something helper, that would do just that

made a constant

…rupted.

…ing event names.

This reverts commit 4e7af606f3fa03ca98fc6a4b3cd17e99947f1d10.

…solution, split into separate files

newmuis · 2018-02-07T17:02:14Z

The queue is per-element, so there's nothing preventing multiple elements from being blessed in parallel. But, for a given element, only a single task is executed at a time.

newmuis · 2018-02-07T17:04:24Z

The performance seems a bit better after the promise changes to amp-story-page, but note that this PR still has a problem where when the 8th page gets preloaded, it throws a JS error which causes the page to not load its video:

TypeError: _this10.newSources_.applyToElement is not a function
    at media-tasks.js:406
    at callTaskNoInline (vsync-impl.js:455)
    at Vsync.runScheduledTasks_ (vsync-impl.js:412)

gmajoulet

Really great work, just a few nits/questions

gmajoulet · 2018-02-07T21:21:58Z

extensions/amp-story/0.1/media-pool.js

   *     to replace the specified element.
   */
  loadInternal_(domMediaEl) {
    if (!isConnectedNode(domMediaEl)) {
      // Don't handle nodes that aren't even in the document.
-      return null;
+      return Promise.resolve();


Should we reject the promise instead?

I'm not sure this is really a rejection; we've done all the loading we want to do for that element, it just happens that we don't want to do any :)

gmajoulet · 2018-02-07T21:24:39Z

extensions/amp-story/0.1/media-pool.js

   */
  register(domMediaEl) {
    const mediaType = this.getMediaType_(domMediaEl);
    if (this.isAllocatedMediaElement_(mediaType, domMediaEl)) {
      // This media element originated from the media pool.
-      return;
+      return Promise.resolve();
    }

    const id = domMediaEl.id || this.createDomMediaElementId_();


Looks like I'm missing something here, but should the result from this.createDomMediaElementId_() be assigned to domMediaEl.id?

Below, we set domMediaEl.id = id, which handles this.

gmajoulet · 2018-02-07T21:26:37Z

extensions/amp-story/0.1/media-pool.js

  }


  /**
   * Preloads the content of the specified media element in the DOM.
   * @param {!HTMLMediaElement} domMediaEl The media element, found in the DOM,
   *     whose content should be loaded.
+   * @return {!Promise<!HTMLMediaElement>} A promise that is resolved when the
+   *     specified media element has been successfully loaded.


s/has been successfully loaded/has successfully started preloading?
This comment makes me think all the media content is loaded and ready to play until the end

Yep, copy/paste error. Changed.

gmajoulet · 2018-02-07T21:30:14Z

extensions/amp-story/0.1/media-pool.js

+    return this.enqueueMediaElementTask_(poolMediaEl, new PauseTask())
+        .then(() => {
+          if (opt_rewindToBeginning) {
+            this.enqueueMediaElementTask_(poolMediaEl, new RewindTask());


Maybe: this.rewindToBeginning(domMediaEl)

Also, do we actually need to wait for the PauseTask to be completed before we rewind the video? Looks like it was working synchronously before

Not sure, this was just to be very cautious. We can experiment later, since none of this is well-documented :)

Didn't use rewindToBeginning to prevent getting the matching element from the pool twice and wasting cycles.

gmajoulet · 2018-02-07T21:51:27Z

extensions/amp-story/0.1/media-tasks.js

@@ -0,0 +1,459 @@
+/**


I think eventually we'll want to have all these in a media-tasks folder.

in separate files*

gmajoulet · 2018-02-07T21:57:42Z

extensions/amp-story/0.1/test/test-media-tasks.js

+      mutatePromise: () => {},
+    };
+    sandbox.stub(vsyncApi, 'mutatePromise')
+        .callsFake(callback => {


Nit, does this work?

sandbox.stub(vsyncApi, 'mutatePromise').resolves(callback => {callback()});

… playing.

ampproject#13259) * Return play promise from playFn to ensure media playback is not interrupted. * Refactor playFn to always return a promise. Improve documentation. * Add debug messages * Remove muted property change, since this is in a separate PR * Fix merge * Implement a task queue for media element functions * Only dispatch event for blessing media, to avoid conflicts with existing event names. * Add debugging * Add element to debug logging * Move debug message * Attempt fix bless promises * Simplify debug logs * Add logging and fix event setup * Add queue contents debugging * Capture event * Logging changes * logging updates * More logging * More logging * Short circuit bless before enqueue * Logging * More logging * Make bless promise * Inline bless play * More logging * Add bless play logging * Add vsync * Revert "Add vsync" This reverts commit 4e7af606f3fa03ca98fc6a4b3cd17e99947f1d10. * Reset media source when deallocated. * Serialize dom el for testing * Update video source as a task * Migrate tasks to classes; schedule microtasks where possible. * Use vsync, make all methods promise, block page methods on promise resolution, split into separate files * Add missing added files * Add unit tests for media tasks * Fix wrong class name * Sort imports * Remove logging * Address review comments * Sort imports in utils * Log queue contents * Remove vsync, add logging * Fix getDefaultSources and isQueueEmpty. * Fix initializing to default source. Don't play for bless when already playing. * Remove allocated elements from unallocated array. * Fix types and address review comments. * Update MediaPool tests

newmuis requested review from alanorozco and gmajoulet February 4, 2018 09:07

googlebot added the cla: yes label Feb 4, 2018

newmuis changed the title ~~Create task queue to serialize function calls to media pool elements~~ Create task queue to serialize function calls to media pool elements (WIP) Feb 4, 2018

gmajoulet reviewed Feb 5, 2018

View reviewed changes

newmuis added 24 commits February 7, 2018 09:53

Return play promise from playFn to ensure media playback is not inter…

376a8b3

…rupted.

Refactor playFn to always return a promise. Improve documentation.

0d1c000

Add debug messages

0121c79

Remove muted property change, since this is in a separate PR

ecb4ac9

Fix merge

45d62f5

Implement a task queue for media element functions

e7329ca

Only dispatch event for blessing media, to avoid conflicts with exist…

c0c0dae

…ing event names.

Add debugging

ca19189

Add element to debug logging

a0eca0a

Move debug message

6a33170

Attempt fix bless promises

1a7c3a1

Simplify debug logs

6ae0580

Add logging and fix event setup

8125d93

Add queue contents debugging

12a4af1

Capture event

43ca7c8

Logging changes

3846ba8

logging updates

7cfb598

More logging

1100335

More logging

4f29710

Short circuit bless before enqueue

c8302f8

Logging

624738b

More logging

c75f195

Make bless promise

9c74321

Inline bless play

a2e54f9

newmuis added 12 commits February 7, 2018 09:54

Revert "Add vsync"

cb9b85e

This reverts commit 4e7af606f3fa03ca98fc6a4b3cd17e99947f1d10.

Reset media source when deallocated.

9bbb3e8

Serialize dom el for testing

0091b11

Update video source as a task

d11a3f5

Migrate tasks to classes; schedule microtasks where possible.

26a66cb

Use vsync, make all methods promise, block page methods on promise re…

d8a12d2

…solution, split into separate files

Add missing added files

b743107

Add unit tests for media tasks

8c52a28

Fix wrong class name

3ea9de2

Sort imports

5f56ac3

Remove logging

1d6d325

Address review comments

83a0e1d

newmuis force-pushed the amp-story-media-queueing branch from 51787f9 to 83a0e1d Compare February 7, 2018 17:00

Sort imports in utils

909552b

gmajoulet approved these changes Feb 7, 2018

View reviewed changes

newmuis added 5 commits February 8, 2018 08:49

Log queue contents

d6b9e30

Remove vsync, add logging

56cf5e7

Fix getDefaultSources and isQueueEmpty.

35c4394

Fix initializing to default source. Don't play for bless when already…

3ad13c0

… playing.

Remove allocated elements from unallocated array.

55f87a0

newmuis changed the title ~~Create task queue to serialize function calls to media pool elements (WIP)~~ Create task queue to synchronize function calls to media pool elements Feb 8, 2018

newmuis added 2 commits February 8, 2018 18:42

Fix types and address review comments.

6ca72f7

Update MediaPool tests

306c714

newmuis merged commit 619bcfb into ampproject:master Feb 9, 2018

ampprojectbot added the PR use: In Stable label Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create task queue to synchronize function calls to media pool elements #13259

Create task queue to synchronize function calls to media pool elements #13259

newmuis commented Feb 4, 2018

newmuis commented Feb 5, 2018

gmajoulet left a comment

gmajoulet Feb 5, 2018

newmuis Feb 7, 2018

gmajoulet Feb 5, 2018

newmuis Feb 7, 2018

gmajoulet Feb 5, 2018

newmuis Feb 7, 2018

gmajoulet Feb 5, 2018

newmuis Feb 7, 2018

gmajoulet Feb 5, 2018

newmuis Feb 7, 2018

newmuis commented Feb 7, 2018

newmuis commented Feb 7, 2018

gmajoulet left a comment

gmajoulet Feb 7, 2018

newmuis Feb 8, 2018

gmajoulet Feb 7, 2018

newmuis Feb 8, 2018

gmajoulet Feb 7, 2018

newmuis Feb 8, 2018

gmajoulet Feb 7, 2018

newmuis Feb 8, 2018

gmajoulet Feb 7, 2018

gmajoulet Feb 7, 2018

newmuis Feb 8, 2018

gmajoulet Feb 7, 2018

newmuis Feb 8, 2018

Create task queue to synchronize function calls to media pool elements #13259

Create task queue to synchronize function calls to media pool elements #13259

Conversation

newmuis commented Feb 4, 2018

newmuis commented Feb 5, 2018

gmajoulet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newmuis commented Feb 7, 2018

newmuis commented Feb 7, 2018

gmajoulet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment