Convert ad iframes to get initialIntersection async #31753

samouri · 2020-12-28T22:25:45Z

summary
Alternative to #31714
Partial for #31540

AFAIK, only ads are supposed to use initialIntersection. The gltf extension happened to be using it as well, but IMO it shouldn't have since the message from viewportCallback should be more than good enough.

Changes:

Introduces new util for async measure with signature: measureIntersection(el) --> Promise<IntersectionObserverEntry
Uses it to measure initialIntersection for ad iframes.
Removes usage of initialIntersection from 3d gltf viewer

Breaking change: rootBounds and intersectionRatio are now according to spec as opposed to the definition we had been using. That means width/height is relative to viewport instead of the document's viewport.

TODO: figure out a less brittle solution for the tests. potentially just delete the assertions on intersectionRect etc. now that we are using the real InOb, so likely not worth testing.

I recommend viewing the changes with whitespace removed

amp-owners-bot · 2020-12-28T22:25:54Z

Hey @alanorozco! These files were changed:

extensions/amp-viqeo-player/0.1/amp-viqeo-player.js

samouri · 2020-12-30T22:00:35Z

I found one surprising attribute about IntersectionObserver intersectionRect while working on this PR. For an implicit-root (root:null) InOb observing an element in a subframe: all of the intersectionRect's have a width/height that equals "how much of the element is currently visible in the main viewport" whereas the positional properties (top/left/right/bottom) are all relative to the subdocument.

Most likely according to spec, but I'm having trouble figuring out why that makes sense.

dvoytenko

This is awesome! But one question. PTAL.

3p/3d-gltf/viewer.js

extensions/amp-a4a/0.1/amp-a4a.js

test/integration/test-amp-ad-3p.js

samouri · 2021-01-08T15:38:17Z

In order to merge the helper without blocking on the ads changes, I've split it into its own PR (#31858)

lannka · 2021-01-11T17:56:24Z

Breaking change: rootBounds and intersectionRatio are now according to spec as opposed to the definition we had been using. That means width/height is relative to viewport instead of the document's viewport.

@samouri could you further elaborate on the change? shouldn't width/height be absolute values?

samouri · 2021-01-11T18:02:06Z

Breaking change: rootBounds and intersectionRatio are now according to spec as opposed to the definition we had been using. That means width/height is relative to viewport instead of the document's viewport.

@samouri could you further elaborate on the change? shouldn't width/height be absolute values?

I believe that both the rootBounds and intersectionRatio will not have an affect on any AMP Viewer that takes up 100% of the browser viewport (which SRP Viewer does). In any other situation, they would behave differently than current implementation. Here are the differences:

Current rootBounds: the height/width of the document
New rootBounds: the height/width of the viewport

Current intersectionRect: the height/width of intersection rectangle between the observed element and the document's viewport (i.e. is it visible within the iframe).
New intersectionRect: the height/width of the intersection rectangle between the observed element and the viewport. (even if it is visible in the iframe, maybe the iframe is offscreen and will therefore be 0/0)

lannka · 2021-01-11T18:06:57Z

Thansk @samouri . That sounds like not a problem to me for those ads.

Another thing:

amphtml/src/utils/intersection.js

Lines 86 to 94 in 090da63

    
           export function intersectionEntryToJson(entry) { 
        
             return { 
        
               time: entry.time, 
        
               rootBounds: layoutRectFromDomRect(entry.rootBounds), 
        
               boundingClientRect: layoutRectFromDomRect(entry.boundingClientRect), 
        
               intersectionRect: layoutRectFromDomRect(entry.intersectionRect), 
        
               intersectionRatio: entry.intersectionRatio, 
        
             }; 
        
           }

We are normalizing the inob entry data object using layoutRectFromDomRect . Did we have that in consideration in the new code?

samouri · 2021-01-11T18:45:36Z

We are normalizing the inob entry data object using layoutRectFromDomRect . Did we have that in consideration in the new code?

Unless I'm missing something, that is the new code :). Previously we didn't need to do anything special since the entries were calculated inside of JS. Now are are using real InObEntry objects which are not serializable, and so we need to convert them here to be plain object (and therefore eligible for postMessage)

lannka · 2021-01-11T19:44:43Z

oh, my bad. then it looks great

lannka

oh wait a second. just saw this PR also touches amp-a4a.
would this change introduce latency to the rendering of an ad?

if it does, to avoid debugging an ads metric regression while promoting RC to prod, we'd better to run an explicit experiment for this change.

samouri · 2021-01-11T20:27:54Z

would this change introduce latency to the rendering of an ad?

I'd expect it to create an ~10ms async delay. Likely unnoticeable in most circumstances (e.g. the resources system already often forces ~2s deprioritization on ads, which would be the rate-limiting factor)

lannka · 2021-01-12T17:54:47Z

For changes that might affect ads performance, we need to run experiment to measure the impact before launch it.
Eventually, we might be fine to launch it as is but just need to be aware of the stats change.
In the past, we had many times seeing regressions on our Beta channel and have to bisect PRs to find which was the cause.

@calebcordry could you give some guidance for setting up such an experiment?

samouri · 2021-01-12T18:12:15Z

@lannka: do you only want the experiment for a4a, or also for name-frame-renderer + amp-ad?

calebcordry · 2021-01-12T20:09:30Z

Agree with @lannka. I would recommend wrapping the a4a/nameframe/safeframe/iframeGet all in an isExperimentOn('exp-name)` block for this PR and then I can help you with a follow up PR to divert/report. Unfortunately we don't have ads metrics for 3p impl, but it still might be good to run as exp in canary first.

samouri · 2021-01-13T19:15:52Z

Agree with @lannka. I would recommend wrapping the a4a/nameframe/safeframe/iframeGet all in an isExperimentOn('exp-name)` block for this PR and then I can help you with a follow up PR to divert/report. Unfortunately we don't have ads metrics for 3p impl, but it still might be good to run as exp in canary first.

done! PTAL

extensions/amp-a4a/0.1/amp-a4a.js

samouri · 2021-01-14T19:03:10Z

Just performed a sanity check and gltf viewer still behaves as expected + ad context still contains intialIntersection.
@ampproject/wg-performance: can I get a bundle-size approval?

* getContextMetadata --> Promise * ad iframes gather initialIntersection async * use experiments * remove log / fix test * revert test change now that under expeirment. * fix test-amp-ad-custom * caleb updates * toggle intersection needs win

samouri marked this pull request as draft December 28, 2020 22:25

amp-owners-bot bot requested a review from nainar December 28, 2020 22:25

samouri removed the request for review from nainar December 28, 2020 22:25

samouri self-assigned this Dec 28, 2020

samouri changed the title ~~Mio~~ Convert iframes to get initialIntersection async Dec 28, 2020

samouri changed the title ~~Convert iframes to get initialIntersection async~~ Convert ad iframes to get initialIntersection async Dec 30, 2020

samouri force-pushed the mio branch from f0e1a5d to e9f8050 Compare December 30, 2020 20:34

samouri added Related to: Performance WG: performance and removed Related to: Performance labels Dec 30, 2020

samouri requested a review from dvoytenko December 30, 2020 21:58

samouri marked this pull request as ready for review December 30, 2020 21:58

samouri mentioned this pull request Jan 5, 2021

wg-performance Status Update 2021-01-05 ampproject/wg-performance#66

Closed

dvoytenko approved these changes Jan 7, 2021

View reviewed changes

3p/3d-gltf/viewer.js Show resolved Hide resolved

extensions/amp-a4a/0.1/amp-a4a.js Outdated Show resolved Hide resolved

test/integration/test-amp-ad-3p.js Outdated Show resolved Hide resolved

test/integration/test-amp-ad-3p.js Outdated Show resolved Hide resolved

samouri requested a review from lannka January 7, 2021 18:45

samouri mentioned this pull request Jan 8, 2021

Introduce measureIntersection helper for async measures. #31858

Merged

samouri added 2 commits January 8, 2021 13:56

getContextMetadata --> Promise

6bc1ddb

ad iframes gather initialIntersection async

a8c415d

samouri force-pushed the mio branch from cd7a131 to a8c415d Compare January 8, 2021 18:56

lannka approved these changes Jan 11, 2021

View reviewed changes

lannka requested changes Jan 11, 2021

View reviewed changes

samouri added 4 commits January 13, 2021 09:52

use experiments

077392c

remove log / fix test

7a3a19b

revert test change now that under expeirment.

7cd3f0b

fix test-amp-ad-custom

6b62f42

samouri mentioned this pull request Jan 13, 2021

removing measurements: getIntersectionChangeEntry #31935

Closed

7 tasks

calebcordry reviewed Jan 13, 2021

View reviewed changes

extensions/amp-a4a/0.1/amp-a4a.js Outdated Show resolved Hide resolved

calebcordry reviewed Jan 13, 2021

View reviewed changes

extensions/amp-a4a/0.1/amp-a4a.js Outdated Show resolved Hide resolved

caleb updates

7eb8bc1

calebcordry approved these changes Jan 13, 2021

View reviewed changes

toggle intersection needs win

4ab70e7

samouri requested a review from lannka January 14, 2021 18:11

erwinmombay approved these changes Jan 14, 2021

View reviewed changes

lannka approved these changes Jan 14, 2021

View reviewed changes

samouri merged commit 75abc6b into ampproject:master Jan 14, 2021

samouri deleted the mio branch January 14, 2021 21:05

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Jan 20, 2021

samouri mentioned this pull request Feb 4, 2021

Remove sync measurement from ad iframes: experiment #32437

Merged

ampprojectbot added the PR use: In LTS label Feb 8, 2021

This was referenced Apr 30, 2021

Ads/Runtime: move getIntersectionChangeEntry out of Runtime and into Ads only #34133

Closed

Ads: move from sync to async initialIntersection #34141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert ad iframes to get initialIntersection async #31753

Convert ad iframes to get initialIntersection async #31753

samouri commented Dec 28, 2020 •

edited

Loading

amp-owners-bot bot commented Dec 28, 2020

samouri commented Dec 30, 2020 •

edited

Loading

dvoytenko left a comment

samouri commented Jan 8, 2021 •

edited

Loading

lannka commented Jan 11, 2021

samouri commented Jan 11, 2021

lannka commented Jan 11, 2021

samouri commented Jan 11, 2021

lannka commented Jan 11, 2021

lannka left a comment •

edited

Loading

samouri commented Jan 11, 2021 •

edited

Loading

lannka commented Jan 12, 2021

samouri commented Jan 12, 2021

calebcordry commented Jan 12, 2021

samouri commented Jan 13, 2021 •

edited

Loading

samouri commented Jan 14, 2021

Convert ad iframes to get initialIntersection async #31753

Convert ad iframes to get initialIntersection async #31753

Conversation

samouri commented Dec 28, 2020 • edited Loading

amp-owners-bot bot commented Dec 28, 2020

samouri commented Dec 30, 2020 • edited Loading

dvoytenko left a comment

Choose a reason for hiding this comment

samouri commented Jan 8, 2021 • edited Loading

lannka commented Jan 11, 2021

samouri commented Jan 11, 2021

lannka commented Jan 11, 2021

samouri commented Jan 11, 2021

lannka commented Jan 11, 2021

lannka left a comment • edited Loading

Choose a reason for hiding this comment

samouri commented Jan 11, 2021 • edited Loading

lannka commented Jan 12, 2021

samouri commented Jan 12, 2021

calebcordry commented Jan 12, 2021

samouri commented Jan 13, 2021 • edited Loading

samouri commented Jan 14, 2021

samouri commented Dec 28, 2020 •

edited

Loading

samouri commented Dec 30, 2020 •

edited

Loading

samouri commented Jan 8, 2021 •

edited

Loading

lannka left a comment •

edited

Loading

samouri commented Jan 11, 2021 •

edited

Loading

samouri commented Jan 13, 2021 •

edited

Loading