Respect window transparency when applying window shape.

Respect window transparency when applying window shape.

Currently, the code clamps alpha up to to 100% within the window shape and
down to 0% outside the shape. This means that windows with shape *and*
transparency enabled effectively lose their transparency.

This change makes it so that the alpha is clamped up to 0% within the window
shape, which preserves the window's transparency.

BUG=405751
Committed: https://crrev.com/eba92949d768e9905fd45e5b10bef8f9f1d21d08
Cr-Commit-Position: refs/heads/master@{#293433}

[danakj, 2014-08-21 23:13:25]

Doesn't this just completely disable the whole thing? I thought the requirements
were that "things you can click must be visibly clickable" and "things yuo can't
click must be visible not clickable".

Should this be clamped to the layer's overall opacity instead of 1.0?

[danakj, 2014-08-21 23:13:39]

Also, tests?

[garykac, 2014-08-22 04:07:03]

On 2014/08/21 23:13:25, danakj wrote:
> Doesn't this just completely disable the whole thing? I thought the
requirements
> were that "things you can click must be visibly clickable" and "things yuo
can't
> click must be visible not clickable".

(summarizing our previous conversation)
Window shape and window transparency (alphaEnabled windows) are 2 different
window features:

* Window shape is a "cut out" that chops off parts that are outside the shape
(by making them completely transparent). User input events are ignored outside
this shape.

* alphaEnabled is still an experimental feature that allows windows to set a
transparent background. It only works on dev channel or for whitelisted apps.
This change respects that transparency values set within the window shape.

Because alphaEnabled has not shipped outside of dev/whitelist, all shaped
windows are currently opaque.  alphaEnabled will not be released on stable until
it has passed security review.

[garykac, 2014-08-22 04:07:15]

On 2014/08/21 23:13:39, danakj wrote:
> Also, tests?

added.

[danakj, 2014-08-25 17:17:38]

One test is failing
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

I don't understand why you have all the platform #ifdefs in there. You have one
layer blending on top of another layer. Why is the platform changing how a layer
blends onto another? Surely if you just did a test with one layer having opacity
< 1 you wouldn't need all these #ifs? What's going on there?

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:892: // Checks that pixels are actually drawn to
the screen with a read back.
This test is checking something about filters, not that pixels are drawn. Can
you make this comment about the reason this test exists?

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:896: // The window should be some non-trivial
size but may not be exactly
drop this copy pasta

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:907: #if defined(USE_AURA) &&
!defined(OS_CHROMEOS)
why is alpha_blend false on chromeos?

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:907: #if defined(USE_AURA) &&
!defined(OS_CHROMEOS)
use_aura is always true in this test file, no?

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:910: if (!ui::win::IsAeroGlassEnabled()) {
what does aero glass have to do with the compositor drawing a transparent layer?

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:914: #endif  // USE_AURA && SK_SUPPORT_GPU
this comment does not match your #if

[garykac, 2014-08-26 01:44:38]

> One test is failing
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

That's because this test succeeds when run locally, but fails on the bots. I
presume that this is because my desktop machine is more capable in terms of
GPU/transparency support when compared to the bots.

To fix this, I think my only option is to have the test allow *either* value.
Otherwise, the bots will succeed but local builds will fail, which isn't very
useful.

> I don't understand why you have all the platform #ifdefs in there. You have
one
> layer blending on top of another layer. Why is the platform changing how a
layer
> blends onto another?

The #ifdefs are required for the tests to pass. It appears that the bots are not
a good testing environment for Aura/transparency since they seem to have some of
these features disabled. I assume that this is because the bots are missing
required hardware to run these tests.

AFAICT, this sounds like a bug with the bots.  I'm planning on filing a bug for
this (since it clearly falls outside the scope of this cl), but I'm not yet sure
who to assign it to.

> Surely if you just did a test with one layer having opacity
> < 1 you wouldn't need all these #ifs? What's going on there?

I don't know the details of why it behaves the way it does. I expected all these
tests to work with transparency and was surprised (and dismayed) when they
didn't.

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:892: // Checks that pixels are actually drawn to
the screen with a read back.
On 2014/08/25 17:17:37, danakj wrote:
> This test is checking something about filters, not that pixels are drawn. Can
> you make this comment about the reason this test exists?

Done.

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:896: // The window should be some non-trivial
size but may not be exactly
On 2014/08/25 17:17:37, danakj wrote:
> drop this copy pasta

Done.

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:907: #if defined(USE_AURA) &&
!defined(OS_CHROMEOS)
On 2014/08/25 17:17:37, danakj wrote:
> why is alpha_blend false on chromeos?

Because the CrOS bots apparently don't alpha blend (as determined empirically).

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:907: #if defined(USE_AURA) &&
!defined(OS_CHROMEOS)
On 2014/08/25 17:17:37, danakj wrote:
> use_aura is always true in this test file, no?

I would have loved for that to have been true, but it is not true on some of the
Windows bots.

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:910: if (!ui::win::IsAeroGlassEnabled()) {
On 2014/08/25 17:17:37, danakj wrote:
> what does aero glass have to do with the compositor drawing a transparent
layer?

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...

I temporarily removed the AeroGlass check to verify that it was needed here.
Sadly, the tests fail without this, so I added it back.

https://codereview.chromium.org/499503002/diff/100001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:914: #endif  // USE_AURA && SK_SUPPORT_GPU
On 2014/08/25 17:17:37, danakj wrote:
> this comment does not match your #if

Done.

[garykac, 2014-08-26 01:45:01]

garykac@chromium.org changed reviewers:
+ wez@chromium.org

[garykac, 2014-08-26 01:45:02]

+wez, FYI

[Wez, 2014-08-26 01:53:39]

Thanks Gary! Let's get this fix landed ASAP.

Where does the new DEPS dependency on ui/base come from?

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:903: SkColor bluish = SkColorSetARGBInline(40,
0, 0, 255);
nit: translucent_blue?

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:905: bool alpha_blend = false;
expect_alpha_blend?

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:909: if (!ui::win::IsAeroGlassEnabled()) {
Do we need a similar test for Linux Aura, since we won't get desktop compositing
there unless we have a suitable window manager, configuration and GPU?

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:920: // Transparent blue overwriting lower
layer.
This is confusingly worded.

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:924: scoped_ptr<Layer> layer(
layer1, or background_layer

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:926: scoped_ptr<Layer> layer2(
or blue_layer or foreground_layer

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:942: // Ignore the edge pixels since the
blending can vary on some platforms.
This only seems to ignore the top-most and left-most, not the right-most nor
bottom-most edges?

[danakj, 2014-08-26 01:59:42]

On Mon, Aug 25, 2014 at 9:44 PM, <garykac@chromium.org> wrote:

> One test is failing
>>
>
> http://build.chromium.org/p/tryserver.chromium.linux/
> builders/linux_chromium_...
>
> That's because this test succeeds when run locally, but fails on the bots.
> I
> presume that this is because my desktop machine is more capable in terms of
> GPU/transparency support when compared to the bots.
>

No, I don't think that is true. There's no "GPUs without support for
transparency" involved here. In fact these tests do not use GPU at all,
they use OSMesa which runs all GL commands on the CPU, and that config is
the same locally as on the bots.

To fix this, I think my only option is to have the test allow *either*
> value.
> Otherwise, the bots will succeed but local builds will fail, which isn't
> very
> useful.
>
>
>  I don't understand why you have all the platform #ifdefs in there. You
>> have
>>
> one
>
>> layer blending on top of another layer. Why is the platform changing how a
>>
> layer
>
>> blends onto another?
>>
>
> The #ifdefs are required for the tests to pass. It appears that the bots
> are not
> a good testing environment for Aura/transparency since they seem to have
> some of
> these features disabled. I assume that this is because the bots are missing
> required hardware to run these tests.
>

There is no GPU hardware involved in these tests. How does it fail
differently with aero enabled vs off? What kind of pixel values do you see?

AFAICT, this sounds like a bug with the bots.  I'm planning on filing a bug
> for
> this (since it clearly falls outside the scope of this cl), but I'm not
> yet sure
> who to assign it to.
>
>
>  Surely if you just did a test with one layer having opacity
>> < 1 you wouldn't need all these #ifs? What's going on there?
>>
>
> I don't know the details of why it behaves the way it does. I expected all
> these
> tests to work with transparency and was surprised (and dismayed) when they
> didn't.
>
>
>
> https://codereview.chromium.org/499503002/diff/100001/ui/
> compositor/layer_unittest.cc
> File ui/compositor/layer_unittest.cc (right):
>
> https://codereview.chromium.org/499503002/diff/100001/ui/
> compositor/layer_unittest.cc#newcode892
> ui/compositor/layer_unittest.cc:892: // Checks that pixels are actually
> drawn to the screen with a read back.
> On 2014/08/25 17:17:37, danakj wrote:
>
>> This test is checking something about filters, not that pixels are
>>
> drawn. Can
>
>> you make this comment about the reason this test exists?
>>
>
> Done.
>
>
> https://codereview.chromium.org/499503002/diff/100001/ui/
> compositor/layer_unittest.cc#newcode896
> ui/compositor/layer_unittest.cc:896: // The window should be some
> non-trivial size but may not be exactly
> On 2014/08/25 17:17:37, danakj wrote:
>
>> drop this copy pasta
>>
>
> Done.
>
>
> https://codereview.chromium.org/499503002/diff/100001/ui/
> compositor/layer_unittest.cc#newcode907
> ui/compositor/layer_unittest.cc:907: #if defined(USE_AURA) &&
> !defined(OS_CHROMEOS)
> On 2014/08/25 17:17:37, danakj wrote:
>
>> why is alpha_blend false on chromeos?
>>
>
> Because the CrOS bots apparently don't alpha blend (as determined
> empirically).
>
>
> https://codereview.chromium.org/499503002/diff/100001/ui/
> compositor/layer_unittest.cc#newcode907
> ui/compositor/layer_unittest.cc:907: #if defined(USE_AURA) &&
> !defined(OS_CHROMEOS)
> On 2014/08/25 17:17:37, danakj wrote:
>
>> use_aura is always true in this test file, no?
>>
>
> I would have loved for that to have been true, but it is not true on
> some of the Windows bots.
>
>
> https://codereview.chromium.org/499503002/diff/100001/ui/
> compositor/layer_unittest.cc#newcode910
> ui/compositor/layer_unittest.cc:910: if (!ui::win::IsAeroGlassEnabled())
> {
> On 2014/08/25 17:17:37, danakj wrote:
>
>> what does aero glass have to do with the compositor drawing a
>>
> transparent layer?
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc&q=
> IsTranslucentWindowOpacitySupported&sq=package:chromium&type=cs&l=430
>
> I temporarily removed the AeroGlass check to verify that it was needed
> here. Sadly, the tests fail without this, so I added it back.
>
>
> https://codereview.chromium.org/499503002/diff/100001/ui/
> compositor/layer_unittest.cc#newcode914
> ui/compositor/layer_unittest.cc:914: #endif  // USE_AURA &&
> SK_SUPPORT_GPU
> On 2014/08/25 17:17:37, danakj wrote:
>
>> this comment does not match your #if
>>
>
> Done.
>
> https://codereview.chromium.org/499503002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-08-26 02:19:01]

On Mon, Aug 25, 2014 at 9:59 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Mon, Aug 25, 2014 at 9:44 PM, <garykac@chromium.org> wrote:
>
>> One test is failing
>>>
>>
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/linux_chromium_...
>>
>> That's because this test succeeds when run locally, but fails on the
>> bots. I
>> presume that this is because my desktop machine is more capable in terms
>> of
>> GPU/transparency support when compared to the bots.
>>
>
> No, I don't think that is true. There's no "GPUs without support for
> transparency" involved here. In fact these tests do not use GPU at all,
> they use OSMesa which runs all GL commands on the CPU, and that config is
> the same locally as on the bots.
>

The LayerWithRealCompositorTest.Opacity seems to render with blending just
fine, this seems like a real problem in the shape code somewhere that needs
to be tracked down.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2014-08-28 20:53:10]

Dana, Gary, can we get this fix landed now and follow-up with a resolution to
the shape/test issues?

[danakj, 2014-08-28 20:55:19]

On Thu, Aug 28, 2014 at 4:53 PM, <wez@chromium.org> wrote:

> Dana, Gary, can we get this fix landed now and follow-up with a resolution
> to
> the shape/test issues?
>

It seems like it's broken, I'd like to have these problems tracked down
rather than potentially add to them.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2014-08-28 20:59:49]

Dana, the original one-line fix resolves a well-understood regression,
which needs merging back to M38.  It's extremely unlikely to introduce any
behaviour that will break things further than they are already in M38.  I'd
prefer that we land the minimal fix ready for merge, and follow-up with
investigation of any remaining issues.


On 28 August 2014 13:54, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Aug 28, 2014 at 4:53 PM, <wez@chromium.org> wrote:
>
>> Dana, Gary, can we get this fix landed now and follow-up with a
>> resolution to
>> the shape/test issues?
>>
>
> It seems like it's broken, I'd like to have these problems tracked down
> rather than potentially add to them.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-08-28 21:43:37]

On Thu, Aug 28, 2014 at 4:59 PM, Wez <wez@chromium.org> wrote:

> Dana, the original one-line fix resolves a well-understood regression,
> which needs merging back to M38.  It's extremely unlikely to introduce any
> behaviour that will break things further than they are already in M38.  I'd
> prefer that we land the minimal fix ready for merge, and follow-up with
> investigation of any remaining issues.
>

Sorry but I don't want to approve a patch we can't test.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2014-08-28 22:16:04]

The exceptions in the new tests that Gary is adding are identical to the
exceptions in the window transparency tests, indicating that whatever issue
there is is in the transparency implementation, and not related in any way
to this CL.



On 28 August 2014 14:43, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Aug 28, 2014 at 4:59 PM, Wez <wez@chromium.org> wrote:
>
>> Dana, the original one-line fix resolves a well-understood regression,
>> which needs merging back to M38.  It's extremely unlikely to introduce any
>> behaviour that will break things further than they are already in M38.  I'd
>> prefer that we land the minimal fix ready for merge, and follow-up with
>> investigation of any remaining issues.
>>
>
> Sorry but I don't want to approve a patch we can't test.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-08-28 22:19:13]

On Thu, Aug 28, 2014 at 6:16 PM, Wez <wez@chromium.org> wrote:

> The exceptions in the new tests that Gary is adding are identical to the
> exceptions in the window transparency tests, indicating that whatever issue
> there is is in the transparency implementation, and not related in any way
> to this CL.
>

The new tests are blending on ui::Layer on top of another ui::Layer. What
does that have to do with window transparency? There's other ui::Layer
tests with transparency in that same file that don't need any
platform-specific exceptions.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2014-08-28 22:30:04]

Sorry; some miscommunication at this end - IIUC that same test without the
SetAlphaShape call will still need the #ifdefs, because transparency seems
broken under some try-bot configs - Gary is verifying that now.


On 28 August 2014 15:18, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Aug 28, 2014 at 6:16 PM, Wez <wez@chromium.org> wrote:
>
>> The exceptions in the new tests that Gary is adding are identical to the
>> exceptions in the window transparency tests, indicating that whatever issue
>> there is is in the transparency implementation, and not related in any way
>> to this CL.
>>
>
> The new tests are blending on ui::Layer on top of another ui::Layer. What
> does that have to do with window transparency? There's other ui::Layer
> tests with transparency in that same file that don't need any
> platform-specific exceptions.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[garykac, 2014-08-29 21:05:38]

On 2014/08/28 22:19:13, danakj wrote:
> On Thu, Aug 28, 2014 at 6:16 PM, Wez <mailto:wez@chromium.org> wrote:
> 
> > The exceptions in the new tests that Gary is adding are identical to the
> > exceptions in the window transparency tests, indicating that whatever issue
> > there is is in the transparency implementation, and not related in any way
> > to this CL.
> >
> 
> The new tests are blending on ui::Layer on top of another ui::Layer. What
> does that have to do with window transparency?

Drawing transparent colors into a layer apparently has a bug with blending for
these cases (hence the #ifdefs).  This is completely independent of using the
AlphaShape filter to enforcing shape - the bug occurs when blending an alpha
layer without shape (I've added a test for this case - I'll be uploading the
patch soon).

I've opened https://code.google.com/p/chromium/issues/detail?id=409150 to track
the issue.

> There's other ui::Layer
> tests with transparency in that same file that don't need any
> platform-specific exceptions.

I don't see any tests in this file that draw into a layer using an alpha-blended
color - they all use solid colors (with no alpha). There are some opacity tests,
but those aren't the same since they apply to an entire layer.

Which tests were you referring to?

[danakj, 2014-08-29 21:11:49]

On 2014/08/29 21:05:38, garykac wrote:
> On 2014/08/28 22:19:13, danakj wrote:
> > On Thu, Aug 28, 2014 at 6:16 PM, Wez <mailto:wez@chromium.org> wrote:
> > 
> > > The exceptions in the new tests that Gary is adding are identical to the
> > > exceptions in the window transparency tests, indicating that whatever
issue
> > > there is is in the transparency implementation, and not related in any way
> > > to this CL.
> > >
> > 
> > The new tests are blending on ui::Layer on top of another ui::Layer. What
> > does that have to do with window transparency?
> 
> Drawing transparent colors into a layer apparently has a bug with blending for
> these cases (hence the #ifdefs).  This is completely independent of using the
> AlphaShape filter to enforcing shape - the bug occurs when blending an alpha
> layer without shape (I've added a test for this case - I'll be uploading the
> patch soon).
> 
> I've opened https://code.google.com/p/chromium/issues/detail?id=409150 to
track
> the issue.

Could it be this at play? If so, could you explain how?

https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...

Or can you explain why this test works? What's the difference?

https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...


> > There's other ui::Layer
> > tests with transparency in that same file that don't need any
> > platform-specific exceptions.
> 
> I don't see any tests in this file that draw into a layer using an
alpha-blended
> color - they all use solid colors (with no alpha). There are some opacity
tests,
> but those aren't the same since they apply to an entire layer.
> 
> Which tests were you referring to?

[Wez, 2014-08-29 21:28:42]

On 2014/08/29 21:11:49, danakj wrote:
> On 2014/08/29 21:05:38, garykac wrote:
> > On 2014/08/28 22:19:13, danakj wrote:
> > > On Thu, Aug 28, 2014 at 6:16 PM, Wez <mailto:wez@chromium.org> wrote:
> > > 
> > > > The exceptions in the new tests that Gary is adding are identical to the
> > > > exceptions in the window transparency tests, indicating that whatever
> issue
> > > > there is is in the transparency implementation, and not related in any
way
> > > > to this CL.
> > > >
> > > 
> > > The new tests are blending on ui::Layer on top of another ui::Layer. What
> > > does that have to do with window transparency?
> > 
> > Drawing transparent colors into a layer apparently has a bug with blending
for
> > these cases (hence the #ifdefs).  This is completely independent of using
the
> > AlphaShape filter to enforcing shape - the bug occurs when blending an alpha
> > layer without shape (I've added a test for this case - I'll be uploading the
> > patch soon).
> > 
> > I've opened https://code.google.com/p/chromium/issues/detail?id=409150 to
> track
> > the issue.
> 
> Could it be this at play? If so, could you explain how?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...

See below; the issue appears to be one of endianness (and see Gary's explanation
in the bug).

> Or can you explain why this test works? What's the difference?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...

As Gary pointed out, that test just tests whole-surface opacity, so it's
explicitly rendering a (solid) surface with translucency applied. The tests that
Gary is adding are instead rendering layers with translucency set via the
pixels' alpha channels, and it appears that there is some endianness issue on
the swarming bots, getting in the way.

[Wez, 2014-08-29 21:42:11]

Gary, can we restructure the shape test to use surface translucency rather than
per-pixel?

If so then let's do that, so that this patch & test can be landed, and tests for
per-pixel translucency with and without shape can be added in a separate CL.

[danakj, 2014-08-29 21:56:48]

On Fri, Aug 29, 2014 at 5:42 PM, <wez@chromium.org> wrote:

> Gary, can we restructure the shape test to use surface translucency rather
> than
> per-pixel?
>
> If so then let's do that, so that this patch & test can be landed, and
> tests for
> per-pixel translucency with and without shape can be added in a separate
> CL.
>

FWIW The Shape code puts a Filter on the cc::Layer which causes it to have
a render surface. The opacity bounds are applied at the time the surface of
the layer is drawn, which is the same time that the layer's opacity is
applied.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2014-08-29 22:04:11]

IIUC you're saying that the shape filter will be applied *before*
surface-wide opacity takes effect?


On 29 August 2014 14:56, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Aug 29, 2014 at 5:42 PM, <wez@chromium.org> wrote:
>
>> Gary, can we restructure the shape test to use surface translucency
>> rather than
>> per-pixel?
>>
>> If so then let's do that, so that this patch & test can be landed, and
>> tests for
>> per-pixel translucency with and without shape can be added in a separate
>> CL.
>>
>
> FWIW The Shape code puts a Filter on the cc::Layer which causes it to have
> a render surface. The opacity bounds are applied at the time the surface of
> the layer is drawn, which is the same time that the layer's opacity is
> applied.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-08-29 22:10:29]

On Fri, Aug 29, 2014 at 6:04 PM, Wez <wez@chromium.org> wrote:

> IIUC you're saying that the shape filter will be applied *before*
> surface-wide opacity takes effect?
>

Yes. They both make a render surface, but the difference is: Shape goes
through the SkImageFilter that was written for this feature, which limits
the alpha in the resulting texture. Then the result is drawn into the
target directly. Layer opacity is done with a shader in cc when the render
surface texture is drawn into the target.


>
>
> On 29 August 2014 14:56, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Fri, Aug 29, 2014 at 5:42 PM, <wez@chromium.org> wrote:
>>
>>> Gary, can we restructure the shape test to use surface translucency
>>> rather than
>>> per-pixel?
>>>
>>> If so then let's do that, so that this patch & test can be landed, and
>>> tests for
>>> per-pixel translucency with and without shape can be added in a separate
>>> CL.
>>>
>>
>> FWIW The Shape code puts a Filter on the cc::Layer which causes it to
>> have a render surface. The opacity bounds are applied at the time the
>> surface of the layer is drawn, which is the same time that the layer's
>> opacity is applied.
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[garykac, 2014-08-29 22:34:17]

On 2014/08/29 22:10:29, danakj wrote:
> On Fri, Aug 29, 2014 at 6:04 PM, Wez <mailto:wez@chromium.org> wrote:
> 
> > IIUC you're saying that the shape filter will be applied *before*
> > surface-wide opacity takes effect?
> >
> 
> Yes. They both make a render surface, but the difference is: Shape goes
> through the SkImageFilter that was written for this feature, which limits
> the alpha in the resulting texture. Then the result is drawn into the
> target directly. Layer opacity is done with a shader in cc when the render
> surface texture is drawn into the target.

I have a local version of this test that is based on Opacity instead of setting
the alpha channel on individual pixels.

If you prefer, I can switch to using that test for this cl and move the current
(#ifdef-laden) tests into a separate cl so that the unrelated transparency bug
can be tracked.

[danakj, 2014-08-29 22:37:52]

On Fri, Aug 29, 2014 at 6:34 PM, <garykac@chromium.org> wrote:

> On 2014/08/29 22:10:29, danakj wrote:
>
>  On Fri, Aug 29, 2014 at 6:04 PM, Wez <mailto:wez@chromium.org> wrote:
>>
>
>  > IIUC you're saying that the shape filter will be applied *before*
>> > surface-wide opacity takes effect?
>> >
>>
>
>  Yes. They both make a render surface, but the difference is: Shape goes
>> through the SkImageFilter that was written for this feature, which limits
>> the alpha in the resulting texture. Then the result is drawn into the
>> target directly. Layer opacity is done with a shader in cc when the render
>> surface texture is drawn into the target.
>>
>
> I have a local version of this test that is based on Opacity instead of
> setting
> the alpha channel on individual pixels.
>
> If you prefer, I can switch to using that test for this cl and move the
> current
> (#ifdef-laden) tests into a separate cl so that the unrelated transparency
> bug
> can be tracked.
>

How does it differ from
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...
?


>
> https://codereview.chromium.org/499503002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[garykac, 2014-08-29 22:53:37]

On 2014/08/29 22:37:52, danakj wrote:
> On Fri, Aug 29, 2014 at 6:34 PM, <mailto:garykac@chromium.org> wrote:
> 
> > On 2014/08/29 22:10:29, danakj wrote:
> >
> >  On Fri, Aug 29, 2014 at 6:04 PM, Wez <mailto:wez@chromium.org> wrote:
> >>
> >
> >  > IIUC you're saying that the shape filter will be applied *before*
> >> > surface-wide opacity takes effect?
> >> >
> >>
> >
> >  Yes. They both make a render surface, but the difference is: Shape goes
> >> through the SkImageFilter that was written for this feature, which limits
> >> the alpha in the resulting texture. Then the result is drawn into the
> >> target directly. Layer opacity is done with a shader in cc when the render
> >> surface texture is drawn into the target.
> >>
> >
> > I have a local version of this test that is based on Opacity instead of
> > setting
> > the alpha channel on individual pixels.
> >
> > If you prefer, I can switch to using that test for this cl and move the
> > current
> > (#ifdef-laden) tests into a separate cl so that the unrelated transparency
> > bug
> > can be tracked.
> >
> 
> How does it differ from
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...
> ?

It calls SetAlphaShape on l11 and then checks the pixels.

[danakj, 2014-08-29 22:57:28]

On Fri, Aug 29, 2014 at 6:53 PM, <garykac@chromium.org> wrote:

> On 2014/08/29 22:37:52, danakj wrote:
>
>  On Fri, Aug 29, 2014 at 6:34 PM, <mailto:garykac@chromium.org> wrote:
>>
>
>  > On 2014/08/29 22:10:29, danakj wrote:
>> >
>> >  On Fri, Aug 29, 2014 at 6:04 PM, Wez <mailto:wez@chromium.org> wrote:
>> >>
>> >
>> >  > IIUC you're saying that the shape filter will be applied *before*
>> >> > surface-wide opacity takes effect?
>> >> >
>> >>
>> >
>> >  Yes. They both make a render surface, but the difference is: Shape goes
>> >> through the SkImageFilter that was written for this feature, which
>> limits
>> >> the alpha in the resulting texture. Then the result is drawn into the
>> >> target directly. Layer opacity is done with a shader in cc when the
>> render
>> >> surface texture is drawn into the target.
>> >>
>> >
>> > I have a local version of this test that is based on Opacity instead of
>> > setting
>> > the alpha channel on individual pixels.
>> >
>> > If you prefer, I can switch to using that test for this cl and move the
>> > current
>> > (#ifdef-laden) tests into a separate cl so that the unrelated
>> transparency
>> > bug
>> > can be tracked.
>> >
>>
>
>  How does it differ from
>>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/ui/compositor/layer_unittest.cc&l=1072
>
>> ?
>>
>
> It calls SetAlphaShape on l11 and then checks the pixels.
>

Ah, but then that isn't testing this CL I guess, because that would work
before this change too? ie that only tests the clamping to 0 outside, it
doesn't test allowing < 255 inside.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[garykac, 2014-08-29 23:05:11]

ah... good point.  Do you have any other suggestions?  I can't think of
anything better than the current tests (with the #ifdefs) and following up
with the bot-transparency bug separately.


On Fri, Aug 29, 2014 at 3:57 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Aug 29, 2014 at 6:53 PM, <garykac@chromium.org> wrote:
>
>> On 2014/08/29 22:37:52, danakj wrote:
>>
>>  On Fri, Aug 29, 2014 at 6:34 PM, <mailto:garykac@chromium.org> wrote:
>>>
>>
>>  > On 2014/08/29 22:10:29, danakj wrote:
>>> >
>>> >  On Fri, Aug 29, 2014 at 6:04 PM, Wez <mailto:wez@chromium.org> wrote:
>>> >>
>>> >
>>> >  > IIUC you're saying that the shape filter will be applied *before*
>>> >> > surface-wide opacity takes effect?
>>> >> >
>>> >>
>>> >
>>> >  Yes. They both make a render surface, but the difference is: Shape
>>> goes
>>> >> through the SkImageFilter that was written for this feature, which
>>> limits
>>> >> the alpha in the resulting texture. Then the result is drawn into the
>>> >> target directly. Layer opacity is done with a shader in cc when the
>>> render
>>> >> surface texture is drawn into the target.
>>> >>
>>> >
>>> > I have a local version of this test that is based on Opacity instead of
>>> > setting
>>> > the alpha channel on individual pixels.
>>> >
>>> > If you prefer, I can switch to using that test for this cl and move the
>>> > current
>>> > (#ifdef-laden) tests into a separate cl so that the unrelated
>>> transparency
>>> > bug
>>> > can be tracked.
>>> >
>>>
>>
>>  How does it differ from
>>>
>>
>> https://code.google.com/p/chromium/codesearch#chromium/
>> src/ui/compositor/layer_unittest.cc&l=1072
>>
>>> ?
>>>
>>
>> It calls SetAlphaShape on l11 and then checks the pixels.
>>
>
> Ah, but then that isn't testing this CL I guess, because that would work
> before this change too? ie that only tests the clamping to 0 outside, it
> doesn't test allowing < 255 inside.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-08-29 23:08:54]

On Fri, Aug 29, 2014 at 7:05 PM, Gary Kacmarcik (Кошмарчик) <
garykac@chromium.org> wrote:

> ah... good point.  Do you have any other suggestions?  I can't think of
> anything better than the current tests (with the #ifdefs) and following up
> with the bot-transparency bug separately.
>

Well, there's no endian difference on the linux bots, so I don't know why
that could be it. I'm highly suspicious that this filter just does not work
correctly with non-opaque interiors. I think we should figure out why it is
broken before we change the behaviour. I don't want to have to maintain it
in that current form because I certainly can not explain what is going on.


>
>
> On Fri, Aug 29, 2014 at 3:57 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Fri, Aug 29, 2014 at 6:53 PM, <garykac@chromium.org> wrote:
>>
>>> On 2014/08/29 22:37:52, danakj wrote:
>>>
>>>  On Fri, Aug 29, 2014 at 6:34 PM, <mailto:garykac@chromium.org> wrote:
>>>>
>>>
>>>  > On 2014/08/29 22:10:29, danakj wrote:
>>>> >
>>>> >  On Fri, Aug 29, 2014 at 6:04 PM, Wez <mailto:wez@chromium.org>
>>>> wrote:
>>>> >>
>>>> >
>>>> >  > IIUC you're saying that the shape filter will be applied *before*
>>>> >> > surface-wide opacity takes effect?
>>>> >> >
>>>> >>
>>>> >
>>>> >  Yes. They both make a render surface, but the difference is: Shape
>>>> goes
>>>> >> through the SkImageFilter that was written for this feature, which
>>>> limits
>>>> >> the alpha in the resulting texture. Then the result is drawn into the
>>>> >> target directly. Layer opacity is done with a shader in cc when the
>>>> render
>>>> >> surface texture is drawn into the target.
>>>> >>
>>>> >
>>>> > I have a local version of this test that is based on Opacity instead
>>>> of
>>>> > setting
>>>> > the alpha channel on individual pixels.
>>>> >
>>>> > If you prefer, I can switch to using that test for this cl and move
>>>> the
>>>> > current
>>>> > (#ifdef-laden) tests into a separate cl so that the unrelated
>>>> transparency
>>>> > bug
>>>> > can be tracked.
>>>> >
>>>>
>>>
>>>  How does it differ from
>>>>
>>>
>>> https://code.google.com/p/chromium/codesearch#chromium/
>>> src/ui/compositor/layer_unittest.cc&l=1072
>>>
>>>> ?
>>>>
>>>
>>> It calls SetAlphaShape on l11 and then checks the pixels.
>>>
>>
>> Ah, but then that isn't testing this CL I guess, because that would work
>> before this change too? ie that only tests the clamping to 0 outside, it
>> doesn't test allowing < 255 inside.
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2014-08-29 23:13:27]

I thought we'd confirmed that the endianness issue affects pixel-level
translucency regardless of whether this filter is in-place?


On 29 August 2014 16:08, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Aug 29, 2014 at 7:05 PM, Gary Kacmarcik (Кошмарчик) <
> garykac@chromium.org> wrote:
>
>> ah... good point.  Do you have any other suggestions?  I can't think of
>> anything better than the current tests (with the #ifdefs) and following up
>> with the bot-transparency bug separately.
>>
>
> Well, there's no endian difference on the linux bots, so I don't know why
> that could be it. I'm highly suspicious that this filter just does not work
> correctly with non-opaque interiors. I think we should figure out why it is
> broken before we change the behaviour. I don't want to have to maintain it
> in that current form because I certainly can not explain what is going on.
>
>
>>
>>
>> On Fri, Aug 29, 2014 at 3:57 PM, Dana Jansens <danakj@chromium.org>
>> wrote:
>>
>>> On Fri, Aug 29, 2014 at 6:53 PM, <garykac@chromium.org> wrote:
>>>
>>>> On 2014/08/29 22:37:52, danakj wrote:
>>>>
>>>>  On Fri, Aug 29, 2014 at 6:34 PM, <mailto:garykac@chromium.org> wrote:
>>>>>
>>>>
>>>>  > On 2014/08/29 22:10:29, danakj wrote:
>>>>> >
>>>>> >  On Fri, Aug 29, 2014 at 6:04 PM, Wez <mailto:wez@chromium.org>
>>>>> wrote:
>>>>> >>
>>>>> >
>>>>> >  > IIUC you're saying that the shape filter will be applied *before*
>>>>> >> > surface-wide opacity takes effect?
>>>>> >> >
>>>>> >>
>>>>> >
>>>>> >  Yes. They both make a render surface, but the difference is: Shape
>>>>> goes
>>>>> >> through the SkImageFilter that was written for this feature, which
>>>>> limits
>>>>> >> the alpha in the resulting texture. Then the result is drawn into
>>>>> the
>>>>> >> target directly. Layer opacity is done with a shader in cc when the
>>>>> render
>>>>> >> surface texture is drawn into the target.
>>>>> >>
>>>>> >
>>>>> > I have a local version of this test that is based on Opacity instead
>>>>> of
>>>>> > setting
>>>>> > the alpha channel on individual pixels.
>>>>> >
>>>>> > If you prefer, I can switch to using that test for this cl and move
>>>>> the
>>>>> > current
>>>>> > (#ifdef-laden) tests into a separate cl so that the unrelated
>>>>> transparency
>>>>> > bug
>>>>> > can be tracked.
>>>>> >
>>>>>
>>>>
>>>>  How does it differ from
>>>>>
>>>>
>>>> https://code.google.com/p/chromium/codesearch#chromium/
>>>> src/ui/compositor/layer_unittest.cc&l=1072
>>>>
>>>>> ?
>>>>>
>>>>
>>>> It calls SetAlphaShape on l11 and then checks the pixels.
>>>>
>>>
>>> Ah, but then that isn't testing this CL I guess, because that would work
>>> before this change too? ie that only tests the clamping to 0 outside, it
>>> doesn't test allowing < 255 inside.
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[garykac, 2014-09-02 19:27:07]

There are 2 tests: one that simply draws alpha pixels, and another that draws
them and applies an AlphaShape filter.

FillsBoundsOpaquely was the magic flag to make this work correctly -- without
it, the tests work fine locally but fail in different ways on the bots.

I'd like to add a DCHECK to catch when we try to draw alpha pixels without this
flag set on the layer, but I didn't see an obvious place to do that.  Any
suggestions for where to put such a check would be appreciated (if you agree
that it's a good idea).  Thanks!

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:905: bool alpha_blend = false;
On 2014/08/26 01:53:39, Wez wrote:
> expect_alpha_blend?

Done.

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:924: scoped_ptr<Layer> layer(
On 2014/08/26 01:53:39, Wez wrote:
> layer1, or background_layer

Done.

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:926: scoped_ptr<Layer> layer2(
On 2014/08/26 01:53:38, Wez wrote:
> or blue_layer or foreground_layer

Done.

https://codereview.chromium.org/499503002/diff/140001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:942: // Ignore the edge pixels since the
blending can vary on some platforms.
On 2014/08/26 01:53:39, Wez wrote:
> This only seems to ignore the top-most and left-most, not the right-most nor
> bottom-most edges?

The test size is much smaller than the window, so the right/bottom edges are
already being ignored.

[danakj, 2014-09-03 19:39:05]

LGTM

https://codereview.chromium.org/499503002/diff/280001/ui/compositor/layer_uni...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/499503002/diff/280001/ui/compositor/layer_uni...
ui/compositor/layer_unittest.cc:850: void ExpectRgba(int x, int y, SkColor
actual_color, SkColor expected_color) {
please pass expected first, to match EXPECT_EQ ordering

[garykac, 2014-09-04 23:07:57]

The CQ bit was checked by garykac@chromium.org

[commit-bot: I haz the power, 2014-09-04 23:09:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/garykac@chromium.org/499503002/300001

[commit-bot: I haz the power, 2014-09-05 04:27:41]

Committed patchset #16 (id:300001) as 14b6edac18f89538d7a4675968e08517d1f924b6

[commit-bot: I haz the power, 2014-09-10 03:36:38]

Patchset 16 (id:??) landed as
https://crrev.com/eba92949d768e9905fd45e5b10bef8f9f1d21d08
Cr-Commit-Position: refs/heads/master@{#293433}