Closed Bug 1025138 Opened 5 years ago Closed 5 years ago

Integrate DrawTargetTiled into layers.

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch WIP v1 (Hacky as hell) (obsolete) — Splinter Review
Assignee: nobody → nical.bugzilla
Attached patch WIP v2 (obsolete) — Splinter Review
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
Attachment #8440026 - Attachment is obsolete: true
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.
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.
Attached patch WIP v3Splinter Review
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.
Attachment #8440061 - Attachment is obsolete: true
Can we get this reviewed and landed soon?
Attachment #8440745 - Flags: review?(matt.woodrow)
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 :)
Attachment #8440745 - Flags: review?(matt.woodrow) → review+
Can you land this soon please Nical? I have follow-up changes waiting to land that need this.
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).
> 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
https://hg.mozilla.org/mozilla-central/rev/7aee66d772f4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.