Closed Bug 695275 Opened 13 years ago Closed 13 years ago

Fix and test ThebesLayer to ImageLayer optimization

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Depends on 1 open bug)

Details

(Whiteboard: [inbound])

Attachments

(4 files, 3 obsolete files)

      No description provided.
Attached patch Fix (obsolete) — Splinter Review
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)
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.
(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?
(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.
Attachment #567689 - Attachment is obsolete: true
Attachment #567689 - Flags: review?(roc)
Attachment #567689 - Flags: feedback?(jones.chris.g)
Attachment #567933 - Flags: review?(roc)
Attachment #567934 - Flags: review?(roc)
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?
(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.
Depends on: 695406
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
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+
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
Closed: 13 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 → ---
Attachment #569580 - Flags: review?(jones.chris.g) → review+
Depends on: 698212
Depends on: 698590
Depends on: 715916
Depends on: 717572
Depends on: 718394
Depends on: 723484
Depends on: 741177
Depends on: 745549
Depends on: 749118
Depends on: 767292
Depends on: 774617
You need to log in before you can comment on or make changes to this bug.