Amphtml: [amp-video] media queries do not filter poster images

[sebastianbenz]

This is bad: poster images ignore the media attribute and are always downloaded.

Example:

<amp-video layout="responsive" width=640 height=480 media="(max-width: 767px)" src="https://ampbyexample.com/video/tokyo.mp4?video=10" poster="https://unsplash.it/640/480?image=10"></amp-video>
<amp-video layout="responsive" width=640 height=480 media="(min-width: 768px)"  src="https://ampbyexample.com/video/tokyo.mp4?video=12" poster="https://unsplash.it/1024/768?image=11"></amp-video>

Expected behavior: 1 poster image and 1 video will be downloaded depending on screen width.
Actual behavior: 2 poster images and 1 video are downloaded depending on screen width.

Live sample: https://jsbin.com/powatilomu/edit?html,output

Both poster images are downloaded:

//cc @aghassemi

[sebastianbenz]

//cc @cvializ @choumx as Ali is OOO

[sebastianbenz]

Hey Carlos! Are there any updates on this bug? Thanks!

[cvializ]

Not yet. The solution will need a better solution than the one in the PR above.

[sebastianbenz]

Any news on this one?

[sparhami]

I have a few cycles to look into this. A related issue is that the poster is always loaded, even if the element is offscreen or somewhere in a carousel.

[sparhami]

So it looks like one concern with Carlos's PR was that it might not allow the poster image propagation on prerender (in cases where the prerender is not allowed due to not being a cached video). After further investigation, it looks like buildCallback is not even run for non-prerenderable elements during the prerender phase:

https://github.com/ampproject/amphtml/blob/3a3440eb5d15faf1b1e26f757662298d047178c3/src/service/resources-impl.js#L561-L563

This check was introduced with https://github.com/ampproject/amphtml/commit/ab9e9acd75559c1b13a379b280a9a960a0e23709.

As a result, moving the propagation of the poster attribute into layout callback does not actually cause any problems for the current state of things.

The bigger problem seems to be the current code seems to indicate that <amp-video> is never being prerendered, as the isPrerenderAllowed_ flag is being set from the buildCallback, which is guarded by prerenderAllowed (#15965).

[sebastianbenz]

Just realised that this is still open. Is there any chance that this can be fixed?

[choumx]

15965 is now fixed. I think Carlos's original fix might be fine since we do call layoutCallback on pre-renderable elements in pre-render mode. Just need to fix that scary-sounding unit test.

[ampprojectbot]

This is a high priority issue but it hasn't been updated in awhile. @cvializ Do you have any updates?

[ampprojectbot]

This is a high priority issue but it hasn't been updated in awhile. @cvializ Do you have any updates?

[ampprojectbot]

This is a high priority issue but it hasn't been updated in awhile. @cvializ Do you have any updates?

[ampprojectbot]

This is a high priority issue but it hasn't been updated in awhile. @cvializ Do you have any updates?

[sebastianbenz]

Pinging this again. Quick reminder how bad this is: if you load https://www.bmw.com/ this bug results in 180kb of unneeded preview image downloads.

[ampprojectbot]

This is a high priority issue but it hasn't been updated in awhile. @cvializ Do you have any updates?

[ampprojectbot]

This is a high priority issue but it hasn't been updated in awhile. @cvializ Do you have any updates?

[sebastianbenz]

@cvializ any updates on this?

[sparhami]

Closed via #23360.