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 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.

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 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.

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.

Fixed a bug with scaled ContainerLayers

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

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

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 :)

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

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

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

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