732091 - MAPLE: Make the compositor know when it's painting a new page

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Currently, when navigation to a new page happens, we need to make sure that java gets the new zoom at the right time. The right time is the first time we composite with the new content.

Therefore, the compositor must be the one to tell java to reset its zoom. To do this, the compositor must have some way of knowing when it is supposed to do this.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

We will probably do this by having something in DOMWindowUtils that sets a bit on the presShell. We'll do this at the same time that we set the resolution.

Comment 2

[Ali Juma [:ajuma]]

Attachment: Part 1: Add isFirstPaint to the PLayers update message

Comment 3

[Ali Juma [:ajuma]]

Attachment: Part 2: Add setIsFirstPaint to nsIDOMWindowUtils and nsIPresShell

Comment 4

[Ali Juma [:ajuma]]

Attachment: Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4: Expose functions on GeckoLayerClient for compositor to call with updates

This compiles but I haven't run it so there may be bugs.

Comment 6

[Ali Juma [:ajuma]]

Attachment: Part 5: Make the compositor notify Java when the page size changes or a first paint occurs.

Comment 7

[Ali Juma [:ajuma]]

Attachment: Part 2: Add {set,get}IsFirstPaint to nsIDOMWindowUtils and nsIPresShell

Comment 8

[Ali Juma [:ajuma]]

Attachment: Part 1: Add isFirstPaint to the PLayers update message

Comment 9

[Ali Juma [:ajuma]]

Attachment: Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

Comment 10

[Ali Juma [:ajuma]]

Attachment: Part 5: Make the compositor notify Java when the page size changes or a first paint occurs.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 602508 [details] [diff] [review]
Part 5: Make the compositor notify Java when the page size changes or a first paint occurs.

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp
>+#ifdef MOZ_WIDGET_ANDROID
>+  if (mIsFirstPaint) {
>+    SendViewport();
>+    mIsFirstPaint = false;
>+    mIsContentSizeChanged = false;
>+  } else if (mIsContentSizeChanged) {
>+    mozilla::AndroidBridge::Bridge()->SetPageSize(mContentSize.width, mContentSize.height);
>+    mIsContentSizeChanged = false;
>+  }
>+#endif
>+

I think we want to do this before calling GetViewTransform. That way we set the updated viewport in Java, and allow it to merge the old and new viewport as necessary, and then we composite using the new merged viewport which will be obtained by GetViewTransform.

>+void
>+CompositorParent::SendViewport()
>+{
>+  Layer* layer = GetPrimaryScrollableLayer();
>+  ContainerLayer* container = layer->AsContainerLayer();
>+  const FrameMetrics* metrics = &container->GetFrameMetrics();
>+
>+  if (metrics && metrics->IsScrollable()) {
>+    float zoom = 1/GetXScale(mLayerManager->GetRoot()->GetTransform());
>+    nsIntPoint scrollOffset = metrics->mViewportScrollOffset;
>+    mozilla::AndroidBridge::Bridge()->SetFirstPaintViewport(scrollOffset.x, scrollOffset.y,
>+                                                            zoom, mContentSize.width,
>+                                                            mContentSize.height);
>+  }

For some reason I'm seeing metrics->IsScrollable() return false, and so SetFirstPaintViewport isn't getting called. Locally I had to comment out the latter half of the if condition to get it to work.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 602506 [details] [diff] [review]
Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -5533,17 +5533,22 @@ PresShell::Paint(nsIView*           aVie
>     nsPresContext* pc = GetPresContext();
>     nsIntRect bounds =
>       pc->GetVisibleArea().ToOutsidePixels(pc->AppUnitsPerDevPixel());
>     bgcolor = NS_ComposeColors(bgcolor, mCanvasBackgroundColor);
>     root->SetColor(bgcolor);
>     root->SetVisibleRegion(bounds);
>     layerManager->SetRoot(root);
>   }
>-  layerManager->EndTransaction(NULL, NULL);
>+  if (mIsFirstPaint) {
>+    layerManager->EndTransaction(NULL, NULL, LayerManager::END_IS_FIRST_PAINT);
>+    mIsFirstPaint = false;
>+  } else {
>+    layerManager->EndTransaction(NULL, NULL);
>+  }
> 

I don't see it going into this code path. It always seems to go into one of the other exits from PresShell:Paint - sometimes it goes into the layerManager->EndEmptyTransaction() code path and sometimes it goes into nsLayoutUtils::PaintFrame. I had to apply a patch similar to the one above to the PaintForFrame function in nsDisplayList.cpp so that it actually passes the flag along to the compositor.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 602506 [details] [diff] [review]
Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp
> void
>-CompositorParent::ShadowLayersUpdated()
>+CompositorParent::ShadowLayersUpdated(bool isFirstPaint)
> {
>+  mIsFirstPaint = isFirstPaint;
>   printf_stderr("ShadowLayersUpdated\n");

Also, should this be "mIsFirstPaint = mIsFirstPaint || isFirstPaint;"? While looking through the code it seemed like if we get multiple layer updates within the 15ms composition timer interval, and the last of these updates doesn't have the isFirstPaint flag set, then it could potentially clobber the flag getting set on one of the previous updates in that interval. Not really sure about this though; I haven't seen any behaviour that might result from this.

Comment 14

[Ali Juma [:ajuma]]

(In reply to Kartikaya Gupta (:kats) from comment #13) 
> >diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp
> > void
> >-CompositorParent::ShadowLayersUpdated()
> >+CompositorParent::ShadowLayersUpdated(bool isFirstPaint)
> > {
> >+  mIsFirstPaint = isFirstPaint;
> >   printf_stderr("ShadowLayersUpdated\n");
> 
> Also, should this be "mIsFirstPaint = mIsFirstPaint || isFirstPaint;"? While
> looking through the code it seemed like if we get multiple layer updates
> within the 15ms composition timer interval, and the last of these updates
> doesn't have the isFirstPaint flag set, then it could potentially clobber
> the flag getting set on one of the previous updates in that interval.

Yes, good catch, I'll fix this. (Though if we ever find that we're actually getting multiple layer updates within 15 ms intervals, we should add throttling to prevent this!)

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4: Expose functions on GeckoLayerClient for compositor to call with updates

Fix my JNI patch so that it actually works.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (:kats) from comment #11)
> 
> For some reason I'm seeing metrics->IsScrollable() return false, and so
> SetFirstPaintViewport isn't getting called. Locally I had to comment out the
> latter half of the if condition to get it to work.

This also affects the mIsContentSizeChanged setter block in CompositorParent::TransformShadowTree. Note also that mIsContentSizeChanged isn't initialized on CompositorParent construction and starts with a garbage value.

Comment 17

[Ali Juma [:ajuma]]

Attachment: Part 5: Make the compositor notify Java when the page size changes or a first paint occurs.

This makes the notifications to Java happen before GetViewTransform(), and removes the dependency on IsScrollable().

Comment 18

[Ali Juma [:ajuma]]

Attachment: Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

This removes the dependency on the specific code path taken from PresShell::Paint to ShadowLayerForwarder::EndTransaction, and also addresses comment 13.

Comment 19

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

QA+ : please check reload + rotating the device while waiting for page load as well when this lands.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

ajuma: I found a few more issues.

One is that in the call to SetFirstPaintViewport, the zoom being passed in (rootScaleX) is actually the reciprocal of what it should be. i.e. when browser.js calls setResolution with 2.0, the rootScaleX comes out as 0.5. Just taking the reciprocal in the call to SetFirstPaintViewport fixes this. I'm not sure if this affects other things too.

The other issue I noticed while debugging the page size problems (and which may never actually happen with that fixed). If metrics->IsScrollable is false, then we go into the code path where the scrollCompensation is 0,0 even if Java provides a non-0,0 view transform. This was resulting in scenarios were I was moving my finger on the page, and the scrollbars were updating appropriately, but the content itself was always stuck at 0,0. Again, I don't know if this will ever happen with the display port set but it might be worth fixing by setting a proper scrollCompensation in the non-scrollable case as well.

The third issue is one I should have realized earlier: when the compositor calls SetPageSize with the mContentSize, that size may be based on a zoom value that is different from the zoom value Java has. So I think the SetPageSize function needs to change to take the zoom as well, and then Java can compare the zoom values and adjust the page size accordingly. I can take care of this change, it will be mostly contained in the JNI patch I uploaded.

Comment 21

[Ali Juma [:ajuma]]

(In reply to Kartikaya Gupta (:kats) from comment #20)
> ajuma: I found a few more issues.
> 
> One is that in the call to SetFirstPaintViewport, the zoom being passed in
> (rootScaleX) is actually the reciprocal of what it should be. i.e. when
> browser.js calls setResolution with 2.0, the rootScaleX comes out as 0.5.
> Just taking the reciprocal in the call to SetFirstPaintViewport fixes this.
> I'm not sure if this affects other things too.

Oops, I meant to take the reciprocal of rootScaleX there. Will fix.

> The other issue I noticed while debugging the page size problems (and which
> may never actually happen with that fixed). If metrics->IsScrollable is
> false, then we go into the code path where the scrollCompensation is 0,0
> even if Java provides a non-0,0 view transform. This was resulting in
> scenarios were I was moving my finger on the page, and the scrollbars were
> updating appropriately, but the content itself was always stuck at 0,0.
> Again, I don't know if this will ever happen with the display port set but
> it might be worth fixing by setting a proper scrollCompensation in the
> non-scrollable case as well.

Trying to scroll a non-scrollable layer is a bad idea, since there's no displayport set on such a layer, so if we translate the layer we're going to see a black region on the screen. Instead, how about adding an argument to SetFirstPaintViewport to notify Java whether the current page is scrollable?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ali Juma [:ajuma] from comment #21)
> Trying to scroll a non-scrollable layer is a bad idea, since there's no
> displayport set on such a layer, so if we translate the layer we're going to
> see a black region on the screen. Instead, how about adding an argument to
> SetFirstPaintViewport to notify Java whether the current page is scrollable?

Then it sounds like if we leave the code as-is it should be fine. If the issue I was seeing comes back then we can figure out what to do about it.

Comment 23

[Ali Juma [:ajuma]]

Attachment: Part 5: Make the compositor notify Java when the page size changes or a first paint occurs

This sends the right zoom to Java.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

Rebase part 3 on top of latest maple; no other changes.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4: Expose functions on GeckoLayerClient for compositor to call with updates

Update setPageSize JNI function to also take a zoom factor along with the page size.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5: Make the compositor notify Java when the page size changes or a first paint occurs

Update part 5 to pass in the zoom as well when calling setPageSize.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4: Expose functions on GeckoLayerClient for compositor to call with updates

Rebase part 4 on top of maple (mostly to deal with the ImmutableViewport).

Comment 28

[Ali Juma [:ajuma]]

See https://wiki.mozilla.org/Fennec/NativeUI/Viewport for the overall plan this bug is part of, and see Bug 732564 for what Java is going to do with the knowledge it will now have about first paints and page resizes.

Comment 29

[Benoit Girard (:BenWa)]

Comment on attachment 602505 [details] [diff] [review]
Part 1: Add isFirstPaint to the PLayers update message

Cjones designed this API. Maybe we can pass this part of Edit[] instead of adding more APIs.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 602500 [details] [diff] [review]
Part 2: Add {set,get}IsFirstPaint to nsIDOMWindowUtils and nsIPresShell


>+  if (presShell) {
>+    presShell->SetIsFirstPaint();
>+    return NS_OK;
>+  } else {
else after return.


But I don't understand the patch. Some layout peer should
review the presshell part.

>+nsDOMWindowUtils::GetIsFirstPaint(bool *aIsFirstPaint)
>+{
>+  if (!IsUniversalXPConnectCapable()) {
>+    return NS_ERROR_DOM_SECURITY_ERR;
>+  }
>+
>+  nsIPresShell* presShell = GetPresShell();
>+  if (presShell) {
>+    *aIsFirstPaint = presShell->GetIsFirstPaint();
>+    return NS_OK;
>+  } else {
else after return.

Comment 31

[Chris Lord [:cwiiis]]

Comment on attachment 603839 [details] [diff] [review]
Part 4: Expose functions on GeckoLayerClient for compositor to call with updates

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

Looks fine to me.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +327,5 @@
> +        synchronized (mLayerController) {
> +            ViewportMetrics currentMetrics = new ViewportMetrics(mLayerController.getViewportMetrics());
> +            currentMetrics.setOrigin(new PointF(offsetX, offsetY));
> +            currentMetrics.setZoomFactor(zoom);
> +            currentMetrics.setPageSize(new FloatSize(pageWidth, pageHeight));

Maybe a bit overkill to make a copy of the layer controller's viewport metrics just for the viewport size?

Comment 32

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #31)
> Part 4: Expose functions on GeckoLayerClient for compositor to call with
> updates
> 
> Maybe a bit overkill to make a copy of the layer controller's viewport
> metrics just for the viewport size?

It's a little harder to get rid of this here; we need to create at least one ViewportMetrics object at some point (or add another constructor to ImmutableViewportMetrics). The alternative I see is to do | currentMetrics = new ViewportMetrics() | instead of | currentMetrics = new ViewportMetrics(mLayerController.getViewportMetrics()) | and then also set the size on currentMetrics. That seems more expensive though because the default constructor for ViewportMetrics still populates itself using a DisplayMetrics object.

I'd rather leave this for now and revisit the whole ViewportMetrics class; I think there's parts of it that we can get rid of now and simplify the code further.

Comment 33

[Benoit Girard (:BenWa)]

Comment on attachment 602505 [details] [diff] [review]
Part 1: Add isFirstPaint to the PLayers update message

This looks fine to me, we need this information to handle this case correctly. We can refactor it in a follow up bug if we're not happy with having it here.

Comment 34

[Benoit Girard (:BenWa)]

Comment on attachment 602505 [details] [diff] [review]
Part 1: Add isFirstPaint to the PLayers update message

Actually can we get a comment that explains exactly what isFirstPaint implies in this signature? This will make this change easier to understand for new developers.

Comment 35

[(no longer active)]

Comment on attachment 602500 [details] [diff] [review]
Part 2: Add {set,get}IsFirstPaint to nsIDOMWindowUtils and nsIPresShell

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

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +198,5 @@
> +   * The caller of these methods must have UniversalXPConnect
> +   * privileges.
> +   */
> +  void setIsFirstPaint();
> +  boolean getIsFirstPaint();

This should just be a boolean attribute.

::: layout/base/nsIPresShell.h
@@ +1153,5 @@
> +
> +  /**
> +   * Get the isFirstPaint flag.
> +   */
> +  bool GetIsFirstPaint() { return mIsFirstPaint; }

Make this const.

Comment 36

[Benoit Girard (:BenWa)]

Comment on attachment 603414 [details] [diff] [review]
Part 5: Make the compositor notify Java when the page size changes or a first paint occurs

I feel like we should have better comments in TransformShadowTree to explain what is going on, it's not evident having a few read throughs.

Comment 37

[Ali Juma [:ajuma]]

Attachment: Part 1: Add isFirstPaint to the PLayers update message

Patch updated to address review comments. Carrying forward r=bgirard.

Comment 38

[Ali Juma [:ajuma]]

Attachment: Part 2: Add {set,get}IsFirstPaint to nsIDOMWindowUtils and nsIPresShell

Patch updated to address smaug's and ehsan's comments.

Comment 39

[Ali Juma [:ajuma]]

Attachment: Part 5: Make the compositor notify Java when the page size changes or a first paint occurs

Patch updated to address review comments. Carrying forward r=bgirard.

Comment 40

[Timothy Nikkel (:tnikkel)]

Comment on attachment 603412 [details] [diff] [review]
Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

I don't know the shadow layers code very well, so someone who does should review that part, although it looks reasonable to me.

Comment 41

[Ali Juma [:ajuma]]

Comment on attachment 603412 [details] [diff] [review]
Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

Requesting review for the shadow layers part of this patch (see Comment 40).

Comment 42

[Benoit Girard (:BenWa)]

Comment on attachment 603412 [details] [diff] [review]
Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

r+ but similar to my first comment I'd like to see a comment somewhere in this patch queue explaining the implication of the first paint to make the code easier to understand.

Comment 43

[Ali Juma [:ajuma]]

Attachment: Part 3: Pass on the isFirstPaint flag from the PresShell to the compositor via the layer manager

Patch updated to address review comments. Carrying forward r=tnikkel,bgirard.

Comment 44

[(no longer active)]

Comment on attachment 604476 [details] [diff] [review]
Part 2: Add {set,get}IsFirstPaint to nsIDOMWindowUtils and nsIPresShell

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

This looks good to me, but I'd like roc to take a look at this post landing and let us know if he has comments on this.

Comment 45

[(no longer active)]

roc, please see comment 44.

Comment 46

[Ali Juma [:ajuma]]

https://hg.mozilla.org/projects/maple/rev/e1a8ff015736
https://hg.mozilla.org/projects/maple/rev/c872eaa32896
https://hg.mozilla.org/projects/maple/rev/bffa115bc37b
https://hg.mozilla.org/projects/maple/rev/b5e523571e45
https://hg.mozilla.org/projects/maple/rev/86d34ff552a7

Comment 47

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 604422 [details] [diff] [review]
Part 1: Add isFirstPaint to the PLayers update message

This feels very wrong.  You need this same information for sub-frames too, right?

I would recommend keeping a paintCount as part of FrameMetrics, and do what you need when paintCount == 1.  (I don't quite understand the problem you're solving here.)

Comment 48

[Ali Juma [:ajuma]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #47)
> Comment on attachment 604422 [details] [diff] [review]
> Part 1: Add isFirstPaint to the PLayers update message
> 
> This feels very wrong.  You need this same information for sub-frames too,
> right?

If we allow sub-frames to be zoomed independently of the rest of the page (which we don't yet on native Fennec), then yes, we'd need this information.
 
> I would recommend keeping a paintCount as part of FrameMetrics, and do what
> you need when paintCount == 1.

Sure, I'll prepare a patch that removes isFirstPaint from PLayers and adds a paintCount to FrameMetrics. In this case, paintCount would really mean "number of paints since the isFirstPaint flag was last set".

> (I don't quite understand the problem you're
> solving here.)

The problem is that Java sends the compositor a requested view transform, but doesn't know exactly when what's being composited changes from one document to the next (e.g. as a result of loading a new page or switching tabs). As a result, without knowing about the first paint following a page load or table switch, Java can ask for zooms or scroll offsets that don't make sense for the currently displayed page (e.g it can ask for a zoom that makes the page look blurry, since it is inconsistent with the page's resolution). See Bug 732564 and https://wiki.mozilla.org/Fennec/NativeUI/Viewport for more about this.

Comment 49

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK.  Possibly a better solution to this problem is to give frames unique IDs, and then discard proposed shadow layer transforms if the requested IDs don't match the frame IDs in the shadow tree.

Comment 50

[Ali Juma [:ajuma]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #49)
> OK.  Possibly a better solution to this problem is to give frames unique
> IDs, and then discard proposed shadow layer transforms if the requested IDs
> don't match the frame IDs in the shadow tree.

Ah, thanks Chris! We already have a unique ID for each scrollable frame in FrameMetrics. This gives us the information we need without requiring an isFirstPaint flag to be sent to the compositor. I'm filing a follow-up bug to remove the isFirstPaint flag and use the information we already have in FrameMetrics instead.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree with comment #49. In fact, we already have IDs (see nsLayoutUtils::FindIDFor), so maybe we can just use those.

Comment 52

[Ali Juma [:ajuma]]

Filed Bug 735029 to remove the isFirstPaint flag and instead use the frame IDs we already have.