Adding support for VSyncProvider to the software drawing path

Adding support for VSyncProvider to the software drawing path

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233985

[dnicoara, 2013-11-05 22:01:14]

Please take a look. I hope this is close to what you were suggesting.

I'm also thinking that I should split up this CL in 2-3 others (CC change,
SoftwareBrowserCompositorOutputSurface change and Ozone change). What do you
think?

[piman, 2013-11-05 23:32:26]

I think the approach is reasonable overall. There's a few things inline.
Feel free to split off the Ozone parts if you think that's easier.

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
File content/browser/aura/software_browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
content/browser/aura/software_browser_compositor_output_surface.cc:39: };
So this class is really only there to forward the call to the OutputSurface and
the Compositor, is it really necessary?
The BrowserCompositorOutputSurface does it all directly in there, I imagined we
could do the same. Possibly, even better, share the code between the 2 version,
maybe by sharing a base class?

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
content/browser/aura/software_browser_compositor_output_surface.cc:55: const
base::TimeDelta interval) {
So, this should be called on the compositor already - the VSyncProvider has to
leave on that thread, right? (otherwise we'd need locks I think). If that's the
case, VSyncListener can simply be RefCounted instead of RefCountedThreadSafe.

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
content/browser/aura/software_browser_compositor_output_surface.cc:57:
surface_->OnVSyncParametersChanged(timebase, interval);
What guarantee do we have that surface_ is still valid? In particular,
VSyncListener is refcounted whereas SoftwareBrowserCompositorOutputSurface is
not, and I don't think there's anything obvious that guarantees the
VSyncProvider won't call the callback after
SoftwareBrowserCompositorOutputSurface is destroyed.

Maybe we need to bind a WeakPtr<VSyncListener> into the VsyncProvider callback,
instead of making it refcounted?

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
content/browser/aura/software_browser_compositor_output_surface.cc:78:
~SoftwareBrowserCompositorOutputSurface() {
nit: this looks indented wrong. git cl format?

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
File content/browser/aura/software_output_device_ozone.cc (right):

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
content/browser/aura/software_output_device_ozone.cc:34: realized_widget_ =
factory->RealizeAcceleratedWidget(compositor_->widget());
Could this be done at construction time?

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
content/browser/aura/software_output_device_ozone.cc:70: realized_widget_);
Do we expect different VSyncProvider across calls? Should we cache it at
construction time?

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
File content/browser/aura/software_output_device_ozone.h (right):

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
content/browser/aura/software_output_device_ozone.h:34: gfx::AcceleratedWidget
realized_widget_;
new field needs initialization

[dnicoara, 2013-11-06 18:11:47]

I'll split the Ozone part in a different CL since I need to make sure we can do
some of the optimizations you've suggested.

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
File content/browser/aura/software_browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/57883007/diff/20001/content/browser/aura/soft...
content/browser/aura/software_browser_compositor_output_surface.cc:39: };
On 2013/11/05 23:32:27, piman wrote:
> So this class is really only there to forward the call to the OutputSurface
and
> the Compositor, is it really necessary?
> The BrowserCompositorOutputSurface does it all directly in there, I imagined
we
> could do the same. Possibly, even better, share the code between the 2
version,
> maybe by sharing a base class?


Yes, this would be similar to the BrowserCompositorOutputSurfaceProxy. I was
reluctant to inherit from BrowserCompositorOutputSurface since it has some GPU
specific code.

I've been reading the code a bit to better understand how the accelerated path
works. On the accelerated path the GPU process would send a message to update
the VSync parameters which would then propagate to the Compositor and
OutputSurface. On the software path there isn't any such process, so I'm trying
to make that call directly from SoftwareCompositorOutputSurface.

Based on your feedback, I think the best course is to refactor
BrowserCompositorOutputSurface a bit such that
SoftwareBrowserCompositorOutputSurface can inherit it. Then I can use the
output_surface_map_ in GpuProcessTransportFactory to keep track of the allocated
BCOS. Then the VSyncListener wouldn't be needed and SBCOS can update the VSync
parameters by directly calling BCOSProxy. This way, it would also solve the
following comments you had.

Does this sound reasonable?

Most of the refactoring looks easy. I'm thinking of leaving BCOS as a base class
and have a GpuBrowserCompositorOutputSurface to provide the GPU specific
implementation. Though I'm not sure if the Reflectors make sense in the software
path since they seem to imply an accelerated path.

Adding danakj@ for reviewers to get more insight, since she left a TODO on
adding the inheritance.

[piman, 2013-11-06 21:31:27]

On Wed, Nov 6, 2013 at 10:11 AM, <dnicoara@chromium.org> wrote:

> I'll split the Ozone part in a different CL since I need to make sure we
> can do
> some of the optimizations you've suggested.
>
>
>
> https://codereview.chromium.org/57883007/diff/20001/
> content/browser/aura/software_browser_compositor_output_surface.cc
> File content/browser/aura/software_browser_compositor_output_surface.cc
> (right):
>
> https://codereview.chromium.org/57883007/diff/20001/
> content/browser/aura/software_browser_compositor_output_
> surface.cc#newcode39
> content/browser/aura/software_browser_compositor_output_surface.cc:39:
> };
> On 2013/11/05 23:32:27, piman wrote:
>
>> So this class is really only there to forward the call to the
>>
> OutputSurface and
>
>> the Compositor, is it really necessary?
>> The BrowserCompositorOutputSurface does it all directly in there, I
>>
> imagined we
>
>> could do the same. Possibly, even better, share the code between the 2
>>
> version,
>
>> maybe by sharing a base class?
>>
>
>
> Yes, this would be similar to the BrowserCompositorOutputSurfaceProxy. I
> was reluctant to inherit from BrowserCompositorOutputSurface since it
> has some GPU specific code.
>
> I've been reading the code a bit to better understand how the
> accelerated path works. On the accelerated path the GPU process would
> send a message to update the VSync parameters which would then propagate
> to the Compositor and OutputSurface. On the software path there isn't
> any such process, so I'm trying to make that call directly from
> SoftwareCompositorOutputSurface.
>
> Based on your feedback, I think the best course is to refactor
> BrowserCompositorOutputSurface a bit such that
> SoftwareBrowserCompositorOutputSurface can inherit it. Then I can use
> the output_surface_map_ in GpuProcessTransportFactory to keep track of
> the allocated BCOS. Then the VSyncListener wouldn't be needed and SBCOS
> can update the VSync parameters by directly calling BCOSProxy. This way,
> it would also solve the following comments you had.
>
> Does this sound reasonable?
>

BCOSProxy is really to bridge the gap between the IO thread and the
compositor impl thread. I'm not sure you need to hook up to it, but maybe.


>
> Most of the refactoring looks easy. I'm thinking of leaving BCOS as a
> base class and have a GpuBrowserCompositorOutputSurface to provide the
> GPU specific implementation. Though I'm not sure if the Reflectors make
> sense in the software path since they seem to imply an accelerated path.
>

So, ideally the Reflector would be supported in software too - that's
what's used to do screen mirroring. Conceptually it's not linked to the
hardware path, it's just that it's not implemented yet (because we don't
currently use the software path on Chrome OS). So it wouldn't hurt to at
least have the hook up for it with this refactoring.

>
> Adding danakj@ for reviewers to get more insight, since she left a TODO
> on adding the inheritance.
>
> https://codereview.chromium.org/57883007/
>

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

[dnicoara, 2013-11-06 22:09:36]

Please take a look. I've updated the code to use BrowserCompositorOutputSurface
as a base class and split the GPU logic in another class.

On 2013/11/06 21:31:27, piman wrote:
> On Wed, Nov 6, 2013 at 10:11 AM, <mailto:dnicoara@chromium.org> wrote:
> 
> > I'll split the Ozone part in a different CL since I need to make sure we
> > can do
> > some of the optimizations you've suggested.
> >
> >
> >
> > https://codereview.chromium.org/57883007/diff/20001/
> > content/browser/aura/software_browser_compositor_output_surface.cc
> > File content/browser/aura/software_browser_compositor_output_surface.cc
> > (right):
> >
> > https://codereview.chromium.org/57883007/diff/20001/
> > content/browser/aura/software_browser_compositor_output_
> > surface.cc#newcode39
> > content/browser/aura/software_browser_compositor_output_surface.cc:39:
> > };
> > On 2013/11/05 23:32:27, piman wrote:
> >
> >> So this class is really only there to forward the call to the
> >>
> > OutputSurface and
> >
> >> the Compositor, is it really necessary?
> >> The BrowserCompositorOutputSurface does it all directly in there, I
> >>
> > imagined we
> >
> >> could do the same. Possibly, even better, share the code between the 2
> >>
> > version,
> >
> >> maybe by sharing a base class?
> >>
> >
> >
> > Yes, this would be similar to the BrowserCompositorOutputSurfaceProxy. I
> > was reluctant to inherit from BrowserCompositorOutputSurface since it
> > has some GPU specific code.
> >
> > I've been reading the code a bit to better understand how the
> > accelerated path works. On the accelerated path the GPU process would
> > send a message to update the VSync parameters which would then propagate
> > to the Compositor and OutputSurface. On the software path there isn't
> > any such process, so I'm trying to make that call directly from
> > SoftwareCompositorOutputSurface.
> >
> > Based on your feedback, I think the best course is to refactor
> > BrowserCompositorOutputSurface a bit such that
> > SoftwareBrowserCompositorOutputSurface can inherit it. Then I can use
> > the output_surface_map_ in GpuProcessTransportFactory to keep track of
> > the allocated BCOS. Then the VSyncListener wouldn't be needed and SBCOS
> > can update the VSync parameters by directly calling BCOSProxy. This way,
> > it would also solve the following comments you had.
> >
> > Does this sound reasonable?
> >
> 
> BCOSProxy is really to bridge the gap between the IO thread and the
> compositor impl thread. I'm not sure you need to hook up to it, but maybe.
> 

Maybe I misunderstood your suggestion then. I'm thinking of reusing it since it
is create in a way that it can be passed in to base::Bind. Also it allows us to
keep the update logic in one place.

> 
> >
> > Most of the refactoring looks easy. I'm thinking of leaving BCOS as a
> > base class and have a GpuBrowserCompositorOutputSurface to provide the
> > GPU specific implementation. Though I'm not sure if the Reflectors make
> > sense in the software path since they seem to imply an accelerated path.
> >
> 
> So, ideally the Reflector would be supported in software too - that's
> what's used to do screen mirroring. Conceptually it's not linked to the
> hardware path, it's just that it's not implemented yet (because we don't
> currently use the software path on Chrome OS). So it wouldn't hurt to at
> least have the hook up for it with this refactoring.
> 

I see, thank you, I think I understand the code better.

> >
> > Adding danakj@ for reviewers to get more insight, since she left a TODO
> > on adding the inheritance.
> >
> > https://codereview.chromium.org/57883007/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2013-11-07 00:45:12]

Some nits, otherwise LGTM.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/bro...
File content/browser/aura/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/bro...
content/browser/aura/browser_compositor_output_surface.cc:40:
capabilities_.adjust_deadline_for_parent = false;
nit: Can we share this code between the 2 constructors? An Init function maybe?

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
File content/browser/aura/gpu_browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
content/browser/aura/gpu_browser_compositor_output_surface.cc:24:
DetachFromThread();
This should be mostly orthogonal to HW vs SW, modulo the fact that the SW
implementations either don't work on the thread (X11, Ozone?) or aren't used on
the thread (Windows).
I would leave this in BrowserCompositorOutputSurface, since that's what's noted
NonThreadSafe.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
File content/browser/aura/gpu_browser_compositor_output_surface.h (right):

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
content/browser/aura/gpu_browser_compositor_output_surface.h:17: 
nit: no need for blank line

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
content/browser/aura/gpu_browser_compositor_output_surface.h:31: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
File content/browser/aura/software_browser_compositor_output_surface_unittest.cc
(right):

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
content/browser/aura/software_browser_compositor_output_surface_unittest.cc:27:
int get_call_count() const { return call_count_; }
nit: call_count()

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
content/browser/aura/software_browser_compositor_output_surface_unittest.cc:108:
content::SoftwareBrowserCompositorOutputSurface*
nit: return a scoped_ptr<SoftwareBrowserCompositorOutputSurface> to clearly
indicate ownership.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
content/browser/aura/software_browser_compositor_output_surface_unittest.cc:126:
static_cast<cc::OutputSurface*>(output_surface_.get())->SwapBuffers(&frame);
nit: you shouldn't need the static cast.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
content/browser/aura/software_browser_compositor_output_surface_unittest.cc:141:
static_cast<cc::OutputSurface*>(output_surface_.get())->SwapBuffers(&frame);
nit: Shouldn't need the static_cast either

[dnicoara, 2013-11-07 15:17:41]

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/bro...
File content/browser/aura/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/bro...
content/browser/aura/browser_compositor_output_surface.cc:40:
capabilities_.adjust_deadline_for_parent = false;
On 2013/11/07 00:45:12, piman wrote:
> nit: Can we share this code between the 2 constructors? An Init function
maybe?

Done.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
File content/browser/aura/gpu_browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
content/browser/aura/gpu_browser_compositor_output_surface.cc:24:
DetachFromThread();
On 2013/11/07 00:45:12, piman wrote:
> This should be mostly orthogonal to HW vs SW, modulo the fact that the SW
> implementations either don't work on the thread (X11, Ozone?) or aren't used
on
> the thread (Windows).
> I would leave this in BrowserCompositorOutputSurface, since that's what's
noted
> NonThreadSafe.

Done.

Ozone should work just fine with the thread.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
File content/browser/aura/gpu_browser_compositor_output_surface.h (right):

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
content/browser/aura/gpu_browser_compositor_output_surface.h:17: 
On 2013/11/07 00:45:12, piman wrote:
> nit: no need for blank line

Done.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/gpu...
content/browser/aura/gpu_browser_compositor_output_surface.h:31: };
On 2013/11/07 00:45:12, piman wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
File content/browser/aura/software_browser_compositor_output_surface_unittest.cc
(right):

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
content/browser/aura/software_browser_compositor_output_surface_unittest.cc:27:
int get_call_count() const { return call_count_; }
On 2013/11/07 00:45:12, piman wrote:
> nit: call_count()

Done.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
content/browser/aura/software_browser_compositor_output_surface_unittest.cc:108:
content::SoftwareBrowserCompositorOutputSurface*
On 2013/11/07 00:45:12, piman wrote:
> nit: return a scoped_ptr<SoftwareBrowserCompositorOutputSurface> to clearly
> indicate ownership.

Done.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
content/browser/aura/software_browser_compositor_output_surface_unittest.cc:126:
static_cast<cc::OutputSurface*>(output_surface_.get())->SwapBuffers(&frame);
On 2013/11/07 00:45:12, piman wrote:
> nit: you shouldn't need the static cast.

Done.

https://codereview.chromium.org/57883007/diff/160001/content/browser/aura/sof...
content/browser/aura/software_browser_compositor_output_surface_unittest.cc:141:
static_cast<cc::OutputSurface*>(output_surface_.get())->SwapBuffers(&frame);
On 2013/11/07 00:45:12, piman wrote:
> nit: Shouldn't need the static_cast either

Done.

[piman, 2013-11-07 20:37:15]

One last thing then LGTM

https://codereview.chromium.org/57883007/diff/440001/content/browser/aura/bro...
File content/browser/aura/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/57883007/diff/440001/content/browser/aura/bro...
content/browser/aura/browser_compositor_output_surface.cc:45: Initialize();
nit: DetachFromThread here too - if/when software works with the thread, we'll
need it.

[dnicoara, 2013-11-07 21:03:15]

I've had to add CONTENT_EXPORT to BCOS, SBCOS and BCOSProxy to make the shared
build happy. This change should be fine, right?

Also, I'm not sure which component is which in CC. Would I need to add anyone
else as a reviewer?

Thank you!

https://codereview.chromium.org/57883007/diff/440001/content/browser/aura/bro...
File content/browser/aura/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/57883007/diff/440001/content/browser/aura/bro...
content/browser/aura/browser_compositor_output_surface.cc:45: Initialize();
On 2013/11/07 20:37:16, piman wrote:
> nit: DetachFromThread here too - if/when software works with the thread, we'll
> need it.

Done.

[piman, 2013-11-07 22:03:09]

On Thu, Nov 7, 2013 at 1:03 PM, <dnicoara@chromium.org> wrote:

> I've had to add CONTENT_EXPORT to BCOS, SBCOS and BCOSProxy to make the
> shared
> build happy. This change should be fine, right?
>

Yes.


> Also, I'm not sure which component is which in CC. Would I need to add
> anyone
> else as a reviewer?
>

My review should cover everything.

Antoine


>
> Thank you!
>
>
>
> https://codereview.chromium.org/57883007/diff/440001/
> content/browser/aura/browser_compositor_output_surface.cc
> File content/browser/aura/browser_compositor_output_surface.cc (right):
>
> https://codereview.chromium.org/57883007/diff/440001/
> content/browser/aura/browser_compositor_output_surface.cc#newcode45
> content/browser/aura/browser_compositor_output_surface.cc:45:
> Initialize();
> On 2013/11/07 20:37:16, piman wrote:
>
>> nit: DetachFromThread here too - if/when software works with the
>>
> thread, we'll
>
>> need it.
>>
>
> Done.
>
> https://codereview.chromium.org/57883007/
>

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

[piman, 2013-11-07 22:03:20]

lgtm

[commit-bot: I haz the power, 2013-11-08 14:22:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/57883007/630002

[commit-bot: I haz the power, 2013-11-08 15:03:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-08 15:19:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/57883007/950001

[commit-bot: I haz the power, 2013-11-08 18:15:15]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-11-08 18:18:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/57883007/950001

[commit-bot: I haz the power, 2013-11-08 20:57:39]

Change committed as 233985