Closed Bug 980207 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(1 file)

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.)
Blocks: 975261
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.
Attachment #8386592 - Flags: review?(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?
Attachment #8386592 - Flags: review?(dzbarsky) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/96f0f09c23c0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.