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.

Sign up for GitHub

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

Jump to bottom

Cached video does not fallback to the origin video #27109

Closed
gmajoulet opened this issue Mar 5, 2020 · 4 comments · Fixed by #27984
Closed

Cached video does not fallback to the origin video #27109

gmajoulet opened this issue Mar 5, 2020 · 4 comments · Fixed by #27984
Assignees
@gmajoulet
Labels
P1: High Priority Type: Bug WG: stories

Comments

@gmajoulet
Copy link
Contributor

The video source in this story 404s, and does not fallback to the origin video:

https://unumbox-com.cdn.ampproject.org/v/s/unumbox.com/website-development-trends-2020-amp-story/?usqp=mq331AQFKAGwASA%3D&amp_js_v=a2&amp_gsa=1#referrer=https%3A%2F%2Fwww.google.com&visibilityState=visible&cap=swipe&ampshare=https%3A%2F%2Funumbox.com%2Fwebsite-development-trends-2020-amp-story%2F

cc @ampproject/wg-stories
@gmajoulet gmajoulet self-assigned this Mar 5, 2020
@gmajoulet
Copy link
Contributor Author

gmajoulet commented Mar 5, 2020 •
edited
Loading

  1. Only on 3 panels desktop
  2. Only if the video is in the first 3 story-pages
  3. Only when the cached source 404s

Edit: actually happens on mobile too, just less often for some reason

@gmajoulet
Copy link
Contributor Author

gmajoulet commented Mar 5, 2020 •
edited
Loading

Looks like it's a race condition between when the media pool removes the sources, and when the video.propagateLayoutChildren_() that adds the source to the origin is called.
I don't really know what exactly is causing the bug yet, but the order in which things run don't seem correct:

  1. Media pool removes the sources
  2. Media pool adds the sources back, if needed
  3. amp-video creates a new source linking to the origin video, but the media pool doesn't know about this one

amp-story-page must be calling some method between (2) and (3) that triggers an error event on the video. The MEDIA_LOAD_FAILURE_SRC_PROPERTY attribute is set on the video, and next time we call play(), the loadPromise will early return and show the error message.
Ideally step 3 should happen before step 1, but in terms of code I'm not sure about the implications.

@newmuis
Copy link
Contributor

newmuis commented Mar 6, 2020

There are reports of this happening on mobile as well, also when served from the AMP cache.

cramforce added a commit to cramforce/amphtml that referenced this issue Apr 14, 2020
@cramforce
Fix nasty race condition in AMP video
c7cb622 
Under this scenario a video might not fall back to the origin on pre-render.
Possible fix for ampproject#27109

Reproduces by navigating (locally to)
http://localhost:8000/proxy/s/stories.nonblocking.io/waffles/?usqp=mq331AQFKAGwASA%3D&amp_js_v=0.1#referrer=https%3A%2F%2Fwww.google.com&visibilityState=prerender&cap=swipe&ampshare=https%3A%2F%2Funumbox.com%2Fwebsite-development-trends-2020-amp-story%2F
waiting for the pre-render load to fail, and then executing `AMP.viewer.receiveMessage('visibilitychange', {'state': 'visible'})` in the console.

If this if not a fix for ampproject#27109, then it might be the same race, but elsewhere. The problem is that you cannot wait for `loadPromise` on a video element that has failed a source and has just had a new source provided.
@cramforce cramforce mentioned this issue Apr 14, 2020
Merged
cramforce added a commit that referenced this issue Apr 15, 2020
@cramforce
Fix nasty race condition in AMP video (#27740)
ef4fd12 
Under this scenario a video might not fall back to the origin on pre-render.
Possible fix for #27109

Reproduces by navigating (locally to)
http://localhost:8000/proxy/s/stories.nonblocking.io/waffles/?usqp=mq331AQFKAGwASA%3D&amp_js_v=0.1#referrer=https%3A%2F%2Fwww.google.com&visibilityState=prerender&cap=swipe&ampshare=https%3A%2F%2Funumbox.com%2Fwebsite-development-trends-2020-amp-story%2F
waiting for the pre-render load to fail, and then executing `AMP.viewer.receiveMessage('visibilitychange', {'state': 'visible'})` in the console.

If this if not a fix for #27109, then it might be the same race, but elsewhere. The problem is that you cannot wait for `loadPromise` on a video element that has failed a source and has just had a new source provided.

## Additional changes:

- Change preload to also preload non-cache sources
- Clarify preconnect docs about the security invariants.
@cramforce
Copy link
Member

One fundamental problem is I think that media-pool works with amp-video elements before they ever had a layoutCallback resolve. That breaks all kinds of assumptions like #27874

Is there any reason that the media pool has to ignore the AMP layoutCallbacks? If yes, should it turn it off and completely take over the video lifecycle instead of letting both run concurrently?

@cramforce cramforce mentioned this issue Apr 18, 2020
Merged
@gmajoulet gmajoulet mentioned this issue Apr 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gmajoulet gmajoulet

Labels
P1: High Priority Type: Bug WG: stories
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Wait for media layout before registering them into the mediapool. gmajoulet/amphtml
3 participants
@cramforce @gmajoulet @newmuis