Add a command-line flag to enable threaded GPU rasterization.

Add a command-line flag to enable threaded GPU rasterization.

For now a no-op in the renderer.

Enable:
  --enable-threaded-gpu-rasterization

Disable (which takes priority over Enable):
  --disable-threaded-gpu-rasterization

BUG=454500
Committed: https://crrev.com/9084b34a9c65689297314f65c389e063556df88e
Cr-Commit-Position: refs/heads/master@{#314418}

[vmiura, 2015-02-02 21:16:16]

Patchset #1 (id:1) has been deleted

[vmiura, 2015-02-02 21:36:47]

vmiura@chromium.org changed reviewers:
+ danakj@chromium.org, hendrikw@chromium.org, piman@chromium.org

[vmiura, 2015-02-02 21:36:47]

danakj@chromium.org: Please review changes in cc/

piman@chromium.org: Please review changes in content/

hendrikw@chromium.org: Please review changes in gpu_rasterizer

[hendrikw, 2015-02-02 21:42:58]

https://codereview.chromium.org/895763002/diff/20001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/895763002/diff/20001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:58: return threaded_gpu_rasterization_enabled_ ?
PrepareTilesMode::PREPARE_NONE
Should you add a new tile mode here while you're at it? Otherwise this looks
weird.

[vmiura, 2015-02-02 21:47:11]

https://codereview.chromium.org/895763002/diff/20001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/895763002/diff/20001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:58: return threaded_gpu_rasterization_enabled_ ?
PrepareTilesMode::PREPARE_NONE
I will change to RASTERIZE_PRIORITIZED_TILES, but for now that's unimplemented
so I want this to be a no-op.

[danakj, 2015-02-02 21:48:38]

https://codereview.chromium.org/895763002/diff/20001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/895763002/diff/20001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:58: return threaded_gpu_rasterization_enabled_ ?
PrepareTilesMode::PREPARE_NONE
On 2015/02/02 21:42:57, hendrikw wrote:
> Should you add a new tile mode here while you're at it? Otherwise this looks
> weird.

Can we remove this function and the PREPARE_PRIORITIZED_TILES enum value?

https://codereview.chromium.org/895763002/diff/20001/content/browser/gpu/comp...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/895763002/diff/20001/content/browser/gpu/comp...
content/browser/gpu/compositor_util.cc:304: else if
(command_line.HasSwitch(switches::kEnableThreadedGpuRasterization))
dont need else after a return

https://codereview.chromium.org/895763002/diff/20001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/895763002/diff/20001/content/public/common/co...
content/public/common/content_switches.cc:505: // Disable threaded GPU
rasterization.
don't line break after period

https://codereview.chromium.org/895763002/diff/20001/content/renderer/gpu/com...
File content/renderer/gpu/compositor_dependencies.h (right):

https://codereview.chromium.org/895763002/diff/20001/content/renderer/gpu/com...
content/renderer/gpu/compositor_dependencies.h:41: virtual bool
IsThreadedGpuRasterizationEnabled() = 0;
maybe move this up by other GpuRaster functions?

https://codereview.chromium.org/895763002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/895763002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.h:195: bool
IsThreadedGpuRasterizationEnabled() override;
ditto by the other GpuRaster functions?

[vmiura, 2015-02-02 22:03:23]

PTAL

https://codereview.chromium.org/895763002/diff/20001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/895763002/diff/20001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:58: return threaded_gpu_rasterization_enabled_ ?
PrepareTilesMode::PREPARE_NONE
This function is overloaded in SoftwareRaterizer.

Removed the condition so we always return PREPARE_NONE for now.

https://codereview.chromium.org/895763002/diff/20001/content/browser/gpu/comp...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/895763002/diff/20001/content/browser/gpu/comp...
content/browser/gpu/compositor_util.cc:304: else if
(command_line.HasSwitch(switches::kEnableThreadedGpuRasterization))
On 2015/02/02 21:48:38, danakj wrote:
> dont need else after a return

Done.

https://codereview.chromium.org/895763002/diff/20001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/895763002/diff/20001/content/public/common/co...
content/public/common/content_switches.cc:505: // Disable threaded GPU
rasterization.
On 2015/02/02 21:48:38, danakj wrote:
> don't line break after period

Done.

https://codereview.chromium.org/895763002/diff/20001/content/renderer/gpu/com...
File content/renderer/gpu/compositor_dependencies.h (right):

https://codereview.chromium.org/895763002/diff/20001/content/renderer/gpu/com...
content/renderer/gpu/compositor_dependencies.h:41: virtual bool
IsThreadedGpuRasterizationEnabled() = 0;
On 2015/02/02 21:48:38, danakj wrote:
> maybe move this up by other GpuRaster functions?

Done.

https://codereview.chromium.org/895763002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/895763002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.h:195: bool
IsThreadedGpuRasterizationEnabled() override;
On 2015/02/02 21:48:38, danakj wrote:
> ditto by the other GpuRaster functions?

Done.

[danakj, 2015-02-02 22:04:26]

https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (left):

https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:58: return tile_prepare_enabled_ ?
PrepareTilesMode::PREPARE_PRIORITIZED_TILES
So what's the story with PrepareTilesMode::PREPARE_PRIORITIZED_TILES? Is it dead
now?

Is this function going to return something other than NONE in the future again?

[vmiura, 2015-02-02 23:17:40]

On 2015/02/02 22:04:26, danakj wrote:
>
https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
> File cc/resources/gpu_rasterizer.cc (left):
> 
>
https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
> cc/resources/gpu_rasterizer.cc:58: return tile_prepare_enabled_ ?
> PrepareTilesMode::PREPARE_PRIORITIZED_TILES
> So what's the story with PrepareTilesMode::PREPARE_PRIORITIZED_TILES? Is it
dead
> now?

Yes, PREPARE_PRIORITIZED_TILES was never used and I'm going to consolidate
PREPARE_PRIORITIZED_TILES & RASTERIZE_PRIORITIZED_TILES into one.

> Is this function going to return something other than NONE in the future
again?

I'm planning to return RASTERIZE_PRIORITIZED_TILES when threaded rasterization
lands & is enabled. 
https://codereview.chromium.org/869433003/diff/170001/cc/resources/gpu_raster...

If it sticks then I'll remove this method & everything associated with the enum.

[danakj, 2015-02-02 23:21:55]

On 2015/02/02 23:17:40, vmiura wrote:
> On 2015/02/02 22:04:26, danakj wrote:
> >
>
https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
> > File cc/resources/gpu_rasterizer.cc (left):
> > 
> >
>
https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
> > cc/resources/gpu_rasterizer.cc:58: return tile_prepare_enabled_ ?
> > PrepareTilesMode::PREPARE_PRIORITIZED_TILES
> > So what's the story with PrepareTilesMode::PREPARE_PRIORITIZED_TILES? Is it
> dead
> > now?
> 
> Yes, PREPARE_PRIORITIZED_TILES was never used and I'm going to consolidate
> PREPARE_PRIORITIZED_TILES & RASTERIZE_PRIORITIZED_TILES into one.

OK, I assume that's coming as another CL after this then?

> > Is this function going to return something other than NONE in the future
> again?
> 
> I'm planning to return RASTERIZE_PRIORITIZED_TILES when threaded rasterization
> lands & is enabled. 
>
https://codereview.chromium.org/869433003/diff/170001/cc/resources/gpu_raster...
> 
> If it sticks then I'll remove this method & everything associated with the
enum.

OK Cool. I see that making this function useful again thanks. It doesn't remove
the PREPARE_PRIORITIZED_TILES either, so that's independent?

LGTM

[vmiura, 2015-02-02 23:28:43]

On 2015/02/02 23:21:55, danakj wrote:
> On 2015/02/02 23:17:40, vmiura wrote:
> > On 2015/02/02 22:04:26, danakj wrote:
> > >
> >
>
https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
> > > File cc/resources/gpu_rasterizer.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
> > > cc/resources/gpu_rasterizer.cc:58: return tile_prepare_enabled_ ?
> > > PrepareTilesMode::PREPARE_PRIORITIZED_TILES
> > > So what's the story with PrepareTilesMode::PREPARE_PRIORITIZED_TILES? Is
it
> > dead
> > > now?
> > 
> > Yes, PREPARE_PRIORITIZED_TILES was never used and I'm going to consolidate
> > PREPARE_PRIORITIZED_TILES & RASTERIZE_PRIORITIZED_TILES into one.
> 
> OK, I assume that's coming as another CL after this then?
> 
> > > Is this function going to return something other than NONE in the future
> > again?
> > 
> > I'm planning to return RASTERIZE_PRIORITIZED_TILES when threaded
rasterization
> > lands & is enabled. 
> >
>
https://codereview.chromium.org/869433003/diff/170001/cc/resources/gpu_raster...
> > 
> > If it sticks then I'll remove this method & everything associated with the
> enum.
> 
> OK Cool. I see that making this function useful again thanks. It doesn't
remove
> the PREPARE_PRIORITIZED_TILES either, so that's independent?

I've removed it in this patch
codereview.chromium.org/895763002/diff/40001/cc/resources/rasterizer.h, and will
re-base the other onto this.

[danakj, 2015-02-02 23:29:32]

On 2015/02/02 23:28:43, vmiura wrote:
> On 2015/02/02 23:21:55, danakj wrote:
> > On 2015/02/02 23:17:40, vmiura wrote:
> > > On 2015/02/02 22:04:26, danakj wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
> > > > File cc/resources/gpu_rasterizer.cc (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/895763002/diff/40001/cc/resources/gpu_rasteri...
> > > > cc/resources/gpu_rasterizer.cc:58: return tile_prepare_enabled_ ?
> > > > PrepareTilesMode::PREPARE_PRIORITIZED_TILES
> > > > So what's the story with PrepareTilesMode::PREPARE_PRIORITIZED_TILES? Is
> it
> > > dead
> > > > now?
> > > 
> > > Yes, PREPARE_PRIORITIZED_TILES was never used and I'm going to consolidate
> > > PREPARE_PRIORITIZED_TILES & RASTERIZE_PRIORITIZED_TILES into one.
> > 
> > OK, I assume that's coming as another CL after this then?
> > 
> > > > Is this function going to return something other than NONE in the future
> > > again?
> > > 
> > > I'm planning to return RASTERIZE_PRIORITIZED_TILES when threaded
> rasterization
> > > lands & is enabled. 
> > >
> >
>
https://codereview.chromium.org/869433003/diff/170001/cc/resources/gpu_raster...
> > > 
> > > If it sticks then I'll remove this method & everything associated with the
> > enum.
> > 
> > OK Cool. I see that making this function useful again thanks. It doesn't
> remove
> > the PREPARE_PRIORITIZED_TILES either, so that's independent?
> 
> I've removed it in this patch
> codereview.chromium.org/895763002/diff/40001/cc/resources/rasterizer.h, and
will
> re-base the other onto this.

Oh! Awesome, double LGTM

[vmiura, 2015-02-03 19:46:44]

Antoine, could I ask you to look at the content/ & chrome/browser flag addition?

[danakj, 2015-02-03 20:04:37]

On Tue, Feb 3, 2015 at 11:46 AM, <vmiura@chromium.org> wrote:

> Antoine, could I ask you to look at the content/ & chrome/browser flag
> addition?
>

You could also try +sievers for lower latency maybe.


>
>
> https://codereview.chromium.org/895763002/
>

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

[piman, 2015-02-03 21:30:32]

lgtm

[vmiura, 2015-02-03 21:50:51]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2015-02-03 21:51:57]

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

[commit-bot: I haz the power, 2015-02-03 22:20:32]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-02-03 22:21:34]

Patchset 2 (id:??) landed as
https://crrev.com/9084b34a9c65689297314f65c389e063556df88e
Cr-Commit-Position: refs/heads/master@{#314418}