Added the Vulkan Context Provider implementation.

Added the Vulkan Context Provider implementation.

R=piman@chromium.org
BUG=582564
582558 582554
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b52ecd969eecd55ed48ab9512232dd3eb7b8c240
Cr-Commit-Position: refs/heads/master@{#388589}

[David Yen, 2016-04-16 01:28:31]

Description was changed from

==========
Added Vulkan Context Provider implementations.

R=piman@chromium.org
BUG=582564 582558 582554
==========

to

==========
Added Vulkan Context Provider implementations.

R=piman@chromium.org
BUG=582564 582558 582554
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[David Yen, 2016-04-16 01:32:39]

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_in_process...
File cc/output/vulkan_in_process_context_provider.cc (right):

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_in_process...
cc/output/vulkan_in_process_context_provider.cc:20: static bool
vulkan_initialized = false;
Not sure where the best place to call this is yet, this should be good enough
for now?

[piman, 2016-04-19 01:36:43]

https://codereview.chromium.org/1892303003/diff/1/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/1892303003/diff/1/cc/output/output_surface.h#...
cc/output/output_surface.h:64: bool use_vulkan);
What's the reason for use_vulkan, it's not used except for a DCHECK?

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_derived_co...
File cc/output/vulkan_derived_context_provider.h (right):

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_derived_co...
cc/output/vulkan_derived_context_provider.h:20: class CC_EXPORT
VulkanDerivedContextProvider : public VulkanContextProvider {
I don't think this class is that useful. cc won't call SwapBuffers directly on
the ContextProvider, it will go through the OutputSurface. So you could have the
VulkanBrowserCompositorOutputSurface own the VulkanSurface, and call SwapBuffers
on it directly.

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_in_process...
File cc/output/vulkan_in_process_context_provider.cc (right):

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_in_process...
cc/output/vulkan_in_process_context_provider.cc:20: static bool
vulkan_initialized = false;
On 2016/04/16 01:32:38, David Yen wrote:
> Not sure where the best place to call this is yet, this should be good enough
> for now?

BrowserMainLoop::BrowserThreadsStarted is probably the right place for it - next
to BrowserGpuChannelHostFactory::Initialize

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.cc:234: #endif
create_gpu_output_surface really means "don't fallback to software", maybe let's
keep it that way? We can have a separate use_vulkan bool, that we'd check on
l.236 (if using vulkan, no need to start a GPU process).

Separately, it would be good to leave vulkan as a run-time option, so that if
it's compiled in, we can still compare GL vs Vulkan (may be useful for
benchmarking). So it would look like
bool use_vulkan =
base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableVulkan) &&
SharedVulkanContextProvider();

It also means the rest of the code doesn't need to have all the ifdefs.

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.h (right):

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.h:105: #if
defined(ENABLE_VULKAN)
Trying to find a way to limit the amount of #ifdef we need... in terms of
interface, VulkanInProcessContextProvider doesn't need Vulkan types explicitly,
maybe we can remove the ifdef here, and limit it to the implementation of
VulkanInProcessContextProvider (i.e. have Initialize fail)

[David Yen, 2016-04-19 22:20:28]

https://codereview.chromium.org/1892303003/diff/1/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/1892303003/diff/1/cc/output/output_surface.h#...
cc/output/output_surface.h:64: bool use_vulkan);
On 2016/04/19 01:36:42, piman wrote:
> What's the reason for use_vulkan, it's not used except for a DCHECK?

This was so I could override the constructor, it conflicts with the
OutputSurface(ContextProvider) constructor because it is the base class.

Perhaps a better way is to just call the constructor with all the arguments and
pass in nullptr for the other arguments.

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_derived_co...
File cc/output/vulkan_derived_context_provider.h (right):

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_derived_co...
cc/output/vulkan_derived_context_provider.h:20: class CC_EXPORT
VulkanDerivedContextProvider : public VulkanContextProvider {
On 2016/04/19 01:36:42, piman wrote:
> I don't think this class is that useful. cc won't call SwapBuffers directly on
> the ContextProvider, it will go through the OutputSurface. So you could have
the
> VulkanBrowserCompositorOutputSurface own the VulkanSurface, and call
SwapBuffers
> on it directly.

Ok. I have removed VulkanDerivedContextProvider and also the SwapBuffers()
function from VulkanContextProvider.

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_in_process...
File cc/output/vulkan_in_process_context_provider.cc (right):

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_in_process...
cc/output/vulkan_in_process_context_provider.cc:20: static bool
vulkan_initialized = false;
On 2016/04/19 01:36:42, piman wrote:
> On 2016/04/16 01:32:38, David Yen wrote:
> > Not sure where the best place to call this is yet, this should be good
enough
> > for now?
> 
> BrowserMainLoop::BrowserThreadsStarted is probably the right place for it -
next
> to BrowserGpuChannelHostFactory::Initialize

Done. I also just removed the InitializeOneOff() function from VulkanSurface, I
think just calling InitializeVulkan from vulkan_implementation should be good
enough. I also added a function to check whether or not the initialization
succeeded "gpu::VulkanSupported()" in vulkan_implementation.

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.cc:234: #endif
On 2016/04/19 01:36:42, piman wrote:
> create_gpu_output_surface really means "don't fallback to software", maybe
let's
> keep it that way? We can have a separate use_vulkan bool, that we'd check on
> l.236 (if using vulkan, no need to start a GPU process).
> 
> Separately, it would be good to leave vulkan as a run-time option, so that if
> it's compiled in, we can still compare GL vs Vulkan (may be useful for
> benchmarking). So it would look like
> bool use_vulkan =
> base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableVulkan) &&
> SharedVulkanContextProvider();
> 
> It also means the rest of the code doesn't need to have all the ifdefs.

Seems we need to check the switch everytime SharedVulkanContextProvider() is
going to be used, perhaps just build it into there?

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.h (right):

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.h:105: #if
defined(ENABLE_VULKAN)
On 2016/04/19 01:36:42, piman wrote:
> Trying to find a way to limit the amount of #ifdef we need... in terms of
> interface, VulkanInProcessContextProvider doesn't need Vulkan types
explicitly,
> maybe we can remove the ifdef here, and limit it to the implementation of
> VulkanInProcessContextProvider (i.e. have Initialize fail)

Done. The only thing is we still have to #ifdef out
VulkanBrowserCompositorOutputSurface, so there is a bit of a mismatch going on
there. A little strange but it works out I think.

If we are going to compile in VulkanInProcessContextProvider, perhaps we should
also remove the #ifdefs in OutputSurface? We discussed that before and decided
not to so that we wouldn't have to compile in that file. But since we are
already including it I think we might as well. What do you think?

[piman, 2016-04-19 23:29:57]

https://codereview.chromium.org/1892303003/diff/1/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/1892303003/diff/1/cc/output/output_surface.h#...
cc/output/output_surface.h:64: bool use_vulkan);
On 2016/04/19 22:20:27, David Yen wrote:
> On 2016/04/19 01:36:42, piman wrote:
> > What's the reason for use_vulkan, it's not used except for a DCHECK?
> 
> This was so I could override the constructor, it conflicts with the
> OutputSurface(ContextProvider) constructor because it is the base class.
> 
> Perhaps a better way is to just call the constructor with all the arguments
and
> pass in nullptr for the other arguments.

I think you're running into a scoped_refptr limitation, should be fixed with
https://codereview.chromium.org/1904603002/

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.h (right):

https://codereview.chromium.org/1892303003/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.h:105: #if
defined(ENABLE_VULKAN)
On 2016/04/19 22:20:28, David Yen wrote:
> On 2016/04/19 01:36:42, piman wrote:
> > Trying to find a way to limit the amount of #ifdef we need... in terms of
> > interface, VulkanInProcessContextProvider doesn't need Vulkan types
> explicitly,
> > maybe we can remove the ifdef here, and limit it to the implementation of
> > VulkanInProcessContextProvider (i.e. have Initialize fail)
> 
> Done. The only thing is we still have to #ifdef out
> VulkanBrowserCompositorOutputSurface, so there is a bit of a mismatch going on
> there. A little strange but it works out I think.
> 
> If we are going to compile in VulkanInProcessContextProvider, perhaps we
should
> also remove the #ifdefs in OutputSurface? We discussed that before and decided
> not to so that we wouldn't have to compile in that file. But since we are
> already including it I think we might as well. What do you think?

Yeah, I think I agree. I'm thinking that eventually we'll want to dynamically
load the loader anyway, at which point everything can compile, even on platforms
where Vulkan is not supported. We may want to keep a #ifdef in the lower levels
to avoid bringing too much code, but if we can avoid it in the higher levels,
it's probably more readable.

[piman, 2016-04-19 23:42:03]

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_in_process...
File cc/output/vulkan_in_process_context_provider.cc (right):

https://codereview.chromium.org/1892303003/diff/1/cc/output/vulkan_in_process...
cc/output/vulkan_in_process_context_provider.cc:20: static bool
vulkan_initialized = false;
On 2016/04/19 22:20:28, David Yen wrote:
> On 2016/04/19 01:36:42, piman wrote:
> > On 2016/04/16 01:32:38, David Yen wrote:
> > > Not sure where the best place to call this is yet, this should be good
> enough
> > > for now?
> > 
> > BrowserMainLoop::BrowserThreadsStarted is probably the right place for it -
> next
> > to BrowserGpuChannelHostFactory::Initialize
> 
> Done. I also just removed the InitializeOneOff() function from VulkanSurface,
I
> think just calling InitializeVulkan from vulkan_implementation should be good
> enough. I also added a function to check whether or not the initialization
> succeeded "gpu::VulkanSupported()" in vulkan_implementation.

Yeah, it sounds better.

https://codereview.chromium.org/1892303003/diff/120001/cc/surfaces/surface_di...
File cc/surfaces/surface_display_output_surface.h (right):

https://codereview.chromium.org/1892303003/diff/120001/cc/surfaces/surface_di...
cc/surfaces/surface_display_output_surface.h:33: #if defined(ENABLE_VULKAN)
Thinking, this may be better without ifdef too.

https://codereview.chromium.org/1892303003/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1892303003/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1226: #endif
nit: move to before ImageTransportFactory::Initialize() (around l.1194), in case
GpuProcessTransportFactory wants to initialize some Vulkan things early, later.

https://codereview.chromium.org/1892303003/diff/120001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.h (right):

https://codereview.chromium.org/1892303003/diff/120001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.h:83: #if
defined(ENABLE_VULKAN)
ditto, works without ifdef?

https://codereview.chromium.org/1892303003/diff/120001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1892303003/diff/120001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:397: #endif
I would really prefer not to interleave compile-time #ifdefs and run-time ifs,
it makes the code really hard to read.

Maybe the surface = std::move(vulkan_surface); bit can move to an else clause on
l.391, and this else can instead be a if (!surface), outside the #ifdef?

https://codereview.chromium.org/1892303003/diff/120001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:470: #endif
Ditto. If we remove the #ifdef for the SurfaceDisplayOutputSurface constructor,
I don't think you need the #ifdef here.

https://codereview.chromium.org/1892303003/diff/120001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1892303003/diff/120001/content/renderer/rende...
content/renderer/render_widget.cc:782: #if defined(ENABLE_VULKAN)
I think you don't need this ifdef any more.

[David Yen, 2016-04-20 17:37:38]

https://codereview.chromium.org/1892303003/diff/120001/cc/surfaces/surface_di...
File cc/surfaces/surface_display_output_surface.h (right):

https://codereview.chromium.org/1892303003/diff/120001/cc/surfaces/surface_di...
cc/surfaces/surface_display_output_surface.h:33: #if defined(ENABLE_VULKAN)
On 2016/04/19 23:42:02, piman wrote:
> Thinking, this may be better without ifdef too.

Done.

https://codereview.chromium.org/1892303003/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1892303003/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1226: #endif
On 2016/04/19 23:42:02, piman wrote:
> nit: move to before ImageTransportFactory::Initialize() (around l.1194), in
case
> GpuProcessTransportFactory wants to initialize some Vulkan things early,
later.

Done.

https://codereview.chromium.org/1892303003/diff/120001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1892303003/diff/120001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:397: #endif
On 2016/04/19 23:42:02, piman wrote:
> I would really prefer not to interleave compile-time #ifdefs and run-time ifs,
> it makes the code really hard to read.
> 
> Maybe the surface = std::move(vulkan_surface); bit can move to an else clause
on
> l.391, and this else can instead be a if (!surface), outside the #ifdef?

Done.

https://codereview.chromium.org/1892303003/diff/120001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:470: #endif
On 2016/04/19 23:42:02, piman wrote:
> Ditto. If we remove the #ifdef for the SurfaceDisplayOutputSurface
constructor,
> I don't think you need the #ifdef here.

Done.

[piman, 2016-04-20 21:08:47]

lgtm

https://codereview.chromium.org/1892303003/diff/250001/cc/output/vulkan_in_pr...
File cc/output/vulkan_in_process_context_provider.h (right):

https://codereview.chromium.org/1892303003/diff/250001/cc/output/vulkan_in_pr...
cc/output/vulkan_in_process_context_provider.h:20: : NON_EXPORTED_BASE(public
VulkanContextProvider) {
nit: let's export VulkanContextProvider too

[David Yen, 2016-04-20 21:12:16]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2016-04-20 21:12:16]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1892303003/#ps270001
(title: "Export VulkanContextProvider")

[David Yen, 2016-04-20 21:12:24]

https://codereview.chromium.org/1892303003/diff/250001/cc/output/vulkan_in_pr...
File cc/output/vulkan_in_process_context_provider.h (right):

https://codereview.chromium.org/1892303003/diff/250001/cc/output/vulkan_in_pr...
cc/output/vulkan_in_process_context_provider.h:20: : NON_EXPORTED_BASE(public
VulkanContextProvider) {
On 2016/04/20 21:08:46, piman wrote:
> nit: let's export VulkanContextProvider too

Done.

[commit-bot: I haz the power, 2016-04-20 21:12:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892303003/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892303003/270001

[David Yen, 2016-04-20 21:37:52]

Description was changed from

==========
Added Vulkan Context Provider implementations.

R=piman@chromium.org
BUG=582564 582558 582554
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added the Vulkan Context Provider implementation.

R=piman@chromium.org
BUG=582564 582558 582554
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-20 23:23:56]

Description was changed from

==========
Added the Vulkan Context Provider implementation.

R=piman@chromium.org
BUG=582564 582558 582554
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added the Vulkan Context Provider implementation.

R=piman@chromium.org
BUG=582564 582558 582554
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-20 23:23:57]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2016-04-22 19:26:57]

Description was changed from

==========
Added the Vulkan Context Provider implementation.

R=piman@chromium.org
BUG=582564 582558 582554
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added the Vulkan Context Provider implementation.

R=piman@chromium.org
BUG=582564 582558 582554
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b52ecd969eecd55ed48ab9512232dd3eb7b8c240
Cr-Commit-Position: refs/heads/master@{#388589}
==========

[commit-bot: I haz the power, 2016-04-22 19:26:58]

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