1248828 - Extend the reftest harness to allow testing compositor-side invalidation

Status: NEW

(Core :: Graphics: Layers, defect, P3)

Description

[Botond Ballo [:botond]]

The reftest harness allows testing content-side invalidation.

Specifically, testing a correctness problem caused by too little invalidation works like this:
  - The reftest harness waits for MozAfterPaint events, which carry
    with them information about what region was invalidated and painted.
    Every time it receives an event, it draws the regions that were
    invalidated onto a canvas.
  - When the test page is loaded, the harness waits for the page to
    reach a stable state. In this process, the entire page is invalidated
    (perhaps in parts) and painted onto the canvas.
  - Once a stable state is reached, the harness sends the test page a 
    MozReftestInvalidate event, which the test page listens to and uses
    to perform an action that causes invalidation.
  - When the next MozAfterPaint event is received, only the invalidated
    part of the canvas is redrawn. If too little invalidation occurred,
    the resulting canvas contents will be incorrect compared to the
    reference page.
(I've just gotten up to speed on how this works today. Please correct me if I misunderstood something.)

Matt and I were discussing today that it would be good to be able to test compositor-side invalidation in a similar way. While this remains to be investigated in detail, Matt outlined the following approach to doing this:
  - Modify the MozAfterPaint event to report the compositor-side
    invalid region, rather than the content-side one. MozAfterPaint
    is already triggered by the compositor via the DidComposite
    message, so this can be implemented by passing that information
    along in that message.
      - This should be sound, because the compositor-side invalid
        region should be a superset of the content-side invalid
        region, and it should be the same except in the presence
        of async transformations (which presumably tests that
        test for content-side invalidation don't have).
  - Ensure that async properties that trigger async transformations
    that can cause additional invalidation on the compositor side
    (notably, reftest-async-scroll-[xy]) are not in effect on the 
    first paint. This ensures that there *is* an invalidation
    caused by them that's detected by the harness.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP patch

There are three missing pieces in this patch.

1. I think invalidation region on the compositor side should be converted to coordinate system on the content side, but not yet converted.
2. I've never checked async transformations are included in the invalidation region.  I just use the value in LayerManagerComposite::UpdateAndRender().
3. It doesn't work on non-E10S.

Comment 2

[Thinker Li [:sinker]]

Will it not send MozAfterPaint twice for the same time of painting?

Comment 3

[Thinker Li [:sinker]]

OK! I have found the code about the timer for NotifyDidPaintForSubtree.  Forget my last comment!

Comment 4

[Thinker Li [:sinker]]

Hiro,
For the test of bug 1208646, the problem is still there.  I guess animationiteration is happened before MozAfterPaint event.  So, the test would be end before the arrival of the MozAfterPaint event.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Yes, I think async animations with reftest-no-flush does not make mFireAfterPaintEvents true, so we have to wait for MozAfterPaint explicitly in the test.  But still the test fails, there are more things I am missing.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP v2

The previous patch has one big mistake.  Calling NotifyInvalidation should have done before mUndeliveredInvalidateRequestsBeforeLastPaint is generated from mInvalidateRequestsSinceLastPaint.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP v3

Thinker noticed to me that async animations does not produce valid transaction id, so there are cases that transation id equals zero in ClientLayerManager::DidComposite.

Also, I've just noticed that we don't need to send region if the region is empty.

Comment 8

[Thinker Li [:sinker]]

Attachment: snapshot-reftest-wait.diff

I have tried another solution for tests with compositor side invalidation.  This patch makes the harness to make a snapshot when |reftest-wait| was removed from the root element.  dbaron suggests to add a conditional check before making a snapshot.

This patch requires tests to add the class |reftest-snapshot-all| to the root element as a flag to make a snapshot being taken when |reftest-wait| was removed.

Comment 9

[Thinker Li [:sinker]]

Attachment: snapshot-reftest-wait.diff

Use more exact conditions.
If there is |reftest-wait|, |reftest-no-flush| and |reftest-snapshot-all| at the root element, a snapshot are taken when |reftest-wait| is removed.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Hi, Thinker.  Have you ever checked that animate-backface-hidden.html with attachment 8763482 [details] [diff] [review] fails without a key fix for backface hidden issue (attachment 8759009 [details] [diff] [review])?  I did check it locally, and it does not fail (in case of non-E10s, the test on E10S does not fail with/without any fix).  I guess attachment 8763482 [details] [diff] [review] causes a paint on the main-thread.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8763482 [details] [diff] [review]
snapshot-reftest-wait.diff

>+                contentRootElement.classList.contains("reftest-snapshot-all")) {

Please use shouldSnapshotWholePage(contentRootElement) here instead of this contentRootElement.classList.contains() test.

>+                // bug 1248828: provide a way to take snapshot with
>+                // latest result at compositor side without flushing
>+                // pending restyles and reflows.

Please don't include the bug number in comments; that's findable by hg blame.

r=dbaron with that

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(That said, it's not clear to me how related Thinker's patch is to this bug; I'd also hope Thinker can answer comment 10.)

Comment 13

[Thinker Li [:sinker]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Hi, Thinker.  Have you ever checked that animate-backface-hidden.html with
> attachment 8763482 [details] [diff] [review] fails without a key fix for
> backface hidden issue (attachment 8759009 [details] [diff] [review])?  I did
> check it locally, and it does not fail (in case of non-E10s, the test on
> E10S does not fail with/without any fix).  I guess attachment 8763482 [details] [diff] [review]
> [details] [diff] [review] causes a paint on the main-thread.

Hi, Hiroyuki.  attachment 8759009 [details] [diff] [review] does not take affect alone.
With this patch, it still play the animation in animate-backface-hidden.html at the main thread of the content process.  But, it works with attachment 8760203 [details] [diff] [review] to play the animation at the compositor thread.

For dbaron's question, this bug tries to address the problem of reftests that do invalidation async at the compositor without repaints at content process.  However, the current implementation of the reftest harness takes snapshots only at MozAfterPaint that is caused by a repaint at the content process.  This patch let tests to trigger the procedure of taking a snapshot at the moment chosen by the test without any repaint at content process side.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Thinker Li [:sinker] from comment #13)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> > Hi, Thinker.  Have you ever checked that animate-backface-hidden.html with
> > attachment 8763482 [details] [diff] [review] fails without a key fix for
> > backface hidden issue (attachment 8759009 [details] [diff] [review])?  I did
> > check it locally, and it does not fail (in case of non-E10s, the test on
> > E10S does not fail with/without any fix).  I guess attachment 8763482 [details] [diff] [review]
> > [details] [diff] [review] causes a paint on the main-thread.
> 
> Hi, Hiroyuki.  attachment 8759009 [details] [diff] [review] does not take
> affect alone.
> With this patch, it still play the animation in animate-backface-hidden.html
> at the main thread of the content process.  But, it works with attachment
> 8760203 [details] [diff] [review] to play the animation at the compositor
> thread.

Yes, I know. I did try the test *with* attachment 8760203 [details] [diff] [review] and *without* attachment 8759009 [details] [diff] [review], so the test should fail, right?

Comment 15

[Thinker Li [:sinker]]

For non-e10s, animations always run on the main thread.  So, repaints always occur, and it should pass the test.  It is not a problem.  For e10s, it is a problem if it does not fail.  Do you mean it still fails with the combination at the comment 14 and with e10s?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Thinker Li [:sinker] from comment #15)
> For non-e10s, animations always run on the main thread.  So, repaints always

Transform/opacity animations can be run on the compositor on *non-E10S* as well.

In case of E10S, there is another problem we should take a look.

Comment 17

[Thinker Li [:sinker]]

After talking with Hiroyuki for the problem, I get some some ideas.

The attribute |reftest-no-sync-layers| is also necessary at the root element to prevent the harness to call |nsDOMWindowUtils::UpdateLayerTree()| before taking a snapshot.  

The reftest harness calls |CanvasRenderingContext2D::DrawWindow()| to take a snapshot at chrome process.  It triggers a flush and repaint at parent process.  It would be OK with e10s since it don't cause a flush and repaint at content processes, but it is no OK for non-e10s, the content of the test would repainted since the test also stays at the chrome process.

If we want to address the non-e10s problem, we need to improve |DrawWindow()|.  IIRC, Jerry tried to implement taking snapshots at compositor thread without flush and repaint.

Hiroyuki, how do you think?

Jerry, am I right?

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Thinker Li [:sinker] from comment #17)
> After talking with Hiroyuki for the problem, I get some some ideas.
> 
> The attribute |reftest-no-sync-layers| is also necessary at the root element
> to prevent the harness to call |nsDOMWindowUtils::UpdateLayerTree()| before
> taking a snapshot.  
> 
> The reftest harness calls |CanvasRenderingContext2D::DrawWindow()| to take a
> snapshot at chrome process.  It triggers a flush and repaint at parent
> process.  It would be OK with e10s since it don't cause a flush and repaint
> at content processes, but it is no OK for non-e10s, the content of the test
> would repainted since the test also stays at the chrome process.

Does DrawWindow on chrome really cause a paint on content in case of non-E10S?  If it's true, I am wondering why we are suffering from this kind of test failures?

Comment 19

[Thinker Li [:sinker]]

For non-e10s case, |DrawWindow()| causes a repaint since they are all in the same process, no content process anymore.  It always pass.

For e10 case, it does not causes a repaint at the content process (only at the chrome process) so it fails without applying attachment 8759009 [details] [diff] [review].  It seems quite reasonable for me.  So, what is your question?

Comment 20

[Thinker Li [:sinker]]

After the discussion with Hiroyuki on irc, there are two things to do.

 1. Make |MozAfterPaint| be sent/triggered from compositor.
 2. Make |DrawWindow()| does not do repaints for the content document.

For now, |DrawWindow()| would cause layers of the chrome doc and the content doc be recreated and repainted (by calling |PaintFrame()|)for non-e10s.  For e10s, only the layers of chrome doc would be recreated and repainted.  Ether e10s or non-e10s, |DrawWindow()| would eventually made |ClientLayerManager| to call |SendMakeSnapshot()| to make compositor composing layers on the given |DrawTarget|.

We don't want to recreate and repaint layers for |DrawWindow()|.  We like to see what the compositor see at the moment, not to change it.  By doing an empty layer transaction with a |DrawTarget|, |ClientLayerManager| would make the compositor to compose all layers on the |DrawTarget| without painting.

Comment 21

[Thinker Li [:sinker]]

Attachment: empty-transaction-paintframe.diff

This patch makes nsLayoutUtils::PaintFrame() run a short cut by making an empty layer transaction to make compositor drawing on a DrawTarget from layers without updating layer tree and content of layers.

Comment 22

[Thinker Li [:sinker]]

Comment on attachment 8765413 [details] [diff] [review]
empty-transaction-paintframe.diff

Review of attachment 8765413 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tools/reftest/reftest.jsm
@@ +1424,5 @@
>  // Fortunately this is pretty cheap.
>  function DoDrawWindow(ctx, x, y, w, h)
>  {
> +    var flags = ctx.DRAWWINDOW_DRAW_CARET | ctx.DRAWWINDOW_DRAW_VIEW |
> +        ctx.DRAWWINDOW_DO_NOT_FLUSH;

This change is necessary for non-e10s.

Since there is one or more animation in effect, DocumentTimeline would keep ticking.  It drives Animations changing animation rules (a style rule) for their target elements.  For animations running at compositor, these changes would not post pending restyles to |RestyleManage| (they are throttled), but keep them in the queue of |EffectCompositor|.  So, it would not cause restyle/reflow and painting immediately.

However, |DrawWindow()| would call |FlushPendingNotifications()|, then call |PostRestyleForThrottledAnimations()| in turn to post these pending restyles.

For e10s, content and reftest harness (reftest.jsm) are running in different processes. Not thing would be in the queue of the |EffectCompositor| at parent process where reftest.jsm is, so it is OK.

But, for non-e10s, there are chagnes in the queue of the |EffectCompositor|, and reftest.jsm is running in the same process, and it will call |DrawWindow()|.

|DRAWWINDOW_DO_NOT_FLUSH| is here to avoid calling |FlushPendingNotifications()|.  It is added unconditional.  I think it only be added for animations running at compositor.  Maybe, put a flag on the root element of the content to control it.

Comment 23

[Thinker Li [:sinker]]

Attachment: empty-transaction-paintframe.diff

Send |reftest:UpdateWholeCanvasForInvalidation| and |reftest:UpdateCanvasForInvalidation| along with the fact if |reftest-no-flush| is on the root element of content.  So, the harness would call |DrawWindow()| with |DRAWWINDOW_DO_NOT_FLUSH| if necessary.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: An automation test to check that DidComposite involved invalidation region on the compositor

Great, thinker!

FWIW, I don't have enough time to finish attachment  8763056 [details] [diff] [review], but I have an automation test to check DidComposite involves invalidation region on the compositor.  I couldn't find a way to obtain content windows' top and left position, so this patch checks only width and height in the invalidation region for now.

Comment 25

[Thinker Li [:sinker]]

Attachment: empty-transaction-paintframe.diff

Fix a typo, and remove debugging messages.

Comment 26

[Thinker Li [:sinker]]

Comment on attachment 8765801 [details] [diff] [review]
empty-transaction-paintframe.diff

Hi Matt,

What do you think about that |PaintFrame()| does not do any reflow, restyles, and paints if a |nsRenderingContext| is given and with |ClientLayerManager|?
I don't check if the layers have gone for the window is hidden, now.
Do you have any concern?

Comment 27

[Matt Woodrow (:mattwoodrow)]

(In reply to Thinker Li [:sinker] from comment #26)
> Comment on attachment 8765801 [details] [diff] [review]
> empty-transaction-paintframe.diff
> 
> Hi Matt,
> 
> What do you think about that |PaintFrame()| does not do any reflow,
> restyles, and paints if a |nsRenderingContext| is given and with
> |ClientLayerManager|?
> I don't check if the layers have gone for the window is hidden, now.
> Do you have any concern?

There's no guarantee that DrawWindow should actually draw what is currently displayed in the window (even though that's what reftests do).

If we really need a way to just capture the window, then we should probably add a new API, something along the lines of https://wiki.mozilla.org/User:Roc/ScreenCaptureAPI

Comment 28

[Thinker Li [:sinker]]

Attachment: empty-transaction-paintframe.diff

Fix some bugs, and pass reftests.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e431c4cd7ee4

This patch would check if the window is hidden and |reftest-no-flush| at |PaintFrame()|.  Since |reftest-no-flush| means not to do any flush, it seems reasonable for me to do an empty layer transaction at |PaintFrame()|.

Comment 29

[Thinker Li [:sinker]]

Matt, how do you think if we use |reftest-no-flush| as a hint to take a snapshot from compositor?

Comment 30

[Jet Villegas (inactive)]

+needinfo: Matt per comment 29

Comment 31

[Matt Woodrow (:mattwoodrow)]

As discussed on irc, I don't think we can add new meaning to existing flags since they have users (including ones in addons).

We should either add a new flag, or preferably an entirely new API call since the current set of flags is getting very confusing.

Comment 32

[Thinker Li [:sinker]]

Attachment: Draw on a canvas with an empty transaction for OMTA

Comment 33

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8801678 [details] [diff] [review]
Draw on a canvas with an empty transaction for OMTA

Review of attachment 8801678 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand how this patch adds testing for compositor-side invalidation.

We want to know if the invalid region computed by the compositor (used for d3d11 partial present, and BasicCompositor) is correct.

If we're not sending those regions back so the reftest harness can requests snapshots of them, how are we testing it?

I think this current patch could be simplified to just adding an EndEmptyTransaction path to PresShell::RenderDocument (identical to the one in PresShell::Paint). I don't think adding the new flag actually helps here, since we always try use the compositor if we can.