893303 - Convert TiledContentClient/Host to the new textures

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Chris Lord [:cwiiis]]

Got a preliminary patch that does the port, not complete yet (but does work, to a fashion). Assigning to me, adding blocker.

Comment 2

[Chris Lord [:cwiiis]]

Attachment: WIP patch

Attaching patch in case anyone wants to look/see where it is atm without having to go to my hg queue.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: First run port

Here's the first rough run. This works on b2g, but I've not made doubly sure it isn't leaking everything/isn't doing lots of silly things. It also needs some tidying, some comments need tidying up, and probably further refactoring.

Untested on Android, but I hope it works - depends on whether I made a mistake with gfxMemorySharedReadLock really.

I'll be PTO until Tuesday - I understand Bas will be picking this up, but if everyone is too busy I'll get back to it then. Otherwise I'll work on double-buffering on top of this next week with Bas and nical.

Comment 4

[Chris Lord [:cwiiis]]

Some quick testing shows what appears to be a serious performance regression using this patch, so either I've messed something up somewhere (likely in the tile updating bits) or we underestimated the extra overhead of new textures. More likely the former.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8375592 [details] [diff] [review]
First run port

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

Quick drive-by review, hope you don't mind :)

::: gfx/layers/client/TiledContentClient.cpp
@@ +419,5 @@
> +      gfxPlatform::GetPlatform()->Optimal2DFormatForContent(GetContentType()),
> +                                                            TEXTURE_IMMEDIATE_UPLOAD |
> +                                                            TEXTURE_DEALLOCATE_CLIENT);
> +    aTile.mTextureClient = writableClient;
> +#ifdef MOZ_ANDROID_OMTC

Use gfxPlatform::GetPlatform()->PreferMemoryOverShmem() for a runtime check instead of an #ifdef. This lets us use memory for parent process content on b2g.

@@ +447,5 @@
> +  RefPtr<DrawTarget> writableDrawTarget = writableClient->AsTextureClientDrawTarget()->GetAsDrawTarget();
> +  RefPtr<gfxContext> ctxt = new gfxContext(writableDrawTarget);
> +
> +  if (oldClient) {
> +    // Copy the old surface to the new one

We should add a profiler label here so that it shows up separately from copying the updated region below. I have a feeling that we're hitting this path more often, causing the performance regression.

Thinking slightly further ahead, we could clip the dirty area out for this copy, since those pixels are going to be copied to immediately afterwards. We have the equivalent optimization when copying front buffer -> back buffer with non-tiled content.

@@ +456,5 @@
> +      RefPtr<gfx::SourceSurface> source = oldTextureDrawTarget->Snapshot();
> +      writableDrawTarget->CopySurface(source,
> +                                      gfx::IntRect(0, 0, GetTileLength(), GetTileLength()),
> +                                      gfx::IntPoint(0, 0));
> +      oldClient->Unlock();

Don't we need to clear out oldTextureDrawTarget/source before calling unlock, as per the comment above?

::: gfx/layers/ipc/LayersMessages.ipdlh
@@ +261,5 @@
> +
> +struct BasicTileDescriptor {
> +  PTexture texture;
> +  uintptr_t sharedLock;
> +};

I think it would make more sense to structure this like:

union SharedLock {
  Shmem;
  uintptr_t;
};

struct TileDescriptor {
  PTexture texture;
  SharedLock lock;
}

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8375592 [details] [diff] [review]
First run port

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

I agree with Matt's comment. Overall looks good!

::: gfx/layers/client/TiledContentClient.h
@@ +120,5 @@
> +
> +    if (mAllocator) {
> +#define MOZ_ALIGN_WORD(x) (((x) + 3) & ~3)
> +      if (mAllocator->AllocUnsafeShmem(MOZ_ALIGN_WORD(sizeof(ShmReadLockInfo)),
> +                                       mozilla::ipc::SharedMemory::TYPE_BASIC, &mShmem)) {

We can easily hit the system limit of open file descriptors so we might want to share the shmem between all the tiles of a given TiledContentClient. Maybe as a followup.

Comment 7

[Chris Lord [:cwiiis]]

It strikes me that the reason this is slow may be due to the Updated call in TiledContentHost not checking to see if aDirtyRect is empty before calling. This may be causing it to re-upload every tile on each update. I'd check, but I have no laptop. Needinfoing Bas in case he's looking at this already.

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Chris Lord [:cwiiis] from comment #7)
> It strikes me that the reason this is slow may be due to the Updated call in
> TiledContentHost not checking to see if aDirtyRect is empty before calling.
> This may be causing it to re-upload every tile on each update. I'd check,
> but I have no laptop. Needinfoing Bas in case he's looking at this already.

Yes, this update is the problem (or at least the largest one I've found so far). I noticed this yesterday and found the cause this afternoon, sadly I'd missed your comment so I found it after you'd already identified the cause on here :-).

Comment 9

[Bas Schouten (:bas.schouten)]

Just for the record, the cause seems to be a little more complex, but I'll still looking into the exact causes, you're suggestion is on the right track, anyway.

Comment 10

[Chris Lord [:cwiiis]]

I had a quick look at this (and that's it for today, need to get ready for travel - but I'll be back on it tomorrow) - the cause of the slow-down vs. tiles without this patch isn't immediately obvious to me. I checked that only the right amount is being uploaded, and that appears to work, and all the expected classes are also being used.

I added the obvious optimisation of not copying from the old texture client if the dirty rect encompasses the tile, but unfortunately it made no real difference (and neither would I expect it to, given that optimisation didn't exist before).

My current thought is that there's a synchronous process where there wasn't before, perhaps AddTextureClient is synchronous?

Bas, nical and I will hopefully knock this one out tomorrow, alongside double-buffering for gralloc/d3d/etc.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Initialize and set the compositor

This fixes a lot of assertions that I saw when using this patch.

I also found that nothing ever frees the shmem lock objects, and we leak them until we run out of file descriptors (800 on OSX).

Comment 12

[Chris Lord [:cwiiis]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Created attachment 8377854 [details] [diff] [review]
> Initialize and set the compositor
> 
> This fixes a lot of assertions that I saw when using this patch.
> 
> I also found that nothing ever frees the shmem lock objects, and we leak
> them until we run out of file descriptors (800 on OSX).

Thanks, that's handy - I also forgot to call SetCompositableBackendSpecificData (which becomes evident when you get a nullptr access using Gralloc textures :))

Note, work on this is happening in this repo now: http://hg.mozilla.org/users/bschouten_mozilla.com/tiling

Comment 13

[Chris Lord [:cwiiis]]

https://hg.mozilla.org/mozilla-central/rev/8394fed3332e

Comment 14

[Benoit Girard (:BenWa)]

Comment on attachment 8375592 [details] [diff] [review]
First run port

We've moved to doing post landing review so unflagging this.