Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 [Story Preview] Enable amp-video to play in preview mode #38149

Merged
merged 21 commits into from May 9, 2022

Conversation

coreymasanto
Copy link
Contributor


Although this PR enabled stories to play in preview mode, video elements still fail to play. This failure occurs due to the amp-video layout callback being unable to resolve in any non-visible visibility state, which ultimately blocks amp-story-page from being able to register and play the video.

Instead of indefinitely blocking registration, this PR registers media elements at the beginning of their layout callbacks, enabling videos in preview mode to play as long as they have cached sources.

@lgtm-com
Copy link

lgtm-com bot commented May 4, 2022

This pull request introduces 2 alerts when merging 2b8f1c2 into d365219 - view on LGTM.com

new alerts:

  • 1 for Implicit operand conversion
  • 1 for Variable not declared before use

@coreymasanto coreymasanto marked this pull request as ready for review May 5, 2022 10:17
@amp-owners-bot
Copy link

amp-owners-bot bot commented May 5, 2022

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-page.js
extensions/amp-story/1.0/media-pool.js
extensions/amp-story/1.0/sources.js

@coreymasanto
Copy link
Contributor Author

I'm testing the case where the video's cached source fails to load during preview mode and the origin source subsequently loads successfully in visible mode. My test story has a video with a single source:
<source src="./video/stampasdf.mp4?CACHED" amp-orig-src="./video/stamp.mp4?ORIGIN" type="video/mp4" i-amphtml-video-cached-source>

The one issue that remains here is that the loadPromise() call in amp-story-page.playMedia_() rejects because of this condition being met, presumably because the video element's first attempted load failed (due to the invalid cached source). This results in the video not playing after transitioning from preview to visible.

@coreymasanto
Copy link
Contributor Author

coreymasanto commented May 6, 2022

I'm testing the case where the video's cached source fails to load during preview mode and the origin source subsequently loads successfully in visible mode.

I've updated the test amp-video's sources as follows:

<!-- Invalid URL --> <source src="./video/stampasdf.mp4?CACHED" type="video/mp4" i-amphtml-video-cached-source>
<!-- Valid URL -->   <source src="./video/stamp.mp4?NOT_CACHED" type="video/mp4">

However, I'm experiencing a testing issue where the video actually plays successfully in preview mode via the NOT_CACHED source. This occurs because the preloading of the video results in a call that adds all of the video's sources, regardless of cached state, due to this code within media-pool.register(). The removeFrom() call uses the sources on amp-video (i.e., those provided by the publisher's HTML) instead of the sources on video (i.e., those added via logic within amp-video.js).

@coreymasanto
Copy link
Contributor Author

I managed to successfully test using my demo site by blocking the requests for the cached video sources using DevTools. As seen in the video below, the following occurs:

  1. The story starts off loaded in prerender mode
  2. Requests for the video's cached URLs fail
  3. The story enters preview mode
  4. The video fails to play and an error message is shown at the bottom of the story
  5. The story enters visible mode
  6. Requests for the video's origin URLs succeed
  7. The video successfully plays and the error message is removed
previewPlayVideo.mov

extensions/amp-story/1.0/amp-story-page.js Outdated Show resolved Hide resolved
@coreymasanto coreymasanto enabled auto-merge (squash) May 9, 2022 20:02
@coreymasanto coreymasanto merged commit 3fe2ae4 into ampproject:main May 9, 2022
@coreymasanto coreymasanto deleted the previewPlayVideo branch May 9, 2022 21:37
@gmajoulet gmajoulet added this to In progress in wg-stories Sprint via automation May 10, 2022
@gmajoulet gmajoulet moved this from In progress to Done in wg-stories Sprint May 10, 2022
westonruter added a commit that referenced this pull request May 20, 2022
…a-menu-images-validator-spec

* 'main' of github.com:ampproject/amphtml: (90 commits)
  🔥 [Story mediapool] Fix videos from mediapool with `noaudio` don't have audio when reused. (#38216)
  Hide progress bar on the control group of auto advance experiment (#38215)
  ✨ Add Bento Autocomplete Component (#37837)
  🌐 [Story subscription] Subscription localization async (#38204)
  Dable: add new optional parameter "channel" (#38199)
  ✨ [AMP Story Paywall] Enable developers to specify a custom subscriptions page index (#38175)
  SwG Release 0.1.22.217 (#38187)
  amp-script: implements new size limits for sandboxed scripts (#38185)
  🖍  Hide the system layer and progress bar in preview mode (#38163)
  added minItems (#38177)
  Prevent expandTemplate from ReDOSing (#38178)
  Change amp-story-subscriptions attribute name to reflect its flexibility (#38176)
  🐛 [Story Preview] Enable amp-video to play in preview mode (#38149)
  Added the possibility to get page count to story messaging api (#38170)
  SwG Release 0.1.22.216 (#38168)
  Allow @newmuis to update OWNERS files (#38169)
  ✨ Add Richaudience to RTC callout vendors (#38160)
  🚀  SunMedia: Update amp-ad (#38128)
  Remove option to deploy PR artifacts to a static website (#38152)
  added some vars and requests in gfksensic.json (#37722)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants