command_buffer: Implement CHROMIUM_framebuffer_mixed_samples extension

command_buffer: Implement CHROMIUM_framebuffer_mixed_samples extension

Implement a pseudo-extension that allows using attachments of different
sample counts in framebuffers.

The extension is implemented by the means of
GL_NV_framebuffer_mixed_samples.

At the present form, should be usable in Skia to implement mixed
samples.

BUG=506765
TBR=jam@chromium.org

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

[Kimmo Kinnunen, 2015-12-07 13:14:53]

kkinnunen@nvidia.com changed reviewers:
+ zmo@google.com

[Kimmo Kinnunen, 2015-12-07 13:14:54]

zmo,
This would be last of the initial features needed for path rendering with "mixed
samples" feature.

The spec text is rather unhelpful, as it is practically impossible for me to
write a correct gles2 extension. You might have noticed similar problem with not
writing CHROMIUM_framebuffer_multisample.

Any thoughts on following:
- Does this need to expose the multisample state variables
(MULTISAMPLE_RASTERIZATION_ALLOWED_EXT, EFFECTIVE_RASTER_SAMPLES_EXT,
COLOR_SAMPLES_CHROMIUM, STENCIL_SAMPLES_CHROMIUM, DEPTH_SAMPLES_CHROMIUM) ? This
adds plenty of complexity, but then the spec text would be easier to copy..
- What about the spec text, is it strictly neccessary?
- The current NV_framebuffer_mixed_samples is specified only for OpenGL, but is
actually exposed to ES too. Is using it in ES without a proper spec a problem?

[Zhenyao Mo, 2015-12-07 20:20:22]

On 2015/12/07 13:14:54, Kimmo Kinnunen wrote:
> zmo,
> This would be last of the initial features needed for path rendering with
"mixed
> samples" feature.
> 
> The spec text is rather unhelpful, as it is practically impossible for me to
> write a correct gles2 extension. You might have noticed similar problem with
not
> writing CHROMIUM_framebuffer_multisample.
> 
> Any thoughts on following:
> - Does this need to expose the multisample state variables
> (MULTISAMPLE_RASTERIZATION_ALLOWED_EXT, EFFECTIVE_RASTER_SAMPLES_EXT,
> COLOR_SAMPLES_CHROMIUM, STENCIL_SAMPLES_CHROMIUM, DEPTH_SAMPLES_CHROMIUM) ?
This
> adds plenty of complexity, but then the spec text would be easier to copy..
> - What about the spec text, is it strictly neccessary?
> - The current NV_framebuffer_mixed_samples is specified only for OpenGL, but
is
> actually exposed to ES too. Is using it in ES without a proper spec a problem?

OK, I'll try to get back to you by Friday (This is our FIXIT week)

[Zhenyao Mo, 2015-12-16 01:48:58]

zmo@chromium.org changed reviewers:
+ zmo@chromium.org

[Zhenyao Mo, 2015-12-16 01:48:59]

The CL looks good, but I do have some questions about the spec.

https://codereview.chromium.org/1218223005/diff/80001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_framebuffer_mixed_samples.txt
(right):

https://codereview.chromium.org/1218223005/diff/80001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_framebuffer_mixed_samples.txt:34: or
stencil samples are a multiple of the number of color samples.
Should this change the DoCheckFramebufferStatue()? I don't think we check for
sample matching right now, but we definitely will add it for ES3.

https://codereview.chromium.org/1218223005/diff/80001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_framebuffer_mixed_samples.txt:44:
attachments.
Should the above two features be implemented in the command buffer?

[Zhenyao Mo, 2015-12-16 01:51:38]

On 2015/12/07 13:14:54, Kimmo Kinnunen wrote:
> zmo,
> This would be last of the initial features needed for path rendering with
"mixed
> samples" feature.
> 
> The spec text is rather unhelpful, as it is practically impossible for me to
> write a correct gles2 extension. You might have noticed similar problem with
not
> writing CHROMIUM_framebuffer_multisample.
> 
> Any thoughts on following:
> - Does this need to expose the multisample state variables
> (MULTISAMPLE_RASTERIZATION_ALLOWED_EXT, EFFECTIVE_RASTER_SAMPLES_EXT,
> COLOR_SAMPLES_CHROMIUM, STENCIL_SAMPLES_CHROMIUM, DEPTH_SAMPLES_CHROMIUM) ?
This
> adds plenty of complexity, but then the spec text would be easier to copy..

Maybe not for the initial round?  Let's see if skia folks want them or not.

> - What about the spec text, is it strictly neccessary?

I think so.  This is a subset of the NV extension, so I think it should be
clearly defined somewhere what are exposed.

> - The current NV_framebuffer_mixed_samples is specified only for OpenGL, but
is
> actually exposed to ES too. Is using it in ES without a proper spec a problem?

Assume it's the same as the desktop extension, let's expose it for now.

[Kimmo Kinnunen, 2015-12-17 13:47:23]

Thanks for the comments. Any further comments on how this patch should be fixed
before it is accepted would be helpful.

https://codereview.chromium.org/1218223005/diff/80001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_framebuffer_mixed_samples.txt
(right):

https://codereview.chromium.org/1218223005/diff/80001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_framebuffer_mixed_samples.txt:34: or
stencil samples are a multiple of the number of color samples.
On 2015/12/16 01:48:58, Zhenyao Mo wrote:
> Should this change the DoCheckFramebufferStatue()? I don't think we check for
> sample matching right now, but we definitely will add it for ES3.

I don't know? I didn't add relaxation of the error checking, since the checks
did not exist in the first place.

The problem with me adding the error checking is that the errors are defined in
either ES3 or CHROMIUM_framebuffer_multisample. The former is not implemented
and the latter is not specified. So I'd need to either use ES3 as the spec or
first specify CHROMIUM_framebuffer_multisample.

I think me adding ES3 checks while not actually implementing ES3, and not being
in sync with the primary ES3 implementors is quite problematic.

I think specifying CHROMIUM_framebuffer_multisample is a huge task.

https://codereview.chromium.org/1218223005/diff/80001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_framebuffer_mixed_samples.txt:44:
attachments.
On 2015/12/16 01:48:58, Zhenyao Mo wrote:
> Should the above two features be implemented in the command buffer?

Do you mean, currently or my opinion about ideal world / entirely finished
command buffer implementation?

If I understand correctly, current command buffer relies on the underlying
driver for all sorts of checks related to multisampling state.

For this patch, it continues to rely on the underlying driver.

[Zhenyao Mo, 2015-12-17 21:25:39]

Let's land this.

LGTM

https://codereview.chromium.org/1218223005/diff/80001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_framebuffer_mixed_samples.txt
(right):

https://codereview.chromium.org/1218223005/diff/80001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_framebuffer_mixed_samples.txt:34: or
stencil samples are a multiple of the number of color samples.
On 2015/12/17 13:47:23, Kimmo Kinnunen wrote:
> On 2015/12/16 01:48:58, Zhenyao Mo wrote:
> > Should this change the DoCheckFramebufferStatue()? I don't think we check
for
> > sample matching right now, but we definitely will add it for ES3.
> 
> I don't know? I didn't add relaxation of the error checking, since the checks
> did not exist in the first place.
> 
> The problem with me adding the error checking is that the errors are defined
in
> either ES3 or CHROMIUM_framebuffer_multisample. The former is not implemented
> and the latter is not specified. So I'd need to either use ES3 as the spec or
> first specify CHROMIUM_framebuffer_multisample.
> 
> I think me adding ES3 checks while not actually implementing ES3, and not
being
> in sync with the primary ES3 implementors is quite problematic.
> 
> I think specifying CHROMIUM_framebuffer_multisample is a huge task.
> 
> 

I'll ask around why CHROMIUM_framebuffer_multisample isn't specified.  I don't
expect it to be a huge task, because we already have
ANGLE_framebuffer_multisample specified and we can just copy from that.

As for the CheckFramebufferStatus(), I do think we need to handle it and take
into consideration this extension.  But this CL can land as is.

https://codereview.chromium.org/1218223005/diff/80001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_framebuffer_mixed_samples.txt:44:
attachments.
On 2015/12/17 13:47:23, Kimmo Kinnunen wrote:
> On 2015/12/16 01:48:58, Zhenyao Mo wrote:
> > Should the above two features be implemented in the command buffer?
> 
> Do you mean, currently or my opinion about ideal world / entirely finished
> command buffer implementation?
> 
> If I understand correctly, current command buffer relies on the underlying
> driver for all sorts of checks related to multisampling state.
> 
> For this patch, it continues to rely on the underlying driver.

My concern is it really depends on command buffer implementation, because we
don't have to go to the driver to query these values.  We could return based on
the data in command buffer.  Again, this CL can land as is.  We just need to
create a bug to log these behaviors and make sure we handle it later.

[Zhenyao Mo, 2015-12-17 21:31:59]

kbr thinks CHROMIUM_framebuffer_multisample is just a wrap around
ANGLE_framebuffer_multisample.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/angle/...

[Kimmo Kinnunen, 2015-12-18 11:03:32]

kkinnunen@nvidia.com changed reviewers:
+ piman@chromium.org

[Kimmo Kinnunen, 2015-12-18 11:03:32]

piman@: this has been looked at by zmo, but would need your stamp for:
    gpu/BUILD.gn
    gpu/gpu.gyp
    gpu/skia_bindings/gl_bindings_skia_cmd_buffer.cc
    mojo/public/c/gles2/gles2_call_visitor_chromium_extension_autogen.h
    ui/gl/generate_bindings.py
    ui/gl/gl_bindings.h
    ui/gl/gl_bindings_api_autogen_gl.h
    ui/gl/gl_bindings_autogen_gl.cc
    ui/gl/gl_bindings_autogen_gl.h
    ui/gl/gl_bindings_autogen_mock.cc
    ui/gl/gl_bindings_autogen_mock.h
    ui/gl/gl_enums_implementation_autogen.h
    ui/gl/gl_mock_autogen_gl.h

[piman, 2015-12-18 14:08:04]

lgtm

[Kimmo Kinnunen, 2015-12-21 11:04:48]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-12-21 11:04:49]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1218223005/#ps100001
(title: "rebase for commit")

[commit-bot: I haz the power, 2015-12-21 11:05:05]

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

[commit-bot: I haz the power, 2015-12-21 11:13:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-21 11:13:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Kimmo Kinnunen, 2015-12-21 11:21:59]

kkinnunen@nvidia.com changed reviewers:
+ jam@chromium.org

[Kimmo Kinnunen, 2015-12-21 11:22:00]

jam@: could you take a look at the mojo/* changes?

[Zhenyao Mo, 2015-12-21 17:15:43]

Description was changed from

==========
command_buffer: Implement CHROMIUM_framebuffer_mixed_samples extension

Implement a pseudo-extension that allows using attachments of different
sample counts in framebuffers.

The extension is implemented by the means of
GL_NV_framebuffer_mixed_samples.

At the present form, should be usable in Skia to implement mixed
samples.

BUG=506765
==========

to

==========
command_buffer: Implement CHROMIUM_framebuffer_mixed_samples extension

Implement a pseudo-extension that allows using attachments of different
sample counts in framebuffers.

The extension is implemented by the means of
GL_NV_framebuffer_mixed_samples.

At the present form, should be usable in Skia to implement mixed
samples.

BUG=506765
TBR=jam@chromium.org
==========

[Zhenyao Mo, 2015-12-21 17:16:15]

Let's land with TRB to jam since it's all auto-generated code under mojo/

[Zhenyao Mo, 2015-12-21 17:16:20]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2015-12-21 17:17:28]

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

[commit-bot: I haz the power, 2015-12-21 17:23:52]

Description was changed from

==========
command_buffer: Implement CHROMIUM_framebuffer_mixed_samples extension

Implement a pseudo-extension that allows using attachments of different
sample counts in framebuffers.

The extension is implemented by the means of
GL_NV_framebuffer_mixed_samples.

At the present form, should be usable in Skia to implement mixed
samples.

BUG=506765
TBR=jam@chromium.org
==========

to

==========
command_buffer: Implement CHROMIUM_framebuffer_mixed_samples extension

Implement a pseudo-extension that allows using attachments of different
sample counts in framebuffers.

The extension is implemented by the means of
GL_NV_framebuffer_mixed_samples.

At the present form, should be usable in Skia to implement mixed
samples.

BUG=506765
TBR=jam@chromium.org
==========

[commit-bot: I haz the power, 2015-12-21 17:23:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-21 17:25:12]

Description was changed from

==========
command_buffer: Implement CHROMIUM_framebuffer_mixed_samples extension

Implement a pseudo-extension that allows using attachments of different
sample counts in framebuffers.

The extension is implemented by the means of
GL_NV_framebuffer_mixed_samples.

At the present form, should be usable in Skia to implement mixed
samples.

BUG=506765
TBR=jam@chromium.org
==========

to

==========
command_buffer: Implement CHROMIUM_framebuffer_mixed_samples extension

Implement a pseudo-extension that allows using attachments of different
sample counts in framebuffers.

The extension is implemented by the means of
GL_NV_framebuffer_mixed_samples.

At the present form, should be usable in Skia to implement mixed
samples.

BUG=506765
TBR=jam@chromium.org

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

[commit-bot: I haz the power, 2015-12-21 17:25:15]

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