Page MenuHomePhabricator
Differential D61424

Bug 1608717 - Support per-tile clip (valid) region for native compositor implementations.
ClosedPublic
Actions

Authored by gw on Feb 3 2020, 5:41 AM.

Details

Reviewers
sotaro
mstange
nical
Commits
rMOZILLACENTRAL174d5be06caa: Bug 1608717 - Support per-tile clip (valid) region for native compositor…
Bugzilla Bug ID
1608717
Summary

For Draw (non-native) and CA modes, we include the per-tile
valid rect in the clip rect from the surface.

For DC (non-virtual) mode, a per-tile clip rect is set on the
visual for each tile, separate from the overall clip rect that
is set on the surface visual.

For DC (virtual) mode, the Trim API is used to remove pixels
in the virtual tile area that are outside the valid / clipped
region.

Note: Although the valid rect is now applied in the native
compositors, it's currently only based on the overall picture
cache bounding rect. Thus, with this patch there isn't any
noticeable performance improvement. Once this lands and is
working correctly, the follow up patch to calculate a smaller
valid region per-tile is a small amount of work.

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.Feb 3 2020, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2020, 5:41 AM
Harbormaster completed remote builds in B157667: Diff 223887.Feb 3 2020, 5:41 AM
phab-bot requested review of this revision.Feb 3 2020, 5:42 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 a parent revision: D61378: Bug 1608280 - Part 1 - Add a valid_rect to picture cache and composite tiles..Feb 3 2020, 5:42 AM
gw added reviewers: sotaro, mstange, nical.

This needs a bit of tidy up - but does the general approach seem reasonable? If you have other ideas for implementing this, let me know!

reviewbot added a subscriber: reviewbot.Feb 3 2020, 6:04 AM

Code analysis found 8 defects in the diff 223887:

  • 8 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/webrender_bindings/DCLayerTree.cpp gfx/webrender_bindings/RenderCompositorANGLE.cpp gfx/webrender_bindings/RenderCompositorOGL.cpp gfx/webrender_bindings/RenderCompositor.cpp (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.Feb 3 2020, 6:04 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 updated this revision to Diff 224143.Feb 3 2020, 8:59 PM
Harbormaster completed remote builds in B157831: Diff 224143.Feb 3 2020, 8:59 PM
reviewbot added a comment.Feb 3 2020, 9:15 PM

Code analysis found 8 defects in the diff 224143:

  • 8 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/webrender_bindings/DCLayerTree.cpp gfx/webrender_bindings/RenderCompositor.cpp gfx/webrender_bindings/RenderCompositorANGLE.cpp gfx/webrender_bindings/RenderCompositorOGL.cpp (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.Feb 3 2020, 9:15 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 added a comment.Feb 3 2020, 11:14 PM

Looks good to me!

gfx/webrender_bindings/DCLayerTree.cpp
457

wr::DeviceIntRect comes from FFI / cbindgen so there are no convenience methods. I recommend converting this to Gecko types, e.g. mozilla::gfx::IntRect, in a different place so that you get all the conveniences that come with it.

gfx/webrender_bindings/RenderCompositorOGL.cpp
352–354

gfx::IntRect validRect = layer->GetValidRect() + layerRect.TopLeft();

357

I see, you're doing the intersection of the clip and the valid rect outside of NativeLayer. I think that's fine. But then we should document on NativeLayer what the expectations around the valid rect are.

gw added inline comments.Feb 3 2020, 11:20 PM
gfx/webrender_bindings/RenderCompositorOGL.cpp
357

Yea, it some seem a bit untidy - but I was a bit unsure of the NativeLayer code regarding how it handles the mutated flags etc, so I did the intersection outside NativeLayer. I'll add some comments explaining this, thanks!

gw updated this revision to Diff 224171.Feb 4 2020, 12:21 AM
Harbormaster completed remote builds in B157850: Diff 224171.Feb 4 2020, 12:21 AM
gw marked 2 inline comments as done.Feb 4 2020, 12:22 AM
gw updated this revision to Diff 224172.Feb 4 2020, 12:24 AM
Harbormaster completed remote builds in B157851: Diff 224172.Feb 4 2020, 12:24 AM
reviewbot added a comment.Feb 4 2020, 12:35 AM

Code analysis found 8 defects in the diff 224171:

  • 8 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/webrender_bindings/DCLayerTree.cpp gfx/webrender_bindings/RenderCompositorOGL.cpp gfx/webrender_bindings/RenderCompositor.cpp gfx/webrender_bindings/RenderCompositorANGLE.cpp (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.Feb 4 2020, 12:35 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.

reviewbot added a comment.Feb 4 2020, 12:37 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 updated this revision to Diff 224193.Feb 4 2020, 3:03 AM
Harbormaster completed remote builds in B157870: Diff 224193.Feb 4 2020, 3:03 AM
gw marked an inline comment as done.Feb 4 2020, 3:04 AM
reviewbot added a comment.Feb 4 2020, 3:17 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 added a comment.EditedFeb 4 2020, 7:18 AM

During loading youtube video like https://www.youtube.com/watch?v=1La4QzGeaaQ, I saw a temporal black rectangle on Windows 10. It seems like a regression by this patch. I am going to re-check it.
I confirmed that backing out the patch addressed the problem.

gw added a comment.Feb 4 2020, 8:10 AM
In D61424#1874929, @sotaro wrote:

During loading youtube video like https://www.youtube.com/watch?v=1La4QzGeaaQ, I saw a temporal black rectangle on Windows 10. It seems like a regression by this patch. I am going to re-check it.
I confirmed that backing out the patch addressed the problem.

No need to re-check - I just saw a black rect on a couple of the tests in a try run too. I'll debug that tomorrow morning, thanks!

nical added inline comments.Feb 4 2020, 9:36 AM
gfx/webrender_bindings/DCLayerTree.cpp
461

Nit: you can do gfx::IntRect validRect = aValidRect.ToUnknownRect();

gw updated this revision to Diff 225714.Feb 7 2020, 5:16 AM
gw removed a parent revision: D61378: Bug 1608280 - Part 1 - Add a valid_rect to picture cache and composite tiles..
Herald added a subscriber: emilio. · View Herald TranscriptFeb 7 2020, 5:16 AM
Harbormaster completed remote builds in B158971: Diff 225714.Feb 7 2020, 5:16 AM
gw added a comment.Feb 7 2020, 5:18 AM

I believe this is ready for proper review now.

I commented out the update rect checks on Mac - the tests all pass, but we probably want to tidy this up / re-add the checks in a way that take the valid rect into account (mstange said he'd take a look at this).

The extra fuzziness I needed to add is related to a known inaccuracy we have in our dashed border rendering - there's a bug on file to look at this in future.

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

reviewbot added a comment.Feb 7 2020, 5:30 AM

Code analysis found 6 defects in the diff 225714:

  • 6 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/layers/NativeLayerCA.mm (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.Feb 7 2020, 5:30 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 added a comment.Feb 7 2020, 11:37 AM

I confirmed that the updated patch addressed the problem. It worked well:) DCLayerTree part looks good!

sotaro accepted this revision.Feb 8 2020, 12:07 AM
This revision is now accepted and ready to land.Feb 8 2020, 12:07 AM
gw updated this revision to Diff 226308.Feb 9 2020, 9:48 PM
Harbormaster completed remote builds in B159433: Diff 226308.Feb 9 2020, 9:48 PM
reviewbot added a comment.Feb 9 2020, 10:02 PM

Code analysis found 6 defects in the diff 226308:

  • 6 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/layers/NativeLayerCA.mm (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.Feb 9 2020, 10:02 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 added 1 blocking reviewer(s): mstange.Feb 9 2020, 11:22 PM
This revision now requires review to proceed.Feb 9 2020, 11:22 PM
gw added a child revision: D62177: Bug 1608280 - Part 2 - Use valid rect to reduce pixel costs..Feb 10 2020, 12:09 AM
gw updated this revision to Diff 226313.Feb 10 2020, 12:55 AM
Harbormaster completed remote builds in B159437: Diff 226313.Feb 10 2020, 12:55 AM
reviewbot added a comment.Feb 10 2020, 1:13 AM

Code analysis found 6 defects in the diff 226313:

  • 6 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/layers/NativeLayerCA.mm (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.Feb 10 2020, 1:13 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 updated this revision to Diff 226822.Feb 10 2020, 8:26 PM
Harbormaster completed remote builds in B159813: Diff 226822.Feb 10 2020, 8:26 PM
reviewbot added a comment.Feb 10 2020, 8:47 PM

Code analysis found 6 defects in the diff 226822:

  • 6 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/layers/NativeLayerCA.mm (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.Feb 10 2020, 8:47 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 227057.Feb 11 2020, 5:27 AM
Harbormaster completed remote builds in B159987: Diff 227057.Feb 11 2020, 5:27 AM
reviewbot added a comment.Feb 11 2020, 5:52 AM

Code analysis found 6 defects in the diff 227057:

  • 6 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/layers/NativeLayerCA.mm (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.Feb 11 2020, 5:52 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 updated this revision to Diff 227514.Feb 11 2020, 9:42 PM
Harbormaster completed remote builds in B160295: Diff 227514.Feb 11 2020, 9:42 PM
reviewbot added a comment.Feb 11 2020, 10:00 PM

Code analysis found 6 defects in the diff 227514:

  • 6 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p gfx/layers/NativeLayerCA.mm (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.Feb 11 2020, 10:00 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 227657.Feb 12 2020, 3:17 AM
Harbormaster completed remote builds in B160396: Diff 227657.Feb 12 2020, 3:17 AM
gw added a comment.Feb 12 2020, 3:19 AM

I updated the patch so that the valid rect is included as part of the Bind API, rather than as a separate API, which is much better. Will kick off a try run shortly, but this shouldn't have any effect.

Markus, this patch still has the commented out mac asserts - not sure if you want to address those before landing this or as a follow up?

reviewbot added a comment.Feb 12 2020, 3:38 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.

mstange added a comment.Feb 12 2020, 4:26 AM
In D61424#1903294, @gw wrote:

I updated the patch so that the valid rect is included as part of the Bind API, rather than as a separate API, which is much better. Will kick off a try run shortly, but this shouldn't have any effect.

Nice!

Markus, this patch still has the commented out mac asserts - not sure if you want to address those before landing this or as a follow up?

I'll do it as a follow-up. I'm planning to remove Set/GetValidRect again and make it a parameter on NextSurfaceAsFramebuffer/DrawTarget, and then intersect copyRegion with the new valid region.

mstange accepted this revision.Feb 12 2020, 4:27 AM
This revision is now accepted and ready to land.Feb 12 2020, 4:27 AM
Closed by commit rMOZILLACENTRAL174d5be06caa: Bug 1608717 - Support per-tile clip (valid) region for native compositor… (authored by gw). · Explain WhyFeb 12 2020, 5:21 AM
This revision was automatically updated to reflect the committed changes.
gw added a commit: rMOZILLACENTRAL174d5be06caa: Bug 1608717 - Support per-tile clip (valid) region for native compositor….

Revision Contents
Changeset List

PathSize
gfx/
layers/
5 lines
3 lines
41 lines
webrender_bindings/
7 lines
39 lines
3 lines
5 lines
3 lines
5 lines
3 lines
13 lines
src/
3 lines
wr/
tileview/
src/
2 lines
webrender/
src/
59 lines
13 lines
138 lines
1 line
3 lines
20 lines
wrench/
reftests/
snap/
transforms/
layout/
reftests/
css-gradients/
2 lines
css-grid/
24 lines

Diff 227682

View Options

gfx/layers/NativeLayer.h

Loading...
View Options

gfx/layers/NativeLayerCA.h

Loading...
View Options

gfx/layers/NativeLayerCA.mm

Loading...
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/tileview/src/main.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/picture.rs

Loading...
View Options

gfx/wr/webrender/src/render_target.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/wrench/reftests/snap/preserve-3d.png

Loading...
View Options

gfx/wr/wrench/reftests/transforms/near-plane-clip.png

Loading...
View Options

gfx/wr/wrench/reftests/transforms/perspective.png

Loading...
View Options

layout/reftests/css-gradients/reftest.list

Loading...
View Options

layout/reftests/css-grid/reftest.list

Loading...
Privacy · Cookies · Legal