Closed Bug 1422392 Opened 2 years ago Closed 2 years ago

Implement the first round of OMTP for tiled painted layers

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(9 files)

59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
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.
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 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 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).
Attachment #8933921 - Flags: review?(dvander) → review+
Comment on attachment 8933920 [details]
SetPermitSubpixelAA for capture draw targets based on mFormat (bug 1422392, )

https://reviewboard.mozilla.org/r/204844/#review210532
Attachment #8933920 - Flags: review?(nical.bugzilla) → review+
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 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
Attachment #8933922 - Flags: review?(nical.bugzilla) → review+
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 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.
Attachment #8933921 - Flags: review?(nical.bugzilla)
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 on attachment 8933923 [details]
Implement record and replay painting for single tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204850/#review210972
Attachment #8933923 - Flags: review?(nical.bugzilla) → review+
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
Attachment #8933924 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8933925 [details]
Enable OMTP on OSX (bug 1422392, )

https://reviewboard.mozilla.org/r/204854/#review210976
Attachment #8933925 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8933921 [details]
Implement record and replay painting for multi tiled layers (bug 1422392, )

https://reviewboard.mozilla.org/r/204846/#review211094
Attachment #8933921 - Flags: review+
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?
Attachment #8934381 - Flags: review?(dvander) → review+
Thanks for doing that refactoring!
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 on attachment 8935104 [details]
Preserve front buffer texture clients for copies (bug 1422392, )

https://reviewboard.mozilla.org/r/205940/#review211906
Attachment #8935104 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8935105 [details]
Move buffer clearing after all copies in SingleTiledContentClient (bug 1422392, )

https://reviewboard.mozilla.org/r/205942/#review211908
Attachment #8935105 - Flags: review?(nical.bugzilla) → review+
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.
Pushed by rhunt@eqrion.net:
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)
Depends on: 1424172
Depends on: 1424325
You need to log in before you can comment on or make changes to this bug.