Validate bound buffer for draw calls

Validate bound buffer for draw calls

DrawArrays or drawElements should generate invalid operation error if no
buffer is bound to enabled attribution. Refer WebGL 1.0 spec for details
in section 6.5 "Enabled Vertex Attributes and Range Checking".

BUG=295792
TEST=conformance2/vertex_arrays/vertex-array-object.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

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

[qiankun, 2016-05-27 09:51:57]

Description was changed from

==========
Validate bound buffer for draw calls

DrawArrays or drawElements should generate invalid operation error if no
buffer is bound to enabled attribution. Refer WebGL 1.0 spec for details
in section 6.5 "Enabled Vertex Attributes and Range Checking".

BUG=295792
TEST=conformance2/vertex_arrays/vertex-array-object.html
==========

to

==========
Validate bound buffer for draw calls

DrawArrays or drawElements should generate invalid operation error if no
buffer is bound to enabled attribution. Refer WebGL 1.0 spec for details
in section 6.5 "Enabled Vertex Attributes and Range Checking".

BUG=295792
TEST=conformance2/vertex_arrays/vertex-array-object.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[qiankun, 2016-05-27 11:48:31]

qiankun.miao@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org

[qiankun, 2016-05-27 11:48:32]

PTAL.
Also a fix in conformance test is added:
https://github.com/KhronosGroup/WebGL/pull/1717. With these patches,
conformance2/vertex_arrays/vertex-array-object.html passes.


The bots fails because gl.vertexAttribPointer is not called after
gl.enableVertexAttribArray(0) before doing drawArray in
conformance/misc/webgl-specific.html. According WebGL spec, this behavior is not
correct. WDYT?

[Zhenyao Mo, 2016-05-27 14:04:02]

On 2016/05/27 11:48:32, qiankun wrote:
> PTAL.
> Also a fix in conformance test is added:
> https://github.com/KhronosGroup/WebGL/pull/1717. With these patches,
> conformance2/vertex_arrays/vertex-array-object.html passes.
> 
> 
> The bots fails because gl.vertexAttribPointer is not called after
> gl.enableVertexAttribArray(0) before doing drawArray in
> conformance/misc/webgl-specific.html. According WebGL spec, this behavior is
not
> correct. WDYT?

According to section 6.5 in webgl1 spec, you are correct.

I lost track when that section was added, but it seems to me if a vertex attrib
is enabled but not consumed by the program, then it should not require a buffer
to be bound in order to draw, just like it does not require the buffer to be of
a certain size.

@kbr: can you shed some light in this situation?

All implementations have passed webgl-specific.html, this seems like a big
behavior change where we haven't added a test (actually we test the opposite in
webgl-specific.html)

[qiankun, 2016-05-31 04:55:18]

On 2016/05/27 14:04:02, Zhenyao Mo wrote:
> On 2016/05/27 11:48:32, qiankun wrote:
> > PTAL.
> > Also a fix in conformance test is added:
> > https://github.com/KhronosGroup/WebGL/pull/1717. With these patches,
> > conformance2/vertex_arrays/vertex-array-object.html passes.
> > 
> > 
> > The bots fails because gl.vertexAttribPointer is not called after
> > gl.enableVertexAttribArray(0) before doing drawArray in
> > conformance/misc/webgl-specific.html. According WebGL spec, this behavior is
> not
> > correct. WDYT?
> 
> According to section 6.5 in webgl1 spec, you are correct.
> 
> I lost track when that section was added, but it seems to me if a vertex
attrib
> is enabled but not consumed by the program, then it should not require a
buffer
> to be bound in order to draw, just like it does not require the buffer to be
of
> a certain size.
> 
> @kbr: can you shed some light in this situation?
> 
> All implementations have passed webgl-specific.html, this seems like a big
> behavior change where we haven't added a test (actually we test the opposite
in
> webgl-specific.html)

Thanks zhenyao for your comments.
I can skip this validation if number of primitive to draw is 0. Or I can attach
buffer to VAO in webgl-specific.html after vertex attrib is enabled.
@kbr what's your opinion?

[Ken Russell (switch to Gerrit), 2016-06-01 00:08:27]

On 2016/05/31 04:55:18, qiankun wrote:
> On 2016/05/27 14:04:02, Zhenyao Mo wrote:
> > On 2016/05/27 11:48:32, qiankun wrote:
> > > PTAL.
> > > Also a fix in conformance test is added:
> > > https://github.com/KhronosGroup/WebGL/pull/1717. With these patches,
> > > conformance2/vertex_arrays/vertex-array-object.html passes.
> > > 
> > > 
> > > The bots fails because gl.vertexAttribPointer is not called after
> > > gl.enableVertexAttribArray(0) before doing drawArray in
> > > conformance/misc/webgl-specific.html. According WebGL spec, this behavior
is
> > not
> > > correct. WDYT?
> > 
> > According to section 6.5 in webgl1 spec, you are correct.
> > 
> > I lost track when that section was added, but it seems to me if a vertex
> attrib
> > is enabled but not consumed by the program, then it should not require a
> buffer
> > to be bound in order to draw, just like it does not require the buffer to be
> of
> > a certain size.
> > 
> > @kbr: can you shed some light in this situation?
> > 
> > All implementations have passed webgl-specific.html, this seems like a big
> > behavior change where we haven't added a test (actually we test the opposite
> in
> > webgl-specific.html)
> 
> Thanks zhenyao for your comments.
> I can skip this validation if number of primitive to draw is 0. Or I can
attach
> buffer to VAO in webgl-specific.html after vertex attrib is enabled.
> @kbr what's your opinion?

This is a good catch. It looks like this aspect of the WebGL 1.0 spec has never
been tested. The other aspects (a vertex attribute is enabled as an array, the
buffer is too small, but the attribute is not consumed) are tested, but not this
one.

It looks to me like there is simply a bug in webgl-specific.html. It looks like
the intent was that the created buffer be set up via vertexAttribPointer.
Qiankun, would you submit a pull request against
https://github.com/KhronosGroup/WebGL fixing this? Sorry for the trouble.

[Zhenyao Mo, 2016-06-01 00:12:10]

On 2016/06/01 00:08:27, Ken Russell wrote:
> On 2016/05/31 04:55:18, qiankun wrote:
> > On 2016/05/27 14:04:02, Zhenyao Mo wrote:
> > > On 2016/05/27 11:48:32, qiankun wrote:
> > > > PTAL.
> > > > Also a fix in conformance test is added:
> > > > https://github.com/KhronosGroup/WebGL/pull/1717. With these patches,
> > > > conformance2/vertex_arrays/vertex-array-object.html passes.
> > > > 
> > > > 
> > > > The bots fails because gl.vertexAttribPointer is not called after
> > > > gl.enableVertexAttribArray(0) before doing drawArray in
> > > > conformance/misc/webgl-specific.html. According WebGL spec, this
behavior
> is
> > > not
> > > > correct. WDYT?
> > > 
> > > According to section 6.5 in webgl1 spec, you are correct.
> > > 
> > > I lost track when that section was added, but it seems to me if a vertex
> > attrib
> > > is enabled but not consumed by the program, then it should not require a
> > buffer
> > > to be bound in order to draw, just like it does not require the buffer to
be
> > of
> > > a certain size.
> > > 
> > > @kbr: can you shed some light in this situation?
> > > 
> > > All implementations have passed webgl-specific.html, this seems like a big
> > > behavior change where we haven't added a test (actually we test the
opposite
> > in
> > > webgl-specific.html)
> > 
> > Thanks zhenyao for your comments.
> > I can skip this validation if number of primitive to draw is 0. Or I can
> attach
> > buffer to VAO in webgl-specific.html after vertex attrib is enabled.
> > @kbr what's your opinion?
> 
> This is a good catch. It looks like this aspect of the WebGL 1.0 spec has
never
> been tested. The other aspects (a vertex attribute is enabled as an array, the
> buffer is too small, but the attribute is not consumed) are tested, but not
this
> one.
> 
> It looks to me like there is simply a bug in webgl-specific.html. It looks
like
> the intent was that the created buffer be set up via vertexAttribPointer.
> Qiankun, would you submit a pull request against
> https://github.com/KhronosGroup/WebGL fixing this? Sorry for the trouble.

What should we do about the untested aspect? No browser is compliant in this
aspect (otherwise they won't be able to pass webgl-specific.html).

[qiankun, 2016-06-01 01:18:35]

On 2016/06/01 00:12:10, Zhenyao Mo wrote:
> > > > 
> > > > According to section 6.5 in webgl1 spec, you are correct.
> > > > 
> > > > I lost track when that section was added, but it seems to me if a vertex
> > > attrib
> > > > is enabled but not consumed by the program, then it should not require a
> > > buffer
> > > > to be bound in order to draw, just like it does not require the buffer
to
> be
> > > of
> > > > a certain size.
> > > > 
> > > > @kbr: can you shed some light in this situation?
> > > > 
> > > > All implementations have passed webgl-specific.html, this seems like a
big
> > > > behavior change where we haven't added a test (actually we test the
> opposite
> > > in
> > > > webgl-specific.html)
> > > 
> > > Thanks zhenyao for your comments.
> > > I can skip this validation if number of primitive to draw is 0. Or I can
> > attach
> > > buffer to VAO in webgl-specific.html after vertex attrib is enabled.
> > > @kbr what's your opinion?
> > 
> > This is a good catch. It looks like this aspect of the WebGL 1.0 spec has
> never
> > been tested. The other aspects (a vertex attribute is enabled as an array,
the
> > buffer is too small, but the attribute is not consumed) are tested, but not
> this
> > one.
> > 
> > It looks to me like there is simply a bug in webgl-specific.html. It looks
> like
> > the intent was that the created buffer be set up via vertexAttribPointer.
> > Qiankun, would you submit a pull request against
> > https://github.com/KhronosGroup/WebGL fixing this? Sorry for the trouble.
> 
> What should we do about the untested aspect? 
I can added a test to cover validation in this CL. But this will make other
browser fail on the test.

> No browser is compliant in this
> aspect (otherwise they won't be able to pass webgl-specific.html).
Setting up buffer via vertexAttribPointer won't break other browsers.

[Ken Russell (switch to Gerrit), 2016-06-01 01:25:35]

On 2016/06/01 01:18:35, qiankun wrote:
> On 2016/06/01 00:12:10, Zhenyao Mo wrote:
> > > > > 
> > > > > According to section 6.5 in webgl1 spec, you are correct.
> > > > > 
> > > > > I lost track when that section was added, but it seems to me if a
vertex
> > > > attrib
> > > > > is enabled but not consumed by the program, then it should not require
a
> > > > buffer
> > > > > to be bound in order to draw, just like it does not require the buffer
> to
> > be
> > > > of
> > > > > a certain size.
> > > > > 
> > > > > @kbr: can you shed some light in this situation?
> > > > > 
> > > > > All implementations have passed webgl-specific.html, this seems like a
> big
> > > > > behavior change where we haven't added a test (actually we test the
> > opposite
> > > > in
> > > > > webgl-specific.html)
> > > > 
> > > > Thanks zhenyao for your comments.
> > > > I can skip this validation if number of primitive to draw is 0. Or I can
> > > attach
> > > > buffer to VAO in webgl-specific.html after vertex attrib is enabled.
> > > > @kbr what's your opinion?
> > > 
> > > This is a good catch. It looks like this aspect of the WebGL 1.0 spec has
> > never
> > > been tested. The other aspects (a vertex attribute is enabled as an array,
> the
> > > buffer is too small, but the attribute is not consumed) are tested, but
not
> > this
> > > one.
> > > 
> > > It looks to me like there is simply a bug in webgl-specific.html. It looks
> > like
> > > the intent was that the created buffer be set up via vertexAttribPointer.
> > > Qiankun, would you submit a pull request against
> > > https://github.com/KhronosGroup/WebGL fixing this? Sorry for the trouble.
> > 
> > What should we do about the untested aspect? 
> I can added a test to cover validation in this CL. But this will make other
> browser fail on the test.

The test would need to go into https://github.com/KhronosGroup/WebGL. Could you
perhaps expand conformance2/vertex_arrays/vertex-array-object.html for this?

> > No browser is compliant in this
> > aspect (otherwise they won't be able to pass webgl-specific.html).
> Setting up buffer via vertexAttribPointer won't break other browsers.

I think Mo's point is not that changing webgl-specific.html won't break other
browsers, but that other browsers are already not spec-compliant.

Mo, I agree, this is an under-tested area. If the above conformance2/ test were
expanded, would that address your concern? Or would you like a test added to the
WebGL 1.0 conformance suite too?

[Zhenyao Mo, 2016-06-01 02:21:46]

On 2016/06/01 01:25:35, Ken Russell wrote:
> On 2016/06/01 01:18:35, qiankun wrote:
> > On 2016/06/01 00:12:10, Zhenyao Mo wrote:
> > > > > > 
> > > > > > According to section 6.5 in webgl1 spec, you are correct.
> > > > > > 
> > > > > > I lost track when that section was added, but it seems to me if a
> vertex
> > > > > attrib
> > > > > > is enabled but not consumed by the program, then it should not
require
> a
> > > > > buffer
> > > > > > to be bound in order to draw, just like it does not require the
buffer
> > to
> > > be
> > > > > of
> > > > > > a certain size.
> > > > > > 
> > > > > > @kbr: can you shed some light in this situation?
> > > > > > 
> > > > > > All implementations have passed webgl-specific.html, this seems like
a
> > big
> > > > > > behavior change where we haven't added a test (actually we test the
> > > opposite
> > > > > in
> > > > > > webgl-specific.html)
> > > > > 
> > > > > Thanks zhenyao for your comments.
> > > > > I can skip this validation if number of primitive to draw is 0. Or I
can
> > > > attach
> > > > > buffer to VAO in webgl-specific.html after vertex attrib is enabled.
> > > > > @kbr what's your opinion?
> > > > 
> > > > This is a good catch. It looks like this aspect of the WebGL 1.0 spec
has
> > > never
> > > > been tested. The other aspects (a vertex attribute is enabled as an
array,
> > the
> > > > buffer is too small, but the attribute is not consumed) are tested, but
> not
> > > this
> > > > one.
> > > > 
> > > > It looks to me like there is simply a bug in webgl-specific.html. It
looks
> > > like
> > > > the intent was that the created buffer be set up via
vertexAttribPointer.
> > > > Qiankun, would you submit a pull request against
> > > > https://github.com/KhronosGroup/WebGL fixing this? Sorry for the
trouble.
> > > 
> > > What should we do about the untested aspect? 
> > I can added a test to cover validation in this CL. But this will make other
> > browser fail on the test.
> 
> The test would need to go into https://github.com/KhronosGroup/WebGL. Could
you
> perhaps expand conformance2/vertex_arrays/vertex-array-object.html for this?
> 
> > > No browser is compliant in this
> > > aspect (otherwise they won't be able to pass webgl-specific.html).
> > Setting up buffer via vertexAttribPointer won't break other browsers.
> 
> I think Mo's point is not that changing webgl-specific.html won't break other
> browsers, but that other browsers are already not spec-compliant.
> 
> Mo, I agree, this is an under-tested area. If the above conformance2/ test
were
> expanded, would that address your concern? Or would you like a test added to
the
> WebGL 1.0 conformance suite too?

My concern is we add test and all browsers change behavior to be conformant, and
that might break content.

It's a pity that in this webgl-specific.html we tested the opposite behavior of
what the spec says, therefore "seducing" browsers to that wrong behavior.

I actually prefer to changing the spec to conform to the current tested behavior
(conformance test can be considered as part of spec too). This is the lowest
risk in my opinion.

[Zhenyao Mo, 2016-06-01 02:38:12]

On 2016/06/01 02:21:46, Zhenyao Mo wrote:
> On 2016/06/01 01:25:35, Ken Russell wrote:
> > On 2016/06/01 01:18:35, qiankun wrote:
> > > On 2016/06/01 00:12:10, Zhenyao Mo wrote:
> > > > > > > 
> > > > > > > According to section 6.5 in webgl1 spec, you are correct.
> > > > > > > 
> > > > > > > I lost track when that section was added, but it seems to me if a
> > vertex
> > > > > > attrib
> > > > > > > is enabled but not consumed by the program, then it should not
> require
> > a
> > > > > > buffer
> > > > > > > to be bound in order to draw, just like it does not require the
> buffer
> > > to
> > > > be
> > > > > > of
> > > > > > > a certain size.
> > > > > > > 
> > > > > > > @kbr: can you shed some light in this situation?
> > > > > > > 
> > > > > > > All implementations have passed webgl-specific.html, this seems
like
> a
> > > big
> > > > > > > behavior change where we haven't added a test (actually we test
the
> > > > opposite
> > > > > > in
> > > > > > > webgl-specific.html)
> > > > > > 
> > > > > > Thanks zhenyao for your comments.
> > > > > > I can skip this validation if number of primitive to draw is 0. Or I
> can
> > > > > attach
> > > > > > buffer to VAO in webgl-specific.html after vertex attrib is enabled.
> > > > > > @kbr what's your opinion?
> > > > > 
> > > > > This is a good catch. It looks like this aspect of the WebGL 1.0 spec
> has
> > > > never
> > > > > been tested. The other aspects (a vertex attribute is enabled as an
> array,
> > > the
> > > > > buffer is too small, but the attribute is not consumed) are tested,
but
> > not
> > > > this
> > > > > one.
> > > > > 
> > > > > It looks to me like there is simply a bug in webgl-specific.html. It
> looks
> > > > like
> > > > > the intent was that the created buffer be set up via
> vertexAttribPointer.
> > > > > Qiankun, would you submit a pull request against
> > > > > https://github.com/KhronosGroup/WebGL fixing this? Sorry for the
> trouble.
> > > > 
> > > > What should we do about the untested aspect? 
> > > I can added a test to cover validation in this CL. But this will make
other
> > > browser fail on the test.
> > 
> > The test would need to go into https://github.com/KhronosGroup/WebGL. Could
> you
> > perhaps expand conformance2/vertex_arrays/vertex-array-object.html for this?
> > 
> > > > No browser is compliant in this
> > > > aspect (otherwise they won't be able to pass webgl-specific.html).
> > > Setting up buffer via vertexAttribPointer won't break other browsers.
> > 
> > I think Mo's point is not that changing webgl-specific.html won't break
other
> > browsers, but that other browsers are already not spec-compliant.
> > 
> > Mo, I agree, this is an under-tested area. If the above conformance2/ test
> were
> > expanded, would that address your concern? Or would you like a test added to
> the
> > WebGL 1.0 conformance suite too?
> 
> My concern is we add test and all browsers change behavior to be conformant,
and
> that might break content.
> 
> It's a pity that in this webgl-specific.html we tested the opposite behavior
of
> what the spec says, therefore "seducing" browsers to that wrong behavior.
> 
> I actually prefer to changing the spec to conform to the current tested
behavior
> (conformance test can be considered as part of spec too). This is the lowest
> risk in my opinion.

It seems WebGL2 conformance tests are already testing the spec behavior. Now I
really don't know which is better, making WebGL1/2 differ in this aspect, or
changing WebGL1 test to force all browsers to change behaviors, likely breaking
some contents.

[Zhenyao Mo, 2016-06-01 02:38:34]

On 2016/06/01 02:38:12, Zhenyao Mo wrote:
> On 2016/06/01 02:21:46, Zhenyao Mo wrote:
> > On 2016/06/01 01:25:35, Ken Russell wrote:
> > > On 2016/06/01 01:18:35, qiankun wrote:
> > > > On 2016/06/01 00:12:10, Zhenyao Mo wrote:
> > > > > > > > 
> > > > > > > > According to section 6.5 in webgl1 spec, you are correct.
> > > > > > > > 
> > > > > > > > I lost track when that section was added, but it seems to me if
a
> > > vertex
> > > > > > > attrib
> > > > > > > > is enabled but not consumed by the program, then it should not
> > require
> > > a
> > > > > > > buffer
> > > > > > > > to be bound in order to draw, just like it does not require the
> > buffer
> > > > to
> > > > > be
> > > > > > > of
> > > > > > > > a certain size.
> > > > > > > > 
> > > > > > > > @kbr: can you shed some light in this situation?
> > > > > > > > 
> > > > > > > > All implementations have passed webgl-specific.html, this seems
> like
> > a
> > > > big
> > > > > > > > behavior change where we haven't added a test (actually we test
> the
> > > > > opposite
> > > > > > > in
> > > > > > > > webgl-specific.html)
> > > > > > > 
> > > > > > > Thanks zhenyao for your comments.
> > > > > > > I can skip this validation if number of primitive to draw is 0. Or
I
> > can
> > > > > > attach
> > > > > > > buffer to VAO in webgl-specific.html after vertex attrib is
enabled.
> > > > > > > @kbr what's your opinion?
> > > > > > 
> > > > > > This is a good catch. It looks like this aspect of the WebGL 1.0
spec
> > has
> > > > > never
> > > > > > been tested. The other aspects (a vertex attribute is enabled as an
> > array,
> > > > the
> > > > > > buffer is too small, but the attribute is not consumed) are tested,
> but
> > > not
> > > > > this
> > > > > > one.
> > > > > > 
> > > > > > It looks to me like there is simply a bug in webgl-specific.html. It
> > looks
> > > > > like
> > > > > > the intent was that the created buffer be set up via
> > vertexAttribPointer.
> > > > > > Qiankun, would you submit a pull request against
> > > > > > https://github.com/KhronosGroup/WebGL fixing this? Sorry for the
> > trouble.
> > > > > 
> > > > > What should we do about the untested aspect? 
> > > > I can added a test to cover validation in this CL. But this will make
> other
> > > > browser fail on the test.
> > > 
> > > The test would need to go into https://github.com/KhronosGroup/WebGL.
Could
> > you
> > > perhaps expand conformance2/vertex_arrays/vertex-array-object.html for
this?
> > > 
> > > > > No browser is compliant in this
> > > > > aspect (otherwise they won't be able to pass webgl-specific.html).
> > > > Setting up buffer via vertexAttribPointer won't break other browsers.
> > > 
> > > I think Mo's point is not that changing webgl-specific.html won't break
> other
> > > browsers, but that other browsers are already not spec-compliant.
> > > 
> > > Mo, I agree, this is an under-tested area. If the above conformance2/ test
> > were
> > > expanded, would that address your concern? Or would you like a test added
to
> > the
> > > WebGL 1.0 conformance suite too?
> > 
> > My concern is we add test and all browsers change behavior to be conformant,
> and
> > that might break content.
> > 
> > It's a pity that in this webgl-specific.html we tested the opposite behavior
> of
> > what the spec says, therefore "seducing" browsers to that wrong behavior.
> > 
> > I actually prefer to changing the spec to conform to the current tested
> behavior
> > (conformance test can be considered as part of spec too). This is the lowest
> > risk in my opinion.
> 
> It seems WebGL2 conformance tests are already testing the spec behavior. Now I
> really don't know which is better, making WebGL1/2 differ in this aspect, or
> changing WebGL1 test to force all browsers to change behaviors, likely
breaking
> some contents.

Or should I say which is worse. :(

[Ken Russell (switch to Gerrit), 2016-06-01 02:42:44]

On 2016/06/01 02:38:34, Zhenyao Mo wrote:
> On 2016/06/01 02:38:12, Zhenyao Mo wrote:
> > On 2016/06/01 02:21:46, Zhenyao Mo wrote:
> > > On 2016/06/01 01:25:35, Ken Russell wrote:
> > > > On 2016/06/01 01:18:35, qiankun wrote:
> > > > > On 2016/06/01 00:12:10, Zhenyao Mo wrote:
> > > > > > > > > 
> > > > > > > > > According to section 6.5 in webgl1 spec, you are correct.
> > > > > > > > > 
> > > > > > > > > I lost track when that section was added, but it seems to me
if
> a
> > > > vertex
> > > > > > > > attrib
> > > > > > > > > is enabled but not consumed by the program, then it should not
> > > require
> > > > a
> > > > > > > > buffer
> > > > > > > > > to be bound in order to draw, just like it does not require
the
> > > buffer
> > > > > to
> > > > > > be
> > > > > > > > of
> > > > > > > > > a certain size.
> > > > > > > > > 
> > > > > > > > > @kbr: can you shed some light in this situation?
> > > > > > > > > 
> > > > > > > > > All implementations have passed webgl-specific.html, this
seems
> > like
> > > a
> > > > > big
> > > > > > > > > behavior change where we haven't added a test (actually we
test
> > the
> > > > > > opposite
> > > > > > > > in
> > > > > > > > > webgl-specific.html)
> > > > > > > > 
> > > > > > > > Thanks zhenyao for your comments.
> > > > > > > > I can skip this validation if number of primitive to draw is 0.
Or
> I
> > > can
> > > > > > > attach
> > > > > > > > buffer to VAO in webgl-specific.html after vertex attrib is
> enabled.
> > > > > > > > @kbr what's your opinion?
> > > > > > > 
> > > > > > > This is a good catch. It looks like this aspect of the WebGL 1.0
> spec
> > > has
> > > > > > never
> > > > > > > been tested. The other aspects (a vertex attribute is enabled as
an
> > > array,
> > > > > the
> > > > > > > buffer is too small, but the attribute is not consumed) are
tested,
> > but
> > > > not
> > > > > > this
> > > > > > > one.
> > > > > > > 
> > > > > > > It looks to me like there is simply a bug in webgl-specific.html.
It
> > > looks
> > > > > > like
> > > > > > > the intent was that the created buffer be set up via
> > > vertexAttribPointer.
> > > > > > > Qiankun, would you submit a pull request against
> > > > > > > https://github.com/KhronosGroup/WebGL fixing this? Sorry for the
> > > trouble.
> > > > > > 
> > > > > > What should we do about the untested aspect? 
> > > > > I can added a test to cover validation in this CL. But this will make
> > other
> > > > > browser fail on the test.
> > > > 
> > > > The test would need to go into https://github.com/KhronosGroup/WebGL.
> Could
> > > you
> > > > perhaps expand conformance2/vertex_arrays/vertex-array-object.html for
> this?
> > > > 
> > > > > > No browser is compliant in this
> > > > > > aspect (otherwise they won't be able to pass webgl-specific.html).
> > > > > Setting up buffer via vertexAttribPointer won't break other browsers.
> > > > 
> > > > I think Mo's point is not that changing webgl-specific.html won't break
> > other
> > > > browsers, but that other browsers are already not spec-compliant.
> > > > 
> > > > Mo, I agree, this is an under-tested area. If the above conformance2/
test
> > > were
> > > > expanded, would that address your concern? Or would you like a test
added
> to
> > > the
> > > > WebGL 1.0 conformance suite too?
> > > 
> > > My concern is we add test and all browsers change behavior to be
conformant,
> > and
> > > that might break content.
> > > 
> > > It's a pity that in this webgl-specific.html we tested the opposite
behavior
> > of
> > > what the spec says, therefore "seducing" browsers to that wrong behavior.
> > > 
> > > I actually prefer to changing the spec to conform to the current tested
> > behavior
> > > (conformance test can be considered as part of spec too). This is the
lowest
> > > risk in my opinion.
> > 
> > It seems WebGL2 conformance tests are already testing the spec behavior. Now
I
> > really don't know which is better, making WebGL1/2 differ in this aspect, or
> > changing WebGL1 test to force all browsers to change behaviors, likely
> breaking
> > some contents.
> 
> Or should I say which is worse. :(

Thanks for carefully considering this Mo. I think you're right that there is a
significant compatibility risk here.

I'll send an email to the working group raising this as an issue. Perhaps the
spec text should be removed, or moved to the WebGL 2.0 spec.

[Ken Russell (switch to Gerrit), 2016-06-01 20:55:24]

https://codereview.chromium.org/2019513004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
(right):

https://codereview.chromium.org/2019513004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:113: bool
WebGLVertexArrayObjectBase::isAllEnabledAttribBufferBound()
This function is potentially very hot -- it will be called during each draw
call. Have you benchmarked this change with something like the Animometer 3D
benchmark:
http://trac.webkit.org/export/HEAD/trunk/PerformanceTests/Animometer/develope...
?

I think it is worth optimizing this now, rather than waiting to see if there are
performance bugs with it. There can be a cached value for the result of this
call. The cache needs to be recomputed if enableVertexAttribArray(i) is called
and m_arrayBufferList[i] is null, or if unbindBuffer gets called. (It's not
legal to call vertexAttribPointer in WebGL unless there is a bound ARRAY_BUFFER,
so the only reason the state would be reset to this situation is that a buffer
is deleted, I think.) 

Could you please double-check my thinking here, implement this and benchmark it?
Thanks.

[qiankun, 2016-06-06 11:28:36]

https://codereview.chromium.org/2019513004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
(right):

https://codereview.chromium.org/2019513004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:113: bool
WebGLVertexArrayObjectBase::isAllEnabledAttribBufferBound()
On 2016/06/01 20:55:24, Ken Russell wrote:
> This function is potentially very hot -- it will be called during each draw
> call. Have you benchmarked this change with something like the Animometer 3D
> benchmark:
>
http://trac.webkit.org/export/HEAD/trunk/PerformanceTests/Animometer/develope...
> ?
> 
> I think it is worth optimizing this now, rather than waiting to see if there
are
> performance bugs with it. There can be a cached value for the result of this
> call. The cache needs to be recomputed if enableVertexAttribArray(i) is called
> and m_arrayBufferList[i] is null, or if unbindBuffer gets called. (It's not
> legal to call vertexAttribPointer in WebGL unless there is a bound
ARRAY_BUFFER,
> so the only reason the state would be reset to this situation is that a buffer
> is deleted, I think.) 
> 
> Could you please double-check my thinking here, implement this and benchmark
it?
> Thanks.

Thanks for your suggestion. I updated this CL to support cache. PTAL.
I also tested Animometer-WebGL tests. By targeting 60FPS and keeping other
options unchanged, I got following socres:
no change: 11578.37
change without cache: 12602.25
change with cache: 12344.05

Suppose the score should be the higher the better. So I think this CL doesn't
take effect on performance.

[Ken Russell (switch to Gerrit), 2016-06-07 04:18:57]

On 2016/06/06 11:28:36, qiankun wrote:
>
https://codereview.chromium.org/2019513004/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2019513004/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:113:
bool
> WebGLVertexArrayObjectBase::isAllEnabledAttribBufferBound()
> On 2016/06/01 20:55:24, Ken Russell wrote:
> > This function is potentially very hot -- it will be called during each draw
> > call. Have you benchmarked this change with something like the Animometer 3D
> > benchmark:
> >
>
http://trac.webkit.org/export/HEAD/trunk/PerformanceTests/Animometer/develope...
> > ?
> > 
> > I think it is worth optimizing this now, rather than waiting to see if there
> are
> > performance bugs with it. There can be a cached value for the result of this
> > call. The cache needs to be recomputed if enableVertexAttribArray(i) is
called
> > and m_arrayBufferList[i] is null, or if unbindBuffer gets called. (It's not
> > legal to call vertexAttribPointer in WebGL unless there is a bound
> ARRAY_BUFFER,
> > so the only reason the state would be reset to this situation is that a
buffer
> > is deleted, I think.) 
> > 
> > Could you please double-check my thinking here, implement this and benchmark
> it?
> > Thanks.
> 
> Thanks for your suggestion. I updated this CL to support cache. PTAL.
> I also tested Animometer-WebGL tests. By targeting 60FPS and keeping other
> options unchanged, I got following socres:
> no change: 11578.37
> change without cache: 12602.25
> change with cache: 12344.05
> 
> Suppose the score should be the higher the better. So I think this CL doesn't
> take effect on performance.

Thanks for checking this. Could you please build the browser in Release mode and
run the following?

./tools/perf/run_benchmark --browser=release smoothness.tough_webgl_cases >
output.txt

Look for "RESULT mean_frame_time" for the animometer benchmark. Can you see any
difference with/without your patch?

[Ken Russell (switch to Gerrit), 2016-06-07 04:25:11]

https://codereview.chromium.org/2019513004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
(right):

https://codereview.chromium.org/2019513004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:107:
updateAttribBufferBoundStatus();
It should be possible to skip this call to updateAttribBufferBoundStatus in at
least some situations.

 - m_isAllEnabledAttribBufferBound is true, enabled is true, and
m_arrayBufferList[index] is non-null
 - m_isAllEnabledAttribBufferBound is true, and enabled is false

Maybe this doesn't matter. Let's see what the actual perf test (not the
Animometer benchmark's score) reveals.

https://codereview.chromium.org/2019513004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h
(right):

https://codereview.chromium.org/2019513004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h:39: void
updateAttribBufferBoundStatus();
updateAttribBufferBoundStatus should be private, or protected if it can't be
private.

[qiankun, 2016-06-07 05:01:35]

https://codereview.chromium.org/2019513004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp
(right):

https://codereview.chromium.org/2019513004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.cpp:107:
updateAttribBufferBoundStatus();
On 2016/06/07 04:25:11, Ken Russell wrote:
> It should be possible to skip this call to updateAttribBufferBoundStatus in at
> least some situations.
> 
>  - m_isAllEnabledAttribBufferBound is true, enabled is true, and
> m_arrayBufferList[index] is non-null
>  - m_isAllEnabledAttribBufferBound is true, and enabled is false
> 
> Maybe this doesn't matter. Let's see what the actual perf test (not the
> Animometer benchmark's score) reveals.

no change: RESULT mean_frame_time:
http___kenrussell.github.io_webgl-animometer_Animometer_tests_3d_webgl.html=
60.022 ms
change without cache: RESULT mean_frame_time:
http___kenrussell.github.io_webgl-animometer_Animometer_tests_3d_webgl.html=
60.319 ms 
change with cache: RESULT mean_frame_time:
http___kenrussell.github.io_webgl-animometer_Animometer_tests_3d_webgl.html=
61.105 ms

https://codereview.chromium.org/2019513004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h
(right):

https://codereview.chromium.org/2019513004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLVertexArrayObjectBase.h:39: void
updateAttribBufferBoundStatus();
On 2016/06/07 04:25:11, Ken Russell wrote:
> updateAttribBufferBoundStatus should be private, or protected if it can't be
> private.

Done.

[Ken Russell (switch to Gerrit), 2016-06-07 05:26:23]

Thanks for testing. LGTM

[qiankun, 2016-06-07 05:28:20]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-07 05:28:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019513004/60001

[commit-bot: I haz the power, 2016-06-07 07:17:03]

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

[commit-bot: I haz the power, 2016-06-07 07:17:04]

Dry run: This issue passed the CQ dry run.

[qiankun, 2016-06-07 08:00:55]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2016-06-07 08:02:08]

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

[commit-bot: I haz the power, 2016-06-07 08:09:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-07 08:11:27]

Description was changed from

==========
Validate bound buffer for draw calls

DrawArrays or drawElements should generate invalid operation error if no
buffer is bound to enabled attribution. Refer WebGL 1.0 spec for details
in section 6.5 "Enabled Vertex Attributes and Range Checking".

BUG=295792
TEST=conformance2/vertex_arrays/vertex-array-object.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Validate bound buffer for draw calls

DrawArrays or drawElements should generate invalid operation error if no
buffer is bound to enabled attribution. Refer WebGL 1.0 spec for details
in section 6.5 "Enabled Vertex Attributes and Range Checking".

BUG=295792
TEST=conformance2/vertex_arrays/vertex-array-object.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-06-07 08:11:28]

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