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] Remove Android poster image from playing videos in preview mode #38291

Merged
merged 9 commits into from Jul 26, 2022

Conversation

coreymasanto
Copy link
Contributor

@coreymasanto coreymasanto commented Jun 13, 2022


Background: Android sometimes shows a blank frame between the poster and the first frame, which is an issue that was resolved by rendering the poster image while the first frame buffers.

Issue: Although the poster image is removed once the video is loaded, this removal occurs in firstLayoutCompleted(), which is only called once layoutCallback() has resolved. The issue is that layoutCallback() cannot resolve in the preview state because of a blocking whenFirstVisible() call that resolves only once the document progresses to the visible state.

Fix: This PR removes the poster image in onVideoLoaded_() (which is already being called from the layoutCallback()), instead of removing it only after the entire layoutCallback() has resolved (i.e., via firstLayoutCompleted()).

@coreymasanto coreymasanto self-assigned this Jun 13, 2022
@coreymasanto coreymasanto marked this pull request as ready for review June 13, 2022 19:04
@amp-owners-bot amp-owners-bot bot requested a review from dmanek June 13, 2022 19:04
@coreymasanto coreymasanto changed the title 🐛 [Story Preview] Remove Android poster image from playing preview videos 🐛 [Story Preview] Remove Android poster image from playing videos in preview mode Jun 13, 2022
@coreymasanto coreymasanto requested review from alanorozco and removed request for dmanek and mszylkowski June 13, 2022 19:08
Copy link
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test on Android if this makes the background flash between the placeholder and the video frames? There are some examples on #31358 of this behavior. As long as it doesn't make the issue worse, it should be fine.

@coreymasanto
Copy link
Contributor Author

Did you test on Android if this makes the background flash between the placeholder and the video frames? There are some examples on #31358 of this behavior. As long as it doesn't make the issue worse, it should be fine.

Tested loading a story in the visible state on Android. On this PR vs at main, I don't see any background flash between the grey city placeholder image and the subsequent video frames.

This PR:

screen-20220621-131550_2.mp4

Main:

screen-20220621-131811_2.mp4

@alanorozco
Copy link
Member

@coreymasanto

Did you try 1) no workaround on 2) a throttled network?

We might not need the workaround anymore.

@coreymasanto
Copy link
Contributor Author

Did you try 1) no workaround on 2) a throttled network?

We might not need the workaround anymore.

When I comment out the this.createPosterForAndroidBug_() workaround and test on various connection speeds at both main and this PR, I still don't see any background flash between the poster image and the first video frame. I'm not confident about removing that method in this PR, though.

@coreymasanto coreymasanto enabled auto-merge (squash) July 18, 2022 16:38
@coreymasanto coreymasanto merged commit b73520d into ampproject:main Jul 26, 2022
@coreymasanto coreymasanto deleted the previewAndroidPoster branch July 26, 2022 17:25
@ampprojectbot
Copy link
Member

Warning: disparity between this PR Percy build and its main build

The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the main branch that this PR was merged into, and there appears to be a mismatch between the two.

This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build / main commit's Percy build > and determine further action:

  • If the disparity appears to be caused by this PR, please create an bug report or send out a new PR to fix
  • If the disparity appears to be a flake, please @-mention ampproject/wg-approvers in a comment
  • If there is no disparity and this comment was created by mistake, please @-mention ampproject/wg-infra
  • If unsure, @-mention ampproject/wg-approvers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants