Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2560723006:
[2/5] Disable transformed rasterization if layer is opaque (Closed)
|
Created:
4 years ago by trchen Modified:
3 years, 8 months ago Reviewers:
enne (OOO) CC:
cc-bugs_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[2/5] Disable transformed rasterization if layer is opaque
This CL disables transformed rasterization for opaque layers.
The reason is because transformed rasterization layers do not paint solid
color paddings around aliased layer edges. Those aliased edges will not
be opaque. However the opaque bound is only known until the actual raster
transform is determined, and in the worst corner case the whole layer
rect could be non-opaque with some extreme scale factor. This defeats many
opaque optimization. Prefer optimization over quality for this particular
case.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2560723006
Cr-Commit-Position: refs/heads/master@{#461613}
Committed: https://chromium.googlesource.com/chromium/src/+/c3593fe02146ff693ce324259b9b292bd43e4910
Patch Set 1 #
Total comments: 4
Patch Set 2 : rebase & add comments #Patch Set 3 : minus patch 1 #Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : change strategy #Patch Set 6 : rebase #Messages
Total messages: 23 (11 generated)
Description was changed from ========== [2/6] Disable occlusion if transformed rasterization is used ========== to ========== [2/6] Disable occlusion if transformed rasterization is used CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
enne@chromium.org changed reviewers: + enne@chromium.org
https://codereview.chromium.org/2560723006/diff/1/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2560723006/diff/1/cc/layers/picture_layer.cc#... cc/layers/picture_layer.cc:105: recording_source_->SetBackgroundColor(UseTransformedRasterization() ? SK_ColorTRANSPARENT : SafeOpaqueBackgroundColor()); This needs a comment as to why this is the case. https://codereview.chromium.org/2560723006/diff/1/cc/layers/picture_layer_imp... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2560723006/diff/1/cc/layers/picture_layer_imp... cc/layers/picture_layer_impl.cc:667: return SimpleEnclosedRegion(); Why not VisibleOpaqueRegion but Inset by 1?
https://codereview.chromium.org/2560723006/diff/1/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2560723006/diff/1/cc/layers/picture_layer.cc#... cc/layers/picture_layer.cc:105: recording_source_->SetBackgroundColor(UseTransformedRasterization() ? SK_ColorTRANSPARENT : SafeOpaqueBackgroundColor()); On 2017/01/03 22:53:07, enne wrote: > This needs a comment as to why this is the case. Done. https://codereview.chromium.org/2560723006/diff/1/cc/layers/picture_layer_imp... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2560723006/diff/1/cc/layers/picture_layer_imp... cc/layers/picture_layer_impl.cc:667: return SimpleEnclosedRegion(); On 2017/01/03 22:53:08, enne wrote: > Why not VisibleOpaqueRegion but Inset by 1? Because it depends on the actual raster scale. For example, a layer of size (99, 99) with raster scale 0.25, its opaque region in raster space would be EnclosedRect(ScaleRect(Rect(99,99), 0.25)) == Rect(24, 24). When mapped back to the layer space, it's only (96, 96). For non-transformed-rasterized layer it is fine because the edge pixel will be filled with opaque background color.
Description was changed from ========== [2/6] Disable occlusion if transformed rasterization is used CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [2/5] Disable occlusion if transformed rasterization is used CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== [2/5] Disable occlusion if transformed rasterization is used CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [2/5] Disable occlusion if transformed rasterization is used This CL disables occlusion (does not occlude layers below it.) for layers that have transformed rasterization. The reason is because transformed rasterization layers do not paint solid color paddings around aliased layer edges. Those aliased edges will not be opaque. However the opaque bound is only known until the actual raster transform is determined, and in the worst corner case the whole layer rect could be non-opaque with some extreme scale factor. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2560723006/diff/60001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2560723006/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer.cc:106: // When transformed rasterization is used, we try to match the rendering I'm not sure I agree with this. I think if a layer is marked as opaque then that should take priority over transformed rasterization. In other words, ShouldUseTransformedRasterization should take into account contents_opaque, and then this change here wouldn't be needed.
https://codereview.chromium.org/2560723006/diff/60001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2560723006/diff/60001/cc/layers/picture_layer... cc/layers/picture_layer.cc:106: // When transformed rasterization is used, we try to match the rendering On 2017/03/29 13:01:42, enne wrote: > I'm not sure I agree with this. I think if a layer is marked as opaque then > that should take priority over transformed rasterization. In other words, > ShouldUseTransformedRasterization should take into account contents_opaque, and > then this change here wouldn't be needed. I thought layer opaqueness is more of a hint than a directive. i.e. The layer client is guaranteed to return a paint record that is opaque within the layer bound. Thus cc could potentially apply optimization like dropping the alpha channel, using kSrc mode, not clearing raster buffer, that kind of stuff. Actually I feel background overfill does more harm than good with translated layers. Maybe I don't fully understand the rationale of it. Do you remember why was it introduced in the first place?
On 2017/03/29 at 21:50:36, trchen wrote: > https://codereview.chromium.org/2560723006/diff/60001/cc/layers/picture_layer.cc > File cc/layers/picture_layer.cc (right): > > https://codereview.chromium.org/2560723006/diff/60001/cc/layers/picture_layer... > cc/layers/picture_layer.cc:106: // When transformed rasterization is used, we try to match the rendering > On 2017/03/29 13:01:42, enne wrote: > > I'm not sure I agree with this. I think if a layer is marked as opaque then > > that should take priority over transformed rasterization. In other words, > > ShouldUseTransformedRasterization should take into account contents_opaque, and > > then this change here wouldn't be needed. > > I thought layer opaqueness is more of a hint than a directive. i.e. The layer client is guaranteed to return a paint record that is opaque within the layer bound. Thus cc could potentially apply optimization like dropping the alpha channel, using kSrc mode, not clearing raster buffer, that kind of stuff. > > Actually I feel background overfill does more harm than good with translated layers. Maybe I don't fully understand the rationale of it. Do you remember why was it introduced in the first place? I added the background overfill during impl-side painting development. There is a performance win to knowing that layers are opaque. There's an occlusion win: if you mark something as opaque, then we don't need to fill solid color gutter quads when producing a compositor frame to make sure that the entire frame is opaque, which helps with overdraw (which hurts a lot on ChromeOS especially). There's also a raster win. It's faster to draw a one pixel column and a one pixel row then it is to clear the entire texture to a color. You need that extra row and column because even if the layer is opaque in the layer bounds, if you scale it up, it's not necessarily opaque in the content bounds. So, layers are marked opaque as performance optimization. It's not just the root layer. There are some parts of the ui that also mark themselves as opaque to avoid the unnecessary clear (and do so because there was a bug and somebody chased down that as a perf optimization, so it's not just speculative). I think layers are not often marked as opaque and are extremely unlikely to be partially translated. We don't have any logic in Blink to support calculating layers as being opaque except for ones we know are guaranteed to be, like the root layer and scrollbars and maybe image layers. (Image layers with known non-alpha contents are likely the only thing that could be partially translated and also contents opaque.) So, I'd say just make contents opaque take precedence over partial translation to avoid performance surprises.
On 2017/03/30 14:51:04, enne wrote: > On 2017/03/29 at 21:50:36, trchen wrote: > > > https://codereview.chromium.org/2560723006/diff/60001/cc/layers/picture_layer.cc > > File cc/layers/picture_layer.cc (right): > > > > > https://codereview.chromium.org/2560723006/diff/60001/cc/layers/picture_layer... > > cc/layers/picture_layer.cc:106: // When transformed rasterization is used, we > try to match the rendering > > On 2017/03/29 13:01:42, enne wrote: > > > I'm not sure I agree with this. I think if a layer is marked as opaque then > > > that should take priority over transformed rasterization. In other words, > > > ShouldUseTransformedRasterization should take into account contents_opaque, > and > > > then this change here wouldn't be needed. > > > > I thought layer opaqueness is more of a hint than a directive. i.e. The layer > client is guaranteed to return a paint record that is opaque within the layer > bound. Thus cc could potentially apply optimization like dropping the alpha > channel, using kSrc mode, not clearing raster buffer, that kind of stuff. > > > > Actually I feel background overfill does more harm than good with translated > layers. Maybe I don't fully understand the rationale of it. Do you remember why > was it introduced in the first place? > > I added the background overfill during impl-side painting development. There is > a performance win to knowing that layers are opaque. There's an occlusion win: > if you mark something as opaque, then we don't need to fill solid color gutter > quads when producing a compositor frame to make sure that the entire frame is > opaque, which helps with overdraw (which hurts a lot on ChromeOS especially). > There's also a raster win. It's faster to draw a one pixel column and a one > pixel row then it is to clear the entire texture to a color. You need that > extra row and column because even if the layer is opaque in the layer bounds, if > you scale it up, it's not necessarily opaque in the content bounds. > > So, layers are marked opaque as performance optimization. It's not just the > root layer. There are some parts of the ui that also mark themselves as opaque > to avoid the unnecessary clear (and do so because there was a bug and somebody > chased down that as a perf optimization, so it's not just speculative). > > I think layers are not often marked as opaque and are extremely unlikely to be > partially translated. We don't have any logic in Blink to support calculating > layers as being opaque except for ones we know are guaranteed to be, like the > root layer and scrollbars and maybe image layers. (Image layers with known > non-alpha contents are likely the only thing that could be partially translated > and also contents opaque.) So, I'd say just make contents opaque take > precedence over partial translation to avoid performance surprises. Got it. Great thanks for your thorough explanation! Although I feel translated & opaque layer maybe more common than expected (because there may be a high positive correlation), I buy your idea to be safe and only proceed with supporting data. I will change this to make opaqueness take precedence. Still working on CL 3/5, will get back to this after.
Description was changed from ========== [2/5] Disable occlusion if transformed rasterization is used This CL disables occlusion (does not occlude layers below it.) for layers that have transformed rasterization. The reason is because transformed rasterization layers do not paint solid color paddings around aliased layer edges. Those aliased edges will not be opaque. However the opaque bound is only known until the actual raster transform is determined, and in the worst corner case the whole layer rect could be non-opaque with some extreme scale factor. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [2/5] Disable transformed rasterization if layer is opaque This CL disables transformed rasterization for opaque layers. The reason is because transformed rasterization layers do not paint solid color paddings around aliased layer edges. Those aliased edges will not be opaque. However the opaque bound is only known until the actual raster transform is determined, and in the worst corner case the whole layer rect could be non-opaque with some extreme scale factor. This defeats many opaque optimization. Prefer optimization over quality for this particular case. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Done.
lgtm, thanks. I am open to trying different approaches in the future, but I think this is a simpler thing to land as a first pass.
The CQ bit was checked by trchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/2560723006/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491267589349660,
"parent_rev": "f5a2ecc6e12c815fd95fc234a6055c1fc81069ad", "commit_rev":
"c3593fe02146ff693ce324259b9b292bd43e4910"}
Message was sent while issue was closed.
Description was changed from ========== [2/5] Disable transformed rasterization if layer is opaque This CL disables transformed rasterization for opaque layers. The reason is because transformed rasterization layers do not paint solid color paddings around aliased layer edges. Those aliased edges will not be opaque. However the opaque bound is only known until the actual raster transform is determined, and in the worst corner case the whole layer rect could be non-opaque with some extreme scale factor. This defeats many opaque optimization. Prefer optimization over quality for this particular case. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [2/5] Disable transformed rasterization if layer is opaque This CL disables transformed rasterization for opaque layers. The reason is because transformed rasterization layers do not paint solid color paddings around aliased layer edges. Those aliased edges will not be opaque. However the opaque bound is only known until the actual raster transform is determined, and in the worst corner case the whole layer rect could be non-opaque with some extreme scale factor. This defeats many opaque optimization. Prefer optimization over quality for this particular case. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2560723006 Cr-Commit-Position: refs/heads/master@{#461613} Committed: https://chromium.googlesource.com/chromium/src/+/c3593fe02146ff693ce324259b9b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c3593fe02146ff693ce324259b9b... |
