Avoid destroying surface before the parent surface stops referencing it.

Avoid destroying surface before the parent surface stops referencing it.

Add surface sequence numbers, which are used to schedule the
destruction of surfaces. The child surface's destruction can
wait on a set of sequence numbers, and the parent surface can
later queue a frame that satisfies those numbers, causing the
former child surface to be destroyed.

Also move ownership of the SurfaceIdAllocator to the
ui::Compositor, so that the surface id namespace for a
compositor will stay the same across all output surfaces it
ever uses.

BUG=411118
Committed: https://crrev.com/fdc3baa3f2ae1554292165b53189196acd7418e4
Cr-Commit-Position: refs/heads/master@{#299022}

[jbauman, 2014-09-11 23:54:26]

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

[jamesr, 2014-09-12 01:08:56]

Awesome!  The cc/ parts lgtm, I want to take a closer look at the rest of the
patch with fresh eyes but it at a first glance it looks pretty nice.

https://codereview.chromium.org/553213003/diff/80001/cc/output/compositor_fra...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/553213003/diff/80001/cc/output/compositor_fra...
cc/output/compositor_frame_metadata.h:52: std::vector<uint32_t>
satisfies_sequence;
satisfies_sequences, since this is a vector?

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface.cc#n...
cc/surfaces/surface.cc:54: factory_->manager()->SatisfiesSequence(
the grammar here is a lil' weird.  How about "DidSatisfySequences" ?

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_aggr...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_aggr...
cc/surfaces/surface_aggregator.cc:61: if (surface_factory)
whoops

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_mana...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_mana...
cc/surfaces/surface_manager.h:9: #include <set>
#include <vector> since you use it on line 48 and aren't allowed to fwd declare
std::vector

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_mana...
cc/surfaces/surface_manager.h:31: // Destroy the Surface once a set of sequence
numbers has been received.
terminology nit: s/received/satisfied/

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_mana...
cc/surfaces/surface_manager.h:51: void AttemptSatisfyDestroy();
i feel like this name could be a bit better.  This is really sweeping all
dependencies to see if any can be satisfied, maybe call it
SearchForSatisfaction()?

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_mana...
cc/surfaces/surface_manager.h:59: typedef std::list<std::pair<Surface*,
std::set<SurfaceSequence> > >
we can do >>> now

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_sequ...
File cc/surfaces/surface_sequence.h (right):

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_sequ...
cc/surfaces/surface_sequence.h:16: SurfaceSequence() : id_namespace(0),
sequence(0) {}
nit: 0u

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

https://codereview.chromium.org/553213003/diff/80001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:384:
std::set<cc::SurfaceSequence> s;
's' seems a bit too terse, maybe 'seq' ?

[jbauman, 2014-09-23 01:58:06]

PTAL.

https://codereview.chromium.org/553213003/diff/80001/cc/output/compositor_fra...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/553213003/diff/80001/cc/output/compositor_fra...
cc/output/compositor_frame_metadata.h:52: std::vector<uint32_t>
satisfies_sequence;
On 2014/09/12 01:08:55, jamesr wrote:
> satisfies_sequences, since this is a vector?

Done.

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_mana...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_mana...
cc/surfaces/surface_manager.h:9: #include <set>
On 2014/09/12 01:08:55, jamesr wrote:
> #include <vector> since you use it on line 48 and aren't allowed to fwd
declare
> std::vector

Done.

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_mana...
cc/surfaces/surface_manager.h:31: // Destroy the Surface once a set of sequence
numbers has been received.
On 2014/09/12 01:08:55, jamesr wrote:
> terminology nit: s/received/satisfied/

Done.

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_mana...
cc/surfaces/surface_manager.h:51: void AttemptSatisfyDestroy();
On 2014/09/12 01:08:55, jamesr wrote:
> i feel like this name could be a bit better.  This is really sweeping all
> dependencies to see if any can be satisfied, maybe call it
> SearchForSatisfaction()?

Done.

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_sequ...
File cc/surfaces/surface_sequence.h (right):

https://codereview.chromium.org/553213003/diff/80001/cc/surfaces/surface_sequ...
cc/surfaces/surface_sequence.h:16: SurfaceSequence() : id_namespace(0),
sequence(0) {}
On 2014/09/12 01:08:55, jamesr wrote:
> nit: 0u

Done.

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

https://codereview.chromium.org/553213003/diff/80001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:384:
std::set<cc::SurfaceSequence> s;
On 2014/09/12 01:08:55, jamesr wrote:
> 's' seems a bit too terse, maybe 'seq' ?

Done.

[jamesr, 2014-09-24 04:51:01]

Sorry, I keep reading through this and getting lost somewhere.  The cc/ parts
are fine, but I don't really understand the reshuffling of the
SurfaceIdAllocator ownership.  Can you give me a hint about what that's for?

https://codereview.chromium.org/553213003/diff/120001/cc/surfaces/surface_seq...
File cc/surfaces/surface_sequence.h (right):

https://codereview.chromium.org/553213003/diff/120001/cc/surfaces/surface_seq...
cc/surfaces/surface_sequence.h:8: #include "base/compiler_specific.h"
don't think you need this (since this type isn't hashable)

https://codereview.chromium.org/553213003/diff/120001/cc/surfaces/surface_seq...
cc/surfaces/surface_sequence.h:17: explicit SurfaceSequence(uint32_t
id_namespace, uint32_t sequence)
no explicit for 2-arg c'tors

https://codereview.chromium.org/553213003/diff/120001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/553213003/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:74: SatisfySwapPromise(uint32_t id) : id_(id) {}
explicit

https://codereview.chromium.org/553213003/diff/120001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/553213003/diff/120001/ui/compositor/composito...
ui/compositor/compositor.h:298: int surface_sequence_number_;
uint32_t ?

[jbauman, 2014-09-24 19:13:12]

On 2014/09/24 04:51:01, jamesr wrote:
> Sorry, I keep reading through this and getting lost somewhere.  The cc/ parts
> are fine, but I don't really understand the reshuffling of the
> SurfaceIdAllocator ownership.  Can you give me a hint about what that's for?

We want the id namespace to remain the same when output surfaces are recreated
or whatever, so that CreateSurfaceSequence will always be using the correct
namespace. Having the ui::Compositor own the SurfaceIdAllocator allows it to do
that and make sure surface ids don't overlap. A pointer is passed to the output
surfaces whenever they're created so that they can use it to create new
surfaces.
> 
>
https://codereview.chromium.org/553213003/diff/120001/cc/surfaces/surface_seq...
> File cc/surfaces/surface_sequence.h (right):
> 
>
https://codereview.chromium.org/553213003/diff/120001/cc/surfaces/surface_seq...
> cc/surfaces/surface_sequence.h:8: #include "base/compiler_specific.h"
> don't think you need this (since this type isn't hashable)
> 
>
https://codereview.chromium.org/553213003/diff/120001/cc/surfaces/surface_seq...
> cc/surfaces/surface_sequence.h:17: explicit SurfaceSequence(uint32_t
> id_namespace, uint32_t sequence)
> no explicit for 2-arg c'tors
> 
>
https://codereview.chromium.org/553213003/diff/120001/ui/compositor/composito...
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/553213003/diff/120001/ui/compositor/composito...
> ui/compositor/compositor.cc:74: SatisfySwapPromise(uint32_t id) : id_(id) {}
> explicit
> 
>
https://codereview.chromium.org/553213003/diff/120001/ui/compositor/compositor.h
> File ui/compositor/compositor.h (right):
> 
>
https://codereview.chromium.org/553213003/diff/120001/ui/compositor/composito...
> ui/compositor/compositor.h:298: int surface_sequence_number_;
> uint32_t ?

[jamesr, 2014-09-25 23:23:47]

OK, that makes perfect sense.  Could you please add that to the patch
description?

lgtm w/ nits

https://codereview.chromium.org/553213003/diff/120001/content/common/cc_messa...
File content/common/cc_messages.h (right):

https://codereview.chromium.org/553213003/diff/120001/content/common/cc_messa...
content/common/cc_messages.h:290: IPC_STRUCT_TRAITS_MEMBER(satisfies_sequences)
we'll have to add this to the mojom interface too, but doesn't have to be in
this patch. if you want to look at it the type is
mojo/services/public/interfaces/surfaces/surface.mojom struct Frame

https://codereview.chromium.org/553213003/diff/120001/mojo/aura/context_facto...
File mojo/aura/context_factory_mojo.cc (right):

https://codereview.chromium.org/553213003/diff/120001/mojo/aura/context_facto...
mojo/aura/context_factory_mojo.cc:98: next_surface_id_namespace_(1) {
nit: 1u

https://codereview.chromium.org/553213003/diff/120001/ui/compositor/test/in_p...
File ui/compositor/test/in_process_context_factory.cc (right):

https://codereview.chromium.org/553213003/diff/120001/ui/compositor/test/in_p...
ui/compositor/test/in_process_context_factory.cc:24:
next_surface_id_namespace_(0) {
why does this one start at 0 but the mojo one start at 1?

add 'u'

[jbauman, 2014-09-25 23:32:49]

jbauman@chromium.org changed reviewers:
+ danakj@chromium.org, tsepez@chromium.org

[jbauman, 2014-09-25 23:32:50]

danakj: ui/compositor OWNERS review
tsepez: content/common/cc_messages.h OWNERS review

[Tom Sepez, 2014-09-25 23:46:53]

Messages LGTM.

[jbauman, 2014-10-02 00:11:17]

danakj, ping

[danakj, 2014-10-02 01:53:46]

https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:386:
surface_factory_->DestroyOnSequence(surface_id_, seq);
Can you help me understand what's going on here?

What does it mean to satisfy?

Why is the Sequence a set instead of a single id?

What's up with the SwapPromise putting a sequence number (without a namespace)
into the metadata when the browser makes a new surface?

https://codereview.chromium.org/553213003/diff/160001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/553213003/diff/160001/ui/compositor/composito...
ui/compositor/compositor.cc:82: // TODO: Send to the SurfaceManager immediately.
put a name on the todo

https://codereview.chromium.org/553213003/diff/160001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/553213003/diff/160001/ui/compositor/composito...
ui/compositor/compositor.h:100: virtual scoped_ptr<cc::SurfaceIdAllocator>
CreateSurfaceIdAllocator() = 0;
add whitespace and a comment for this

[danakj, 2014-10-02 16:21:35]

One more Q: Why is this special to ui/compositor. Why don't android browser and
renderer compositors need these things?

[jbauman, 2014-10-03 20:33:50]

On 2014/10/02 01:53:46, danakj wrote:
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> File content/browser/compositor/delegated_frame_host.cc (right):
> 
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> content/browser/compositor/delegated_frame_host.cc:386:
> surface_factory_->DestroyOnSequence(surface_id_, seq);
> Can you help me understand what's going on here?

FYI,
https://docs.google.com/a/chromium.org/document/d/1eVNYy6Fb37BS3bQOW6FqK38QLP...
is the overall design doc, though only some parts are implemented yet.
> 
> What does it mean to satisfy?

Destruction requires a certain set of sequence numbers to exist on submitted
frames (in the metadat) for it to occur. If a submitted frame lists a sequence
number, then it satisfies that number. Once all the dependencies have been
satisfied, then the surface can destroyed. 
> 
> Why is the Sequence a set instead of a single id?

In the future multiple frames could be using the one surface, and it would need
to wait for each of them to reach sequence numbers for destruction to be ok.
> 
> What's up with the SwapPromise putting a sequence number (without a namespace)
> into the metadata when the browser makes a new surface?

A Surface can only satisfy sequence numbers within its ID namespace. I don't
specify the namespace in the SwapPromise or metadata, because that would be
redundant.
> 
>
https://codereview.chromium.org/553213003/diff/160001/ui/compositor/composito...
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/553213003/diff/160001/ui/compositor/composito...
> ui/compositor/compositor.cc:82: // TODO: Send to the SurfaceManager
immediately.
> put a name on the todo
> 
>
https://codereview.chromium.org/553213003/diff/160001/ui/compositor/compositor.h
> File ui/compositor/compositor.h (right):
> 
>
https://codereview.chromium.org/553213003/diff/160001/ui/compositor/composito...
> ui/compositor/compositor.h:100: virtual scoped_ptr<cc::SurfaceIdAllocator>
> CreateSurfaceIdAllocator() = 0;
> add whitespace and a comment for this

[jbauman, 2014-10-03 20:36:41]

On 2014/10/02 16:21:35, danakj wrote:
> One more Q: Why is this special to ui/compositor. Why don't android browser
and
> renderer compositors need these things?

Currently neither of those understand Surfaces at the moment - they don't know
about surface id namespaces or anything. When we add support for them I can move
whatever common SwapPromise logic to cc/ (though first I'll probably want to
figure out how to handle the DidNotSwap case).

[danakj, 2014-10-07 20:08:26]

https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:385:
seq.insert(compositor->CreateSurfaceSequence());
What step (in that 1-10 steps) in your design document is this
CreateSurfaceSequence?

[jbauman, 2014-10-07 20:15:25]

On 2014/10/07 20:08:26, danakj wrote:
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> File content/browser/compositor/delegated_frame_host.cc (right):
> 
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> content/browser/compositor/delegated_frame_host.cc:385:
> seq.insert(compositor->CreateSurfaceSequence());
> What step (in that 1-10 steps) in your design document is this
> CreateSurfaceSequence?

That's step 1. Some of the other steps are slightly different (sync points 201
and 301 aren't needed/created) for now because the renderers don't understand
surfaces yet, just the DelegatedFrameHost, and that's on the UI thread so
synchronization is a bit simpler.

[danakj, 2014-10-07 20:33:19]

https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:375: id_allocator_ =
You said "Also move ownership of the SurfaceIdAllocator to the
ui::Compositor, so that the surface id namespace for a
compositor will stay the same across all output surfaces it
ever uses."

Why isn't this id_allocator_ here enough? Do you need a different namespace for
the frames submitted from this host?

And the Compositor allocator only exists to grab a namespace? Maybe we can do
that without requiring us to own an allocator there? Can someone not generate a
namespace and give that to here so we can generate a sequence for the frame?

https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:385:
seq.insert(compositor->CreateSurfaceSequence());
What if the layer moves to another compositor after this sequence has been
inserted? Is that possible?

It seems to me like this sequence should be created here and given to the
ui::Layer along with the frame, so we can be sure they travel together? Maybe
the ui::Layer could invoke a swap promise to insert it, once it knows a frame is
happening?

[jbauman, 2014-10-07 21:08:04]

On 2014/10/07 20:33:19, danakj wrote:
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> File content/browser/compositor/delegated_frame_host.cc (right):
> 
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> content/browser/compositor/delegated_frame_host.cc:375: id_allocator_ =
> You said "Also move ownership of the SurfaceIdAllocator to the
> ui::Compositor, so that the surface id namespace for a
> compositor will stay the same across all output surfaces it
> ever uses."
> 
> Why isn't this id_allocator_ here enough? Do you need a different namespace
for
> the frames submitted from this host?

Yes, this has a different SurfaceFactory and should use a different surface
namespace. This helps if e.g. the renderer dies and we want to destroy all
surfaces and sequence numbers from its namespace. Also it'd be very useful if we
want the cc::Display to live on a different thread from whatever's creating the
renderer surfaces, though that isn't the case quite yet.
> 
> And the Compositor allocator only exists to grab a namespace? Maybe we can do
> that without requiring us to own an allocator there? Can someone not generate
a
> namespace and give that to here so we can generate a sequence for the frame?

The allocator is also used to create surface ids for the browser compositor
output. It needs to live as long as the compositor does so that surface ids
continue to increment even after the browser compositor output surface is
recreated so they don't overlap, so it makes for it to live in the compositor.
> 
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> content/browser/compositor/delegated_frame_host.cc:385:
> seq.insert(compositor->CreateSurfaceSequence());
> What if the layer moves to another compositor after this sequence has been
> inserted? Is that possible?

That's fine. This is the wait for destroying the previous surface that was used
to draw to the screen. Even if the layer is later moved to a different
compositor, it still has to wait for the previous compositor to commit/draw a
frame that doesn't reference that surface for the surface to be destroyed. The
new compositor doesn't matter.
> 
> It seems to me like this sequence should be created here and given to the
> ui::Layer along with the frame, so we can be sure they travel together? Maybe
> the ui::Layer could invoke a swap promise to insert it, once it knows a frame
is
> happening?

It's not really tied to the ui::Layer that will contains the surface, but rather
to the compositor that used to contain it. e.g on tab switching even if the
layer isn't contained in a compositor now we want to still wait from a frame
(with a different layer) that doesn't reference this surface to be committed and
drawn before we destroy the old surface.

I realize that the current implementation doesn't handle all cases (it needs to
handle DelegatedFrameHost::RemovingFromWindow and the case where a frame was
already committed), but I'd prefer to fix that in a follow-up patch as this one
is a bit large already.

[danakj, 2014-10-08 21:51:21]

On 2014/10/07 21:08:04, jbauman wrote:
> On 2014/10/07 20:33:19, danakj wrote:
> >
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> > File content/browser/compositor/delegated_frame_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> > content/browser/compositor/delegated_frame_host.cc:375: id_allocator_ =
> > You said "Also move ownership of the SurfaceIdAllocator to the
> > ui::Compositor, so that the surface id namespace for a
> > compositor will stay the same across all output surfaces it
> > ever uses."
> > 
> > Why isn't this id_allocator_ here enough? Do you need a different namespace
> for
> > the frames submitted from this host?
> 
> Yes, this has a different SurfaceFactory and should use a different surface
> namespace. This helps if e.g. the renderer dies and we want to destroy all
> surfaces and sequence numbers from its namespace.

In the case you wanted to destroy all the surfaces from the dead renderer, you
mean you would be destroying the surfaces created in RWHVA with the
RWHVA::id_allocator_?

If the surface is destroyed, what benefit is having the numbers used to satisfy
destruction of that surface in a different namespace? The surfaces are destroyed
so those satisfaction numbers don't matter any more at that point do they?

> Also it'd be very useful if we
> want the cc::Display to live on a different thread from whatever's creating
the
> renderer surfaces, though that isn't the case quite yet.

I'm totally lost how threading comes into play here. Can you explain a bit more?
Here RWHVA is calling to the Compositor to get an id with a different namespace.
That wouldn't be a cross thread operation if the cc::Display is on another
thread right?

Or you want to have 2 namespaces to help with threading? The compositor and the
RWHVA would still be on the same thread tho, no? Sorry for not getting this part
at all yet. ><

> > 
> > And the Compositor allocator only exists to grab a namespace? Maybe we can
do
> > that without requiring us to own an allocator there? Can someone not
generate
> a
> > namespace and give that to here so we can generate a sequence for the frame?
> 
> The allocator is also used to create surface ids for the browser compositor
> output. It needs to live as long as the compositor does so that surface ids
> continue to increment even after the browser compositor output surface is
> recreated so they don't overlap, so it makes for it to live in the compositor.
> > 
> >
>
https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
> > content/browser/compositor/delegated_frame_host.cc:385:
> > seq.insert(compositor->CreateSurfaceSequence());
> > What if the layer moves to another compositor after this sequence has been
> > inserted? Is that possible?
> 
> That's fine. This is the wait for destroying the previous surface that was
used
> to draw to the screen. Even if the layer is later moved to a different
> compositor, it still has to wait for the previous compositor to commit/draw a
> frame that doesn't reference that surface for the surface to be destroyed. The
> new compositor doesn't matter.

Ok awesome, that makes a lot of sense. Can you add a comment saying something
about this?

> > 
> > It seems to me like this sequence should be created here and given to the
> > ui::Layer along with the frame, so we can be sure they travel together?
Maybe
> > the ui::Layer could invoke a swap promise to insert it, once it knows a
frame
> is
> > happening?
> 
> It's not really tied to the ui::Layer that will contains the surface, but
rather
> to the compositor that used to contain it. e.g on tab switching even if the
> layer isn't contained in a compositor now we want to still wait from a frame
> (with a different layer) that doesn't reference this surface to be committed
and
> drawn before we destroy the old surface.
> 
> I realize that the current implementation doesn't handle all cases (it needs
to
> handle DelegatedFrameHost::RemovingFromWindow and the case where a frame was
> already committed), but I'd prefer to fix that in a follow-up patch as this
one
> is a bit large already.

Thanks for explaining, I think I get this part now :)

https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/553213003/diff/160001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:385:
seq.insert(compositor->CreateSurfaceSequence());
Comment here please explaining that we want the compositor which /had the old
surface/ to submit a frame before we destroy the old surface (not the compositor
that the layer will be attached to when it's drawn).

[jbauman, 2014-10-08 23:41:57]

On Wed, Oct 8, 2014 at 2:51 PM, <danakj@chromium.org> wrote:

> On 2014/10/07 21:08:04, jbauman wrote:
>
>> On 2014/10/07 20:33:19, danakj wrote:
>> >
>>
>
> https://codereview.chromium.org/553213003/diff/160001/
> content/browser/compositor/delegated_frame_host.cc
>
>> > File content/browser/compositor/delegated_frame_host.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/553213003/diff/160001/
> content/browser/compositor/delegated_frame_host.cc#newcode375
>
>> > content/browser/compositor/delegated_frame_host.cc:375: id_allocator_ =
>> > You said "Also move ownership of the SurfaceIdAllocator to the
>> > ui::Compositor, so that the surface id namespace for a
>> > compositor will stay the same across all output surfaces it
>> > ever uses."
>> >
>> > Why isn't this id_allocator_ here enough? Do you need a different
>> namespace
>> for
>> > the frames submitted from this host?
>>
>
>  Yes, this has a different SurfaceFactory and should use a different
>> surface
>> namespace. This helps if e.g. the renderer dies and we want to destroy all
>> surfaces and sequence numbers from its namespace.
>>
>
> In the case you wanted to destroy all the surfaces from the dead renderer,
> you
> mean you would be destroying the surfaces created in RWHVA with the
> RWHVA::id_allocator_?
>
> If the surface is destroyed, what benefit is having the numbers used to
> satisfy
> destruction of that surface in a different namespace? The surfaces are
> destroyed
> so those satisfaction numbers don't matter any more at that point do they?


If a browser compositor is destroyed, then all the surface sequence numbers
from its namespace should be ignored, because it's not displaying anything
anyway so it doesn't matter. However, if a delegated frame host is
destroyed, its surfaces may have to live until the browser compositor
commits a new frame with a new surface sequence number. So that surface
sequence number has to live past the destruction of the delegated frame
host, and should be allocated in the browser compositor's id namespace.

>
>
>  Also it'd be very useful if we
>> want the cc::Display to live on a different thread from whatever's
>> creating
>>
> the
>
>> renderer surfaces, though that isn't the case quite yet.
>>
>
> I'm totally lost how threading comes into play here. Can you explain a bit
> more?
> Here RWHVA is calling to the Compositor to get an id with a different
> namespace.
> That wouldn't be a cross thread operation if the cc::Display is on another
> thread right?


> Or you want to have 2 namespaces to help with threading? The compositor
> and the
> RWHVA would still be on the same thread tho, no? Sorry for not getting
> this part
> at all yet. ><


To be clear, both surface id numbers and sequence numbers are allocated in
the same namespace, but can overlap.
Browser compositor sequence numbers are allocated on the UI thread, browser
compositor surface ids are allocated on the browser impl thread, renderer
sequence numbers currently aren't used, and renderer surface id numbers are
allocated in the browser UI thread.

I'd like to push renderer surface id allocation into the renderer itself,
from the RWHVA, so they'll need different namespaces in that case. Then the
surface sequence would be passed from the brower ui thread to the renderer
impl thread.

>
>
>  >
>> > And the Compositor allocator only exists to grab a namespace? Maybe we
>> can
>>
> do
>
>> > that without requiring us to own an allocator there? Can someone not
>>
> generate
>
>> a
>> > namespace and give that to here so we can generate a sequence for the
>> frame?
>>
>
>  The allocator is also used to create surface ids for the browser
>> compositor
>> output. It needs to live as long as the compositor does so that surface
>> ids
>> continue to increment even after the browser compositor output surface is
>> recreated so they don't overlap, so it makes for it to live in the
>> compositor.
>> >
>> >
>>
>
> https://codereview.chromium.org/553213003/diff/160001/
> content/browser/compositor/delegated_frame_host.cc#newcode385
>
>> > content/browser/compositor/delegated_frame_host.cc:385:
>> > seq.insert(compositor->CreateSurfaceSequence());
>> > What if the layer moves to another compositor after this sequence has
>> been
>> > inserted? Is that possible?
>>
>
>  That's fine. This is the wait for destroying the previous surface that was
>>
> used
>
>> to draw to the screen. Even if the layer is later moved to a different
>> compositor, it still has to wait for the previous compositor to
>> commit/draw a
>> frame that doesn't reference that surface for the surface to be
>> destroyed. The
>> new compositor doesn't matter.
>>
>
> Ok awesome, that makes a lot of sense. Can you add a comment saying
> something
> about this?


Ok, sure.

>
>
>  >
>> > It seems to me like this sequence should be created here and given to
>> the
>> > ui::Layer along with the frame, so we can be sure they travel together?
>>
> Maybe
>
>> > the ui::Layer could invoke a swap promise to insert it, once it knows a
>>
> frame
>
>> is
>> > happening?
>>
>
>  It's not really tied to the ui::Layer that will contains the surface, but
>>
> rather
>
>> to the compositor that used to contain it. e.g on tab switching even if
>> the
>> layer isn't contained in a compositor now we want to still wait from a
>> frame
>> (with a different layer) that doesn't reference this surface to be
>> committed
>>
> and
>
>> drawn before we destroy the old surface.
>>
>
>  I realize that the current implementation doesn't handle all cases (it
>> needs
>>
> to
>
>> handle DelegatedFrameHost::RemovingFromWindow and the case where a frame
>> was
>> already committed), but I'd prefer to fix that in a follow-up patch as
>> this
>>
> one
>
>> is a bit large already.
>>
>
> Thanks for explaining, I think I get this part now :)
>
>
> https://codereview.chromium.org/553213003/diff/160001/
> content/browser/compositor/delegated_frame_host.cc
> File content/browser/compositor/delegated_frame_host.cc (right):
>
> https://codereview.chromium.org/553213003/diff/160001/
> content/browser/compositor/delegated_frame_host.cc#newcode385
> content/browser/compositor/delegated_frame_host.cc:385:
> seq.insert(compositor->CreateSurfaceSequence());
> Comment here please explaining that we want the compositor which /had
> the old surface/ to submit a frame before we destroy the old surface
> (not the compositor that the layer will be attached to when it's drawn).
>
> https://codereview.chromium.org/553213003/
>

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

[danakj, 2014-10-09 00:19:02]

On Wed, Oct 8, 2014 at 7:41 PM, John Bauman <jbauman@chromium.org> wrote:

>
>
> On Wed, Oct 8, 2014 at 2:51 PM, <danakj@chromium.org> wrote:
>
>> On 2014/10/07 21:08:04, jbauman wrote:
>>
>>> On 2014/10/07 20:33:19, danakj wrote:
>>> >
>>>
>>
>> https://codereview.chromium.org/553213003/diff/160001/
>> content/browser/compositor/delegated_frame_host.cc
>>
>>> > File content/browser/compositor/delegated_frame_host.cc (right):
>>> >
>>> >
>>>
>>
>> https://codereview.chromium.org/553213003/diff/160001/
>> content/browser/compositor/delegated_frame_host.cc#newcode375
>>
>>> > content/browser/compositor/delegated_frame_host.cc:375: id_allocator_
>>> =
>>> > You said "Also move ownership of the SurfaceIdAllocator to the
>>> > ui::Compositor, so that the surface id namespace for a
>>> > compositor will stay the same across all output surfaces it
>>> > ever uses."
>>> >
>>> > Why isn't this id_allocator_ here enough? Do you need a different
>>> namespace
>>> for
>>> > the frames submitted from this host?
>>>
>>
>>  Yes, this has a different SurfaceFactory and should use a different
>>> surface
>>> namespace. This helps if e.g. the renderer dies and we want to destroy
>>> all
>>> surfaces and sequence numbers from its namespace.
>>>
>>
>> In the case you wanted to destroy all the surfaces from the dead
>> renderer, you
>> mean you would be destroying the surfaces created in RWHVA with the
>> RWHVA::id_allocator_?
>>
>> If the surface is destroyed, what benefit is having the numbers used to
>> satisfy
>> destruction of that surface in a different namespace? The surfaces are
>> destroyed
>> so those satisfaction numbers don't matter any more at that point do they?
>
>
> If a browser compositor is destroyed, then all the surface sequence
> numbers from its namespace should be ignored, because it's not displaying
> anything anyway so it doesn't matter. However, if a delegated frame host is
> destroyed, its surfaces may have to live until the browser compositor
> commits a new frame with a new surface sequence number. So that surface
> sequence number has to live past the destruction of the delegated frame
> host, and should be allocated in the browser compositor's id namespace.
>
>>
>>
>>  Also it'd be very useful if we
>>> want the cc::Display to live on a different thread from whatever's
>>> creating
>>>
>> the
>>
>>> renderer surfaces, though that isn't the case quite yet.
>>>
>>
>> I'm totally lost how threading comes into play here. Can you explain a
>> bit more?
>> Here RWHVA is calling to the Compositor to get an id with a different
>> namespace.
>> That wouldn't be a cross thread operation if the cc::Display is on another
>> thread right?
>
>
>> Or you want to have 2 namespaces to help with threading? The compositor
>> and the
>> RWHVA would still be on the same thread tho, no? Sorry for not getting
>> this part
>> at all yet. ><
>
>
> To be clear, both surface id numbers and sequence numbers are allocated in
> the same namespace, but can overlap.
> Browser compositor sequence numbers are allocated on the UI thread,
> browser compositor surface ids are allocated on the browser impl thread,
> renderer sequence numbers currently aren't used, and renderer surface id
> numbers are allocated in the browser UI thread.
>

Where is the code that generates the browser compositor surface ids? I
don't see anything on the impl thread in this patch. Is that already landed?


>
> I'd like to push renderer surface id allocation into the renderer itself,
> from the RWHVA, so they'll need different namespaces in that case. Then the
> surface sequence would be passed from the brower ui thread to the renderer
> impl thread.
>
>>
>>
>>  >
>>> > And the Compositor allocator only exists to grab a namespace? Maybe we
>>> can
>>>
>> do
>>
>>> > that without requiring us to own an allocator there? Can someone not
>>>
>> generate
>>
>>> a
>>> > namespace and give that to here so we can generate a sequence for the
>>> frame?
>>>
>>
>>  The allocator is also used to create surface ids for the browser
>>> compositor
>>> output. It needs to live as long as the compositor does so that surface
>>> ids
>>> continue to increment even after the browser compositor output surface is
>>> recreated so they don't overlap, so it makes for it to live in the
>>> compositor.
>>> >
>>> >
>>>
>>
>> https://codereview.chromium.org/553213003/diff/160001/
>> content/browser/compositor/delegated_frame_host.cc#newcode385
>>
>>> > content/browser/compositor/delegated_frame_host.cc:385:
>>> > seq.insert(compositor->CreateSurfaceSequence());
>>> > What if the layer moves to another compositor after this sequence has
>>> been
>>> > inserted? Is that possible?
>>>
>>
>>  That's fine. This is the wait for destroying the previous surface that
>>> was
>>>
>> used
>>
>>> to draw to the screen. Even if the layer is later moved to a different
>>> compositor, it still has to wait for the previous compositor to
>>> commit/draw a
>>> frame that doesn't reference that surface for the surface to be
>>> destroyed. The
>>> new compositor doesn't matter.
>>>
>>
>> Ok awesome, that makes a lot of sense. Can you add a comment saying
>> something
>> about this?
>
>
> Ok, sure.
>
>>
>>
>>  >
>>> > It seems to me like this sequence should be created here and given to
>>> the
>>> > ui::Layer along with the frame, so we can be sure they travel together?
>>>
>> Maybe
>>
>>> > the ui::Layer could invoke a swap promise to insert it, once it knows a
>>>
>> frame
>>
>>> is
>>> > happening?
>>>
>>
>>  It's not really tied to the ui::Layer that will contains the surface, but
>>>
>> rather
>>
>>> to the compositor that used to contain it. e.g on tab switching even if
>>> the
>>> layer isn't contained in a compositor now we want to still wait from a
>>> frame
>>> (with a different layer) that doesn't reference this surface to be
>>> committed
>>>
>> and
>>
>>> drawn before we destroy the old surface.
>>>
>>
>>  I realize that the current implementation doesn't handle all cases (it
>>> needs
>>>
>> to
>>
>>> handle DelegatedFrameHost::RemovingFromWindow and the case where a
>>> frame was
>>> already committed), but I'd prefer to fix that in a follow-up patch as
>>> this
>>>
>> one
>>
>>> is a bit large already.
>>>
>>
>> Thanks for explaining, I think I get this part now :)
>>
>>
>> https://codereview.chromium.org/553213003/diff/160001/
>> content/browser/compositor/delegated_frame_host.cc
>> File content/browser/compositor/delegated_frame_host.cc (right):
>>
>> https://codereview.chromium.org/553213003/diff/160001/
>> content/browser/compositor/delegated_frame_host.cc#newcode385
>> content/browser/compositor/delegated_frame_host.cc:385:
>> seq.insert(compositor->CreateSurfaceSequence());
>> Comment here please explaining that we want the compositor which /had
>> the old surface/ to submit a frame before we destroy the old surface
>> (not the compositor that the layer will be attached to when it's drawn).
>>
>> https://codereview.chromium.org/553213003/
>>
>
>

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

[danakj, 2014-10-09 00:23:53]

On 2014/10/09 00:19:02, danakj wrote:
> On Wed, Oct 8, 2014 at 7:41 PM, John Bauman <mailto:jbauman@chromium.org>
wrote:
> 
> >
> >
> > On Wed, Oct 8, 2014 at 2:51 PM, <mailto:danakj@chromium.org> wrote:
> >
> >> On 2014/10/07 21:08:04, jbauman wrote:
> >>
> >>> On 2014/10/07 20:33:19, danakj wrote:
> >>> >
> >>>
> >>
> >> https://codereview.chromium.org/553213003/diff/160001/
> >> content/browser/compositor/delegated_frame_host.cc
> >>
> >>> > File content/browser/compositor/delegated_frame_host.cc (right):
> >>> >
> >>> >
> >>>
> >>
> >> https://codereview.chromium.org/553213003/diff/160001/
> >> content/browser/compositor/delegated_frame_host.cc#newcode375
> >>
> >>> > content/browser/compositor/delegated_frame_host.cc:375: id_allocator_
> >>> =
> >>> > You said "Also move ownership of the SurfaceIdAllocator to the
> >>> > ui::Compositor, so that the surface id namespace for a
> >>> > compositor will stay the same across all output surfaces it
> >>> > ever uses."
> >>> >
> >>> > Why isn't this id_allocator_ here enough? Do you need a different
> >>> namespace
> >>> for
> >>> > the frames submitted from this host?
> >>>
> >>
> >>  Yes, this has a different SurfaceFactory and should use a different
> >>> surface
> >>> namespace. This helps if e.g. the renderer dies and we want to destroy
> >>> all
> >>> surfaces and sequence numbers from its namespace.
> >>>
> >>
> >> In the case you wanted to destroy all the surfaces from the dead
> >> renderer, you
> >> mean you would be destroying the surfaces created in RWHVA with the
> >> RWHVA::id_allocator_?
> >>
> >> If the surface is destroyed, what benefit is having the numbers used to
> >> satisfy
> >> destruction of that surface in a different namespace? The surfaces are
> >> destroyed
> >> so those satisfaction numbers don't matter any more at that point do they?
> >
> >
> > If a browser compositor is destroyed, then all the surface sequence
> > numbers from its namespace should be ignored, because it's not displaying
> > anything anyway so it doesn't matter. However, if a delegated frame host is
> > destroyed, its surfaces may have to live until the browser compositor
> > commits a new frame with a new surface sequence number. So that surface
> > sequence number has to live past the destruction of the delegated frame
> > host, and should be allocated in the browser compositor's id namespace.

OK one more question to explore what's going on. Thanks this is helping a lot.
What's the benefit to putting this ID generator on each compositor instead of
having one per process (ie one in ContextFactory, one in RenderThreadImpl in the
renderer process)?

https://codereview.chromium.org/553213003/diff/160001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/553213003/diff/160001/ui/compositor/composito...
ui/compositor/compositor.cc:430: cc::SurfaceSequence
Compositor::CreateSurfaceSequence() {
Can you give this a name that says its going to use that sequence number in the
next compositor frame as well. I think that's the important part to capture
rather than just a name.

[jbauman, 2014-10-09 00:43:06]

That's in SurfaceDisplayOutputSurface::SwapBuffers; this patch modifies it,
but it's been checked in for a while.

On Wed, Oct 8, 2014 at 5:18 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Oct 8, 2014 at 7:41 PM, John Bauman <jbauman@chromium.org> wrote:
>
>>
>>
>> On Wed, Oct 8, 2014 at 2:51 PM, <danakj@chromium.org> wrote:
>>
>>> On 2014/10/07 21:08:04, jbauman wrote:
>>>
>>>> On 2014/10/07 20:33:19, danakj wrote:
>>>> >
>>>>
>>>
>>> https://codereview.chromium.org/553213003/diff/160001/
>>> content/browser/compositor/delegated_frame_host.cc
>>>
>>>> > File content/browser/compositor/delegated_frame_host.cc (right):
>>>> >
>>>> >
>>>>
>>>
>>> https://codereview.chromium.org/553213003/diff/160001/
>>> content/browser/compositor/delegated_frame_host.cc#newcode375
>>>
>>>> > content/browser/compositor/delegated_frame_host.cc:375:
>>>> id_allocator_ =
>>>> > You said "Also move ownership of the SurfaceIdAllocator to the
>>>> > ui::Compositor, so that the surface id namespace for a
>>>> > compositor will stay the same across all output surfaces it
>>>> > ever uses."
>>>> >
>>>> > Why isn't this id_allocator_ here enough? Do you need a different
>>>> namespace
>>>> for
>>>> > the frames submitted from this host?
>>>>
>>>
>>>  Yes, this has a different SurfaceFactory and should use a different
>>>> surface
>>>> namespace. This helps if e.g. the renderer dies and we want to destroy
>>>> all
>>>> surfaces and sequence numbers from its namespace.
>>>>
>>>
>>> In the case you wanted to destroy all the surfaces from the dead
>>> renderer, you
>>> mean you would be destroying the surfaces created in RWHVA with the
>>> RWHVA::id_allocator_?
>>>
>>> If the surface is destroyed, what benefit is having the numbers used to
>>> satisfy
>>> destruction of that surface in a different namespace? The surfaces are
>>> destroyed
>>> so those satisfaction numbers don't matter any more at that point do
>>> they?
>>
>>
>> If a browser compositor is destroyed, then all the surface sequence
>> numbers from its namespace should be ignored, because it's not displaying
>> anything anyway so it doesn't matter. However, if a delegated frame host is
>> destroyed, its surfaces may have to live until the browser compositor
>> commits a new frame with a new surface sequence number. So that surface
>> sequence number has to live past the destruction of the delegated frame
>> host, and should be allocated in the browser compositor's id namespace.
>>
>>>
>>>
>>>  Also it'd be very useful if we
>>>> want the cc::Display to live on a different thread from whatever's
>>>> creating
>>>>
>>> the
>>>
>>>> renderer surfaces, though that isn't the case quite yet.
>>>>
>>>
>>> I'm totally lost how threading comes into play here. Can you explain a
>>> bit more?
>>> Here RWHVA is calling to the Compositor to get an id with a different
>>> namespace.
>>> That wouldn't be a cross thread operation if the cc::Display is on
>>> another
>>> thread right?
>>
>>
>>> Or you want to have 2 namespaces to help with threading? The compositor
>>> and the
>>> RWHVA would still be on the same thread tho, no? Sorry for not getting
>>> this part
>>> at all yet. ><
>>
>>
>> To be clear, both surface id numbers and sequence numbers are allocated
>> in the same namespace, but can overlap.
>> Browser compositor sequence numbers are allocated on the UI thread,
>> browser compositor surface ids are allocated on the browser impl thread,
>> renderer sequence numbers currently aren't used, and renderer surface id
>> numbers are allocated in the browser UI thread.
>>
>
> Where is the code that generates the browser compositor surface ids? I
> don't see anything on the impl thread in this patch. Is that already landed?
>
>
>>
>> I'd like to push renderer surface id allocation into the renderer itself,
>> from the RWHVA, so they'll need different namespaces in that case. Then the
>> surface sequence would be passed from the brower ui thread to the renderer
>> impl thread.
>>
>>>
>>>
>>>  >
>>>> > And the Compositor allocator only exists to grab a namespace? Maybe
>>>> we can
>>>>
>>> do
>>>
>>>> > that without requiring us to own an allocator there? Can someone not
>>>>
>>> generate
>>>
>>>> a
>>>> > namespace and give that to here so we can generate a sequence for the
>>>> frame?
>>>>
>>>
>>>  The allocator is also used to create surface ids for the browser
>>>> compositor
>>>> output. It needs to live as long as the compositor does so that surface
>>>> ids
>>>> continue to increment even after the browser compositor output surface
>>>> is
>>>> recreated so they don't overlap, so it makes for it to live in the
>>>> compositor.
>>>> >
>>>> >
>>>>
>>>
>>> https://codereview.chromium.org/553213003/diff/160001/
>>> content/browser/compositor/delegated_frame_host.cc#newcode385
>>>
>>>> > content/browser/compositor/delegated_frame_host.cc:385:
>>>> > seq.insert(compositor->CreateSurfaceSequence());
>>>> > What if the layer moves to another compositor after this sequence has
>>>> been
>>>> > inserted? Is that possible?
>>>>
>>>
>>>  That's fine. This is the wait for destroying the previous surface that
>>>> was
>>>>
>>> used
>>>
>>>> to draw to the screen. Even if the layer is later moved to a different
>>>> compositor, it still has to wait for the previous compositor to
>>>> commit/draw a
>>>> frame that doesn't reference that surface for the surface to be
>>>> destroyed. The
>>>> new compositor doesn't matter.
>>>>
>>>
>>> Ok awesome, that makes a lot of sense. Can you add a comment saying
>>> something
>>> about this?
>>
>>
>> Ok, sure.
>>
>>>
>>>
>>>  >
>>>> > It seems to me like this sequence should be created here and given to
>>>> the
>>>> > ui::Layer along with the frame, so we can be sure they travel
>>>> together?
>>>>
>>> Maybe
>>>
>>>> > the ui::Layer could invoke a swap promise to insert it, once it knows
>>>> a
>>>>
>>> frame
>>>
>>>> is
>>>> > happening?
>>>>
>>>
>>>  It's not really tied to the ui::Layer that will contains the surface,
>>>> but
>>>>
>>> rather
>>>
>>>> to the compositor that used to contain it. e.g on tab switching even if
>>>> the
>>>> layer isn't contained in a compositor now we want to still wait from a
>>>> frame
>>>> (with a different layer) that doesn't reference this surface to be
>>>> committed
>>>>
>>> and
>>>
>>>> drawn before we destroy the old surface.
>>>>
>>>
>>>  I realize that the current implementation doesn't handle all cases (it
>>>> needs
>>>>
>>> to
>>>
>>>> handle DelegatedFrameHost::RemovingFromWindow and the case where a
>>>> frame was
>>>> already committed), but I'd prefer to fix that in a follow-up patch as
>>>> this
>>>>
>>> one
>>>
>>>> is a bit large already.
>>>>
>>>
>>> Thanks for explaining, I think I get this part now :)
>>>
>>>
>>> https://codereview.chromium.org/553213003/diff/160001/
>>> content/browser/compositor/delegated_frame_host.cc
>>> File content/browser/compositor/delegated_frame_host.cc (right):
>>>
>>> https://codereview.chromium.org/553213003/diff/160001/
>>> content/browser/compositor/delegated_frame_host.cc#newcode385
>>> content/browser/compositor/delegated_frame_host.cc:385:
>>> seq.insert(compositor->CreateSurfaceSequence());
>>> Comment here please explaining that we want the compositor which /had
>>> the old surface/ to submit a frame before we destroy the old surface
>>> (not the compositor that the layer will be attached to when it's drawn).
>>>
>>> https://codereview.chromium.org/553213003/
>>>
>>
>>
>

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

[jbauman, 2014-10-09 00:48:28]

On Wed, Oct 8, 2014 at 5:23 PM, <danakj@chromium.org> wrote:

> On 2014/10/09 00:19:02, danakj wrote:
>
>> On Wed, Oct 8, 2014 at 7:41 PM, John Bauman <mailto:jbauman@chromium.org>
>>
> wrote:
>
>
>  >
>> >
>> > On Wed, Oct 8, 2014 at 2:51 PM, <mailto:danakj@chromium.org> wrote:
>> >
>> >> On 2014/10/07 21:08:04, jbauman wrote:
>> >>
>> >>> On 2014/10/07 20:33:19, danakj wrote:
>> >>> >
>> >>>
>> >>
>> >> https://codereview.chromium.org/553213003/diff/160001/
>> >> content/browser/compositor/delegated_frame_host.cc
>> >>
>> >>> > File content/browser/compositor/delegated_frame_host.cc (right):
>> >>> >
>> >>> >
>> >>>
>> >>
>> >> https://codereview.chromium.org/553213003/diff/160001/
>> >> content/browser/compositor/delegated_frame_host.cc#newcode375
>> >>
>> >>> > content/browser/compositor/delegated_frame_host.cc:375:
>> id_allocator_
>> >>> =
>> >>> > You said "Also move ownership of the SurfaceIdAllocator to the
>> >>> > ui::Compositor, so that the surface id namespace for a
>> >>> > compositor will stay the same across all output surfaces it
>> >>> > ever uses."
>> >>> >
>> >>> > Why isn't this id_allocator_ here enough? Do you need a different
>> >>> namespace
>> >>> for
>> >>> > the frames submitted from this host?
>> >>>
>> >>
>> >>  Yes, this has a different SurfaceFactory and should use a different
>> >>> surface
>> >>> namespace. This helps if e.g. the renderer dies and we want to destroy
>> >>> all
>> >>> surfaces and sequence numbers from its namespace.
>> >>>
>> >>
>> >> In the case you wanted to destroy all the surfaces from the dead
>> >> renderer, you
>> >> mean you would be destroying the surfaces created in RWHVA with the
>> >> RWHVA::id_allocator_?
>> >>
>> >> If the surface is destroyed, what benefit is having the numbers used to
>> >> satisfy
>> >> destruction of that surface in a different namespace? The surfaces are
>> >> destroyed
>> >> so those satisfaction numbers don't matter any more at that point do
>> they?
>> >
>> >
>> > If a browser compositor is destroyed, then all the surface sequence
>> > numbers from its namespace should be ignored, because it's not
>> displaying
>> > anything anyway so it doesn't matter. However, if a delegated frame
>> host is
>> > destroyed, its surfaces may have to live until the browser compositor
>> > commits a new frame with a new surface sequence number. So that surface
>> > sequence number has to live past the destruction of the delegated frame
>> > host, and should be allocated in the browser compositor's id namespace.
>>
>
> OK one more question to explore what's going on. Thanks this is helping a
> lot.
> What's the benefit to putting this ID generator on each compositor instead
> of
> having one per process (ie one in ContextFactory, one in RenderThreadImpl
> in the
> renderer process)?
>
> Different compositors can shut down at down at different times. So it's
nice to be able to shut down one browser compositor and say that all its
sequence numbers don't matter, because it's not going to be displayed
anymore. Using global per-process allocators doesn't give us that
granularity. I don't think it particularly matters for our use-cases,
though.


>
> https://codereview.chromium.org/553213003/diff/160001/ui/
> compositor/compositor.cc
> File ui/compositor/compositor.cc (right):
>
> https://codereview.chromium.org/553213003/diff/160001/ui/
> compositor/compositor.cc#newcode430
> ui/compositor/compositor.cc:430: cc::SurfaceSequence
> Compositor::CreateSurfaceSequence() {
> Can you give this a name that says its going to use that sequence number
> in the next compositor frame as well. I think that's the important part
> to capture rather than just a name.
>
> https://codereview.chromium.org/553213003/
>

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

[jbauman, 2014-10-09 02:10:28]

PTAL. Added a few more comments and renamed CreateSurfaceSequence

[danakj, 2014-10-09 17:47:38]

I see, so the allocator used to live on the output surface, and moved from there
to Compositor. I thought it was moving from RWHVA to Compositor. I think I
understand what is going on now... thanks for the patient explanations.

ui/compositor LGTM

https://codereview.chromium.org/553213003/diff/180001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/553213003/diff/180001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:386: // TODO(jbauman): Handle
cases where the compositor has been
Maybe you need to also think about if RecreateLayer happened and the surface was
shown in multiple compositors. That's another form of "compositor changed"
maybe, but if you kill the surface here it would destroy the content in some
other compositor also sharing it.

https://codereview.chromium.org/553213003/diff/180001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/553213003/diff/180001/ui/compositor/composito...
ui/compositor/compositor.cc:429: cc::SurfaceSequence
Compositor::CreateSurfaceSequenceForNextFrame() {
Create -> Insert ?

[jbauman, 2014-10-09 23:48:27]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-10-09 23:50:36]

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

[commit-bot: I haz the power, 2014-10-10 00:22:26]

Committed patchset #12 (id:390001)

[commit-bot: I haz the power, 2014-10-10 00:23:52]

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