Closed Bug 893303 Opened 7 years ago Closed 6 years ago

Convert TiledContentClient/Host to the new textures

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: nical, Assigned: cwiiis)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Got a preliminary patch that does the port, not complete yet (but does work, to a fashion). Assigning to me, adding blocker.
Assignee: nobody → chrislord.net
Blocks: b2g-tiling
Status: NEW → ASSIGNED
Attached patch WIP patch (obsolete) — Splinter Review
Attaching patch in case anyone wants to look/see where it is atm without having to go to my hg queue.
Attached patch First run portSplinter Review
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.
Attachment #8374828 - Attachment is obsolete: true
Attachment #8375592 - Flags: feedback?(nical.bugzilla)
Attachment #8375592 - Flags: feedback?(bgirard)
Attachment #8375592 - Flags: feedback?(bas)
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.
Depends on: 972619
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 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.
Attachment #8375592 - Flags: feedback?(nical.bugzilla) → feedback+
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.
Flags: needinfo?(bas)
(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 :-).
Flags: needinfo?(bas)
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.
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.
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).
(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
Blocks: can-tiles
https://hg.mozilla.org/mozilla-central/rev/8394fed3332e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8375592 [details] [diff] [review]
First run port

We've moved to doing post landing review so unflagging this.
Attachment #8375592 - Flags: feedback?(bgirard)
Attachment #8375592 - Flags: feedback?(bas)
You need to log in before you can comment on or make changes to this bug.