Last Comment Bug 695275 - Fix and test ThebesLayer to ImageLayer optimization
: Fix and test ThebesLayer to ImageLayer optimization
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Matt Woodrow (:mattwoodrow)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 741177 745549 695406 698212 698590 715916 717572 718394 723484 749118 767292 774617
Blocks: 689760 586683
  Show dependency treegraph
 
Reported: 2011-10-18 00:13 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2012-12-13 10:14 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (6.90 KB, patch)
2011-10-18 00:15 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Add CheckAndClearPaintedState to test painting (6.34 KB, patch)
2011-10-18 16:56 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Fix conversion of ThebesLayers to ImageLayers (4.82 KB, patch)
2011-10-18 16:56 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Add test for ThebesLayer -> ImageLayer conversion (3.20 KB, patch)
2011-10-18 16:57 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
WIP ShadowLayers fixes and improvements (2.81 KB, patch)
2011-10-18 16:57 PDT, Matt Woodrow (:mattwoodrow)
no flags Details | Diff | Splinter Review
Fix conversion of ThebesLayers to ImageLayers (6.11 KB, patch)
2011-10-18 21:53 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review
Fix crashtest failures with BasicShadowableImageLayer (1.07 KB, patch)
2011-10-25 18:53 PDT, Matt Woodrow (:mattwoodrow)
roc: review+
Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2011-10-18 00:13:23 PDT

    
Comment 1 Matt Woodrow (:mattwoodrow) 2011-10-18 00:15:16 PDT
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.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-18 02:33:12 PDT
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.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-18 02:39:07 PDT
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.
Comment 4 Matt Woodrow (:mattwoodrow) 2011-10-18 02:48:36 PDT
(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.
Comment 5 Oleg Romashin (:romaxa) 2011-10-18 11:04:03 PDT
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
Comment 6 Oleg Romashin (:romaxa) 2011-10-18 11:26:06 PDT
Ok, filled bug 695406 about opaque shadowImage layers problem
Comment 7 Oleg Romashin (:romaxa) 2011-10-18 12:05:53 PDT
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.
Comment 8 Oleg Romashin (:romaxa) 2011-10-18 12:25:06 PDT
oh, no I'm wrong, (printed values incorrectly),  paintTime does return null when image not changed
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-18 13:13:49 PDT
(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?
Comment 10 Matt Woodrow (:mattwoodrow) 2011-10-18 14:00:03 PDT
(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.
Comment 11 Matt Woodrow (:mattwoodrow) 2011-10-18 16:56:01 PDT
Created attachment 567933 [details] [diff] [review]
Add CheckAndClearPaintedState to test painting
Comment 12 Matt Woodrow (:mattwoodrow) 2011-10-18 16:56:33 PDT
Created attachment 567934 [details] [diff] [review]
Fix conversion of ThebesLayers to ImageLayers
Comment 13 Matt Woodrow (:mattwoodrow) 2011-10-18 16:57:11 PDT
Created attachment 567935 [details] [diff] [review]
Add test for ThebesLayer -> ImageLayer conversion
Comment 14 Matt Woodrow (:mattwoodrow) 2011-10-18 16:57:46 PDT
Created attachment 567936 [details] [diff] [review]
WIP ShadowLayers fixes and improvements
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-18 17:23:04 PDT
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.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-18 17:24:16 PDT
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?
Comment 17 Matt Woodrow (:mattwoodrow) 2011-10-18 19:15:34 PDT
(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.
Comment 18 Matt Woodrow (:mattwoodrow) 2011-10-18 19:30:20 PDT
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.
Comment 19 Matt Woodrow (:mattwoodrow) 2011-10-18 21:53:00 PDT
Created attachment 567968 [details] [diff] [review]
Fix conversion of ThebesLayers to ImageLayers

Fixed a bug with scaled ContainerLayers
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-18 22:10:27 PDT
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
Comment 23 Marco Bonardo [::mak] 2011-10-21 06:30:02 PDT
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
Comment 25 Matt Woodrow (:mattwoodrow) 2011-10-25 18:53:53 PDT
Created attachment 569580 [details] [diff] [review]
Fix crashtest failures with BasicShadowableImageLayer

Note You need to log in before you can comment on or make changes to this bug.