cc, gpu: Use RGBA when using msaa on systems that don't support BGRA

cc, gpu: Use RGBA when using msaa on systems that don't support BGRA

The pixel is compiled to prefer BGRA, but doesn't support BGRA for
render buffers.  When we try to use msaa, it tries to create a BGRA
render buffer and fails.

Fix is, plumb the setting to the gpu raster code, and prefer RGBA
when using msaa on a system that doesn't support BGRA.

BUG=497478
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/316921858b47cf38d65f242435782d87f6f78b93
Cr-Commit-Position: refs/heads/master@{#335395}

[hendrikw, 2015-06-09 22:52:20]

hendrikw@chromium.org changed reviewers:
+ reveman@chromium.org, vmiura@chromium.org

[hendrikw, 2015-06-09 22:52:21]

PTAL

I'm handling the case where we're on a BGRA system and we don't support BGRA
render buffers, do you think I should also handle the case where I'm on an RGBA
system without RGBA render buffers support?

[vmiura, 2015-06-09 23:05:11]

https://codereview.chromium.org/1174733003/diff/20001/cc/raster/gpu_tile_task...
File cc/raster/gpu_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1174733003/diff/20001/cc/raster/gpu_tile_task...
cc/raster/gpu_tile_task_worker_pool.cc:211: ResourceFormat format =
We call this per-tile.  I think it'd be good to pre-calc this and return via
something like ResourceProvider::best_renderbuffer_format().

https://codereview.chromium.org/1174733003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/1174733003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:516:
feature_flags_.ext_render_buffer_format_bgra8888 = true;
Could you add a FeatureInfoTest for this?

[hendrikw, 2015-06-10 01:23:07]

PTAL again :D

[hendrikw, 2015-06-10 01:41:42]

Also removed unused function

[vmiura, 2015-06-10 01:48:25]

lgtm

https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:300: draw_info.contents_swizzled();
nit: It would be nice if draw_info.contents_swizzled() == false for GPU raster,
rather than draw_info.contents_swizzled() == LIES for GPU raster :).

[hendrikw, 2015-06-10 02:04:44]

On 2015/06/10 01:48:25, vmiura wrote:
> lgtm
> 
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> File cc/layers/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> cc/layers/picture_layer_impl.cc:300: draw_info.contents_swizzled();
> nit: It would be nice if draw_info.contents_swizzled() == false for GPU
raster,
> rather than draw_info.contents_swizzled() == LIES for GPU raster :).

Hmm, yeah, doesn't seem right, draw_info doesn't know anything about gpu raster,
it just checks the the format against skia's format.  I could plumb the gpu
raster state through the draw tile info, but that seems like overkill

or I could pass gpu raster to contents_swizzled and just return false if gpu
raster is on?  The code would basically be the same as it is now, maybe less
lies though??

[vmiura, 2015-06-10 04:58:32]

On 2015/06/10 02:04:44, hendrikw wrote:
> On 2015/06/10 01:48:25, vmiura wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> > File cc/layers/picture_layer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> > cc/layers/picture_layer_impl.cc:300: draw_info.contents_swizzled();
> > nit: It would be nice if draw_info.contents_swizzled() == false for GPU
> raster,
> > rather than draw_info.contents_swizzled() == LIES for GPU raster :).
> 
> Hmm, yeah, doesn't seem right, draw_info doesn't know anything about gpu
raster,
> it just checks the the format against skia's format.  I could plumb the gpu
> raster state through the draw tile info, but that seems like overkill
> 
> or I could pass gpu raster to contents_swizzled and just return false if gpu
> raster is on?  The code would basically be the same as it is now, maybe less
> lies though??

Could we set "draw_info.contents_swizzled_ = true/false" in
TileManager::UpdateTileDrawInfo()?

[hendrikw, 2015-06-10 15:28:32]

On 2015/06/10 04:58:32, vmiura wrote:
> On 2015/06/10 02:04:44, hendrikw wrote:
> > On 2015/06/10 01:48:25, vmiura wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> > > File cc/layers/picture_layer_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> > > cc/layers/picture_layer_impl.cc:300: draw_info.contents_swizzled();
> > > nit: It would be nice if draw_info.contents_swizzled() == false for GPU
> > raster,
> > > rather than draw_info.contents_swizzled() == LIES for GPU raster :).
> > 
> > Hmm, yeah, doesn't seem right, draw_info doesn't know anything about gpu
> raster,
> > it just checks the the format against skia's format.  I could plumb the gpu
> > raster state through the draw tile info, but that seems like overkill
> > 
> > or I could pass gpu raster to contents_swizzled and just return false if gpu
> > raster is on?  The code would basically be the same as it is now, maybe less
> > lies though??
> 
> Could we set "draw_info.contents_swizzled_ = true/false" in
> TileManager::UpdateTileDrawInfo()?

There is no contents_swizzled_ in TileDrawInfo, and I would rather keep it
simple than plumb that through when it's really not needed.

How about I renamed contents_swizzled() to format_matches_platform()?  This way,
there is no lie, and no plumbing.

[vmiura, 2015-06-10 15:53:57]

On 2015/06/10 15:28:32, hendrikw wrote:
> On 2015/06/10 04:58:32, vmiura wrote:
> > On 2015/06/10 02:04:44, hendrikw wrote:
> > > On 2015/06/10 01:48:25, vmiura wrote:
> > > > lgtm
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> > > > File cc/layers/picture_layer_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> > > > cc/layers/picture_layer_impl.cc:300: draw_info.contents_swizzled();
> > > > nit: It would be nice if draw_info.contents_swizzled() == false for GPU
> > > raster,
> > > > rather than draw_info.contents_swizzled() == LIES for GPU raster :).
> > > 
> > > Hmm, yeah, doesn't seem right, draw_info doesn't know anything about gpu
> > raster,
> > > it just checks the the format against skia's format.  I could plumb the
gpu
> > > raster state through the draw tile info, but that seems like overkill
> > > 
> > > or I could pass gpu raster to contents_swizzled and just return false if
gpu
> > > raster is on?  The code would basically be the same as it is now, maybe
less
> > > lies though??
> > 
> > Could we set "draw_info.contents_swizzled_ = true/false" in
> > TileManager::UpdateTileDrawInfo()?
> 
> There is no contents_swizzled_ in TileDrawInfo, and I would rather keep it
> simple than plumb that through when it's really not needed.

> How about I renamed contents_swizzled() to format_matches_platform()?  This
way,
> there is no lie, and no plumbing.

I think having contents_swizzeld_ in TileDrawInfo, flowing from the rasterizer
would be nicest, but I'm OK either way.

[hendrikw, 2015-06-10 16:05:57]

On 2015/06/10 15:53:57, vmiura wrote:
> On 2015/06/10 15:28:32, hendrikw wrote:
> > On 2015/06/10 04:58:32, vmiura wrote:
> > > On 2015/06/10 02:04:44, hendrikw wrote:
> > > > On 2015/06/10 01:48:25, vmiura wrote:
> > > > > lgtm
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> > > > > File cc/layers/picture_layer_impl.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1174733003/diff/60001/cc/layers/picture_layer...
> > > > > cc/layers/picture_layer_impl.cc:300: draw_info.contents_swizzled();
> > > > > nit: It would be nice if draw_info.contents_swizzled() == false for
GPU
> > > > raster,
> > > > > rather than draw_info.contents_swizzled() == LIES for GPU raster :).
> > > > 
> > > > Hmm, yeah, doesn't seem right, draw_info doesn't know anything about gpu
> > > raster,
> > > > it just checks the the format against skia's format.  I could plumb the
> gpu
> > > > raster state through the draw tile info, but that seems like overkill
> > > > 
> > > > or I could pass gpu raster to contents_swizzled and just return false if
> gpu
> > > > raster is on?  The code would basically be the same as it is now, maybe
> less
> > > > lies though??
> > > 
> > > Could we set "draw_info.contents_swizzled_ = true/false" in
> > > TileManager::UpdateTileDrawInfo()?
> > 
> > There is no contents_swizzled_ in TileDrawInfo, and I would rather keep it
> > simple than plumb that through when it's really not needed.
> 
> > How about I renamed contents_swizzled() to format_matches_platform()?  This
> way,
> > there is no lie, and no plumbing.
> 
> I think having contents_swizzeld_ in TileDrawInfo, flowing from the rasterizer
> would be nicest, but I'm OK either way.

Plumbing unnecessary variables through adds to the spaghetti.

I've renamed the function.

[vmiura, 2015-06-10 17:49:21]

vmiura@chromium.org changed reviewers:
+ danakj@chromium.org

[vmiura, 2015-06-10 17:49:22]

> > I think having contents_swizzeld_ in TileDrawInfo, flowing from the
rasterizer
> > would be nicest, but I'm OK either way.
> 
> Plumbing unnecessary variables through adds to the spaghetti.
> 
> I've renamed the function.

As we spoke offline, I think "if(use_gpu_rasterization) { assume ... }" adds
some complexity too so just considering the tradeoffs.

In general the less code needs to know if it's dealing with GPU or software the
better. There have also been subtle bugs with when LTHI::use_gpu_rasterization
changed, i.e. use_gpu_rasterization could flip to false while the trees had GPU
rasterized resources; the new code would pass the wrong swizzle mode in that
case.

I think that's been recently fixed though, so I don't think there's a bug here.

[hendrikw, 2015-06-18 18:58:43]

Added new virtual function to plumb the swizzle.

PTAL.

Thanks.

[vmiura, 2015-06-18 22:30:53]

lgtm % comment.  Thanks!

https://codereview.chromium.org/1174733003/diff/140001/cc/tiles/tile_draw_info.h
File cc/tiles/tile_draw_info.h (right):

https://codereview.chromium.org/1174733003/diff/140001/cc/tiles/tile_draw_inf...
cc/tiles/tile_draw_info.h:99: bool contents_swizzled_;
'contents_swizzled_' missing initialization.

[hendrikw, 2015-06-18 22:35:11]

On 2015/06/18 22:30:53, vmiura wrote:
> lgtm % comment.  Thanks!
> 
>
https://codereview.chromium.org/1174733003/diff/140001/cc/tiles/tile_draw_info.h
> File cc/tiles/tile_draw_info.h (right):
> 
>
https://codereview.chromium.org/1174733003/diff/140001/cc/tiles/tile_draw_inf...
> cc/tiles/tile_draw_info.h:99: bool contents_swizzled_;
> 'contents_swizzled_' missing initialization.

Then you reviewed the wrong patch :)

[vmiura, 2015-06-18 22:52:50]

On 2015/06/18 22:35:11, hendrikw wrote:
> On 2015/06/18 22:30:53, vmiura wrote:
> > lgtm % comment.  Thanks!
> > 
> >
>
https://codereview.chromium.org/1174733003/diff/140001/cc/tiles/tile_draw_info.h
> > File cc/tiles/tile_draw_info.h (right):
> > 
> >
>
https://codereview.chromium.org/1174733003/diff/140001/cc/tiles/tile_draw_inf...
> > cc/tiles/tile_draw_info.h:99: bool contents_swizzled_;
> > 'contents_swizzled_' missing initialization.
> 
> Then you reviewed the wrong patch :)

Nice, well by 10 minutes...  lgtm

[reveman, 2015-06-19 21:32:42]

lgtm

[hendrikw, 2015-06-19 21:35:50]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-06-19 21:36:37]

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

[commit-bot: I haz the power, 2015-06-19 23:45:52]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-06-19 23:46:41]

Patchset 9 (id:??) landed as
https://crrev.com/316921858b47cf38d65f242435782d87f6f78b93
Cr-Commit-Position: refs/heads/master@{#335395}