1025138 - Integrate DrawTargetTiled into layers.

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Nicolas Silva [:nical]]

Attachment: WIP v1 (Hacky as hell)

Comment 2

[Nicolas Silva [:nical]]

Attachment: WIP v2

Note that TiledDrawTarget might not perform as well for native theme drawing on Linux because it will fail to borrow a cairo context here:
http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxXlibNativeRenderer.cpp?from=gfxXLibNativeRenderer.cpp#151

Comment 3

[Matt Woodrow (:mattwoodrow)]

We could improve the native drawing situation by making the borrow function request an area that it wants to draw to. If that only covers a single tile (something I assume would be fairly common), then we can just borrow the context for that tile.

Comment 4

[Nicolas Silva [:nical]]

At the moment this patch seems to have clipping issues. It's not entirely clear to me whether the code in TiledContentClient is not properly tracking valid regions or if the clipping issue is in the DrawTargetTiled itself.
When turning paint-flashing on I can see that the whole tile is flashed every time rather than just the paint region, and I think that the paint region in FrameLayerBuilder isn't affect by any region code from TiledContentClient that I could have messed up so i'm inclined to think that something is wrong in the tiled DrawTarget itself. I tested with skia instead of cairo and it has the same problem.

Comment 5

[Nicolas Silva [:nical]]

Attachment: WIP v3

This patch has the clipping fixed that Bas pointed out to me, and postpone unlocking the front and back buffers so that we can avoid Flushing the DrawTarget after copying the front into the back buffer, and avoid creating a new DrawTarget for painting.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Can we get this reviewed and landed soon?

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8440745 [details] [diff] [review]
WIP v3

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

::: gfx/layers/TiledLayerBuffer.h
@@ +516,5 @@
>  
> +  AsDerived().PostValidate(aPaintRegion);
> +  for (unsigned int i = 0; i < newRetainedTiles.Length(); ++i) {
> +    AsDerived().UnlockTile(newRetainedTiles[i]);
> +    //MOZ_ASSERT(!IsPlaceholder(newRetainedTiles[i]), "invalid tile");

Why is this commented out?

::: gfx/layers/client/TiledContentClient.cpp
@@ +660,2 @@
>  
> +  } else if (useSinglePaintBuffer) {

if (useSinglePaintBuffer && !gfxPrefs::TiledDrawTargetEnabled()) instead of having the empty branch.

@@ +776,5 @@
> +      return aTile;
> +    }
> +  }
> +
> +  if (gfxPrefs::TiledDrawTargetEnabled()) {

This function is enormous now, maybe split this block into a separate function?

::: modules/libpref/src/init/all.js
@@ +3767,5 @@
>  pref("layers.draw-tile-borders", false);
>  pref("layers.draw-bigimage-borders", false);
>  pref("layers.frame-counter", false);
>  pref("layers.enable-tiles", false);
> +pref("layers.tiled-drawtarget.enabled", true);

Probably want to remove this for now :)

Comment 8

[Matt Woodrow (:mattwoodrow)]

Can you land this soon please Nical? I have follow-up changes waiting to land that need this.

Comment 9

[Nicolas Silva [:nical]]

adressed some of the comments and landed
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aee66d772f4

I looked into making this a bit cleaner but the whole client side stuff needs to be split into clean chuncks so i'll spend some time doing this as a followup (after OMTC sticks at least on windows).

Comment 10

[Nicolas Silva [:nical]]

> I looked into making this a bit cleaner but the whole client side stuff
> needs to be split into clean chuncks so i'll spend some time doing this as a
> followup (after OMTC sticks at least on windows).

sorry a bit tired, i meant the whole client side tiling code, so TiledLayerBuffer and TiledContentClient

Comment 11

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/7aee66d772f4