Making OutputSurface a begin frame source and vsync source.

Making OutputSurface a begin frame source and vsync source.

BUG=345459

[mithro-old, 2014-09-16 14:43:13]

mithro@mithis.com changed reviewers:
+ brianderson@chromium.org, skyostil@chromium.org

[mithro-old, 2014-09-16 14:43:14]

Hi Brian & Sami,

This patch extends https://codereview.chromium.org/267783004/ to make the
OutputSurface the BeginFrameSource rather than the LayerTreeHostImpl.

The patch doesn't quite work yet, but you can see the general idea. I think
there are potential issues with holding onto an OutputSurface pointer inside the
scheduler.....

Can you take a look and tell me what you think?

Tim

[danakj, 2014-09-16 15:15:26]

On Tue, Sep 16, 2014 at 10:43 AM, <mithro@mithis.com> wrote:

> Reviewers: brianderson, Sami,
>
> Message:
> Hi Brian & Sami,
>
> This patch extends https://codereview.chromium.org/267783004/ to make the
> OutputSurface the BeginFrameSource rather than the LayerTreeHostImpl.
>
> The patch doesn't quite work yet, but you can see the general idea. I think
> there are potential issues with holding onto an OutputSurface pointer
> inside the
> scheduler.....
>

Yes, that sounds scary to me. OutputSurfaces come and go, and the scheduler
outlives them all. I don't think scheduler should be tightly coupled to
output surface like that, we should keep some encapsulation between them.


>
> Can you take a look and tell me what you think?
>
> Tim
>
> Description:
> Making OutputSurface a begin frame source and vsync source.
>
> BUG=345459
>
> Please review this at https://codereview.chromium.org/577643002/
>
> Affected files (+209, -146 lines):
>   M cc/output/managed_memory_policy.h
>   M cc/output/managed_memory_policy.cc
>   M cc/output/output_surface.h
>   M cc/output/output_surface.cc
>   M cc/output/output_surface_client.h
>   M cc/output/output_surface_unittest.cc
>   M cc/scheduler/scheduler.h
>   M cc/scheduler/scheduler.cc
>   M cc/scheduler/scheduler_state_machine.h
>   M cc/scheduler/scheduler_unittest.cc
>   M cc/surfaces/display.h
>   M cc/test/fake_layer_tree_host_impl_client.h
>   M cc/test/fake_output_surface.h
>   M cc/test/fake_output_surface.cc
>   M cc/test/fake_output_surface_client.h
>   M cc/test/fake_output_surface_client.cc
>   M cc/trees/layer_tree_host_impl.h
>   M cc/trees/layer_tree_host_impl.cc
>   M cc/trees/layer_tree_host_impl_unittest.cc
>   M cc/trees/single_thread_proxy.h
>   M cc/trees/single_thread_proxy.cc
>   M cc/trees/thread_proxy.h
>   M cc/trees/thread_proxy.cc
>   M content/browser/android/in_process/synchronous_
> compositor_output_surface.cc
>   M content/renderer/gpu/compositor_output_surface.cc
>
>
>

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

[mithro-old, 2014-09-17 09:41:35]

> Yes, that sounds scary to me. OutputSurfaces come and go, and the scheduler
> outlives them all. I don't think scheduler should be tightly coupled to
> output surface like that, we should keep some encapsulation between them.

Hi Dana,

If I understand correctly, the scheduler already needs to intimately understand
the state of the OutputSurface so that it schedules the right actions. If that
is correct, then I think it /should/ be okay in the end. I'd obviously like
feedback from Brian (who suggested this idea) about if this is going in the
right direction.

Obviously we want tests to make sure that the express the output surface life
cycles and makes sure the scheduler does the right thing.

Do you have a suggestion of a better way to do this?

Thanks!

Tim

[danakj, 2014-09-18 23:17:38]

On Wed, Sep 17, 2014 at 5:41 AM, <mithro@mithis.com> wrote:

> Yes, that sounds scary to me. OutputSurfaces come and go, and the scheduler
>> outlives them all. I don't think scheduler should be tightly coupled to
>> output surface like that, we should keep some encapsulation between them.
>>
>
> Hi Dana,
>
> If I understand correctly, the scheduler already needs to intimately
> understand
> the state of the OutputSurface so that it schedules the right actions. If
> that
> is correct, then I think it /should/ be okay in the end. I'd obviously like
> feedback from Brian (who suggested this idea) about if this is going in the
> right direction.
>
> Obviously we want tests to make sure that the express the output surface
> life
> cycles and makes sure the scheduler does the right thing.
>
> Do you have a suggestion of a better way to do this?
>

So I have two concerns here, one is wrt scaling the codebase, and one is
wrt pointers.

Tightly coupling OutputSurface to scheduler puts a new dependency from
cc/scheduler/ to cc/output/Stuff (other than BeginFrameArgs which is a
simple struct) that didn't exist before, and now requires scheduler tests
to make OutputSurfaces and manipulate them to test the scheduler, and
changes in one component are tied to changes in the other. It's a bit more
concise but it doesn't scale well. If we can add an interface or something
that would be better.

Storing a pointer on Scheduler sounds scary at first because they can go
away, but maybe it is less scary as I think about it. It's similar to the
lost context bool. I was worried it would add a whole lot of complexity to
the scheduler to deal with the pointer being changed, but it's probably
about the same as that bool.

One other possibility could be a struct of state that the OutputSurface can
export and the Scheduler can hold onto that instead. I'm not sure if that
buys anything nice for tests or scheduler internals, but it's a thought.

I think as long as we're not including "output_surface.h" into the
scheduler code that any solution will be reasonable.

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

[piman, 2014-09-18 23:28:46]

On 2014/09/18 23:17:38, danakj wrote:
> On Wed, Sep 17, 2014 at 5:41 AM, <mailto:mithro@mithis.com> wrote:
> 
> > Yes, that sounds scary to me. OutputSurfaces come and go, and the scheduler
> >> outlives them all. I don't think scheduler should be tightly coupled to
> >> output surface like that, we should keep some encapsulation between them.
> >>
> >
> > Hi Dana,
> >
> > If I understand correctly, the scheduler already needs to intimately
> > understand
> > the state of the OutputSurface so that it schedules the right actions. If
> > that
> > is correct, then I think it /should/ be okay in the end. I'd obviously like
> > feedback from Brian (who suggested this idea) about if this is going in the
> > right direction.
> >
> > Obviously we want tests to make sure that the express the output surface
> > life
> > cycles and makes sure the scheduler does the right thing.
> >
> > Do you have a suggestion of a better way to do this?
> >
> 
> So I have two concerns here, one is wrt scaling the codebase, and one is
> wrt pointers.
> 
> Tightly coupling OutputSurface to scheduler puts a new dependency from
> cc/scheduler/ to cc/output/Stuff (other than BeginFrameArgs which is a
> simple struct) that didn't exist before, and now requires scheduler tests
> to make OutputSurfaces and manipulate them to test the scheduler, and
> changes in one component are tied to changes in the other. It's a bit more
> concise but it doesn't scale well. If we can add an interface or something
> that would be better.
> 
> Storing a pointer on Scheduler sounds scary at first because they can go
> away, but maybe it is less scary as I think about it. It's similar to the
> lost context bool. I was worried it would add a whole lot of complexity to
> the scheduler to deal with the pointer being changed, but it's probably
> about the same as that bool.
> 
> One other possibility could be a struct of state that the OutputSurface can
> export and the Scheduler can hold onto that instead. I'm not sure if that
> buys anything nice for tests or scheduler internals, but it's a thought.
> 
> I think as long as we're not including "output_surface.h" into the
> scheduler code that any solution will be reasonable.

Agreed... OutputSurface should never have been made a concrete class... We have
many implementation of OutputSurface, and I don't like the idea of cc relying on
the "state" of the output surface.

BTW, cc can't depend on ui/compositor.

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

[brianderson, 2014-09-19 00:22:49]

Sounds like there are two options:

1) If we are going to keep the OutputSurface as the conceptual BeginFrame
source, we should have it inherit from a pure virtual BeginFrameSource (or at
least one that has stub methods implemented as NOTREACHED). Ideally only a
subset of OutputSurfaces would inherit from BeginFrameSource, but I think we've
lost type information by the time we get the OutputSurface on the impl thread.

2) We could decouple the BeginFrame message from the OutputSurface and have the
ViewMsg_BeginFrame go straight to the cc::Scheduler. We don't necessarily lose
the ability to receive BeginFrames when we lose the OutputSurface, so maybe I'm
trying to force something that makes sense on the surface, but doesn't really.

[danakj, 2014-09-19 00:30:48]

On Thu, Sep 18, 2014 at 8:22 PM, <brianderson@chromium.org> wrote:

> Sounds like there are two options:
>
> 1) If we are going to keep the OutputSurface as the conceptual BeginFrame
> source, we should have it inherit from a pure virtual BeginFrameSource (or
> at
> least one that has stub methods implemented as NOTREACHED). Ideally only a
> subset of OutputSurfaces would inherit from BeginFrameSource, but I think
> we've
>

Which subset? How come?


> lost type information by the time we get the OutputSurface on the impl
> thread.
>
> 2) We could decouple the BeginFrame message from the OutputSurface and
> have the
> ViewMsg_BeginFrame go straight to the cc::Scheduler. We don't necessarily
> lose
> the ability to receive BeginFrames when we lose the OutputSurface, so
> maybe I'm
> trying to force something that makes sense on the surface, but doesn't
> really.
>

When surface creation is properly asynchronous, we may be waiting for a
surface to come back while the browser is doing frames and giving us
deadlines. Is that what you mean?

OutputSurface is supposed to be an interface for dependency injection from
content/, for cc to use things in content/ to send/receive IPCs
essentially. Of course it is not that now.

And that sounds a bit to me like what you described here wrt scheduling at
least. If we were to make a (few) new pure virtual interface(s) and just
delete OutputSurface that may be a way to restore sanity to the universe.

If BeginFrame stuff doesn't get "lost" like GL upload/drawing stuff does,
maybe it makes a lot of sense to provide that through a separate virtual
interface entirely.

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

[mithro-old, 2014-09-19 02:57:44]

I guess I'm arguing against my own CL here :P

I'd like to strongly support the idea of having separate interface(s) for
BeginFrame and VSync information rather than using the OutputSurface.
(However, I have no idea about the work involved in creating such a cross
process boundary interface.)

I believe if we were to design the BeginFrame+VSync messaging from scratch
we wouldn't attach it to the concept of an output surface and the only
reason we are considering it now is because these concepts have already
been added to the OutputSurface and it is the path of "least work".

Divorcing these concepts from the OutputSurface lifecycle seems like a good
idea to improve future flexibility too. As Brian indicated there is a
distinct possibility that we could want to send BeginFrame messages (and
maybe vsync info too) even when the OutputSurface is lost.

Does that make sense, are people happy with moving forward in that
direction?

Tim


On 19 September 2014 10:30, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Sep 18, 2014 at 8:22 PM, <brianderson@chromium.org> wrote:
>
>> Sounds like there are two options:
>>
>> 1) If we are going to keep the OutputSurface as the conceptual BeginFrame
>> source, we should have it inherit from a pure virtual BeginFrameSource
>> (or at
>> least one that has stub methods implemented as NOTREACHED). Ideally only a
>> subset of OutputSurfaces would inherit from BeginFrameSource, but I think
>> we've
>>
>
> Which subset? How come?
>
>
>> lost type information by the time we get the OutputSurface on the impl
>> thread.
>>
>> 2) We could decouple the BeginFrame message from the OutputSurface and
>> have the
>> ViewMsg_BeginFrame go straight to the cc::Scheduler. We don't necessarily
>> lose
>> the ability to receive BeginFrames when we lose the OutputSurface, so
>> maybe I'm
>> trying to force something that makes sense on the surface, but doesn't
>> really.
>>
>
> When surface creation is properly asynchronous, we may be waiting for a
> surface to come back while the browser is doing frames and giving us
> deadlines. Is that what you mean?
>
> OutputSurface is supposed to be an interface for dependency injection from
> content/, for cc to use things in content/ to send/receive IPCs
> essentially. Of course it is not that now.
>
> And that sounds a bit to me like what you described here wrt scheduling at
> least. If we were to make a (few) new pure virtual interface(s) and just
> delete OutputSurface that may be a way to restore sanity to the universe.
>
> If BeginFrame stuff doesn't get "lost" like GL upload/drawing stuff does,
> maybe it makes a lot of sense to provide that through a separate virtual
> interface entirely.
>

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

[danakj, 2014-09-19 17:11:58]

On Thu, Sep 18, 2014 at 10:57 PM, Tim Ansell <mithro@mithis.com> wrote:

> I guess I'm arguing against my own CL here :P
>
> I'd like to strongly support the idea of having separate interface(s) for
> BeginFrame and VSync information rather than using the OutputSurface.
> (However, I have no idea about the work involved in creating such a cross
> process boundary interface.)
>
> I believe if we were to design the BeginFrame+VSync messaging from scratch
> we wouldn't attach it to the concept of an output surface and the only
> reason we are considering it now is because these concepts have already
> been added to the OutputSurface and it is the path of "least work".
>
> Divorcing these concepts from the OutputSurface lifecycle seems like a
> good idea to improve future flexibility too. As Brian indicated there is a
> distinct possibility that we could want to send BeginFrame messages (and
> maybe vsync info too) even when the OutputSurface is lost.
>
> Does that make sense, are people happy with moving forward in that
> direction?
>

Yes, this sounds good. I can see many benefits of this to our codebase,
including maintainability, simple clear lifetime and ownership semantics,
and more independent components within cc.


>
> Tim
>
>
> On 19 September 2014 10:30, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Thu, Sep 18, 2014 at 8:22 PM, <brianderson@chromium.org> wrote:
>>
>>> Sounds like there are two options:
>>>
>>> 1) If we are going to keep the OutputSurface as the conceptual BeginFrame
>>> source, we should have it inherit from a pure virtual BeginFrameSource
>>> (or at
>>> least one that has stub methods implemented as NOTREACHED). Ideally only
>>> a
>>> subset of OutputSurfaces would inherit from BeginFrameSource, but I
>>> think we've
>>>
>>
>> Which subset? How come?
>>
>>
>>> lost type information by the time we get the OutputSurface on the impl
>>> thread.
>>>
>>> 2) We could decouple the BeginFrame message from the OutputSurface and
>>> have the
>>> ViewMsg_BeginFrame go straight to the cc::Scheduler. We don't
>>> necessarily lose
>>> the ability to receive BeginFrames when we lose the OutputSurface, so
>>> maybe I'm
>>> trying to force something that makes sense on the surface, but doesn't
>>> really.
>>>
>>
>> When surface creation is properly asynchronous, we may be waiting for a
>> surface to come back while the browser is doing frames and giving us
>> deadlines. Is that what you mean?
>>
>> OutputSurface is supposed to be an interface for dependency injection
>> from content/, for cc to use things in content/ to send/receive IPCs
>> essentially. Of course it is not that now.
>>
>> And that sounds a bit to me like what you described here wrt scheduling
>> at least. If we were to make a (few) new pure virtual interface(s) and just
>> delete OutputSurface that may be a way to restore sanity to the universe.
>>
>> If BeginFrame stuff doesn't get "lost" like GL upload/drawing stuff does,
>> maybe it makes a lot of sense to provide that through a separate virtual
>> interface entirely.
>>
>
>

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

[brianderson, 2014-09-20 00:49:53]

A third option might be to add a common ExternalBeginFrameSource class that can
optionally be returned by each implementation of the OutputSurface using
composition rather than inheritance.

Or four: have the newer longer-lifetime method of /content dependency injection
optionally return the ExternalBeginFrameSource class.

> Which subset? How come?

Currently, only Android's Renderer OutputSurface supports the message. After the
unified BeginFrame patch, all Renderer's OutputSurfaces will support this.

The Browser's OutputSurface will not have this on all platforms, however. Only
Android and Mac get vsync callbacks directly from the OS. For other platforms,
we will still rely on the SyntheticBeginFrameSource in the Browser's compositor
scheduler.

> When surface creation is properly asynchronous, we may be waiting for a
> surface to come back while the browser is doing frames and giving us
> deadlines. Is that what you mean?

I hadn't thought of that, but it would be a good use case if we can get there. I
was just thinking that we don't necessarily lose our BeginFrame source (the
Browser) if we lose the OutputSurface.

> If BeginFrame stuff doesn't get "lost" like GL upload/drawing stuff does,
> maybe it makes a lot of sense to provide that through a separate virtual
> interface entirely.

I don't understand the overall cost/benefits well enough to know the best path
to take here. I'm not sure how many other concepts would benefit being divorced
from the OutputSurface life cycle.

[danakj, 2014-09-22 15:06:54]

My first thought on what this would look like is something like this.

class cc::ExternalBeginFrameSource {
  virtual void SetClient(cc::EBFSClient* client) = 0;
};

class cc::EBFSClient {
  virtual void ThingsThatGiveBeginFramesToTheScheduler(BeginFrames) = 0;
};

cc::Scheduler owns scoped_ptr<cc:ExternalBeginFrameSource> and inherits
EBFSClient;

cc::LayerTreeHost::SetLayerTreeHostClientReady(scoped_ptr<cc::ExternalBeginFrameSource>);
- This is passed thru proxy to the scheduler who owns it until the
scheduler is destroyed.
- The scheduler calls SetClient() to itself.
- This method is a nice one to call because it's the thing that starts the
scheduler running. If we never call it, we don't need to ever create an
ExternBeginFrameSource.

content::RenderWidgetCompositor::setSurfaceReady() will create a class that
implements cc::ExternalBeginFrameSource and pass it to cc::LTH.

On Fri, Sep 19, 2014 at 8:49 PM, <brianderson@chromium.org> wrote:

> A third option might be to add a common ExternalBeginFrameSource class
> that can
> optionally be returned by each implementation of the OutputSurface using
> composition rather than inheritance.
>

Then you have to add code to OutputSurface and there's many impls of
OutputSurface so you'd duplicate or force inheritance up in the output
surface impls.


>
> Or four: have the newer longer-lifetime method of /content dependency
> injection
> optionally return the ExternalBeginFrameSource class.
>

I thought about this also, but I don't think it buys anything because we
never replace the ExternalBeginFrameSource instance, it's one time only so
just make it the thing we give to cc.


>
>  Which subset? How come?
>>
>
> Currently, only Android's Renderer OutputSurface supports the message.
> After the
> unified BeginFrame patch, all Renderer's OutputSurfaces will support this.
>
> The Browser's OutputSurface will not have this on all platforms, however.
> Only
> Android and Mac get vsync callbacks directly from the OS. For other
> platforms,
> we will still rely on the SyntheticBeginFrameSource in the Browser's
> compositor
> scheduler.


Would the SyntheticBeginFrameSource not become an impl of the
ExternalBeginFrameSource?

 When surface creation is properly asynchronous, we may be waiting for a
>> surface to come back while the browser is doing frames and giving us
>> deadlines. Is that what you mean?
>>
>
> I hadn't thought of that, but it would be a good use case if we can get
> there. I
> was just thinking that we don't necessarily lose our BeginFrame source (the
> Browser) if we lose the OutputSurface.
>
>  If BeginFrame stuff doesn't get "lost" like GL upload/drawing stuff does,
>> maybe it makes a lot of sense to provide that through a separate virtual
>> interface entirely.
>>
>
> I don't understand the overall cost/benefits well enough to know the best
> path
> to take here. I'm not sure how many other concepts would benefit being
> divorced
> from the OutputSurface life cycle.
>

I really like that startup and shutdown is much more clear for this object,
and that we divorce things from OutputSurface.

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

[mithro-old, 2014-09-25 07:05:28]

Just as a FYI. This change has been abandoned and discussion taken to the
following bug https://code.google.com/p/chromium/issues/detail?id=416760

Tim