|
|
DescriptionImprove overdraw with multiple windows
When 2 windows are opened simultaneously (overlapped or non-overlapped)
namely window 1 and window 2, before this patch, defining the occluded
rect is used to computing the largest unionized rectangle that is
formed with these 2 windows and mark layers behind it occluded. However,
when we compute occlusion, each step is more likely to be occluded by
things added to this region more recently due to the way we build
scenes with overlapping elements adjacent to each other in the Z order.
For example, if a unionized rectangle uses area from both window 1
and window 2: if a pixel is from window 1, it occludes 1 layer of
overdrawing (drawing the pixel from the wallpaper); if a pixel is from
window 2, it occludes 2 layer of overdrawing (drawing the pixel from the
wallpaper and the pixel from browser UI).
In this patch, a weighted function is introduced to optimize the
calculate the unionized rectangle where the area in window 2 is
multiplied by 2, so the unionized rectangle is the rectangle with
the highest weight.
BUG=680636
TEST=SimpleEnclosedRegionTest.Union
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2866063002
Cr-Commit-Position: refs/heads/master@{#474451}
Committed: https://chromium.googlesource.com/chromium/src/+/34c7a895b3da858147c3bc836b23584a8c01410a
Patch Set 1 #
Total comments: 27
Patch Set 2 : address comments #
Total comments: 1
Patch Set 3 : remove occlusion_track_unittests and rebase #
Messages
Total messages: 62 (53 generated)
Description was changed from ========== dana patch BUG= ========== to ========== dana patch BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== dana patch BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Improve overdraw with multiple windows BUG=680636 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:100001) has been deleted
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Improve overdraw with multiple windows BUG=680636 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, pixels in the unionized rectangle has different weights: if a pixel from window 1 is marked occluded, it saves 1 layer of overdrawing (drawing the pixel from the wallpaper behind it); if a pixel from window 2 is marked occluded, it saves 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI behind it). In this patch, when computing an unionized rectangle which can save most of overdrawing, the area window 2 is multiplied by 2 and area in window 1 is not. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, pixels in the unionized rectangle has different weights: if a pixel from window 1 is marked occluded, it saves 1 layer of overdrawing (drawing the pixel from the wallpaper behind it); if a pixel from window 2 is marked occluded, it saves 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI behind it). In this patch, when computing an unionized rectangle which can save most of overdrawing, the area window 2 is multiplied by 2 and area in window 1 is not. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, pixels in the unionized rectangle has different weights: if a pixel from window 1 is marked occluded, it saves 1 layer of overdrawing (drawing the pixel from the wallpaper behind it); if a pixel from window 2 is marked occluded, it saves 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI behind it). In this patch, a weighted function is introduced to optimize the calculate the unionized function where the area window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, pixels in the unionized rectangle has different weights: if a pixel from window 1 is marked occluded, it saves 1 layer of overdrawing (drawing the pixel from the wallpaper behind it); if a pixel from window 2 is marked occluded, it saves 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI behind it). In this patch, a weighted function is introduced to optimize the calculate the unionized function where the area window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, pixels in the unionized rectangle has different weights: if a pixel from window 1 is marked occluded, it saves 1 layer of overdrawing (drawing the pixel from the wallpaper behind it); if a pixel from window 2 is marked occluded, it saves 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI behind it). In this patch, a weighted function is introduced to optimize the calculate the unionized function where the area window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, pixels in the unionized rectangle has different weights: if a pixel from window 1 is marked occluded, it saves 1 layer of overdrawing (drawing the pixel from the wallpaper behind it); if a pixel from window 2 is marked occluded, it saves 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI behind it). In this patch, a weighted function is introduced to optimize the calculate the unionized function where the area window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, pixels in the unionized rectangle has different weights: if a pixel from window 1 is marked occluded, it saves 1 layer of overdrawing (drawing the pixel from the wallpaper behind it); if a pixel from window 2 is marked occluded, it saves 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI behind it). In this patch, a weighted function is introduced to optimize the calculate the unionized rectangle where the area in window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yiyix@chromium.org changed reviewers: + danakj@chromium.org
@danakj, could you please review this patch? Thank you very much.
https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... File cc/base/simple_enclosed_region.cc (right): https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:133: int64_t weighted_rect_area = I suggest to build 3 rects, old_rect_area, overlap_area, new_rect_area, then add/multiply etc along with compare in the if () statement. This separates the data from the logic/algorithm and would make this more clear to read I think. https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:134: static_cast<int64_t>(rect_.width() * rect_.height()) + cast just the width, that will promote before the multiply instead of after. if you promote after it will overflow at the size of width()'s type then upcast to more bits afterward. https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:135: static_cast<int64_t>(overlap.width() * overlap.height()); same https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:137: 2 * static_cast<int64_t>(adjusted_new_rect.width() * same https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:139: if (weighted_adjusted_new_rect_area > weighted_rect_area) Can you put a comment here explaining roughly what you say in the CL description to explain the weights? > if a pixel from window 2 is marked > occluded, it saves 2 layer of overdrawing (drawing the pixel from the > wallpaper and the pixel from browser UI behind it). This is based on the assumption that as we compute occlusion, each step is more likely to be occluded by things added to this region more recently due to the way we build scenes with overlapping elements adjacent to eachother in the Z order. Saying something to this effect would be very useful (both here and in the CL description). Maybe with a concrete example of 2 layers, one containing the other, similar to the browser UI? https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... File cc/base/simple_enclosed_region_unittest.cc (right): https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region_unittest.cc:416: r.Union(gfx::Rect(18, 8, 4, 4)); Can you use cases where the area of area(rect 2)*2 is > area(rect 1), so overlap is what decides it? These are all 2*4*4=2*16=32 < 10*10=100, so we won't choose it no matter what the overlap is. Or am I misunderstanding these cases? Maybe 8x8? Or 6x10? https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... File cc/trees/occlusion_tracker_unittest.cc (left): https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1466: // The right half of the occlusion is close enough to cast a shadow I think this part of the comment should be kept, explaining that the shadow is visible in the background filter. https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1468: // 3*5 + 10 = 25 pixels to the right. But I don't actually understand what's shrinking the rect more from what you wrote there. Theres a (0,0,50,200) area that could occlude. The shadow shrinks that on the left side by 25. But what shrinks it from the right and bottom sides? Did we pick something else to use as the occlusion instead of |occluding_layer|? https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... File cc/trees/occlusion_tracker_unittest.cc (right): https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1466: // The occlusion needs for the shadow reaches 3*5+10 pixel to the pixels https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1467: // right and the rect in filtered_surface which is (25, 25, 20*80), so 20, 80 https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1468: // the expected occlusion is (0, 25, 45*80). 45, 80 (it reads like a triple with a product otherwise without more context)
Patchset #1 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, pixels in the unionized rectangle has different weights: if a pixel from window 1 is marked occluded, it saves 1 layer of overdrawing (drawing the pixel from the wallpaper behind it); if a pixel from window 2 is marked occluded, it saves 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI behind it). In this patch, a weighted function is introduced to optimize the calculate the unionized rectangle where the area in window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, when we compute occlusion, each step is more likely to be occluded by things added to this region more recently due to the way we build scenes with overlapping elements adjacent to each other in the Z order. For example, if the unionized rectangle is uses area from both window 1 and window 2: if a pixel is from window 1, it occludes 1 layer of overdrawing (drawing the pixel from the wallpaper); if a pixel is from window 2, it occludes 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI). In this patch, a weighted function is introduced to optimize the calculate the unionized rectangle where the area in window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, our algorithm used to find the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, when we compute occlusion, each step is more likely to be occluded by things added to this region more recently due to the way we build scenes with overlapping elements adjacent to each other in the Z order. For example, if the unionized rectangle is uses area from both window 1 and window 2: if a pixel is from window 1, it occludes 1 layer of overdrawing (drawing the pixel from the wallpaper); if a pixel is from window 2, it occludes 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI). In this patch, a weighted function is introduced to optimize the calculate the unionized rectangle where the area in window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, defining the occluded rect is used to computing the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, when we compute occlusion, each step is more likely to be occluded by things added to this region more recently due to the way we build scenes with overlapping elements adjacent to each other in the Z order. For example, if a unionized rectangle uses area from both window 1 and window 2: if a pixel is from window 1, it occludes 1 layer of overdrawing (drawing the pixel from the wallpaper); if a pixel is from window 2, it occludes 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI). In this patch, a weighted function is introduced to optimize the calculate the unionized rectangle where the area in window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
@danakj, could you please review this updated pathed? Thank you https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... File cc/base/simple_enclosed_region.cc (right): https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:133: int64_t weighted_rect_area = On 2017/05/15 21:47:42, danakj wrote: > I suggest to build 3 rects, old_rect_area, overlap_area, new_rect_area, then > add/multiply etc along with compare in the if () statement. This separates the > data from the logic/algorithm and would make this more clear to read I think. I agree. If the area calculation is done separately, the combining function is easier to be understand. https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:134: static_cast<int64_t>(rect_.width() * rect_.height()) + On 2017/05/15 21:47:42, danakj wrote: > cast just the width, that will promote before the multiply instead of after. if > you promote after it will overflow at the size of width()'s type then upcast to > more bits afterward. Done. https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:135: static_cast<int64_t>(overlap.width() * overlap.height()); On 2017/05/15 21:47:42, danakj wrote: > same Done. https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:137: 2 * static_cast<int64_t>(adjusted_new_rect.width() * On 2017/05/15 21:47:42, danakj wrote: > same Done. https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:139: if (weighted_adjusted_new_rect_area > weighted_rect_area) On 2017/05/15 21:47:42, danakj wrote: > Can you put a comment here explaining roughly what you say in the CL description > to explain the weights? > > > if a pixel from window 2 is marked > > occluded, it saves 2 layer of overdrawing (drawing the pixel from the > > wallpaper and the pixel from browser UI behind it). > > This is based on the assumption that as we compute occlusion, each step is more > likely to be occluded by things added to this region more recently due to the > way we build scenes with overlapping elements adjacent to eachother in the Z > order. Saying something to this effect would be very useful (both here and in > the CL description). Maybe with a concrete example of 2 layers, one containing > the other, similar to the browser UI? I will add this to both. Your wording is better! Thanks you. https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... File cc/base/simple_enclosed_region_unittest.cc (right): https://codereview.chromium.org/2866063002/diff/140001/cc/base/simple_enclose... cc/base/simple_enclosed_region_unittest.cc:416: r.Union(gfx::Rect(18, 8, 4, 4)); On 2017/05/15 21:47:42, danakj wrote: > Can you use cases where the area of area(rect 2)*2 is > area(rect 1), so overlap > is what decides it? These are all 2*4*4=2*16=32 < 10*10=100, so we won't choose > it no matter what the overlap is. Or am I misunderstanding these cases? > > Maybe 8x8? Or 6x10? This would be a good test! I have this one for the sake of completeness. rect 2 can be ignored as in the case of disjoint rectangles. https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... File cc/trees/occlusion_tracker_unittest.cc (left): https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1466: // The right half of the occlusion is close enough to cast a shadow On 2017/05/15 21:47:43, danakj wrote: > I think this part of the comment should be kept, explaining that the shadow is > visible in the background filter. Done. https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1468: // 3*5 + 10 = 25 pixels to the right. On 2017/05/15 21:47:43, danakj wrote: > But I don't actually understand what's shrinking the rect more from what you > wrote there. Theres a (0,0,50,200) area that could occlude. The shadow shrinks > that on the left side by 25. But what shrinks it from the right and bottom > sides? > > Did we pick something else to use as the occlusion instead of |occluding_layer|? From what I understand, there are 2 rects: occluding_layer: (0, 0, 50. 200) && filtered_surface: (50, 50, 100, 100). The filter surface is first transformed to (50, 50, 50, 50) by |scale_by_half|, then it become (25, 25, 80, 80) after applying filter. After "expanding the non-opaque pixels outside the rect", the rect in filter surface become (25, 25, 20, 80). This process is not explained in the original test description. I can put some transformation details in the description. https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... File cc/trees/occlusion_tracker_unittest.cc (right): https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1466: // The occlusion needs for the shadow reaches 3*5+10 pixel to the On 2017/05/15 21:47:42, danakj wrote: > pixels Done. https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1467: // right and the rect in filtered_surface which is (25, 25, 20*80), so On 2017/05/15 21:47:43, danakj wrote: > 20, 80 Done. https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1468: // the expected occlusion is (0, 25, 45*80). On 2017/05/15 21:47:43, danakj wrote: > 45, 80 (it reads like a triple with a product otherwise without more context) Done.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I'm mostly concerned about the occlusion tracker test. I've left a bunch of comments explaining my take on it. The place where we move from the filtered_surface back to the root, and consider the background filters is in ReduceOcclusionBelowSurface(). Could you have a look at what's changing in there before/after this CL? https://cs.chromium.org/chromium/src/cc/trees/occlusion_tracker.cc?rcl=3dec17... https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... File cc/trees/occlusion_tracker_unittest.cc (right): https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1441: this->VisitLayer(occluding_layer, &occlusion); This line would create an occluded area of 0,0,50,200 in the root target (from occluding_layer) https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1447: this->VisitLayer(filtered_surface, &occlusion); filtered_surface is not opaque, so no occlusion added. but this moves our current context to be relative to the filtered_surface target instead of the root. the occlusion from the root surface is now |occlusion_from_outside_target|, which is (-50, -50, 50, 200) because in this context the the origin is the top-left of the filtered_surface's target https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1453: EXPECT_EQ(occlusion_inside_surface.ToString(), which is verified here https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1460: this->VisitContributingSurface(filtered_surface, &occlusion); this is moving the current context back to the root target, and applying any occlusion or state from inside the filtered_surface's target to the root. part of that is the fact there's this background filter, so we apply it by reducing the occlusion in the root target. I don't think that your patch should have changed anything up till here, since there's only been a single rect occluding anything, and that's seen by the expectations not changing. https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:1461: this->EnterLayer(parent, &occlusion); this enters the root layer to ask what parts of it are still considered occluded. without the background filter, this would have been 0,0,50,200. but we had to shrink this rect from the right side of it because the background filter is going to expose some of those pixels. before, we shrank that rect uniformly by 25px. leaving us with 0,0,25,200. After this patch tho, we have 0,25,45,80. That means we shrank the rect from the right by less, only by 5 pixels now. But also from the top by 25, shrank it from the bottom by 95. This is super surprising to me. Why would changing how we add rects to the region mean that this now subtracts a smaller area from the right, and is subtracting from the top and bottom. Also, subtracting less from the right means we're considering pixels as occluded that used to be not. The old comment said that the background filter exposes 25 pixels, but now we're only exposing 5 pixels from the right, so we'll keep occluded pixels that the filter uses?
https://codereview.chromium.org/2866063002/diff/160001/cc/base/simple_enclose... File cc/base/simple_enclosed_region.cc (right): https://codereview.chromium.org/2866063002/diff/160001/cc/base/simple_enclose... cc/base/simple_enclosed_region.cc:141: // in the Z order. So, the area of the new rect as a weight of 2 in the has a weight
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/17 15:15:57, danakj wrote: > Thanks, I'm mostly concerned about the occlusion tracker test. I've left a bunch > of comments explaining my take on it. The place where we move from the > filtered_surface back to the root, and consider the background filters is in > ReduceOcclusionBelowSurface(). Could you have a look at what's changing in there > before/after this CL? > > https://cs.chromium.org/chromium/src/cc/trees/occlusion_tracker.cc?rcl=3dec17... > > https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... > File cc/trees/occlusion_tracker_unittest.cc (right): > > https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... > cc/trees/occlusion_tracker_unittest.cc:1441: this->VisitLayer(occluding_layer, > &occlusion); > This line would create an occluded area of 0,0,50,200 in the root target (from > occluding_layer) > > https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... > cc/trees/occlusion_tracker_unittest.cc:1447: this->VisitLayer(filtered_surface, > &occlusion); > filtered_surface is not opaque, so no occlusion added. but this moves our > current context to be relative to the filtered_surface target instead of the > root. > > the occlusion from the root surface is now |occlusion_from_outside_target|, > which is (-50, -50, 50, 200) because in this context the the origin is the > top-left of the filtered_surface's target > > https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... > cc/trees/occlusion_tracker_unittest.cc:1453: > EXPECT_EQ(occlusion_inside_surface.ToString(), > which is verified here > > https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... > cc/trees/occlusion_tracker_unittest.cc:1460: > this->VisitContributingSurface(filtered_surface, &occlusion); > this is moving the current context back to the root target, and applying any > occlusion or state from inside the filtered_surface's target to the root. > > part of that is the fact there's this background filter, so we apply it by > reducing the occlusion in the root target. > > I don't think that your patch should have changed anything up till here, since > there's only been a single rect occluding anything, and that's seen by the > expectations not changing. > > https://codereview.chromium.org/2866063002/diff/140001/cc/trees/occlusion_tra... > cc/trees/occlusion_tracker_unittest.cc:1461: this->EnterLayer(parent, > &occlusion); > this enters the root layer to ask what parts of it are still considered > occluded. > > without the background filter, this would have been 0,0,50,200. but we had to > shrink this rect from the right side of it because the background filter is > going to expose some of those pixels. > > before, we shrank that rect uniformly by 25px. leaving us with 0,0,25,200. > > After this patch tho, we have 0,25,45,80. That means we shrank the rect from the > right by less, only by 5 pixels now. But also from the top by 25, shrank it from > the bottom by 95. > > This is super surprising to me. Why would changing how we add rects to the > region mean that this now subtracts a smaller area from the right, and is > subtracting from the top and bottom. > > Also, subtracting less from the right means we're considering pixels as occluded > that used to be not. The old comment said that the background filter exposes 25 > pixels, but now we're only exposing 5 pixels from the right, so we'll keep > occluded pixels that the filter uses? As we have discussed offline, the failure in occlusion_tracker_unittest is caused by a bug in the existing code. Please refer to https://codereview.chromium.org/2892073002/ for details. @danakj, could you please review this new patch? Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by yiyix@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": 220001, "attempt_start_ts": 1495664570503190, "parent_rev": "12dedc4fa09fc54ee6d0bd54d7ffde5c5ab2bdba", "commit_rev": "34c7a895b3da858147c3bc836b23584a8c01410a"}
Message was sent while issue was closed.
Description was changed from ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, defining the occluded rect is used to computing the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, when we compute occlusion, each step is more likely to be occluded by things added to this region more recently due to the way we build scenes with overlapping elements adjacent to each other in the Z order. For example, if a unionized rectangle uses area from both window 1 and window 2: if a pixel is from window 1, it occludes 1 layer of overdrawing (drawing the pixel from the wallpaper); if a pixel is from window 2, it occludes 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI). In this patch, a weighted function is introduced to optimize the calculate the unionized rectangle where the area in window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Improve overdraw with multiple windows When 2 windows are opened simultaneously (overlapped or non-overlapped) namely window 1 and window 2, before this patch, defining the occluded rect is used to computing the largest unionized rectangle that is formed with these 2 windows and mark layers behind it occluded. However, when we compute occlusion, each step is more likely to be occluded by things added to this region more recently due to the way we build scenes with overlapping elements adjacent to each other in the Z order. For example, if a unionized rectangle uses area from both window 1 and window 2: if a pixel is from window 1, it occludes 1 layer of overdrawing (drawing the pixel from the wallpaper); if a pixel is from window 2, it occludes 2 layer of overdrawing (drawing the pixel from the wallpaper and the pixel from browser UI). In this patch, a weighted function is introduced to optimize the calculate the unionized rectangle where the area in window 2 is multiplied by 2, so the unionized rectangle is the rectangle with the highest weight. BUG=680636 TEST=SimpleEnclosedRegionTest.Union CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2866063002 Cr-Commit-Position: refs/heads/master@{#474451} Committed: https://chromium.googlesource.com/chromium/src/+/34c7a895b3da858147c3bc836b23... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:220001) as https://chromium.googlesource.com/chromium/src/+/34c7a895b3da858147c3bc836b23... |