Fix and test ThebesLayer to ImageLayer optimization

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

unspecified
mozilla10
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(4 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 567689 [details] [diff] [review]
Fix

This gets this working again, and fixed some problems that I found with multiple processes.

We still need to think of a way to test this so that it doesn't get broken again.

Asking for feedback from cjones since I think that the 'does buffer contain the image already' check may not be thorough enough.
Attachment #567689 - Flags: review?(roc)
Attachment #567689 - Flags: feedback?(jones.chris.g)
(Assignee)

Updated

6 years ago
Blocks: 586683, 689760
Comment on attachment 567689 [details] [diff] [review]
Fix

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +2492,5 @@
>        NS_RUNTIMEABORT("creating ImageLayer 'front buffer' failed!");
> +  } else if (!paintTime.IsNull()) {
> +    // If the image hasn't changed then we already have it in our backbuffer
> +    // We probably also need to check that the image container hasn't changed, and
> +    // that it wasn't painted by something else.

The image container for the layer can't change.

I'm not sure why it matters whether it was painted by something else.

It seems to me that the PlanarYCbCr path doesn't call NotifyPaintedImage so this won't work there.

Otherwise this should be OK, although using paintTime is a little obscure. Seems to me it would make more sense to add an explicit "dirty" flag to the ImageContainer which is set whenever the image changes, and use that flag directly from BasicShadowableImageLayer::Paint to control when the image is re-uploaded.
The rest looks OK and you should split out the layout changes from the ShadowableImageLayer changes into separate patches.

I wonder what is the best way to test this kind of thing. One possibility would be to define a new frame state bit which means "a display item for this frame was painted", and set that state bit from FrameLayerBuilder::DrawThebesLayer's drawing loop (super-cheap). Then add an API nsDOMWindowUtils::CheckAndClearPaintedState(element) which walks the frame subtree for the element, checking to see if any of those painted bits are set, and clears them as it goes. Then we could clear the bits, do some DOM operations, wait for an after-paint event, and check the bits to make sure they haven't been set.

This would be useful for testing scrolling, and also testing invalidation --- anywhere we need to make sure we're not painting too much.
(Assignee)

Comment 4

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Comment on attachment 567689 [details] [diff] [review] [diff] [details] [review]
> Fix
> 
> Review of attachment 567689 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure why it matters whether it was painted by something else.
> 
> It seems to me that the PlanarYCbCr path doesn't call NotifyPaintedImage so
> this won't work there.
> 
> Otherwise this should be OK, although using paintTime is a little obscure.
> Seems to me it would make more sense to add an explicit "dirty" flag to the
> ImageContainer which is set whenever the image changes, and use that flag
> directly from BasicShadowableImageLayer::Paint to control when the image is
> re-uploaded.

If someone else paints with the same ImageContainer, then it will reset the dirty flag (or give us a valid paintTime in the current code) and we won't re-upload.

I'm not sure if we ever actually share ImageContainer currently, but it seems like we could in test cases like this where the same image is used in multiple places. Though maybe we can share the Shadowable/ShadowImageLayer too, and avoid the issue that way.
Found that sometimes some leafs having opaque background with this patch, it looks a bit faster, but it seems there are still some re-paints are happening
Ok, filled bug 695406 about opaque shadowImage layers problem
paintTime isNull definitely does not work, should not we just remove image from container as soon it painted to ShadowableImageLayer buffer? then later we will ignore updates until someone call SetCurrentImage again?

Also I checked mContainer->GetPaintCount() == 0, when image was not set... and with plugins for example it has value 1 after each update.
oh, no I'm wrong, (printed values incorrectly),  paintTime does return null when image not changed
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> If someone else paints with the same ImageContainer, then it will reset the
> dirty flag (or give us a valid paintTime in the current code) and we won't
> re-upload.

Oh right. This is all because ImageContainers and Images aren't reflected using IPC. Can we fix that?
(Assignee)

Comment 10

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)

> Oh right. This is all because ImageContainers and Images aren't reflected
> using IPC. Can we fix that?

This does sound like the best way to go.

Having ShadowableImageLayers act like ThebesLayers, and trying to detect if the ImageContainer contents are identical to our buffer is a bit forced.
(Assignee)

Comment 11

6 years ago
Created attachment 567933 [details] [diff] [review]
Add CheckAndClearPaintedState to test painting
Attachment #567689 - Attachment is obsolete: true
Attachment #567689 - Flags: review?(roc)
Attachment #567689 - Flags: feedback?(jones.chris.g)
Attachment #567933 - Flags: review?(roc)
(Assignee)

Comment 12

6 years ago
Created attachment 567934 [details] [diff] [review]
Fix conversion of ThebesLayers to ImageLayers
Attachment #567934 - Flags: review?(roc)
(Assignee)

Comment 13

6 years ago
Created attachment 567935 [details] [diff] [review]
Add test for ThebesLayer -> ImageLayer conversion
Attachment #567935 - Flags: review?(roc)
(Assignee)

Comment 14

6 years ago
Created attachment 567936 [details] [diff] [review]
WIP ShadowLayers fixes and improvements
Comment on attachment 567933 [details] [diff] [review]
Add CheckAndClearPaintedState to test painting

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +1937,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsIFrame* frame = content->GetPrimaryFrame();
> +
> +  *aResult = frame->CheckAndClearPaintedState();

null-check frame. I guess if there's no frame, we can declare it's not painted.
Attachment #567933 - Flags: review?(roc) → review+
Comment on attachment 567934 [details] [diff] [review]
Fix conversion of ThebesLayers to ImageLayers

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

::: layout/generic/nsImageFrame.cpp
@@ +621,5 @@
>  {
>    nsPresContext *presContext = PresContext();
>    nsIPresShell *presShell = presContext->GetPresShell();
>    NS_ASSERTION(presShell, "No PresShell.");
> +  mImageContainer = nsnull;

Do we need to do this whenever the image changes?
Attachment #567935 - Flags: review?(roc) → review+
(Assignee)

Comment 17

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
 
> Do we need to do this whenever the image changes?

I'm not sure what you're asking exactly.

This particular check (OnStopDecode), I'm not sure about, but added it for safety.

The other one I added (OnStartContainer) is definitely required, and the other two callbacks from the listener (OnDataAvailable and FrameChanged) already do this.
(Assignee)

Updated

6 years ago
Depends on: 695406
(Assignee)

Comment 18

6 years ago
Comment on attachment 567936 [details] [diff] [review]
WIP ShadowLayers fixes and improvements

Handling the ShadowLayers bugs separately in bug 695406.

I think we should also look at the performance problems separately, probably by mirroring ImageContainers (and Images?) across IPC.
Attachment #567936 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
Created attachment 567968 [details] [diff] [review]
Fix conversion of ThebesLayers to ImageLayers

Fixed a bug with scaled ContainerLayers
Attachment #567934 - Attachment is obsolete: true
Attachment #567934 - Flags: review?(roc)
Attachment #567968 - Flags: review?(roc)
Comment on attachment 567968 [details] [diff] [review]
Fix conversion of ThebesLayers to ImageLayers

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

::: layout/base/FrameLayerBuilder.cpp
@@ +972,5 @@
>        data->mImage->ConfigureLayer(imageLayer);
> +      if (mParameters.mInActiveTransformedSubtree) {
> +        // The layer's current transform is applied first, then the result is scaled.
> +        gfx3DMatrix transform = imageLayer->GetTransform()*
> +        gfx3DMatrix::ScalingMatrix(mParameters.mXScale, mParameters.mYScale, 1.0f);

Indent last line
Attachment #567968 - Flags: review?(roc) → review+
(Assignee)

Comment 21

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2e6a61338cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/608feeb4539c
https://hg.mozilla.org/integration/mozilla-inbound/rev/049ba0a8823f
https://hg.mozilla.org/integration/mozilla-inbound/rev/564144b09c4b
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/d2e6a61338cc
https://hg.mozilla.org/mozilla-central/rev/608feeb4539c
https://hg.mozilla.org/mozilla-central/rev/049ba0a8823f
https://hg.mozilla.org/mozilla-central/rev/564144b09c4b

Please, assign bugs to yourself when working on them :)
Assignee: nobody → matt.woodrow
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
Backed out since this caused Android Crashtests to permanent fail

REFTEST TEST-START | http://10.250.48.211:30091/tests/gfx/tests/crashtests/408754-1.html | 1813 / 1947 (93%)
REFTEST TEST-PASS | http://10.250.48.211:30091/tests/gfx/tests/crashtests/408754-1.html | (LOAD ONLY)
REFTEST INFO | Loadin
command timed out: 2400 seconds without output, killing pid 325
process killed by signal 9
program finished with exit code -1
elapsedTime=2920.524583
TinderboxPrint: crashtest<br/><em class="testfail">T-FAIL</em>
buildbot.slave.commands.TimeoutError: command timed out: 2400 seconds without output, killing pid 325
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla10 → ---
some logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6966995&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=6965450&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=6966094&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=6964903&tree=Mozilla-Inbound
(Assignee)

Comment 25

6 years ago
Created attachment 569580 [details] [diff] [review]
Fix crashtest failures with BasicShadowableImageLayer
Attachment #569580 - Flags: review?(jones.chris.g)
Attachment #569580 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 26

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/68d6ef3d84a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b89d7037306
https://hg.mozilla.org/integration/mozilla-inbound/rev/715363a6428b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7f3bfc7cd46
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/68d6ef3d84a3
https://hg.mozilla.org/mozilla-central/rev/8b89d7037306
https://hg.mozilla.org/mozilla-central/rev/715363a6428b
https://hg.mozilla.org/mozilla-central/rev/d7f3bfc7cd46
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Updated

6 years ago
Depends on: 698212

Updated

6 years ago
Depends on: 698590

Updated

6 years ago
Depends on: 715916

Updated

6 years ago
Depends on: 717572
Depends on: 718394

Updated

5 years ago
Depends on: 723484

Updated

5 years ago
Depends on: 741177

Updated

5 years ago
Depends on: 745549

Updated

5 years ago
Depends on: 749118

Updated

5 years ago
Depends on: 767292

Updated

5 years ago
Depends on: 774617
You need to log in before you can comment on or make changes to this bug.