Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
880023005:
cc: Ignore certain quads that would otherwise block overlays (Closed)
Descriptioncc: Ignore certain quads that would otherwise block overlays
If a quad is fully transparent then it shouldn't block overlays. For
now we allow some SOLID_COLOR quads through
Committed: https://crrev.com/1db1789f9a2a7a4bdf44785352ecce9b2818f6a4
Cr-Commit-Position: refs/heads/master@{#315106}
Patch Set 1 #Patch Set 2 : add tests #
Total comments: 4
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 8
Patch Set 5 : fix tests #Patch Set 6 : #Patch Set 7 : add new test #
Total comments: 5
Patch Set 8 : change test name #
Messages
Total messages: 20 (3 generated)
achaulk@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/880023005/diff/20001/cc/output/overlay_strate... File cc/output/overlay_strategy_single_on_top.cc (right): https://codereview.chromium.org/880023005/diff/20001/cc/output/overlay_strate... cc/output/overlay_strategy_single_on_top.cc:131: return !solid_quad->ShouldDrawWithBlending() || !ShouldDrawWithBlending does not mean transparent like the comment suggests, rather it means it is opaque and so no blending is needed. https://codereview.chromium.org/880023005/diff/20001/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/20001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:611: shared_state->opacity = 0.f; can you test 1 opacity with a transparent color too?
https://codereview.chromium.org/880023005/diff/20001/cc/output/overlay_strate... File cc/output/overlay_strategy_single_on_top.cc (right): https://codereview.chromium.org/880023005/diff/20001/cc/output/overlay_strate... cc/output/overlay_strategy_single_on_top.cc:131: return !solid_quad->ShouldDrawWithBlending() || On 2015/02/04 18:42:32, danakj wrote: > !ShouldDrawWithBlending does not mean transparent like the comment suggests, > rather it means it is opaque and so no blending is needed. Done. https://codereview.chromium.org/880023005/diff/20001/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/20001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:611: shared_state->opacity = 0.f; On 2015/02/04 18:42:32, danakj wrote: > can you test 1 opacity with a transparent color too? Done.
https://codereview.chromium.org/880023005/diff/40001/cc/output/overlay_strate... File cc/output/overlay_strategy_single_on_top.cc (right): https://codereview.chromium.org/880023005/diff/40001/cc/output/overlay_strate... cc/output/overlay_strategy_single_on_top.cc:130: return solid_quad->ShouldDrawWithBlending() && Ok this makes more sense. Can you also add a test where this condition changes the behaviour?
https://codereview.chromium.org/880023005/diff/40001/cc/output/overlay_strate... File cc/output/overlay_strategy_single_on_top.cc (right): https://codereview.chromium.org/880023005/diff/40001/cc/output/overlay_strate... cc/output/overlay_strategy_single_on_top.cc:130: return solid_quad->ShouldDrawWithBlending() && On 2015/02/04 22:24:48, danakj wrote: > Ok this makes more sense. Can you also add a test where this condition changes > the behaviour? Done.
https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:585: CreateCandidateQuadAt(resource_provider_.get(), you moved the candidate on top? thats not what the test name says its doing..? https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:612: CreateSolidColorQuadAt(shared_state, SK_ColorBLACK, pass.get(), now the transparent is below?
https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:648: TEST_F(SingleOverlayOnTopTest, RejectOpaqueColorOnTop) { this new test is good for testing the color checks, but it would pass with or without the quad needs blending check wouldnt it?
https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:585: CreateCandidateQuadAt(resource_provider_.get(), On 2015/02/05 17:27:02, danakj wrote: > you moved the candidate on top? thats not what the test name says its doing..? Oh oops, this one wasn't supposed to move https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:612: CreateSolidColorQuadAt(shared_state, SK_ColorBLACK, pass.get(), On 2015/02/05 17:27:02, danakj wrote: > now the transparent is below? It was always below, that's what this was supposed to fix, but I must have forgotten to save ro something because that's not what I have locally. New CLs fix it https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:648: TEST_F(SingleOverlayOnTopTest, RejectOpaqueColorOnTop) { On 2015/02/05 17:28:25, danakj wrote: > this new test is good for testing the color checks, but it would pass with or > without the quad needs blending check wouldnt it? I'm not sure what you mean. You want a transparent color without blending? Just passing in the transparent color by itself is enough to make it accept it so something is being calculated off of the alpha value
https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:648: TEST_F(SingleOverlayOnTopTest, RejectOpaqueColorOnTop) { On 2015/02/05 18:12:19, achaulk wrote: > On 2015/02/05 17:28:25, danakj wrote: > > this new test is good for testing the color checks, but it would pass with or > > without the quad needs blending check wouldnt it? > > I'm not sure what you mean. You want a transparent color without blending? Just > passing in the transparent color by itself is enough to make it accept it so > something is being calculated off of the alpha value yes. that's the reason the needsblending check is there isnt it? otherwise checking the color is enough. but if its transparent but wont blend then that wont work with an overlay
https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/60001/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:648: TEST_F(SingleOverlayOnTopTest, RejectOpaqueColorOnTop) { On 2015/02/05 18:18:37, danakj wrote: > On 2015/02/05 18:12:19, achaulk wrote: > > On 2015/02/05 17:28:25, danakj wrote: > > > this new test is good for testing the color checks, but it would pass with > or > > > without the quad needs blending check wouldnt it? > > > > I'm not sure what you mean. You want a transparent color without blending? > Just > > passing in the transparent color by itself is enough to make it accept it so > > something is being calculated off of the alpha value > > yes. that's the reason the needsblending check is there isnt it? otherwise > checking the color is enough. but if its transparent but wont blend then that > wont work with an overlay Done.
LGTM w/ one more comment https://codereview.chromium.org/880023005/diff/40002/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/40002/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:673: TEST_F(SingleOverlayOnTopTest, RejectOpaqueColorOnTopWithBlending) { RejectTransparentOnTopWithoutBlending? https://codereview.chromium.org/880023005/diff/40002/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:677: kOverlayBottomRightRect)->opaque_rect = ah this wont happen, instead can you set the needs_blending bool to true? this is how it can occur, which is used for holepunching
https://codereview.chromium.org/880023005/diff/40002/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/40002/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:677: kOverlayBottomRightRect)->opaque_rect = On 2015/02/05 19:35:40, danakj wrote: > ah this wont happen, instead can you set the needs_blending bool to true? this > is how it can occur, which is used for holepunching Blended transparent quads should not block overlaying. This tests for ShouldDrawWithBlending() returning false with alpha=0 - in the other test it will return true and then fail due to alpha. Holepunching is done with needs_blending=false and sets the opaque_rect, which is exactly what this one does
LGTM https://codereview.chromium.org/880023005/diff/40002/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/40002/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:677: kOverlayBottomRightRect)->opaque_rect = On 2015/02/05 19:44:56, achaulk wrote: > On 2015/02/05 19:35:40, danakj wrote: > > ah this wont happen, instead can you set the needs_blending bool to true? this > > is how it can occur, which is used for holepunching > > Blended transparent quads should not block overlaying. This tests for > ShouldDrawWithBlending() returning false with alpha=0 - in the other test it > will return true and then fail due to alpha. Holepunching is done with > needs_blending=false and sets the opaque_rect, which is exactly what this one > does ahh sorry you're right. Test name should be WithoutBlendingThough right?
https://codereview.chromium.org/880023005/diff/40002/cc/output/overlay_unitte... File cc/output/overlay_unittest.cc (right): https://codereview.chromium.org/880023005/diff/40002/cc/output/overlay_unitte... cc/output/overlay_unittest.cc:677: kOverlayBottomRightRect)->opaque_rect = On 2015/02/05 19:59:22, danakj wrote: > On 2015/02/05 19:44:56, achaulk wrote: > > On 2015/02/05 19:35:40, danakj wrote: > > > ah this wont happen, instead can you set the needs_blending bool to true? > this > > > is how it can occur, which is used for holepunching > > > > Blended transparent quads should not block overlaying. This tests for > > ShouldDrawWithBlending() returning false with alpha=0 - in the other test it > > will return true and then fail due to alpha. Holepunching is done with > > needs_blending=false and sets the opaque_rect, which is exactly what this one > > does > > ahh sorry you're right. Test name should be WithoutBlendingThough right? oh right, yes i should rename it
New patchsets have been uploaded after l-g-t-m from danakj@chromium.org
The CQ bit was checked by achaulk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880023005/130001
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1db1789f9a2a7a4bdf44785352ecce9b2818f6a4 Cr-Commit-Position: refs/heads/master@{#315106} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
