Page MenuHomePhabricator
Differential D57415

Bug 1604383 - Refactor the Compositor trait to allow support for DC virtual surface API.
ClosedPublic
Actions

Authored by gw on Dec 17 2019, 3:27 AM.

Details

Reviewers
mstange
sotaro
kvark
Commits
rMOZILLACENTRALc81cf0c409bc: Bug 1604383 - Refactor the Compositor trait to allow support for DC virtual…
Bugzilla Bug ID
1604383

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gw created this revision.Dec 17 2019, 3:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2019, 3:27 AM
Harbormaster completed remote builds in B147105: Diff 208884.Dec 17 2019, 3:27 AM
phab-bot requested review of this revision.Dec 17 2019, 3:28 AM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: Restricted Project.
gw added reviewers: mstange, sotaro.Dec 17 2019, 3:29 AM

WORK IN PROGRESS

This isn't complete yet - I haven't done the Mac implementation, and it still needs some tidy up / comments / commit splitting.

But, putting it up now in case anyone has early feedback / comments / questions.

gw added a comment.Dec 17 2019, 3:30 AM

Of note: This doesn't use the virtual surface DC APIs yet. The idea is to refactor the interface but maintain the existing DC API usage, and then land the virtual surface API usage as a follow up (which should be a small amount of work).

gw added a comment.Dec 17 2019, 3:34 AM

The synopsis is:

  • The Compositor trait now exposes both the concept of surfaces and tiles.
  • A surface maps 1:1 with a picture cache slice.
  • Tiles exist within a given surface.
  • A given surface has a constant tile size (the surface is re-created if the tile size changes).
  • A tile is uniquely identified by (surface id, x, y) where (x, y) are the index of the tile on each axis (signed).
  • The DC implementation is expanded to have DCSurface and DCTile (currently the old name DCLayer).
  • A DC visual is created for each tile, and also one for each surface.
  • Tile visuals are attached to the surface visual, and placed in a local space offset.
  • Surface visuals are attached to the root. Clip rect and scroll position are applied to the surface visual.
gw added a comment.Dec 17 2019, 3:41 AM

Oh, I also tried to make the hashmaps in DCLayerTree a bit more type-safe, since there are now two levels of hashing (root -> surface -> tile).

reviewbot added a subscriber: reviewbot.Dec 17 2019, 3:46 AM

Code analysis found 25 defects in the diff 208884:

  • 25 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/webrender_bindings/RenderCompositor.h gfx/webrender_bindings/RenderCompositorANGLE.cpp gfx/webrender_bindings/RenderCompositor.cpp gfx/webrender_bindings/DCLayerTree.cpp gfx/webrender_bindings/RenderCompositorOGL.h gfx/webrender_bindings/RenderCompositorOGL.cpp gfx/webrender_bindings/RenderCompositorANGLE.h gfx/webrender_bindings/DCLayerTree.h (C/C++)

For your convenience, here is a patch that fixes all the source-test-clang-format defects (use it in your repository with hg import or git apply -p0).

If you see a problem in this automated review, please report it here.

reviewbot added a comment.Dec 17 2019, 3:46 AM

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:

Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

gw added a reviewer: kvark.Dec 17 2019, 4:18 AM
gw updated this revision to Diff 208888.Dec 17 2019, 4:40 AM
Harbormaster completed remote builds in B147108: Diff 208888.Dec 17 2019, 4:40 AM
gw added a comment.Dec 17 2019, 4:41 AM

Rebased, ran clang-format, and added a simple Mac implementation that retains the existing behavior.

The C++ bits are a bit rough, but this should be ready for an initial review now, I think!

gw added a comment.Dec 17 2019, 4:46 AM

I can try to split this up into separate commits, if that would simplify things - although it's not obvious that I can do that without each individual commit being broken, which probably isn't ideal?

reviewbot added a comment.Dec 17 2019, 5:09 AM

Code analysis found 2 defects in the diff 208888:

  • 2 defects found by clang-tidy

You can run this analysis locally with:

  • ./mach static-analysis check gfx/webrender_bindings/RenderCompositorOGL.cpp gfx/webrender_bindings/RenderCompositorOGL.h (C/C++)

If you see a problem in this automated review, please report it here.

gfx/webrender_bindings/RenderCompositorOGL.cpp
180

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]
Checker reliability is high, meaning that the false positive ratio is low.

gfx/webrender_bindings/RenderCompositorOGL.h
87

Warning: Bad implicit conversion constructor for 'Surface' [clang-tidy: mozilla-implicit-constructor]
Checker reliability is high, meaning that the false positive ratio is low.

reviewbot added a comment.Dec 17 2019, 5:09 AM

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:

Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

sotaro accepted this revision.Dec 17 2019, 7:13 AM

The patch worked well on windows and looks good.

gfx/wr/example-compositor/compositor-windows/src/lib.cpp
59

Is there a reason why hash function is different from DCLayerTree.h and RenderCompositorOGL.h?

This revision is now accepted and ready to land.Dec 17 2019, 7:13 AM
gw updated this revision to Diff 209266.Dec 17 2019, 7:16 PM
Harbormaster completed remote builds in B147383: Diff 209266.Dec 17 2019, 7:16 PM
reviewbot added a comment.Dec 17 2019, 7:39 PM

Code analysis found 1 defect in the diff 209266:

  • 1 defect found by clang-tidy

You can run this analysis locally with:

  • ./mach static-analysis check gfx/webrender_bindings/RenderCompositorOGL.cpp (C/C++)

If you see a problem in this automated review, please report it here.

gfx/webrender_bindings/RenderCompositorOGL.cpp
180

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]
Checker reliability is high, meaning that the false positive ratio is low.

reviewbot added a comment.Dec 17 2019, 7:39 PM

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:

Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

mstange accepted this revision.Dec 17 2019, 8:01 PM

I like the OpenGL implementation that you chose. I didn't look at the rust code.

gfx/webrender_bindings/RenderCompositor.h
94–96

Mozilla style usually avoids plain int and uses int32_t instead.

gfx/webrender_bindings/RenderCompositorOGL.cpp
179

typo: layout -> layer

180

Yeah, please change this to for (const auto& it : mSurfaces).

gfx/webrender_bindings/RenderCompositorOGL.h
79–81

Might as well do return HashGeneric(aId.mX, aID.mY); for shorter code.

gw updated this revision to Diff 209304.Dec 17 2019, 8:18 PM
gw marked 5 inline comments as done.
Harbormaster completed remote builds in B147409: Diff 209304.Dec 17 2019, 8:18 PM
gw added inline comments.Dec 17 2019, 8:18 PM
gfx/webrender_bindings/RenderCompositor.h
94–96

Switched the new interface methods and also TileKey to use int32_t

gfx/wr/example-compositor/compositor-windows/src/lib.cpp
59

Just because this is in the example-compositor so doesn't have HashGeneric available.

reviewbot added a comment.Dec 17 2019, 8:37 PM

Code analysis found 3 defects in the diff 209304:

  • 3 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/webrender_bindings/RenderCompositor.h gfx/webrender_bindings/DCLayerTree.h (C/C++)

For your convenience, here is a patch that fixes all the source-test-clang-format defects (use it in your repository with hg import or git apply -p0).

If you see a problem in this automated review, please report it here.

reviewbot added a comment.Dec 17 2019, 8:37 PM

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:

Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

gw updated this revision to Diff 209369.Dec 17 2019, 9:22 PM
Harbormaster completed remote builds in B147468: Diff 209369.Dec 17 2019, 9:22 PM
reviewbot added a comment.Dec 17 2019, 9:43 PM

In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:

Should they have tests, or are they dead code ?

  • You can file a bug blocking Bug 1415824 for untested files that should be tested.
  • You can file a bug blocking Bug 1415819 for untested files that should be removed.

If you see a problem in this automated review, please report it here.

Closed by commit rMOZILLACENTRALc81cf0c409bc: Bug 1604383 - Refactor the Compositor trait to allow support for DC virtual… (authored by gw). · Explain WhyDec 17 2019, 9:59 PM
This revision was automatically updated to reflect the committed changes.
gw added a commit: rMOZILLACENTRALc81cf0c409bc: Bug 1604383 - Refactor the Compositor trait to allow support for DC virtual….

Revision Contents
Changeset List

PathSize
gfx/
webrender_bindings/
84 lines
159 lines
9 lines
18 lines
11 lines
17 lines
46 lines
81 lines
src/
58 lines
wr/
example-compositor/
compositor-windows/
src/
155 lines
74 lines
compositor/
src/
40 lines
webrender/
src/
47 lines
196 lines
9 lines
2 lines
102 lines
3 lines
2 lines
60 lines
51 lines

Diff 209423

View Options

gfx/webrender_bindings/DCLayerTree.h

Loading...
View Options

gfx/webrender_bindings/DCLayerTree.cpp

Loading...
View Options

gfx/webrender_bindings/RenderCompositor.h

Loading...
View Options

gfx/webrender_bindings/RenderCompositor.cpp

Loading...
View Options

gfx/webrender_bindings/RenderCompositorANGLE.h

Loading...
View Options

gfx/webrender_bindings/RenderCompositorANGLE.cpp

Loading...
View Options

gfx/webrender_bindings/RenderCompositorOGL.h

Loading...
View Options

gfx/webrender_bindings/RenderCompositorOGL.cpp

Loading...
View Options

gfx/webrender_bindings/src/bindings.rs

Loading...
View Options

gfx/wr/example-compositor/compositor-windows/src/lib.cpp

Loading...
View Options

gfx/wr/example-compositor/compositor-windows/src/lib.rs

Loading...
View Options

gfx/wr/example-compositor/compositor/src/main.rs

Loading...
View Options

gfx/wr/webrender/src/batch.rs

Loading...
View Options

gfx/wr/webrender/src/composite.rs

Loading...
View Options

gfx/wr/webrender/src/frame_builder.rs

Loading...
View Options

gfx/wr/webrender/src/lib.rs

Loading...
View Options

gfx/wr/webrender/src/picture.rs

Loading...
View Options

gfx/wr/webrender/src/render_backend.rs

Loading...
View Options

gfx/wr/webrender/src/render_task.rs

Loading...
View Options

gfx/wr/webrender/src/renderer.rs

Loading...
View Options

gfx/wr/webrender/src/resource_cache.rs

Loading...
Privacy · Cookies · Legal