compositor: Fix texture flipping for SW mirroring with surfaceless

compositor: Fix texture flipping for SW mirroring with surfaceless

Ozone-surfaceless renders the scene flipped, so the texture contents are
inverted from what they should be. This fixes the mirroring logic to
un-flip the texture when needed, and also to send the right sub-buffer
update rect.

BUG=434115
Committed: https://crrev.com/c6a481fe79e0c4851a576495105edf2600bfe5be
Cr-Commit-Position: refs/heads/master@{#312979}

[achaulk, 2015-01-12 18:16:58]

achaulk@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-01-12 18:27:30]

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
File content/browser/compositor/buffer_queue.cc (right):

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:78: // CopySubBufferDamage binds its
own framebuffer, restore ours.
Should this be a separate CL?

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:187: flip_texture_ =
!output_surface->capabilities().flipped_output_surface;
This is getting the flipped-ness of the current output surface, but setting
flipped for the texture mailbox, which came from a different output surface,
right? How can you guarantee that these match?

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:231: void
ReflectorImpl::UpdateSubBufferOnMainThread(gfx::Size size,
while you're here, these should be const& arguments

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:243:
main.mirroring_layer->SchedulePaint(rect);
How about just doing rect math inside an if/else and keep the function call
outside, so there's only one?

[achaulk, 2015-01-12 18:47:35]

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
File content/browser/compositor/buffer_queue.cc (right):

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:78: // CopySubBufferDamage binds its
own framebuffer, restore ours.
On 2015/01/12 18:27:30, danakj wrote:
> Should this be a separate CL?

It could be, but SW mirroring with partial swap is broken without it since the
mirroring process will attempt to copy out of the temporary framebuffer

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:187: flip_texture_ =
!output_surface->capabilities().flipped_output_surface;
On 2015/01/12 18:27:30, danakj wrote:
> This is getting the flipped-ness of the current output surface, but setting
> flipped for the texture mailbox, which came from a different output surface,
> right? How can you guarantee that these match?

All that matters is the flipped-ness of the source surface, since that's where
the texture contents are coming from. This ensures that the layer contains the
same data either way (normal painting and flipped=true or inverted painting and
flipped=false). The properties of the output surface for the mirroring layer
shouldn't matter

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:231: void
ReflectorImpl::UpdateSubBufferOnMainThread(gfx::Size size,
On 2015/01/12 18:27:30, danakj wrote:
> while you're here, these should be const& arguments

Done.

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:243:
main.mirroring_layer->SchedulePaint(rect);
On 2015/01/12 18:27:30, danakj wrote:
> How about just doing rect math inside an if/else and keep the function call
> outside, so there's only one?

Sure

[danakj, 2015-01-12 19:00:07]

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
File content/browser/compositor/buffer_queue.cc (right):

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:78: // CopySubBufferDamage binds its
own framebuffer, restore ours.
On 2015/01/12 18:47:35, achaulk wrote:
> On 2015/01/12 18:27:30, danakj wrote:
> > Should this be a separate CL?
> 
> It could be, but SW mirroring with partial swap is broken without it since the
> mirroring process will attempt to copy out of the temporary framebuffer

It doesn't seem related to flipping tho? So It would be nice to do this in
another CL with tests for it.

[danakj, 2015-01-12 19:02:42]

Can you add a test to ui/compositor unittests for the new method to ensure it
does something?

Are there some tests for reflector you can add to verify this change and guard
it against regressions?

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:187: flip_texture_ =
!output_surface->capabilities().flipped_output_surface;
On 2015/01/12 18:47:35, achaulk wrote:
> On 2015/01/12 18:27:30, danakj wrote:
> > This is getting the flipped-ness of the current output surface, but setting
> > flipped for the texture mailbox, which came from a different output surface,
> > right? How can you guarantee that these match?
> 
> All that matters is the flipped-ness of the source surface, since that's where
> the texture contents are coming from. This ensures that the layer contains the
> same data either way (normal painting and flipped=true or inverted painting
and
> flipped=false). The properties of the output surface for the mirroring layer
> shouldn't matter

Oh this is the source surface? OK great :) I read it as the target surface at
first!

[achaulk, 2015-01-12 19:56:52]

Removed buffer_queue change

On 2015/01/12 19:02:42, danakj wrote:
> Can you add a test to ui/compositor unittests for the new method to ensure it
> does something?

For the setter?

> 
> Are there some tests for reflector you can add to verify this change and guard
> it against regressions?

Not easily. There are no existing tests and the code doesn't seem to be very
testable, requiring at least a pair of message loops, a compositor, probably a
full gl context and a BrowserCompositorOutputSurface to init it

> 
>
https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
> File content/browser/compositor/reflector_impl.cc (right):
> 
>
https://codereview.chromium.org/846063002/diff/20001/content/browser/composit...
> content/browser/compositor/reflector_impl.cc:187: flip_texture_ =
> !output_surface->capabilities().flipped_output_surface;
> On 2015/01/12 18:47:35, achaulk wrote:
> > On 2015/01/12 18:27:30, danakj wrote:
> > > This is getting the flipped-ness of the current output surface, but
setting
> > > flipped for the texture mailbox, which came from a different output
surface,
> > > right? How can you guarantee that these match?
> > 
> > All that matters is the flipped-ness of the source surface, since that's
where
> > the texture contents are coming from. This ensures that the layer contains
the
> > same data either way (normal painting and flipped=true or inverted painting
> and
> > flipped=false). The properties of the output surface for the mirroring layer
> > shouldn't matter
> 
> Oh this is the source surface? OK great :) I read it as the target surface at
> first!

[danakj, 2015-01-12 20:04:00]

On Mon, Jan 12, 2015 at 11:56 AM, <achaulk@chromium.org> wrote:

> Removed buffer_queue change
>
> On 2015/01/12 19:02:42, danakj wrote:
>
>> Can you add a test to ui/compositor unittests for the new method to
>> ensure it
>> does something?
>>
>
> For the setter?


Mmh, just don't like adding functions without a test calling them. I guess
it's just a pass though, and hopefully no one deletes it. :)


>
>
>  Are there some tests for reflector you can add to verify this change and
>> guard
>> it against regressions?
>>
>
> Not easily. There are no existing tests and the code doesn't seem to be
> very
> testable, requiring at least a pair of message loops, a compositor,
> probably a
> full gl context and a BrowserCompositorOutputSurface to init it


Those all seem fine to have inside a unit test. I'm not sure what's wrong
with that?

Alternatively (and slightly less happily) how about a browser test? That
has all of those things.

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

[achaulk, 2015-01-12 20:31:48]

On 2015/01/12 20:04:00, danakj wrote:
> On Mon, Jan 12, 2015 at 11:56 AM, <mailto:achaulk@chromium.org> wrote:
> 
> > Removed buffer_queue change
> >
> > On 2015/01/12 19:02:42, danakj wrote:
> >
> >> Can you add a test to ui/compositor unittests for the new method to
> >> ensure it
> >> does something?
> >>
> >
> > For the setter?
> 
> 
> Mmh, just don't like adding functions without a test calling them. I guess
> it's just a pass though, and hopefully no one deletes it. :)

Well, that would cause build errors at least.

> 
> 
> >
> >
> >  Are there some tests for reflector you can add to verify this change and
> >> guard
> >> it against regressions?
> >>
> >
> > Not easily. There are no existing tests and the code doesn't seem to be
> > very
> > testable, requiring at least a pair of message loops, a compositor,
> > probably a
> > full gl context and a BrowserCompositorOutputSurface to init it
> 
> 
> Those all seem fine to have inside a unit test. I'm not sure what's wrong
> with that?
> 

Nothing, it's just an awful lot of setup for a pair of
TEST(...) {
UpdateTextureSizeOnMainThread(gfx::Size())
EXPECT_TRUE/FALSE(main.mirroring_layer->TextureFlipped())
}

> Alternatively (and slightly less happily) how about a browser test? That
> has all of those things.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-01-12 20:33:45]

On Mon, Jan 12, 2015 at 12:31 PM, <achaulk@chromium.org> wrote:

> On 2015/01/12 20:04:00, danakj wrote:
>
>> On Mon, Jan 12, 2015 at 11:56 AM, <mailto:achaulk@chromium.org> wrote:
>>
>
>  > Removed buffer_queue change
>> >
>> > On 2015/01/12 19:02:42, danakj wrote:
>> >
>> >> Can you add a test to ui/compositor unittests for the new method to
>> >> ensure it
>> >> does something?
>> >>
>> >
>> > For the setter?
>>
>
>
>  Mmh, just don't like adding functions without a test calling them. I guess
>> it's just a pass though, and hopefully no one deletes it. :)
>>
>
> Well, that would cause build errors at least.
>
>
>
>  >
>> >
>> >  Are there some tests for reflector you can add to verify this change
>> and
>> >> guard
>> >> it against regressions?
>> >>
>> >
>> > Not easily. There are no existing tests and the code doesn't seem to be
>> > very
>> > testable, requiring at least a pair of message loops, a compositor,
>> > probably a
>> > full gl context and a BrowserCompositorOutputSurface to init it
>>
>
>
>  Those all seem fine to have inside a unit test. I'm not sure what's wrong
>> with that?
>>
>
>
> Nothing, it's just an awful lot of setup for a pair of
> TEST(...) {
> UpdateTextureSizeOnMainThread(gfx::Size())
> EXPECT_TRUE/FALSE(main.mirroring_layer->TextureFlipped())
> }
>

I'm sorry if you're the first person to write a test here and have to do
that setup code, but that doesn't seem like a good reason to avoid writing
a regression test that demonstrates the bug this CL is fixing..

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

[achaulk, 2015-01-14 20:32:38]

Tests added

[achaulk, 2015-01-15 22:14:12]

ping, we would like to get this in the 41 branch

[achaulk, 2015-01-20 16:12:15]

ping

[danakj, 2015-01-22 01:04:21]

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:187: flip_texture_ =
!output_surface->capabilities().flipped_output_surface;
capabilities are not initialized until the context is bound (the line below
this)

https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:200:
base::Bind(&ReflectorImpl::FullRedrawContentOnMainThread,
Maybe pass the flipped bool along here, so you can store it in the
MainThreadData?

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
File content/browser/compositor/reflector_impl.h (right):

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl.h:139: bool flip_texture_;
Thanks for the test this looks nice except I think this is a data race. This
variable should be a member of ImplThreadData or MainThreadData and used on the
correct thread or posted to the other.

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
File content/browser/compositor/reflector_impl_unittest.cc (right):

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl_unittest.cc:31: void SetFlip(bool
flip) { capabilities_.flipped_output_surface = flip; }
can you override BindToCurrentThread() and set this capability in there instead?
(probably need to store something on this class until Bind happens)

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl_unittest.cc:36: const gfx::Size size =
gfx::Size(256, 256);
kSomeDescriptiveNameSize?

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl_unittest.cc:37: }  // namespace
normally i think we just wrap the whole file in a nested anonymous namespace.
any reason why you end it here and start it again later?

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl_unittest.cc:68: ~ReflectorImplTest()
override {
I think you want these in Shutdown() rather than the destructor?

[achaulk, 2015-01-23 16:45:53]

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl.cc:187: flip_texture_ =
!output_surface->capabilities().flipped_output_surface;
On 2015/01/22 01:04:20, danakj wrote:
> capabilities are not initialized until the context is bound (the line below
> this)
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

Done.

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
File content/browser/compositor/reflector_impl.h (right):

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl.h:139: bool flip_texture_;
On 2015/01/22 01:04:20, danakj wrote:
> Thanks for the test this looks nice except I think this is a data race. This
> variable should be a member of ImplThreadData or MainThreadData and used on
the
> correct thread or posted to the other.

Done.

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
File content/browser/compositor/reflector_impl_unittest.cc (right):

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl_unittest.cc:31: void SetFlip(bool
flip) { capabilities_.flipped_output_surface = flip; }
On 2015/01/22 01:04:20, danakj wrote:
> can you override BindToCurrentThread() and set this capability in there
instead?
> (probably need to store something on this class until Bind happens)

BindToCurrentThread is in the context provider though, not the output surface

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl_unittest.cc:36: const gfx::Size size =
gfx::Size(256, 256);
On 2015/01/22 01:04:20, danakj wrote:
> kSomeDescriptiveNameSize?

Done.

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl_unittest.cc:37: }  // namespace
On 2015/01/22 01:04:21, danakj wrote:
> normally i think we just wrap the whole file in a nested anonymous namespace.
> any reason why you end it here and start it again later?

ReflectorImplTest is a friend of ReflectorImpl

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl_unittest.cc:68: ~ReflectorImplTest()
override {
On 2015/01/22 01:04:20, danakj wrote:
> I think you want these in Shutdown() rather than the destructor?

Done.

[danakj, 2015-01-23 17:48:29]

LGTM

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
File content/browser/compositor/reflector_impl_unittest.cc (right):

https://codereview.chromium.org/846063002/diff/80001/content/browser/composit...
content/browser/compositor/reflector_impl_unittest.cc:31: void SetFlip(bool
flip) { capabilities_.flipped_output_surface = flip; }
On 2015/01/23 16:45:52, achaulk wrote:
> On 2015/01/22 01:04:20, danakj wrote:
> > can you override BindToCurrentThread() and set this capability in there
> instead?
> > (probably need to store something on this class until Bind happens)
> 
> BindToCurrentThread is in the context provider though, not the output surface

Argh right sorry. Fuzzy head.

[achaulk, 2015-01-23 17:49:04]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-01-23 17:49:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/846063002/100001

[commit-bot: I haz the power, 2015-01-23 18:30:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 18:30:16]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achaulk, 2015-01-23 18:36:56]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-01-23 18:38:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/846063002/120001

[commit-bot: I haz the power, 2015-01-23 19:19:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 19:19:21]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[achaulk, 2015-01-23 19:41:26]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-01-23 19:42:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/846063002/140001

[achaulk, 2015-01-23 20:14:51]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-01-23 20:16:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/846063002/160001

[commit-bot: I haz the power, 2015-01-23 21:16:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:16:43]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[achaulk, 2015-01-23 21:36:55]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:37:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/846063002/180001

[commit-bot: I haz the power, 2015-01-23 21:44:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:44:12]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achaulk, 2015-01-23 21:45:58]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:46:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/846063002/200001

[commit-bot: I haz the power, 2015-01-23 22:00:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 22:00:25]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achaulk, 2015-01-23 22:32:15]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-01-23 22:34:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/846063002/220001

[commit-bot: I haz the power, 2015-01-23 23:10:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 23:10:37]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achaulk, 2015-01-23 23:21:04]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-01-23 23:21:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/846063002/240001

[commit-bot: I haz the power, 2015-01-24 00:29:36]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-01-24 00:32:01]

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

[Daniel Kurtz, 2015-01-28 06:16:04]

djkurtz@chromium.org changed reviewers:
+ djkurtz@chromium.org

[Daniel Kurtz, 2015-01-28 06:16:06]

-1 Verified:  This patch causes crbug.com/451804 .

[Daniel Kurtz, 2015-01-28 18:06:53]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/867483003/ by djkurtz@chromium.org.

The reason for reverting is: The original patch breaks software mirroring mode
on at least veyron_jerry,
and quite possibly all non-freon systems.

BUG=451804
.