980207 - When the refresh driver is under test control, don't force updates to the compositor thread unless they would run otherwise

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Brian Birtles (:birtles)]

I've noticed some bugs in regular usage of OMTA. However, making a reproducible test case is difficult because code in CompositorParent forces updates on the compositor thread when the refresh driver is under test control that do not occur in regular usage.

I would like to alter the code in CompositorParent to better match the behavior in regular usage. For example, by only doing an update if there is already a composite scheduled.

(Arguably these sort of bugs could be reproduced using reftests but we are currently unable to control the refresh driver in reftests due to our policy of making reftests, as best as is possible, cross-browser. We could try and set up each test so that it reaches the bug scenario within a few milliseconds and then snapshot it then but that approach is often flaky and time-consuming since we have to allow a generous window of time to be sure the animation has actually started. So, I think it's appropriate to adapt the testing methods here to allow testing these sort of scenarios using mochitests.)

Comment 1

[Brian Birtles (:birtles)]

Attachment: When the refresh driver is under test control, make compositor thread only update when a composite is scheduled

In order to make test behavior better match real-world sampling behavior, this
patch updates the test code in CompositorParent so that it only updates the
layer when a composite is scheduled. This enables creating mochitests that
reproduce bugs observed in regular usage.

Comment 2

[David Zbarsky (:dzbarsky)]

Comment on attachment 8386592 [details] [diff] [review]
When the refresh driver is under test control, make compositor thread only update when a composite is scheduled

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

LGMT, but do you think it's worth adding another method or a boolean param that would force a flush?  Probably not worth worrying about until we need it...

::: gfx/layers/ipc/CompositorParent.cpp
@@ +384,5 @@
> +      mCurrentCompositeTask->Cancel();
> +      mCurrentCompositeTask = nullptr;
> +    }
> +  }
> +

You repeat this pattern twice, factor it out?

Comment 3

[Brian Birtles (:birtles)]

(In reply to David Zbarsky (:dzbarsky) from comment #2)
> Comment on attachment 8386592 [details] [diff] [review]
> ...
> LGMT, but do you think it's worth adding another method or a boolean param
> that would force a flush?  Probably not worth worrying about until we need
> it...

Yeah, I'd rather leave this until we need it. I think it's best if, by default, we test in conditions that are as close as possible to regular sampling behavior. If we need to force a flush we should first stop and ask why. (i.e. why isn't just waiting for the next paint enough?)

> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +384,5 @@
> > +      mCurrentCompositeTask->Cancel();
> > +      mCurrentCompositeTask = nullptr;
> > +    }
> > +  }
> > +
> 
> You repeat this pattern twice, factor it out?

Sounds good. I'll add that next week.

Comment 4

[Brian Birtles (:birtles)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96f0f09c23c0

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96f0f09c23c0