Bug 72182 – [chromium] Compositor asserts on scroll/zoom changes in single-threaded mode

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Adrienne Walker]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=compositing%2Fscaling%2Ftiled-layer-recursion.html%2Cplatform%2Fchromium%2Fcompositing%2Fzoom-animator-scale-test2.html&group=%40ToT%20GPU%20Mesa%20-%20chromium.org

Revision range is http://trac.webkit.org/log/?verbose=on&rev=99780&stop_rev=99772, which almost certainly points at http://trac.webkit.org/changeset/99774/.

It's failing on CCSingleThreadProxy.cpp:170, where it's assuming that in the single-threaded cases we never get zooms or scales.  Is this not true anymore?

[James Robinson]

We shouldn't get scrolls from the impl side in the single-threaded case. I can debug

[Adrienne Walker]

(In reply to comment #1)
> We shouldn't get scrolls from the impl side in the single-threaded case. I can debug

Thanks.  This probably didn't get noticed because I haven't had a chance to fix those BUGENNE entries on these two.  (Scrollbar issues from the other day.)

[James Robinson]

The problem is here: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp?rev=99774#L204.  That logic changes m_scaleDelta on the impl tree when committing a page scale change, which then gets pushed back to the main thread via the next BFAC since it looks like there's an accumulated scale delta from the impl side.  That looks wrong - afraid I missed it in review :/.  Normally we try to keep deltas on the impl side only and 'real' values on the main side.

I'm not sure exactly how concurrent scale delta + page scale changes are supposed to react, but think I have a fairly easy patch that might work.

[James Robinson]

Created attachment 114778 [details]
Patch

[Alexandre Elias]

After talking it over, LGTM to remove this logic.  But it should be replaced it with setScaleDelta(1), since the delta is changed nowhere else.

[Adrienne Walker]

Comment on attachment 114778 [details]
Patch

I don't think the math works out here.

During begin frame, you send back page scale as pageScale * scaleDelta.  This comes back during commit and sets pageScale to pageScale * scaleDelta, which causes the final scale to be (pageScale * scaleDelta) * scaleDelta.  I think this is why there's the setScaleDelta call up there to prevent that.

I'm wondering if maybe we should be baking the scale delta into the page scale and resetting the scale delta to 1.0 during processScrollDeltas.

[James Robinson]

Comment on attachment 114778 [details]
Patch

still working on this

[Alexandre Elias]

Right, I agree with Enne.  My suggestion of setting scaleDelta to 1 in setPageScale will address that issue for now.  I already saw that this works in Clank: https://gerrit-int.chromium.org/#change,7575

[Adrienne Walker]

(In reply to comment #8)
> Right, I agree with Enne.  My suggestion of setting scaleDelta to 1 in setPageScale will address that issue for now.  I already saw that this works in Clank: https://gerrit-int.chromium.org/#change,7575

Hrm.  I think setting scale delta to 1 during setPageScale will throw away any scale deltas that have accumulated between begin frame and commit.  That's why I suggested doing it during processScaleDeltas.

[James Robinson]

I think the answer is something like what we do for scroll deltas:

If scale changes on main tree, change lth::m_pageScale

If scale changes on impl tree, adjust only lthi::m_scaleDelta

In BFAC send m_scaleDelta in update message and continue to modify m_scaleDelta

When applying the scroll infos, multiply scaleDelta into lth::m_pageScale, unapply the scroll info's delta to lthi::m_scaleDelta, then push new pageScale to lthi and re-apply clamping.

[Alexandre Elias]

(In reply to comment #10)
> I think the answer is something like what we do for scroll deltas:
> 
> If scale changes on main tree, change lth::m_pageScale
> 
> If scale changes on impl tree, adjust only lthi::m_scaleDelta
> 
> In BFAC send m_scaleDelta in update message and continue to modify m_scaleDelta
> 
> When applying the scroll infos, multiply scaleDelta into lth::m_pageScale, unapply the scroll info's delta to lthi::m_scaleDelta, then push new pageScale to lthi and re-apply clamping.

Right.  This is the same to the current logic except that the delta calculation would change to: setScaleDelta(m_scaleDelta / lastSentScrollInfoScaleDelta)

I ran this through a few examples and I agree this will work, including in the weird mid-pinch-zoom cases.

[James Robinson]

Created attachment 114798 [details]
passing deltas back and forth

[James Robinson]

Here's what I have in mind, I need to do some testing before landing.

[Alexandre Elias]

I'm getting "not authorized to edit attachment" error so I can't do a proper inline code review.  Here's a cut-and-paste of my preview, anyway:



Looking good.  Comments from initial readover (haven't tested it yet):



View in context: https://bugs.webkit.org/attachment.cgi?id=114798&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:482
> +    applyScrollsAndScale(layerTreeHost, scrollInfo);

The template intermediaries are confusing.  Can we just call directly from here:

layerTreeHost->applyScrollAndScale(scrollInfo.scrolls[0].scrollDelta, scrollInfo.pageScaleDelta);

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:487
> +    applyScrollsAndScale(layerTreeHostImpl, scrollInfo);

Likewise, how about:

layerTreeHostImpl->unapplyScrollAndScale(scrollInfo.scrolls[0].scrollDelta, scrollInfo.pageScaleDelta);

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:340
>          return scrollInfo.release();

In order to maintain the behavior that pageScale isn't sent down during pinch zoom, we should have in this if() clause: 

scrollInfo->pageScaleDelta = 1;

(Note: things should still work if you don't add this.  It will just do fullscreen repaints as fast as it can.)

[James Robinson]

I want to bottleneck the mapping from id -> layer since I think that will get complicated soon, even though we're only considering the root for now, hence all the template crap.

That may be a premature optimization.

[Alexandre Elias]

I see.  I don't think this template approach is the right way to bottleneck it though.  I think it's generally a bad idea to have a function with a name like XorNotX :).  Probably something like a GetScrollingLayerID() call that both non-template functions used would make more sense.  You could make it assert and return 0 for now.

[Adrienne Walker]

I still think this is a complicated path to be walking down.  To recap: 

(1) fixed layer positions need scroll delta ->
(2) add an apply/unapply step to handle scrolls ->
(3) make scales mimic this behavior ->
(4) template all the things

(3) doesn't necessarily follow from (2).  Also, (1) can be fixed, removing the need for (2) and (4).  I think we should ultimately only need id -> layer lookups on the main thread side of the fence.

[Alexandre Elias]

That's not exactly the reasoning here.  It's more that for many different reasons, we require a way to distinguish events (scroll or scale) originating from the impl side, versus originating from the webkit side.  I think this is an elegant way to achieve that.

The alternate path I was envisioning earlier was complex in a different way: I was going to enforce the impl side was the canonical source of scale changes, and the webkit side could only effect a change by first sending a message to impl thread.  This would've worked (and does in the browser scrolling world), but the problem with it is that it's inconsistent with the scrolling model (where WebKit can change the offset whenever it wants).

[James Robinson]

(In reply to comment #18)
> That's not exactly the reasoning here.  It's more that for many different reasons, we require a way to distinguish events (scroll or scale) originating from the impl side, versus originating from the webkit side.  I think this is an elegant way to achieve that.

We already have that by maintaining deltas on the impl side (scrollDelta / scaleDelta) - those are for impl-side changes, everything else is webkit side.  It worked pretty well except that Alpha put a hack in for fixedpos and we've been living with that.

After some reflection I agree with Adrienne that we'd be better off killing that hack and going back to the old world, that is:

1.) LTH maintains m_pageScale, LTHI maintains m_pageScale and m_pageScaleDelta

2.) On impl-side scaling, LTHI modifies m_pageScaleDelta and sets needs commit bit

3.) When generating BFAC message LTHI passes m_pageScaleDelta in the ScrollInfo bundle and multiplies it into LTHI::m_pageScale. LTHI::m_pageScaleDelta then reset to 1

4.) If any thread updates happen while the commit is pending, they go into m_pageScaleDelta

5.) On main thread LTH multiplies ScrollInfo's pageScaleDelta into its m_pageScale (thus combining it with any main-thread changes that may have been made since the last commit).

6.) Property push pushes LTH's m_pageScale to LTHI's m_pageScale

Impl side now has the most recent main thread scale in LTHI::m_pageScale and the pending (not yet committed) deltas in m_pageScaleDelta.



If we really need to track changes on the impl side separately while a commit is pending for some reason then we can store them in another var alongside m_pageScaleDelta (m_pendingPageScaleDelta or something), but multiplying it into LTHI::m_pageScale in step 3 should be fine

[Alexandre Elias]

I think it's weird to hack m_scrollPosition and m_pageScale on the impl side at BFAC-time.  It seems cleaner to only sync them during the regular commit process, like everything else.  If we change them ahead of time, we might run into bugs when our assumption about what they are "going to be" is wrong.

For example, what if we send a pinch zoom intermediate BFAC, and then before the commit completes, the user ends the pinch zoom and we send another BFAC and set scaleDelta = 1.0?  If we then receive the commit and set m_pageScale to the old value, we'll see a few frames where the user-visible scale is wrong (before the final m_pageScale arrives).  Does the commit flow prevent this from ever happening (i.e. we never issue a second BFAC before the commit completes)?

[James Robinson]

(In reply to comment #20)
> I think it's weird to hack m_scrollPosition and m_pageScale on the impl side at BFAC-time.  It seems cleaner to only sync them during the regular commit process, like everything else.  If we change them ahead of time, we might run into bugs when our assumption about what they are "going to be" is wrong.
> 
> For example, what if we send a pinch zoom intermediate BFAC, and then before the commit completes, the user ends the pinch zoom and we send another BFAC and set scaleDelta = 1.0?  If we then receive the commit and set m_pageScale to the old value, we'll see a few frames where the user-visible scale is wrong (before the final m_pageScale arrives).  Does the commit flow prevent this from ever happening (i.e. we never issue a second BFAC before the commit completes)?

Correct, one pending commit at a time.  A whole lot more depends on than that this one feature.

[James Robinson]

Rolled https://bugs.webkit.org/show_bug.cgi?id=71916 out.  Let's back up and do this right from the beginning.

[Alexandre Elias]

OK, good.  Here's a second problem: if we hack m_pageScale and scaleDelta in BFAC, then the pageScaleMatrix will be incorrect until the commit arrives.  scaleDelta needs to always be relative to the tile scale (as painted).  The scroll offset transformation will also be problematic (if done in BFAC, it will be incorrect until the commit arrives; if done during commit, then we don't know what to multiply it by).

[James Robinson]

(In reply to comment #20)
> I think it's weird to hack m_scrollPosition and m_pageScale on the impl side at BFAC-time.

You can think of them as being kept separately if it helps you sleep better at night, although mathematically it works out the same way.

The high level bit to optimize for is keeping the data flow simple and unidirectional.

The impl thread accumulates deltas and passes them to the main thread via BFAC

The main thread maintains values that can be manipulated from WebKit. In the commit, it combines this value with the delta and passes back a new value via the property commit step.

That's all there is, there are no additional ways to pass state or values around.

[Adrienne Walker]

(In reply to comment #24)

> The high level bit to optimize for is keeping the data flow simple and unidirectional.

So very much yes.  Thanks for calling out the real goal here.

[Alexandre Elias]

(In reply to comment #24)
> (In reply to comment #20)
> > I think it's weird to hack m_scrollPosition and m_pageScale on the impl side at BFAC-time.
> 
> You can think of them as being kept separately if it helps you sleep better at night, although mathematically it works out the same way.

I think we do *actually* need to keep them separately, in order to solve the problem I just mentioned in #23.  If you're willing to add that extra field on the impl-side, then I think the old-style commit flow will work.

[James Robinson]

(In reply to comment #23)
> OK, good.  Here's a second problem: if we hack m_pageScale and scaleDelta in BFAC, then the pageScaleMatrix will be incorrect until the commit arrives.  scaleDelta needs to always be relative to the tile scale (as painted).  The scroll offset transformation will also be problematic (if done in BFAC, it will be incorrect until the commit arrives; if done during commit, then we don't know what to multiply it by).

For scroll offsets, I believe that we need to adjust it whenever the product pageScale * scaleDelta changes, since it's maintained in that space.  That can change when the scaleDelta is changed on the thread by an input handler or animation system or whatever or when a new pageScale is comitted, but it doesn't change in the BFAC since doing:

pageScale *= scaleDelta;
scaleDelta = 1;

won't change the product.  So it doesn't add any additional handling - we need to tweak it when changing scaleDelta and in the commit, but we have that logic anyway.

I'm less certain about the pageScaleMatrix, but doesn't it depend on the same product?

If it ends up being helpful we can definitely keep 3 data bags on the impl side:

pageScale - value from the last commit, never mutated outside commit
scaleDelta - value manipulated directly on the thread by input, animation systems, etc
pendingCommitScaleDelta - value of scaleDelta as of the last BFAC message generation

when generating BFAC do this:

scrollInfo.pageScaleDelta = scaleDelta
pendingCommitScaleDelta = scaleDelta
scaleDelta = 1

then the complete scale at any point in time is pageScale * scaleDelta * pendingCommitScaleDelta

I think we'll do something like that short-term for scroll deltas, although longer term we don't need to.  I'm not sure whether this is necessary for scales or not.

[Alexandre Elias]

(In reply to comment #27)
> (In reply to comment #23)
> > OK, good.  Here's a second problem: if we hack m_pageScale and scaleDelta in BFAC, then the pageScaleMatrix will be incorrect until the commit arrives.  scaleDelta needs to always be relative to the tile scale (as painted).  The scroll offset transformation will also be problematic (if done in BFAC, it will be incorrect until the commit arrives; if done during commit, then we don't know what to multiply it by).
> 
> For scroll offsets, I believe that we need to adjust it whenever the product pageScale * scaleDelta changes, since it's maintained in that space.

Actually, scroll offsets are currently maintained in pageScale space (not pageScale * pageDelta).  That's why we only need to multiply them at commit-time, instead of on every pinch event.

This is not necessarily the best space to keep them in, it was just the way that involved the least extra code and the least prone to float -> int rounding errors.  If we turn the scroll offsets into floats, we can switch to maintaining them in normalized-as-if-scale=1 space and avoid this whole problem, like I mentioned a few weeks ago when I was first thinking about this.  (I wouldn't support pageScale * scaleDelta space -- we would need to mutate the scroll offsets too often.)

There's no hurry though -- provided we keep the "pending" value, the current pageScale-space approach will continue to work.


> I'm less certain about the pageScaleMatrix, but doesn't it depend on the same product?

pageScaleMatrix is currently simply the matrix expression of scaleDelta and nothing else.  If we introduce pendingCommitScaleDelta, then pageScaleMatrix would have to be scaleDelta * pendingCommitScaleDelta instead.

> 
> If it ends up being helpful we can definitely keep 3 data bags on the impl side:
> 
> pageScale - value from the last commit, never mutated outside commit
> scaleDelta - value manipulated directly on the thread by input, animation systems, etc
> pendingCommitScaleDelta - value of scaleDelta as of the last BFAC message generation
> 
> when generating BFAC do this:
> 
> scrollInfo.pageScaleDelta = scaleDelta
> pendingCommitScaleDelta = scaleDelta
> scaleDelta = 1
> 
> then the complete scale at any point in time is pageScale * scaleDelta * pendingCommitScaleDelta
> 
> I think we'll do something like that short-term for scroll deltas, although longer term we don't need to.  I'm not sure whether this is necessary for scales or not.

I'm fine with this proposal.  I was thinking of a slightly different variant, having "m_pendingCommitPageScale" instead, and this field would be equal to m_pageScale at all times except for in between BFAC and commit -- when it represents the "future" scale.  But either that or the pending delta would by fine by me (just the math changes a little).

[Hin-Chung Lam]

(In reply to comment #22)
> Rolled https://bugs.webkit.org/show_bug.cgi?id=71916 out.  Let's back up and do this right from the beginning.

Sorry for the confusion with this change! After some more thinking this hack makes it very difficult to work with page scale.. In fact using a transient state like scroll delta is not a reliable approach..