Closed Bug 732091 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jrmuizel, Assigned: ajuma)

References

(Depends on 1 open bug)

Details

(Whiteboard: maple, qa+)

Attachments

(5 files, 16 obsolete files)

9.62 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
3.08 KB, patch
ajuma
: review+
cjones
: feedback-
Details | Diff | Splinter Review
6.09 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
4.67 KB, patch
ajuma
: review+
Details | Diff | Splinter Review
11.38 KB, patch
ajuma
: review+
Details | Diff | Splinter Review
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.
Blocks: 731897
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.
Attachment #602444 - Attachment is obsolete: true
Attachment #602500 - Attachment description: Add {set,get}IsFirstPaint to nsIDOMWindowUtils and nsIPresShell → Part 2: Add {set,get}IsFirstPaint to nsIDOMWindowUtils and nsIPresShell
Attachment #602442 - Attachment is obsolete: true
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.
Attachment #602508 - Flags: review-
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.
Attachment #602506 - Flags: review-
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.
(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!)
(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.
This makes the notifications to Java happen before GetViewTransform(), and removes the dependency on IsScrollable().
Attachment #602508 - Attachment is obsolete: true
This removes the dependency on the specific code path taken from PresShell::Paint to ShadowLayerForwarder::EndTransaction, and also addresses comment 13.
Attachment #602506 - Attachment is obsolete: true
QA+ : please check reload + rotating the device while waiting for page load as well when this lands.
Whiteboard: maple → maple, qa+
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.
(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?
(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.
This sends the right zoom to Java.
Attachment #603043 - Attachment is obsolete: true
Rebase part 3 on top of latest maple; no other changes.
Attachment #603300 - Attachment is obsolete: true
Update setPageSize JNI function to also take a zoom factor along with the page size.
Attachment #602653 - Attachment is obsolete: true
Update part 5 to pass in the zoom as well when calling setPageSize.
Attachment #603388 - Attachment is obsolete: true
Rebase part 4 on top of maple (mostly to deal with the ImmutableViewport).
Attachment #603413 - Attachment is obsolete: true
Attachment #602505 - Flags: review?(bgirard)
Attachment #603412 - Flags: review?(matt.woodrow)
Attachment #603414 - Flags: review?(bgirard)
Attachment #602500 - Flags: review?(bugs)
Attachment #603839 - Flags: review?(chrislord.net)
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 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.
Attachment #602505 - Flags: feedback?(jones.chris.g)
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.
Attachment #602500 - Flags: review?(bugs) → review?(dbaron)
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?
Attachment #603839 - Flags: review?(chrislord.net) → review+
(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 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.
Attachment #602505 - Flags: review?(bgirard)
Attachment #602505 - Flags: review+
Attachment #602505 - Flags: feedback?(jones.chris.g)
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 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 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.
Attachment #603414 - Flags: review?(bgirard) → review+
Attachment #603412 - Flags: review?(matt.woodrow) → review?(tnikkel)
Attachment #602500 - Flags: review?(dbaron)
Patch updated to address review comments. Carrying forward r=bgirard.
Attachment #602505 - Attachment is obsolete: true
Attachment #604422 - Flags: review+
Patch updated to address smaug's and ehsan's comments.
Attachment #602500 - Attachment is obsolete: true
Attachment #604476 - Flags: review?(ehsan)
Patch updated to address review comments. Carrying forward r=bgirard.
Attachment #603414 - Attachment is obsolete: true
Attachment #604483 - Flags: review+
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.
Attachment #603412 - Flags: review?(tnikkel) → review+
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).
Attachment #603412 - Flags: review?(bgirard)
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.
Attachment #603412 - Flags: review?(bgirard) → review+
Patch updated to address review comments. Carrying forward r=tnikkel,bgirard.
Attachment #603412 - Attachment is obsolete: true
Attachment #604912 - Flags: review+
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.
Attachment #604476 - Flags: review?(ehsan) → review+
roc, please see comment 44.
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.)
Attachment #604422 - Flags: feedback-
(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.
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.
(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.
I agree with comment #49. In fact, we already have IDs (see nsLayoutUtils::FindIDFor), so maybe we can just use those.
Depends on: 735029
Filed Bug 735029 to remove the isFirstPaint flag and instead use the frame IDs we already have.
You need to log in before you can comment on or make changes to this bug.