1422392 - Implement the first round of OMTP for tiled painted layers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement, P3)

Description

[Ryan Hunt [:rhunt]]

This is for the first round of implementation for OMTP tiled layers, which is:

1. Front to back buffer copying OMT
1. Back buffer clearing OMT
1. Record and replay painting OMT
1. No parallel painting
1. No support for edge padding
1. No support for progressive painting
1. For MultiTiledContentClient's only ***

*** The implementation for SingleTiledContentClient is very close and may make it into this, but I think this could be worth landing without it

The lack of support for edge padding and progressive painting means that this won't work for android yet. Adding edge padding would be super trivial. Progressive painting will need some threading review because it does some weird things with repeat transactions that need to be verified as safe or made safe.

Right now the patches are passing try for OSX, and I'd like to enable it there first.

Comment 1

[Ryan Hunt [:rhunt]]

Okay it looks like I was able to get SingleTiledContentClient working, so that will be in this bug.

Nical could you look at these patches? I'll also get David to look at the OMTP heavy parts, but it's all mixed up in these patches.

Some try runs:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08aaf83819c8b393de6b0abc5d7c7fdb2a22460e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fe43c178ce2685f8eae382da00194ceda5903b9

Note that this is just for OSX right now, so it's okay that linux is orange.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: SetPermitSubpixelAA for capture draw targets based on mFormat (bug 1422392, )

Other draw targets do this in their initialization code, so capture should also.

Review commit: https://reviewboard.mozilla.org/r/204844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204844/

Comment 3

[Ryan Hunt [:rhunt]]

Attachment: Implement record and replay painting for multi tiled layers (bug 1422392, )

This commit modifies MultiTiledContentClient to record drawing commands and
replay them to the tiled draw target on the paint thread when OMTP is enabled.

Review commit: https://reviewboard.mozilla.org/r/204846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204846/

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Implement buffer copying and clearing on the paint thread for multi tiled layers (bug 1422392, )

This implements recording of buffer preparing commands for MultiTiledContentClient
and replaying it on the paint thread.

The order of locking for tiling is important and for buffer copying the front
buffer needs to be attempted to be locked before the back buffer. This means
we can't do this on the paint thread like we do for ContentClient, so instead
I've changed the OpenMode::ASYNC to be a flag that can be applied to
OpenMode::OPEN_READ as well as WRITE, as it looks like we can apply the same
logic for READ as WRITE.

Review commit: https://reviewboard.mozilla.org/r/204848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204848/

Comment 5

[Ryan Hunt [:rhunt]]

Attachment: Implement record and replay painting for single tiled layers (bug 1422392, )

Review commit: https://reviewboard.mozilla.org/r/204850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204850/

Comment 6

[Ryan Hunt [:rhunt]]

Attachment: Implement buffer copying and clearing on the paint thread for single tiled layers (bug 1422392, )

Review commit: https://reviewboard.mozilla.org/r/204852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204852/

Comment 7

[Ryan Hunt [:rhunt]]

Attachment: Enable OMTP on OSX (bug 1422392, )

Review commit: https://reviewboard.mozilla.org/r/204854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204854/

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204846/#review210336


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/layers/client/TiledContentClient.cpp:992
(Diff revision 1)
>      }
>  
> -    if (!mMoz2DTiles.empty()) {
> +    if (!mPaintTiles.empty()) {
> +      // Create a tiled draw target
>        gfx::TileSet tileset;
> -      for (size_t i = 0; i < mMoz2DTiles.size(); ++i) {
> +      for (size_t i = 0; i < mPaintTiles.size(); ++i) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204846/#review210338

::: gfx/layers/client/TiledContentClient.cpp:481
(Diff revision 1)
>  
>  static bool
>  CopyFrontToBack(TextureClient* aFront,
>                  TextureClient* aBack,
> -                const gfx::IntRect& aRectToCopy)
> +                const gfx::IntRect& aRectToCopy,
> +                bool aAsyncPaint)

I pref enum classes over bools when it comes to parameters. There's a big readability difference between:

  method(false)
  
and:

  method(PaintMode::Async);
  
Also something like "if (!aAsyncPaint)" reads as a double negative instead of "this is a sync paint".
  
But I don't have a strong objection here, it's up to you.

::: gfx/layers/client/TiledContentClient.cpp:1028
(Diff revision 1)
> +          new CapturedTiledPaintState(drawTarget,
> +                                      captureDT);
> +        capturedState->mClients = std::move(mPaintTilesTextureClients);
> +
> +        PaintThread::Get()->PaintTiledContents(capturedState);
> +        mManager->SetQueuedAsyncPaints();

Is there any reason to consider AutoQueuedAsyncPaint here?

::: gfx/layers/ipc/CompositorBridgeChild.cpp:1290
(Diff revision 1)
> +  // We must not be waiting for paints to complete yet. This would imply we
> +  // started a new paint without waiting for a previous one, which could lead to
> +  // incorrect rendering or IPDL deadlocks.
> +  MOZ_ASSERT(!mIsDelayingForAsyncPaints);
> +
> +  mOutstandingAsyncPaints++;

It's too bad there's no easy way to share this with NotifyBeginAsyncPaint. Maybe the state could have a virtual "ForEachTextureClient" iterator, and a virtual "NotifyFinished" method?

Or alternately just factor out common helpers, since this logic is fairly critical to correct operation, it's nice to only have one copy.

(Same comment applies to NotifyFinishedAsyncTiledPaint).

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8933920 [details]
SetPermitSubpixelAA for capture draw targets based on mFormat (bug 1422392, )

https://reviewboard.mozilla.org/r/204844/#review210532

Comment 11

[Nicolas Silva [:nical]]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204846/#review210566

::: gfx/thebes/gfxQuartzNativeDrawing.cpp:27
(Diff revision 1)
>  gfxQuartzNativeDrawing::BeginNativeDrawing()
>  {
>    NS_ASSERTION(!mCGContext, "BeginNativeDrawing called when drawing already in progress");
>  
>    DrawTarget *dt = mDrawTarget;
> -  if (dt->IsDualDrawTarget() || dt->IsTiledDrawTarget() ||
> +  if (dt->IsDualDrawTarget() || dt->IsTiledDrawTarget() || dt->IsCaptureDT() ||

You most likely need to do the same in the other places where we check that a DT is or is not a dual or tiled target:
 - in GetImageForSourceSurface (FilterNodeD2D1.cpp)
 - in BorrowedCairoContext::BorrowCairoContextFromDrawTarget

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8933922 [details]
Implement buffer copying and clearing on the paint thread for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204848/#review210574

Comment 13

[Ryan Hunt [:rhunt]]

Attachment: Refactor NotifyBeginAsyncPaint logic to not be repeated (bug 1422392, )

Review commit: https://reviewboard.mozilla.org/r/205306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205306/

Comment 14

[Ryan Hunt [:rhunt]]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204846/diff/1-2/

Comment 15

[Ryan Hunt [:rhunt]]

Comment on attachment 8933922 [details]
Implement buffer copying and clearing on the paint thread for multi tiled layers (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204848/diff/1-2/

Comment 16

[Ryan Hunt [:rhunt]]

Comment on attachment 8933923 [details]
Implement record and replay painting for single tiled layers (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204850/diff/1-2/

Comment 17

[Ryan Hunt [:rhunt]]

Comment on attachment 8933924 [details]
Implement buffer copying and clearing on the paint thread for single tiled layers (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204852/diff/1-2/

Comment 18

[Ryan Hunt [:rhunt]]

Comment on attachment 8933925 [details]
Enable OMTP on OSX (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204854/diff/1-2/

Comment 19

[Ryan Hunt [:rhunt]]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204846/#review210338

> I pref enum classes over bools when it comes to parameters. There's a big readability difference between:
> 
>   method(false)
>   
> and:
> 
>   method(PaintMode::Async);
>   
> Also something like "if (!aAsyncPaint)" reads as a double negative instead of "this is a sync paint".
>   
> But I don't have a strong objection here, it's up to you.

Yes that's a good idea. I've added a new flags enum with None, Async, and Progressive options.

> Is there any reason to consider AutoQueuedAsyncPaint here?

I think this usage is okay. We only ever queue one operation with no early returns or anything tricky.

> It's too bad there's no easy way to share this with NotifyBeginAsyncPaint. Maybe the state could have a virtual "ForEachTextureClient" iterator, and a virtual "NotifyFinished" method?
> 
> Or alternately just factor out common helpers, since this logic is fairly critical to correct operation, it's nice to only have one copy.
> 
> (Same comment applies to NotifyFinishedAsyncTiledPaint).

Good idea, I agree this sensitive code shouldn't be repeated three times. I added a commit that tries to address this.

Comment 20

[Ryan Hunt [:rhunt]]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204846/#review210566

> You most likely need to do the same in the other places where we check that a DT is or is not a dual or tiled target:
>  - in GetImageForSourceSurface (FilterNodeD2D1.cpp)
>  - in BorrowedCairoContext::BorrowCairoContextFromDrawTarget

I don't think we need to do this for Cairo as OMTP explicitly doesn't support it, but to be safe I've added a check there.

Comment 21

[Code Review Bot [:reviewbot]]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204846/#review210878


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/layers/client/TiledContentClient.cpp:992
(Diff revision 2)
>      }
>  
> -    if (!mMoz2DTiles.empty()) {
> +    if (!mPaintTiles.empty()) {
> +      // Create a tiled draw target
>        gfx::TileSet tileset;
> -      for (size_t i = 0; i < mMoz2DTiles.size(); ++i) {
> +      for (size_t i = 0; i < mPaintTiles.size(); ++i) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

Comment 22

[Nicolas Silva [:nical]]

Comment on attachment 8933923 [details]
Implement record and replay painting for single tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204850/#review210972

Comment 23

[Nicolas Silva [:nical]]

Comment on attachment 8933924 [details]
Implement buffer copying and clearing on the paint thread for single tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204852/#review210974

Comment 24

[Nicolas Silva [:nical]]

Comment on attachment 8933925 [details]
Enable OMTP on OSX (bug 1422392, )

https://reviewboard.mozilla.org/r/204854/#review210976

Comment 25

[Nicolas Silva [:nical]]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204846/#review211094

Comment 26

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8934381 [details]
Refactor NotifyBeginAsyncPaint logic to not be repeated (bug 1422392, )

https://reviewboard.mozilla.org/r/205306/#review211162

::: gfx/layers/PaintThread.h:228
(Diff revision 1)
>    {}
>  
> +  template<typename F>
> +  void ForEachTextureClient(F aClosure) const
> +  {
> +    for (auto client : mClients) {

is |const auto&| preferred/needed here?

Comment 27

[David Anderson [:dvander] - inactive, e-mail if emergency]

Thanks for doing that refactoring!

Comment 28

[Ryan Hunt [:rhunt]]

Attachment: Preserve front buffer texture clients for copies (bug 1422392, )

We collect the back buffer texture clients to preserve while
async painting is happening, but if we do a buffer copy we
should preserve front buffer clients as well.

Review commit: https://reviewboard.mozilla.org/r/205940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205940/

Comment 29

[Ryan Hunt [:rhunt]]

Attachment: Move buffer clearing after all copies in SingleTiledContentClient (bug 1422392, )

CaptureTiledPaintState will execute all buffer copy operations first,
followed by all buffer clears. This works for MultiTiledContentClient
as it does those operations in that order. SingleTiledContentClient
can do copies, followed by a clear, followed by some copies.

If the clears are hoisted after the copies, graphical corruption can
happen. I think the easiest way to fix this is to move the clear
after the copies and sub out the region that has been successfully
copied.

Review commit: https://reviewboard.mozilla.org/r/205942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205942/

Comment 30

[Ryan Hunt [:rhunt]]

Comment on attachment 8933920 [details]
SetPermitSubpixelAA for capture draw targets based on mFormat (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204844/diff/1-2/

Comment 31

[Ryan Hunt [:rhunt]]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204846/diff/2-3/

Comment 32

[Ryan Hunt [:rhunt]]

Comment on attachment 8933922 [details]
Implement buffer copying and clearing on the paint thread for multi tiled layers (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204848/diff/2-3/

Comment 33

[Ryan Hunt [:rhunt]]

Comment on attachment 8933923 [details]
Implement record and replay painting for single tiled layers (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204850/diff/2-3/

Comment 34

[Ryan Hunt [:rhunt]]

Comment on attachment 8933924 [details]
Implement buffer copying and clearing on the paint thread for single tiled layers (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204852/diff/2-3/

Comment 35

[Ryan Hunt [:rhunt]]

Comment on attachment 8934381 [details]
Refactor NotifyBeginAsyncPaint logic to not be repeated (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205306/diff/1-2/

Comment 36

[Ryan Hunt [:rhunt]]

Comment on attachment 8933925 [details]
Enable OMTP on OSX (bug 1422392, )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204854/diff/2-3/

Comment 37

[Code Review Bot [:reviewbot]]

Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204846/#review211568


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/layers/client/TiledContentClient.cpp:992
(Diff revision 3)
>      }
>  
> -    if (!mMoz2DTiles.empty()) {
> +    if (!mPaintTiles.empty()) {
> +      // Create a tiled draw target
>        gfx::TileSet tileset;
> -      for (size_t i = 0; i < mMoz2DTiles.size(); ++i) {
> +      for (size_t i = 0; i < mPaintTiles.size(); ++i) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

Comment 38

[Nicolas Silva [:nical]]

Comment on attachment 8935104 [details]
Preserve front buffer texture clients for copies (bug 1422392, )

https://reviewboard.mozilla.org/r/205940/#review211906

Comment 39

[Nicolas Silva [:nical]]

Comment on attachment 8935105 [details]
Move buffer clearing after all copies in SingleTiledContentClient (bug 1422392, )

https://reviewboard.mozilla.org/r/205942/#review211908

Comment 40

[Ryan Hunt [:rhunt]]

Updated try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e410db411e73e8528fc67326ff0bb425f175b549

I've fixed the android build failure locally, just an unused lambda capture.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29af0b03d3b3
SetPermitSubpixelAA for capture draw targets based on mFormat (bug 1422392, r=nical)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a8d5147a416
Implement record and replay painting for multi tiled layers (bug 1422392, r=nical)
https://hg.mozilla.org/integration/mozilla-inbound/rev/88bd590938cd
Implement buffer copying and clearing on the paint thread for multi tiled layers (bug 1422392, r=nical)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0adaea9c41a
Implement record and replay painting for single tiled layers (bug 1422392, r=nical)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c48303c8f66
Implement buffer copying and clearing on the paint thread for single tiled layers (bug 1422392, r=nical)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47fbbd343d9
Refactor NotifyBeginAsyncPaint logic to not be repeated (bug 1422392, r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/91901ff18dfe
Preserve front buffer texture clients for copies (bug 1422392, r=nical)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab8ca96cba0
Move buffer clearing after all copies in SingleTiledContentClient (bug 1422392, r=nical)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebf4af04d10c
Enable OMTP on OSX (bug 1422392, r=nical)

Comment 42

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/29af0b03d3b3
https://hg.mozilla.org/mozilla-central/rev/6a8d5147a416
https://hg.mozilla.org/mozilla-central/rev/88bd590938cd
https://hg.mozilla.org/mozilla-central/rev/f0adaea9c41a
https://hg.mozilla.org/mozilla-central/rev/3c48303c8f66
https://hg.mozilla.org/mozilla-central/rev/b47fbbd343d9
https://hg.mozilla.org/mozilla-central/rev/91901ff18dfe
https://hg.mozilla.org/mozilla-central/rev/3ab8ca96cba0
https://hg.mozilla.org/mozilla-central/rev/ebf4af04d10c