gpu: Support ES3 msaa and depth/stencil formats

gpu: Support ES3 msaa and depth/stencil formats

BUG=314214
R=kbr@chromium.org, piman@chromium.org, skyostil@chromium.org, zmo@chromium.org

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

[no sievers, 2013-11-14 00:40:22]

ptal

[Ken Russell (switch to Gerrit), 2013-11-14 01:13:56]

+piman; I'm not an OWNER of gpu/command_buffer/

[Ken Russell (switch to Gerrit), 2013-11-14 01:18:37]

+zmo for discussion about the driver bug workaround

The code looks good overall. Want to see clean try jobs before approving.

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.cc:214: is_es3 = (lstr.substr(0, 12) ==
"opengl es 3.");
Is this going to scale well to some hypothetical OpenGL ES 4.0? Is there a
better way to do this feature detection?

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
gpu/config/gpu_driver_bug_list_json.cc:711: "disable_framebuffer_multisample"
Is this the best name for this workaround? Can it be more precise? i.e., do we
fall back to a different multisampling extension? Please discuss with zmo.

[Zhenyao Mo, 2013-11-14 01:38:11]

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
gpu/config/gpu_driver_bug_workaround_type.h:34: disable_framebuffer_multisample)
                   \
I think we should combine this and the disable_angle_framebuffer_multisample
into one.

[no sievers, 2013-11-14 01:51:27]

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
gpu/config/gpu_driver_bug_workaround_type.h:34: disable_framebuffer_multisample)
                   \
On 2013/11/14 01:38:11, Zhenyao Mo wrote:
> I think we should combine this and the disable_angle_framebuffer_multisample
> into one.

The problem is that the exact same vendor somehow incorrectly advertised
GL_ANGLE_framebuffer_multisample, see id 15 :)

Alex says he remembers it was only on specific devices (not qC drivers in
general). So it might also have been some OEM who accidentally added it or so.
It's hard to verify if it won't still be advertised on some QC ES3 driver, which
would be a problem once the other bugs are fixed.

Do you mind keeping both, one disabling the specific extension, the other one
disabling multisampling in general?

I can also rename it to disable_es3_framebuffer_multisample or so, but not sure
that it makes things better.

[Zhenyao Mo, 2013-11-14 02:04:18]

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.h (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.h:30: bool core_framebuffer_multisample;
Please add a comment to explain this variable.

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
gpu/config/gpu_driver_bug_workaround_type.h:34: disable_framebuffer_multisample)
                   \
On 2013/11/14 01:51:28, sievers wrote:
> On 2013/11/14 01:38:11, Zhenyao Mo wrote:
> > I think we should combine this and the disable_angle_framebuffer_multisample
> > into one.
> 
> The problem is that the exact same vendor somehow incorrectly advertised
> GL_ANGLE_framebuffer_multisample, see id 15 :)
> 
> Alex says he remembers it was only on specific devices (not qC drivers in
> general). So it might also have been some OEM who accidentally added it or so.
> It's hard to verify if it won't still be advertised on some QC ES3 driver,
which
> would be a problem once the other bugs are fixed.
> 
> Do you mind keeping both, one disabling the specific extension, the other one
> disabling multisampling in general?
> 
> I can also rename it to disable_es3_framebuffer_multisample or so, but not
sure
> that it makes things better.
> 
> 

Are we allowing GL_ANGLE_framebuffer_multisample if it's not ANGLE backend?  If
not, then we don't need both names.  If yes, then we can keep both names.

[Sami, 2013-11-14 15:48:53]

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.cc:214: is_es3 = (lstr.substr(0, 12) ==
"opengl es 3.");
On 2013/11/14 01:18:37, Ken Russell wrote:
> Is this going to scale well to some hypothetical OpenGL ES 4.0? Is there a
> better way to do this feature detection?

You could also check for glGetIntegerv({MAJOR_VERSION,MINOR_VERSION}) --
although that won't distinguish between ES vs. desktop but I'm not sure if it
makes a difference here. Note that this query isn't supported by ES 2.0 though
:)

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.h (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.h:30: bool core_framebuffer_multisample;
On 2013/11/14 02:04:20, Zhenyao Mo wrote:
> Please add a comment to explain this variable.

+1, and nit: use_core_framebuffer_multisample?

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/config/gpu_driver_bug...
gpu/config/gpu_driver_bug_list_json.cc:711: "disable_framebuffer_multisample"
On 2013/11/14 01:18:37, Ken Russell wrote:
> Is this the best name for this workaround? Can it be more precise? i.e., do we
> fall back to a different multisampling extension? Please discuss with zmo.

Could you also add a sentence or two to the commit message about why we need
this multiplexing the workaround for Qualcomm.

[no sievers, 2013-11-15 00:21:35]

how's that?

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.h (right):

https://codereview.chromium.org/72173002/diff/50001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.h:30: bool core_framebuffer_multisample;
On 2013/11/14 02:04:20, Zhenyao Mo wrote:
> Please add a comment to explain this variable.

Done and done.

[Zhenyao Mo, 2013-11-15 00:29:04]

LGTM

[Ken Russell (switch to Gerrit), 2013-11-15 00:31:08]

lgtm

[Sami, 2013-11-15 14:08:57]

lgtm

[piman, 2013-11-15 16:58:50]

LGTM + 1 nit.

https://codereview.chromium.org/72173002/diff/220001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/72173002/diff/220001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:410: bool
AllocateStorage(GLES2DecoderImpl* decoder,
nit: kloveless@ tried to decouple these from the decoder. You only need the
feature_info, right? Can you pass just that in?

[no sievers, 2013-11-15 19:41:47]

https://codereview.chromium.org/72173002/diff/220001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/72173002/diff/220001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:410: bool
AllocateStorage(GLES2DecoderImpl* decoder,
On 2013/11/15 16:58:50, piman wrote:
> nit: kloveless@ tried to decouple these from the decoder. You only need the
> feature_info, right? Can you pass just that in?

Done.

[no sievers, 2013-11-15 21:46:39]

Committed patchset #5 manually as r235435 (presubmit successful).

[mtomasz, 2013-11-21 10:36:40]

On 2013/11/15 21:46:39, sievers wrote:
> Committed patchset #5 manually as r235435 (presubmit successful).

This patch makes the new Camera app (go/cros-camera) almost 6 times slower on
ARM devices.
FPS decrease from 50 to 9.