exo: Recreate Surface resources on context lost.

exo: Recreate Surface resources on context lost.

When the GPU context is lost the GpuMemoryBuffers may remain valid, but
the textures pointed to be the cc::Surface's frame are invalid. The
Surface should create a new TransferableResource from the Buffer and
submit it in a new frame to the cc::Surface.

Also add a ui::ContextFactoryObserver so the Surface can be notified
when the SharedMainThreadContextProvider it relies on is lost. Other
users of ImageTransportFactoryObserver are converted to the new
interface.

BUG=b/29239573

Committed: https://crrev.com/45c06866093fa86f96da6e8da0dee04740b7aee1
Cr-Commit-Position: refs/heads/master@{#401684}

[jbauman, 2016-06-20 23:41:16]

Description was changed from

==========
exo: Recreate Surface resources on context lost.

When the GPU context is lost the GpuMemoryBuffers may remain valid, but
the textures pointed to be the cc::Surface's frame are invalid. The
Surface should create a new TransferableResource from the Buffer and
submit it in a new frame to the cc::Surface.

Also add a ui::ContextFactoryObserver so the Surface can be notified
when the SharedMainThreadContextProvider it relies on is lost. Other
users of ImageTransportFactoryObserver are converted to the new
interface.

BUG=
==========

to

==========
exo: Recreate Surface resources on context lost.

When the GPU context is lost the GpuMemoryBuffers may remain valid, but
the textures pointed to be the cc::Surface's frame are invalid. The
Surface should create a new TransferableResource from the Buffer and
submit it in a new frame to the cc::Surface.

Also add a ui::ContextFactoryObserver so the Surface can be notified
when the SharedMainThreadContextProvider it relies on is lost. Other
users of ImageTransportFactoryObserver are converted to the new
interface.

BUG=b/29239573
==========

[jbauman, 2016-06-21 00:46:35]

jbauman@chromium.org changed reviewers:
+ piman@chromium.org, reveman@chromium.org

[piman, 2016-06-21 01:14:14]

https://codereview.chromium.org/2083853002/diff/20001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2083853002/diff/20001/ui/compositor/composito...
ui/compositor/compositor.h:152: base::ObserverList<ContextFactoryObserver>
observer_list_;
Can we keep this class pure virtual? I realize you're trying to save code, but
because we need multiple inheritance, we should stick to pure interfaces.

[reveman, 2016-06-21 16:25:47]

https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
components/exo/surface.cc:669: // This can fail even if there's a buffer if
for_commit is false, zero-copy
Can we fix this by not releasing buffers that are currently attached to a
surface? One-copy will still provide the benefit that the buffers are released
immediately when a new buffer is attached. Arc++ uses shmem buffers in some
cases so I think this is needed.

https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
components/exo/surface.cc:728: gfx::Rect damage_rect = for_commit
Can damage_rect be an argument to this function instead of |for_commit|?

https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
components/exo/surface.cc:753: state_.blend_mode == SkXfermode::kSrc_Mode ||
The idea was always to handle this by turning off blending. The reason it was
used for opaque rect comes from a lack of "disable-blending" API for ui::Layers.
Now that we're not using that anymore, can we just disable blending more
explicitly? And maybe as a result of that we also don't need to add this
frame_is_opaque_ state..

[jbauman, 2016-06-21 23:31:03]

On 2016/06/21 16:25:47, reveman wrote:
>
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface.cc
> File components/exo/surface.cc (right):
> 
>
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
> components/exo/surface.cc:669: // This can fail even if there's a buffer if
> for_commit is false, zero-copy
> Can we fix this by not releasing buffers that are currently attached to a
> surface? One-copy will still provide the benefit that the buffers are released
> immediately when a new buffer is attached. Arc++ uses shmem buffers in some
> cases so I think this is needed.

Done.
> 
>
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
> components/exo/surface.cc:728: gfx::Rect damage_rect = for_commit
> Can damage_rect be an argument to this function instead of |for_commit|?

|contents_surface_size| isn't calculated until inside this method, so we
wouldn't know what to have as a fallback.
> 
>
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
> components/exo/surface.cc:753: state_.blend_mode == SkXfermode::kSrc_Mode ||
> The idea was always to handle this by turning off blending. The reason it was
> used for opaque rect comes from a lack of "disable-blending" API for
ui::Layers.
> Now that we're not using that anymore, can we just disable blending more
> explicitly? And maybe as a result of that we also don't need to add this
> frame_is_opaque_ state..

Making the layer opaque also allows the browser compositor to reduce overdraw by
avoiding drawing contents underneath the surface. So it'd be nice to keep that
in.

[jbauman, 2016-06-21 23:31:48]

On 2016/06/21 01:14:14, piman wrote:
>
https://codereview.chromium.org/2083853002/diff/20001/ui/compositor/compositor.h
> File ui/compositor/compositor.h (right):
> 
>
https://codereview.chromium.org/2083853002/diff/20001/ui/compositor/composito...
> ui/compositor/compositor.h:152: base::ObserverList<ContextFactoryObserver>
> observer_list_;
> Can we keep this class pure virtual? I realize you're trying to save code, but
> because we need multiple inheritance, we should stick to pure interfaces.

Done.

[piman, 2016-06-22 00:59:16]

lgtm

[reveman, 2016-06-22 17:15:26]

On 2016/06/21 at 23:31:03, jbauman wrote:
> On 2016/06/21 16:25:47, reveman wrote:
> >
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface.cc
> > File components/exo/surface.cc (right):
> > 
> >
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
> > components/exo/surface.cc:669: // This can fail even if there's a buffer if
> > for_commit is false, zero-copy
> > Can we fix this by not releasing buffers that are currently attached to a
> > surface? One-copy will still provide the benefit that the buffers are
released
> > immediately when a new buffer is attached. Arc++ uses shmem buffers in some
> > cases so I think this is needed.
> 
> Done.
> > 
> >
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
> > components/exo/surface.cc:728: gfx::Rect damage_rect = for_commit
> > Can damage_rect be an argument to this function instead of |for_commit|?
> 
> |contents_surface_size| isn't calculated until inside this method, so we
wouldn't know what to have as a fallback.

hm, but there's not really any damage to the buffer contents in this case, it's
the same as before, we just need to rebind it. shouldn't the damage rect be
empty in the lost context case?

> > 
> >
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
> > components/exo/surface.cc:753: state_.blend_mode == SkXfermode::kSrc_Mode ||
> > The idea was always to handle this by turning off blending. The reason it
was
> > used for opaque rect comes from a lack of "disable-blending" API for
ui::Layers.
> > Now that we're not using that anymore, can we just disable blending more
> > explicitly? And maybe as a result of that we also don't need to add this
> > frame_is_opaque_ state..
> 
> Making the layer opaque also allows the browser compositor to reduce overdraw
by avoiding drawing contents underneath the surface. So it'd be nice to keep
that in.

It's fine to rely on the client setting opaque_region_ properly for this. The
difference between opaque_region and blending is that opaque_region_ is just a
hint that we can ignore and will still result in correct output while blending
mode is a requirement and if we ignore it we'd be producing incorrect results as
alpha components can be less than 1.0.

https://codereview.chromium.org/2083853002/diff/40001/components/exo/buffer.h
File components/exo/buffer.h (right):

https://codereview.chromium.org/2083853002/diff/40001/components/exo/buffer.h...
components/exo/buffer.h:65: base::Closure* detach_callback);
hm, I see how this works but it took a minute for me to understand it.

Instead of returning a callback here. What do you think about just adding two
functions to the Buffer class, OnAttach+OnDetach and just call those directly
from the Surface class when appropriate? Potentially adding some private
BufferAttachment utility class to exo::Surface to handle the call to OnDetach
automatically. OnAttach/Detach can keep a attachment count and that we can also
improve debug output by logging something when a buffer is attached to two
different surfaces.

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface....
components/exo/surface.cc:659: void Surface::CreateResource(bool for_commit) {
nit: s/for_commit/client_usage/

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface....
components/exo/surface.cc:665: texture_mailbox_release_callback =
current_buffer_->ProduceTextureMailbox(
What would it look like if we moved this ProduceTextureMailbox logic to the
caller and just passed a std::unique_ptr<cc::SingleReleaseCallback> to this
function? Fine to leave this as is but I'm just curious as it seems like most of
the conditionals here are only needed from one of the call sites so maybe it
would make for somewhat easier to understand code with more possibilities for
DCHECKs.

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface....
components/exo/surface.h:278: void CreateResource(bool for_commit);
nit: UpdateResource?

[jbauman, 2016-06-22 23:45:48]

PTAL

On 2016/06/22 17:15:26, reveman wrote:
> On 2016/06/21 at 23:31:03, jbauman wrote:
> > On 2016/06/21 16:25:47, reveman wrote:
> > >
>
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface.cc
> > > File components/exo/surface.cc (right):
> > > 
> > >
> > > 
> > >
>
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
> > > components/exo/surface.cc:728: gfx::Rect damage_rect = for_commit
> > > Can damage_rect be an argument to this function instead of |for_commit|?
> > 
> > |contents_surface_size| isn't calculated until inside this method, so we
> wouldn't know what to have as a fallback.
> 
> hm, but there's not really any damage to the buffer contents in this case,
it's
> the same as before, we just need to rebind it. shouldn't the damage rect be
> empty in the lost context case?

It's possible the Display could redraw the Surface while it's contents are lost
(if the Display is recreated before the lost resources callback is run). In that
case we'd need to make sure the new correct contents are drawn to the screen.
> 
> > > 
> > >
>
https://codereview.chromium.org/2083853002/diff/20001/components/exo/surface....
> > > components/exo/surface.cc:753: state_.blend_mode == SkXfermode::kSrc_Mode
||
> > > The idea was always to handle this by turning off blending. The reason it
> was
> > > used for opaque rect comes from a lack of "disable-blending" API for
> ui::Layers.
> > > Now that we're not using that anymore, can we just disable blending more
> > > explicitly? And maybe as a result of that we also don't need to add this
> > > frame_is_opaque_ state..
> > 
> > Making the layer opaque also allows the browser compositor to reduce
overdraw
> by avoiding drawing contents underneath the surface. So it'd be nice to keep
> that in.
> 
> It's fine to rely on the client setting opaque_region_ properly for this. The
> difference between opaque_region and blending is that opaque_region_ is just a
> hint that we can ignore and will still result in correct output while blending
> mode is a requirement and if we ignore it we'd be producing incorrect results
as
> alpha components can be less than 1.0.
> 
Done.

https://codereview.chromium.org/2083853002/diff/40001/components/exo/buffer.h
File components/exo/buffer.h (right):

https://codereview.chromium.org/2083853002/diff/40001/components/exo/buffer.h...
components/exo/buffer.h:65: base::Closure* detach_callback);
On 2016/06/22 17:15:26, reveman wrote:
> hm, I see how this works but it took a minute for me to understand it.
> 
> Instead of returning a callback here. What do you think about just adding two
> functions to the Buffer class, OnAttach+OnDetach and just call those directly
> from the Surface class when appropriate? Potentially adding some private
> BufferAttachment utility class to exo::Surface to handle the call to OnDetach
> automatically. OnAttach/Detach can keep a attachment count and that we can
also
> improve debug output by logging something when a buffer is attached to two
> different surfaces.

Done.

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface....
components/exo/surface.cc:659: void Surface::CreateResource(bool for_commit) {
On 2016/06/22 17:15:26, reveman wrote:
> nit: s/for_commit/client_usage/

Done.

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface....
components/exo/surface.cc:665: texture_mailbox_release_callback =
current_buffer_->ProduceTextureMailbox(
On 2016/06/22 17:15:26, reveman wrote:
> What would it look like if we moved this ProduceTextureMailbox logic to the
> caller and just passed a std::unique_ptr<cc::SingleReleaseCallback> to this
> function? Fine to leave this as is but I'm just curious as it seems like most
of
> the conditionals here are only needed from one of the call sites so maybe it
> would make for somewhat easier to understand code with more possibilities for
> DCHECKs.

There's still some duplication of checking only_visible_on_secure_output and so
on, so I think it makes sense to keep it as is.

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/2083853002/diff/40001/components/exo/surface....
components/exo/surface.h:278: void CreateResource(bool for_commit);
On 2016/06/22 17:15:26, reveman wrote:
> nit: UpdateResource?

Done.

[reveman, 2016-06-23 00:44:39]

very nice. lgtm % a minor request and some nits

https://codereview.chromium.org/2083853002/diff/80001/components/exo/buffer.h
File components/exo/buffer.h (right):

https://codereview.chromium.org/2083853002/diff/80001/components/exo/buffer.h...
components/exo/buffer.h:66: void OnAttach() { ++attach_count_; }
Can you move this to .cc file and add a DLOG(WARNING) for when a buffer gets
attached to multiple surfaces as that's a result of bad behavior by the client?

https://codereview.chromium.org/2083853002/diff/80001/components/exo/surface.cc
File components/exo/surface.cc (right):

https://codereview.chromium.org/2083853002/diff/80001/components/exo/surface....
components/exo/surface.cc:632: Surface::BufferAttachment::~BufferAttachment() {
nit: blank line before this

https://codereview.chromium.org/2083853002/diff/80001/components/exo/surface....
components/exo/surface.cc:717: void Surface::UpdateSurface(bool for_commit) {
nit: s/for_commit/full_damage/

https://codereview.chromium.org/2083853002/diff/80001/components/exo/surface....
components/exo/surface.cc:753: gfx::Rect damage_rect = for_commit
nit: s/for_commit/!full_damage/

https://codereview.chromium.org/2083853002/diff/80001/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/2083853002/diff/80001/components/exo/surface....
components/exo/surface.h:268: DISALLOW_COPY_AND_ASSIGN(BufferAttachment);
nit: blank line before this

https://codereview.chromium.org/2083853002/diff/80001/components/exo/surface....
components/exo/surface.h:295: void UpdateSurface(bool for_commit);
nit: s/for_commit/full_damage/

https://codereview.chromium.org/2083853002/diff/80001/components/exo/surface_...
File components/exo/surface_unittest.cc (right):

https://codereview.chromium.org/2083853002/diff/80001/components/exo/surface_...
components/exo/surface_unittest.cc:216: ->ShouldDrawWithBlending());
Thanks for doing this

[jbauman, 2016-06-23 01:35:18]

jbauman@chromium.org changed reviewers:
+ sky@chromium.org

[jbauman, 2016-06-23 01:35:19]

sky: ash/sysui and ui/views/ OWNERS review.

[sky, 2016-06-23 15:29:06]

LGTM

[jbauman, 2016-06-23 19:29:12]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2016-06-23 19:29:12]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2083853002/#ps100001
(title: "fix nits")

[commit-bot: I haz the power, 2016-06-23 19:29:48]

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

[commit-bot: I haz the power, 2016-06-23 19:35:15]

Description was changed from

==========
exo: Recreate Surface resources on context lost.

When the GPU context is lost the GpuMemoryBuffers may remain valid, but
the textures pointed to be the cc::Surface's frame are invalid. The
Surface should create a new TransferableResource from the Buffer and
submit it in a new frame to the cc::Surface.

Also add a ui::ContextFactoryObserver so the Surface can be notified
when the SharedMainThreadContextProvider it relies on is lost. Other
users of ImageTransportFactoryObserver are converted to the new
interface.

BUG=b/29239573
==========

to

==========
exo: Recreate Surface resources on context lost.

When the GPU context is lost the GpuMemoryBuffers may remain valid, but
the textures pointed to be the cc::Surface's frame are invalid. The
Surface should create a new TransferableResource from the Buffer and
submit it in a new frame to the cc::Surface.

Also add a ui::ContextFactoryObserver so the Surface can be notified
when the SharedMainThreadContextProvider it relies on is lost. Other
users of ImageTransportFactoryObserver are converted to the new
interface.

BUG=b/29239573
==========

[commit-bot: I haz the power, 2016-06-23 19:35:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-23 19:37:00]

Description was changed from

==========
exo: Recreate Surface resources on context lost.

When the GPU context is lost the GpuMemoryBuffers may remain valid, but
the textures pointed to be the cc::Surface's frame are invalid. The
Surface should create a new TransferableResource from the Buffer and
submit it in a new frame to the cc::Surface.

Also add a ui::ContextFactoryObserver so the Surface can be notified
when the SharedMainThreadContextProvider it relies on is lost. Other
users of ImageTransportFactoryObserver are converted to the new
interface.

BUG=b/29239573
==========

to

==========
exo: Recreate Surface resources on context lost.

When the GPU context is lost the GpuMemoryBuffers may remain valid, but
the textures pointed to be the cc::Surface's frame are invalid. The
Surface should create a new TransferableResource from the Buffer and
submit it in a new frame to the cc::Surface.

Also add a ui::ContextFactoryObserver so the Surface can be notified
when the SharedMainThreadContextProvider it relies on is lost. Other
users of ImageTransportFactoryObserver are converted to the new
interface.

BUG=b/29239573

Committed: https://crrev.com/45c06866093fa86f96da6e8da0dee04740b7aee1
Cr-Commit-Position: refs/heads/master@{#401684}
==========

[commit-bot: I haz the power, 2016-06-23 19:37:01]

Patchset 6 (id:??) landed as
https://crrev.com/45c06866093fa86f96da6e8da0dee04740b7aee1
Cr-Commit-Position: refs/heads/master@{#401684}