command_buffer: Implement path rendering functions for CHROMIUM_path_rendering

command_buffer: Implement path rendering functions for CHROMIUM_path_rendering

Add support for path rendering functions with CHROMIUM_path_rendering in
gpu command buffer. CHROMIUM_path_rendering pseudo extension will use
NV_path_rendering (NVPR).

After this patch, CHROMIUM_path_rendering is able to draw solid color
paths.

Adds following functions to the command buffer:
glGenPathsCHROMIUM
glDeletePathsCHROMIUM
glIsPathCHROMIUM
glPathCommandsCHROMIUM
glPathParameterfCHROMIUM
glPathParameteriCHROMIUM
glPathStencilFuncCHROMIUM
glStencilFillPathCHROMIUM
glStencilStrokePathCHROMIUM
glCoverFillPathCHROMIUM
glCoverStrokePathCHROMIUM
glStencilThenCoverFillPathCHROMIUM
glStencilThenCoverStrokePathCHROMIUM

Noteworthy items:

NVPR has calls with multiple data arrays. These will be passed by
memcpy:ing the arrays into the same transfer buffer. The arrays
containing GLfloats are copied first, because floats have more strict
alignment needs than whatever is in the other array.

The functions exposed by the command buffer take in subset of the values
that plain NVPR functions accept. Also some functions do not take
exact same amount of parameters, rather only those parameters that
are expected to vary based on Skia implementation.

The change does not add vertexshader-less shader program support to the
command buffer. The caller is expected to use a dummy vertex
shader. NVPR calls will ignore the vertex shader.

Adds gpu::gles2::PathManager class that should be similar to other
resource manager classes. The intention is that paths can be shared in
similar manner as other resources.

Changes generator to generate feature_info check for Is -type of
functions.

Moves the path matrix related constants from glchromium.h to
gl2extchromium.h, the constants are not "normal" GLES constants, rather
they are part of the extension.

Skia will not yet use the extension, because texturing support and
instanced variants of the draw functions are not exposed by
CHROMIUM_path_rendering.

The extension will be available through flag --enable-gl-path-rendering
BUG=344330
Committed: https://crrev.com/b959a8467d84feb3431809fe4abafb9649c44b24
Cr-Commit-Position: refs/heads/master@{#338666}

[vmiura, 2014-04-17 20:54:02]

What is the status of this patch?  Is there any input you need from Chrome guys
at this time?

Thanks, Victor

[Kimmo Kinnunen, 2014-04-21 18:45:04]

On 2014/04/17 20:54:02, vmiura wrote:
> What is the status of this patch?  Is there any input you need from Chrome
guys
> at this time?

No input needed for now. 
I'll need to rebase this and do some skis patches according to the changes done
in the other patch. I expect to this to be done end of this week..

[Kimmo Kinnunen, 2014-04-23 12:28:58]

This is now rebased and ready for review. However, it still lacks the TODO item,
the unittest result verification for the *Instanced* call variants. Feel free to
either review or ignore until that's been done.

[Kimmo Kinnunen, 2014-04-23 12:44:22]

https://codereview.chromium.org/169403005/diff/160001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/160001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:95: uint
GenPathsCHROMIUM(sizei range);
TODO: I'll write the extension text for these.

[piman, 2014-04-24 22:26:35]

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:183: allowed by the
StencilFunc command.
Why do we need a separate state from the regular GL state?

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:362:
CoverStrokePathCHROMIUM.
Can you describe how these texture coordinates interact with the GL state?
Is it expected that they will be picked up by a fragment shader? How are they
bound to input varyings? Are there additions to the glsl language to support
this?
Since my understanding is that Cover*Path* skips the vertex shader, what does it
mean exactly wrt a fully linked program? What happens to the fragment shader's
input varyings that are not filled by the vertex shader + rasterizer?

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:383: determined by the
/genMode/ in the same manner as PathColorGenCHROMIUM's
PathColorGenCHROMIUM isn't in this extension.

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:476: void
CoverFillPathCHROMIUM(uint path, enum coverMode)
Do we need coverMode if there can only be BOUNDING_BOX_CHROMIUM?

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:482: framebuffer
guaranteed to include all samples that would be have a net
typo: "would be have" ->  "would have"

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:546: enum
pathNameType, const void *paths,
since pathNameType is always UNSIGNED_INT, why not omitting it and have paths be
a GLuint*?

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:577: bool stopEarly =
getPathName(pathNameType, paths, pathBase, pathName);
getPathName returns an int instead of a bool... make it consistent?

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:823: void
CoverStrokePathInstancedCHROMIUM(sizei numPaths,
Would it make sense to combine this with CoverFillPathInstancedCHROMIUM, and
having an enum to specify fill vs stroke?
It would save a bit on API footprint (thinking testing, security validation,
etc.)

Similarly maybe for CoverFillPathCHROMIUM vs CoverStrokePathCHROMIUM

I'd suggest the Stencil* versions as well, but they have different parameters
between the stroke and the fill versions, so it probably doesn't make as much
sense.

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:868: MOVE_TO_NV       
          Absolute move          'M'         2
nit: _CHROMIUM (here and below)

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:2694: 'immediate': False, # FIXME:
cannot create immediate command
nit: in chromium code, we use TODO rather than FIXME.

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4389: case
GL_MAX_TEXTURE_COORDS:
_CHROMIUM

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4389: case
GL_MAX_TEXTURE_COORDS:
can you DCHECK that the extension is present in the FeatureInfo?

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10848: error::Error
GLES2DecoderImpl::HandleGenPathsCHROMIUM(
We'll want unit tests for all these new functions, testing both valid and
invalid input.

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10890: GLsizei numCommands =
static_cast<GLsizei>(c.numCommands);
nit: please use unix_style for local variables (here and many instances below).

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10920: service_id, numCommands,
commands, numCoords, coordType, coords);
I would like us to validate the syntax of the commands and the corresponding
number of coords before we pass them to the driver.

1- it guarantees we only give valid values to the driver (error paths are
notoriously untested in drivers, and we can't test all versions of all drivers)
2- it guarantees consistent errors wrt other implementations (which, in the
future, may not all be based on NV_path_rendering)
3- it allows proper unit tests for the error conditions.

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10951: // TODO: value not
checked.
We need to.

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10983: // TODO: value not
checked.
We need to.

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11121: const GLfloat*
transforms;
nit: there are paths in PrepareInstancedPathCommand that don't initialize
paths/transforms. Please initialize to NULL here.

Same in other instances below.

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11279: sizeof(GLfloat) *
components * (genMode == GL_EYE_LINEAR_CHROMIUM ? 4 : 3);
We'll want a safe multiply here. base::CheckedNumeric is the way to go.

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11281: if (coeffs_size < 0) {
This can't happen.

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:30: // Creates a path
nit: period at the end of comments (here and below.)

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_chromium_path_rendering_unittest.cc (right):

https://codereview.chromium.org/169403005/diff/180001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_chromium_path_rendering_unittest.cc:230:
TEST_F(CHROMIUMPathRenderingTest, TestTexCoordShader) {
We'll have to skip those tests if the extension isn't present

[Kimmo Kinnunen, 2014-04-25 12:46:29]

Thanks. I updated the .txt from the points that I could.

Wrt the "API discussion".
Deviating the CHROMIUM_path_rendering from NV_path_rendering because "we
need/better/re-designed/general API" has been covered in the design phase, but
rather subtly and briefly. Probably also not by agreeing anything. I tried to
lay out the current design intentions better in the document, see the similar
"API discussion" section in then document below.

https://docs.google.com/document/d/1bpmw-VPEdODdtkNLMPUVzliFOMvTNENwuEs5zNOOE...

I'd appreciate if you'd fix the design doc the way you see this feature being
done. I'll refrain from making code changes until we clarify the API design
issue and the whole purpose of the API.

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:183: allowed by the
StencilFunc command.
On 2014/04/24 22:26:36, piman wrote:
> Why do we need a separate state from the regular GL state?

Tried to answer this in the added "Issues" section. (Different use-case in
practice, so values differ.)

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:362:
CoverStrokePathCHROMIUM.
On 2014/04/24 22:26:36, piman wrote:
> Can you describe how these texture coordinates interact with the GL state?
> Is it expected that they will be picked up by a fragment shader? How are they
> bound to input varyings? Are there additions to the glsl language to support
> this?

Good point, forgot these.

Added explanation that the values go to a new built-in variable array,
gl_TexCoord.

I added section for GLSL extension.

> Since my understanding is that Cover*Path* skips the vertex shader, what does
it
> mean exactly wrt a fully linked program? What happens to the fragment shader's
> input varyings that are not filled by the vertex shader + rasterizer?

Added an answer in the "Issues" section. (They should be zeros)

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:383: determined by the
/genMode/ in the same manner as PathColorGenCHROMIUM's
On 2014/04/24 22:26:36, piman wrote:
> PathColorGenCHROMIUM isn't in this extension.

Removed. Added some explanation above.

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:476: void
CoverFillPathCHROMIUM(uint path, enum coverMode)
On 2014/04/24 22:26:36, piman wrote:
> Do we need coverMode if there can only be BOUNDING_BOX_CHROMIUM?

This is the "API discussion", see below.

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:482: framebuffer
guaranteed to include all samples that would be have a net
On 2014/04/24 22:26:36, piman wrote:
> typo: "would be have" ->  "would have"

Done.

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:546: enum
pathNameType, const void *paths,
On 2014/04/24 22:26:36, piman wrote:
> since pathNameType is always UNSIGNED_INT, why not omitting it and have paths
be
> a GLuint*?

"API discussion"

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:577: bool stopEarly =
getPathName(pathNameType, paths, pathBase, pathName);
On 2014/04/24 22:26:36, piman wrote:
> getPathName returns an int instead of a bool... make it consistent?
Done. (Made getPathName to return (GL)bool).

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:823: void
CoverStrokePathInstancedCHROMIUM(sizei numPaths,
On 2014/04/24 22:26:36, piman wrote:
> Would it make sense to combine this with CoverFillPathInstancedCHROMIUM, and
> having an enum to specify fill vs stroke?
> It would save a bit on API footprint (thinking testing, security validation,
> etc.)
> 
> Similarly maybe for CoverFillPathCHROMIUM vs CoverStrokePathCHROMIUM
> 
> I'd suggest the Stencil* versions as well, but they have different parameters
> between the stroke and the fill versions, so it probably doesn't make as much
> sense.

This is the "API discussion".

https://codereview.chromium.org/169403005/diff/180001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:868: MOVE_TO_NV       
          Absolute move          'M'         2
On 2014/04/24 22:26:36, piman wrote:
> nit: _CHROMIUM (here and below)

Done.

[Kimmo Kinnunen, 2014-09-08 11:19:15]

ptal if you have time..

[Kimmo Kinnunen, 2014-10-02 07:48:39]

Still ready for review.

Updated to depend on:
https://codereview.chromium.org/621673002/

[vmiura, 2014-10-03 22:56:01]

I've started reviewing, but it may take some time.  Thanks.

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:157: specified path
parameter to that clamped value.
I don't see parameters specified to be clamped in Table 5.pathParameters.  Is
this intentional?

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:217: The /mask/
parameter controls what subset of stencil bits are affected
What are the constraints on /mask/ values?

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/gl2chromium.h
File gpu/GLES2/gl2chromium.h (right):

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/gl2chromium.h...
gpu/GLES2/gl2chromium.h:17: #define GL_FLAT_CHROMIUM 0x1D00
Why is GL_FLAT_CHROMIUM defined here, and other enums in gl2extchromium.h?

[Kimmo Kinnunen, 2014-10-06 11:26:14]

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:157: specified path
parameter to that clamped value.
On 2014/10/03 22:56:01, vmiura wrote:
> I don't see parameters specified to be clamped in Table 5.pathParameters.  Is
> this intentional?

Good catch, the sentence refers to a property that's not there in CHROMIUM_p_r.
Removed.

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:217: The /mask/
parameter controls what subset of stencil bits are affected
On 2014/10/03 22:56:01, vmiura wrote:
> What are the constraints on /mask/ values?

Depends on your viewpoint? See below.

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:237: the effective
/mask/+1 is not an integer power of two.
So from one perspective, all /mask/ values are valid, and it's error to have
/fillMode/ COUNT_UP or COUNT_DOWN with some masks.

From other perspective, /mask/+1 has to be integer power of two when /fillMode/
is COUNT_UP/DOWN.

Do we need to specify it in more detail, eg. which property is responsible for
the conflict?

Codewise, I think currently this is visible in the error message only, saying
"mask+1 is npot".

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/gl2chromium.h
File gpu/GLES2/gl2chromium.h (right):

https://codereview.chromium.org/169403005/diff/340001/gpu/GLES2/gl2chromium.h...
gpu/GLES2/gl2chromium.h:17: #define GL_FLAT_CHROMIUM 0x1D00
On 2014/10/03 22:56:01, vmiura wrote:
> Why is GL_FLAT_CHROMIUM defined here, and other enums in gl2extchromium.h?

Good point, I forgot it. Since they were renamed _CHROMIUM it should not be
there. Same with the PATH constants above.

All moved to gl2extchromium.h, added commit msg part related to moving the path
constants too.

[Kimmo Kinnunen, 2014-10-21 10:29:39]

BTW: do not hesitate to ask for splitting the patch, adding more testing,
rebasing or anything, should it make the review easier or more pleasant. I've
worked for quite long time for this already, so adding some effort is no big
deal.

[reed1, 2014-10-30 14:54:59]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-10-30 14:55:00]

api comments on the extension:

1. need support for conics. these are already enabled in skia, and soon we
expect them to be very high-frequency (e.g. will be used for ovals, round-rects,
etc.)

2. nit: initial/terminal cap types interesting, but could be dropped from our
perspective (i.e. we only ever support 1 cap type for both ends)

3. what is the defined behavior when stroking if GR_GL_PATH_STROKE_WIDTH is
zero?

[Kimmo Kinnunen, 2014-11-03 23:39:41]

Thanks for looking at this.
Btw: I'll be in Mountain View for BlinkOn (tue/wed) if anybody wants to discuss
the patches irl.

On 2014/10/30 14:55:00, reed1 wrote:
> api comments on the extension:
> 
> 1. need support for conics. these are already enabled in skia, and soon we
> expect them to be very high-frequency (e.g. will be used for ovals,
round-rects,
> etc.

Done.
 
> 2. nit: initial/terminal cap types interesting, but could be dropped from our
> perspective (i.e. we only ever support 1 cap type for both ends)

Done.
 
> 3. what is the defined behavior when stroking if GR_GL_PATH_STROKE_WIDTH is
> zero?

Added explanation that stroking a path with zero width stroke does not produce
any visible results.
Was there some gotcha wrt this definition that you are implying?

[vmiura, 2015-06-19 20:22:26]

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

[vmiura, 2015-06-19 20:25:19]

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

[vmiura, 2015-06-19 21:55:22]

Let's unblock NVidia on this.

piman@: could you please do a 2nd review, and add security reviewers?
sievers@: could you please review bindings?

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5850: if
(!base::IsValueInRangeForNumericType<int32_t>(range)) {
Should there be an error if (first_client_id + range) > GLuint range?

SafeAddUInt32 to check.

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5912: uint32 coords_size =
coord_type_size * num_coords;
if (!SafeMultiplyUint32(width, bytes_per_group, &coords_size)) {
  // error
}

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5914: ScopedTransferBufferPtr
buffer(coords_size + num_commands, helper_,
SafeAddUInt32 for "coords_size + num_commands".

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5924: memcpy(coords_addr,
coords, coords_size);
Memcpy from NULL pointer is undefined behavior I believe.

if (coords_size > 0)
  memcpy(...)

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5928: memcpy(commands_addr,
commands, num_commands);
Same here

if (num_commands > 0)
  memcpy(...)

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:336:
id_allocator_.FreeIDRange(first_id, range);
Does this require holding lock_?

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.cc (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
Now -> 2015

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
Now -> 2015

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager_unittest.cc (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager_unittest.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
Now -> 2015.

https://codereview.chromium.org/169403005/diff/680001/ppapi/c/dev/ppb_opengle...
File ppapi/c/dev/ppb_opengles2ext_dev.h (right):

https://codereview.chromium.org/169403005/diff/680001/ppapi/c/dev/ppb_opengle...
ppapi/c/dev/ppb_opengles2ext_dev.h:16: "PPB_OpenGLES2DrawBuffers(Dev);1.0"
Remove changes in this file?

https://codereview.chromium.org/169403005/diff/680001/ppapi/c/ppb_opengles2.h
File ppapi/c/ppb_opengles2.h (right):

https://codereview.chromium.org/169403005/diff/680001/ppapi/c/ppb_opengles2.h...
ppapi/c/ppb_opengles2.h:18: "PPB_OpenGLES2InstancedArrays;1.0"
Remove changes in this file?

[Kimmo Kinnunen, 2015-06-23 12:03:11]

Thanks for the review. How about now?

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5850: if
(!base::IsValueInRangeForNumericType<int32_t>(range)) {
On 2015/06/19 21:55:21, vmiura wrote:
> Should there be an error if (first_client_id + range) > GLuint range?
> 
> SafeAddUInt32 to check.

Done.

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5912: uint32 coords_size =
coord_type_size * num_coords;
On 2015/06/19 21:55:21, vmiura wrote:
> if (!SafeMultiplyUint32(width, bytes_per_group, &coords_size)) {
>   // error
> }

Done.

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5914: ScopedTransferBufferPtr
buffer(coords_size + num_commands, helper_,
On 2015/06/19 21:55:22, vmiura wrote:
> SafeAddUInt32 for "coords_size + num_commands".

Done.

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5924: memcpy(coords_addr,
coords, coords_size);
On 2015/06/19 21:55:22, vmiura wrote:
> Memcpy from NULL pointer is undefined behavior I believe.
> 
> if (coords_size > 0)
>   memcpy(...)

Done.

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5928: memcpy(commands_addr,
commands, num_commands);
On 2015/06/19 21:55:21, vmiura wrote:
> Same here
> 
> if (num_commands > 0)
>   memcpy(...)

Done.

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
File gpu/command_buffer/client/share_group.cc (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/clie...
gpu/command_buffer/client/share_group.cc:336:
id_allocator_.FreeIDRange(first_id, range);
On 2015/06/19 21:55:22, vmiura wrote:
> Does this require holding lock_?

Done.

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.cc (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2015/06/19 21:55:22, vmiura wrote:
> Now -> 2015

Done.

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2015/06/19 21:55:22, vmiura wrote:
> Now -> 2015

Done.

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager_unittest.cc (right):

https://codereview.chromium.org/169403005/diff/680001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager_unittest.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2015/06/19 21:55:22, vmiura wrote:
> Now -> 2015.

Done.

https://codereview.chromium.org/169403005/diff/680001/ppapi/c/dev/ppb_opengle...
File ppapi/c/dev/ppb_opengles2ext_dev.h (right):

https://codereview.chromium.org/169403005/diff/680001/ppapi/c/dev/ppb_opengle...
ppapi/c/dev/ppb_opengles2ext_dev.h:16: "PPB_OpenGLES2DrawBuffers(Dev);1.0"
On 2015/06/19 21:55:22, vmiura wrote:
> Remove changes in this file?

Done.

https://codereview.chromium.org/169403005/diff/680001/ppapi/c/ppb_opengles2.h
File ppapi/c/ppb_opengles2.h (right):

https://codereview.chromium.org/169403005/diff/680001/ppapi/c/ppb_opengles2.h...
ppapi/c/ppb_opengles2.h:18: "PPB_OpenGLES2InstancedArrays;1.0"
On 2015/06/19 21:55:22, vmiura wrote:
> Remove changes in this file?

Done.

[piman, 2015-06-24 03:27:45]

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:151: The error
INVALID_VALUE is generated if /numCoords/ or /numCommands/
nit: that'd typically be an INVALID_OPERATION error case.

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:155: allow.
Is there any way to query such limit, or ensure a minimum valid size? Forcing a
round trip on every path creation (to check for error) is very unfortunate.

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:561: zero these
undefined input varying values.
I'm not sure what that means... a program fails to link if the fragment shader
uses varyings that don't exist in the vertex shader (see 4.3.5 Varying in GLSL
ES spec
https://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf
). If there's no vertex shader, does it mean you must have no input varying in
the fragment shader?

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

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:1009: gl_version_info_->is_es3) {
Can we gate this behind a command-line flag? We're adding a significant security
surface with a complex extension, it's a good idea to be conservative here. Once
we prove the end-to-end benefits, we can turn on by default.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3368: if (first_service_id == 0)
{
Is it guaranteed tha first_service_id + range doesn't wrap? If so, can we
DCHECK? If not, can we fail here so that we know this won't cause issues later
in the PathManager?

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13854: GLsizei
num_coords_expected = 0;
You may need a CheckedNumeric<GLsizei> to detect / avoid overflow? Or another
way to guarantee an upper bound (e.g. on numCommands?)

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:14048: GLES2Util::IsNPOT(mask +
1)) {
Did you mean !GLES2Util::IsNPOT(mask + 1) ?

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:14157: GLES2Util::IsNPOT(mask +
1)) {
Same here, !GLES2Util::IsNPOT() ?

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.cc (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:11: void callDeletePaths(GLuint
first_id, GLuint range) {
nit: CallDeletePaths

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:21: first_id += irange + 1;
Why +1? It doesn't seem right, and on top of it you'd have a guaranteed overflow
if irange == std::numeric_limits<GLsizei>::max()

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:49: DCHECK(first_service_id > 0);
nit: also DCHECK(first_client_id > 0) - this was checked by the decoder.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:62: // returns > first_client_id.
FirstClientId returns something that was guaranteed to be >0 anyway (since it
comes from a value passed by the client, that we checked).

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:67: // first_service_id > 0.
Can you add DCHECKs for these?

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:85: }
I find this all a bit hard to follow. I would like to suggest an alternative:
1- insert the new range, unconditionally - this actually returns an iterator to
the new element
2- merge (if possible) with the previous range
3- merge (if possible) with the next range.

I think the code would be a bit simpler and symmetric, and would allow a single
map traversal (on insert).

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:49: GLuint last_client_id;
nit: is it really better than storing |range|? It seems it ends up adding a
bunch of +1/-1 which makes it hard to follow.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager_unittest.cc (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager_unittest.cc:53: // Check trying to a
remove non-existent paths does not crash.
nit: "trying to a remove" -> "trying to remove"

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager_unittest.cc:106: // deletes.
For this kind of somewhat complex data structure, it's useful to write a bool
PathManager::CheckConsistency() function that you could use here, that would
check:
1- each range is consistent (last>first)
2- there is no overlapping range
3- there are no contiguous ranges that should have been merged.

You can use in unit tests such as here and/or as pre/post sanity DCHECKs on all
functions that mutate the list.

[Kimmo Kinnunen, 2015-06-24 12:49:02]

Thanks for the thorough review. I did not change all suggestions yet, so if you
have time, see the replies. Uploaded a new version.

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:151: The error
INVALID_VALUE is generated if /numCoords/ or /numCommands/
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> nit: that'd typically be an INVALID_OPERATION error case.

Done.
Though, _typically_, the command buffer uses INVALID_VALUE for the integer
overflow errors. Maybe my wording here misleads that the parameters would be
checked individually. The 32-bit limiting checks indeed are INVALID_OPERATION.

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:155: allow.
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> Is there any way to query such limit, or ensure a minimum valid size? Forcing
a
> round trip on every path creation (to check for error) is very unfortunate.

Just so that I understand: the problem is that correct parameters might fail due
to implementation details. The expectation of callers would be to implement the
calling code so that the parameters are always correct by spec, and then just
omit the error check. Is this correct interpretation of the problem?

The limit is sizeof(coordType) * num_coords + num_commands not overflowing
32-bit uint. I could write that in the spec?

Though: why is it worry here, but not worry in other functions that have the
similar problem?

PathCommands is sort of similar to BufferData. BufferData, for example,
validates arguments for command buffer limitations and then fails (without the
error documented?). So you can have BufferData call with size == X to succeed
and you can have size == X+1 fail. If I understand correctly from the callers
perspective both should succeed if the first succeeds, and the caller should be
"allowed" to omit the error check.

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:561: zero these
undefined input varying values.
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> I'm not sure what that means... a program fails to link if the fragment shader
> uses varyings that don't exist in the vertex shader (see 4.3.5 Varying in GLSL
> ES spec
>
https://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf
> ). If there's no vertex shader, does it mean you must have no input varying in
> the fragment shader?

I think the spec is written in the way that it does not explicitly say that
there is no vertex shader. E.g. linking rules and such are agnostic to this, if
I read this correctly.

It says that vertex shader is *skipped* when Cover function is run. Then
somebody might point out that vertex shader fills the fragment shader varyings,
and then ask that what fills the varyings if vertex shader is skipped. This
answer is an answer to this question.

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

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:1009: gl_version_info_->is_es3) {
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> Can we gate this behind a command-line flag? We're adding a significant
security
> surface with a complex extension, it's a good idea to be conservative here.
Once
> we prove the end-to-end benefits, we can turn on by default.

Done.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3368: if (first_service_id == 0)
{
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> Is it guaranteed tha first_service_id + range doesn't wrap?

Yes.

> If so, can we DCHECK? 
Done.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13854: GLsizei
num_coords_expected = 0;
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> You may need a CheckedNumeric<GLsizei> to detect / avoid overflow? Or another
> way to guarantee an upper bound (e.g. on numCommands?)

Done.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:14048: GLES2Util::IsNPOT(mask +
1)) {
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> Did you mean !GLES2Util::IsNPOT(mask + 1) ?

IsNPOT means "Is Not Power of Two"

It is error if |mask + 1| is not power of two.

Thus IsNPOT(mask + 1) is the correct check.

The error message might have misled you, I'll fix it.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:14157: GLES2Util::IsNPOT(mask +
1)) {
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> Same here, !GLES2Util::IsNPOT() ?

IsNPOT is intended. Fixed the error message.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.cc (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:11: void callDeletePaths(GLuint
first_id, GLuint range) {
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> nit: CallDeletePaths

Done.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:21: first_id += irange + 1;
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> Why +1? It doesn't seem right, and on top of it you'd have a guaranteed
overflow
> if irange == std::numeric_limits<GLsizei>::max()

Definitely not right, oops.

Done, added a test to test it.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:49: DCHECK(first_service_id > 0);
On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> nit: also DCHECK(first_client_id > 0) - this was checked by the decoder.

Done.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:62: // returns > first_client_id.
On 2015/06/24 03:27:45, piman (Very slow to review) wrote:
> FirstClientId returns something that was guaranteed to be >0 anyway (since it
> comes from a value passed by the client, that we checked).

Done.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:67: // first_service_id > 0.
On 2015/06/24 03:27:45, piman (Very slow to review) wrote:
> Can you add DCHECKs for these?

Done. I removed the middle comment, as I don't even understand why I thought it
would be significant.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:85: }
On 2015/06/24 03:27:45, piman (Very slow to review) wrote:
> I find this all a bit hard to follow. I would like to suggest an alternative:
> 1- insert the new range, unconditionally - this actually returns an iterator
to
> the new element
> 2- merge (if possible) with the previous range
> 3- merge (if possible) with the next range.

Right..
Current code has the good property that the expected default case never inserts,
so there's no allocations nor destructions. With your proposal, it would always
insert. Most of the time it would end up erasing. Sometimes it would erase
twice.

> I think the code would be a bit simpler and symmetric, and would allow a
single
> map traversal (on insert).

I don't think there's much difference in the amount of traversals in practice.
This implementation tries to capture the "fast" path for the expected common
case..

Code wise, I think proposal would amount to moving the insert from below to the
top, removing lower_bound and replacing "return" above with erase.

If you still confirm that this is simpler in your eyes, is worth the tradeoff of
"slower" operation and thus has to be done, I will course do it...

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:49: GLuint last_client_id;
On 2015/06/24 03:27:45, piman (Very slow to review) wrote:
> nit: is it really better than storing |range|? It seems it ends up adding a
> bunch of +1/-1 which makes it hard to follow.

True, it's a bit tricky, but IIRC from the year ago, I tried to sketch out the
other variants too. Storing range would end up having |+ range - 1|, which was
more confusing for me.

Using |last_client_id| has the nice property that it is used in the tricky part
of the code, e.g. in the actual allocation. The range is just used during NVPR
delete path call. Also, |last_client_id| comes from the API, so it'd be shame to
convert it back to a range. Also, using |last_client_id| in the API is nice in
the way that it forces the caller to take the action related to overflow, so
there's one place less to have questions about the overflow.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager_unittest.cc (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager_unittest.cc:53: // Check trying to a
remove non-existent paths does not crash.
On 2015/06/24 03:27:45, piman (Very slow to review) wrote:
> nit: "trying to a remove" -> "trying to remove"

Done.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager_unittest.cc:106: // deletes.
On 2015/06/24 03:27:45, piman (Very slow to review) wrote:
> For this kind of somewhat complex data structure, it's useful to write a bool
> PathManager::CheckConsistency() function that you could use here, that would
> check:
> 1- each range is consistent (last>first)
> 2- there is no overlapping range
> 3- there are no contiguous ranges that should have been merged.
> 
> You can use in unit tests such as here and/or as pre/post sanity DCHECKs on
all
> functions that mutate the list.
Done.

[piman, 2015-06-24 22:50:18]

Some comments, but I don't see your latest patch (latest PS is #32 for me)

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:155: allow.
On 2015/06/24 12:49:01, Kimmo Kinnunen wrote:
> On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> > Is there any way to query such limit, or ensure a minimum valid size?
Forcing
> a
> > round trip on every path creation (to check for error) is very unfortunate.
> 
> Just so that I understand: the problem is that correct parameters might fail
due
> to implementation details. The expectation of callers would be to implement
the
> calling code so that the parameters are always correct by spec, and then just
> omit the error check. Is this correct interpretation of the problem?
> 
> The limit is sizeof(coordType) * num_coords + num_commands not overflowing
> 32-bit uint. I could write that in the spec?

Yes, please. If that's the limitation, then it is known statically and the
caller doesn't need to round-trip to the service side. The current wording
suggests that it can fail for arbitrary reasons (from the caller's point of
view) and so the caller has to call GetError which forces a round-trip because
it might fail on the service side.

> 
> Though: why is it worry here, but not worry in other functions that have the
> similar problem?
> 
> PathCommands is sort of similar to BufferData. BufferData, for example,
> validates arguments for command buffer limitations and then fails (without the
> error documented?). So you can have BufferData call with size == X to succeed
> and you can have size == X+1 fail. If I understand correctly from the callers
> perspective both should succeed if the first succeeds, and the caller should
be
> "allowed" to omit the error check.

glBufferData is allowed to fail with GL_OUT_OF_MEMORY, like all gl calls, but
it's not explicitly called in the spec for BufferData (instead mentioned at the
top of the spec that every call can fail with that).

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:561: zero these
undefined input varying values.
On 2015/06/24 12:49:01, Kimmo Kinnunen wrote:
> On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> > I'm not sure what that means... a program fails to link if the fragment
shader
> > uses varyings that don't exist in the vertex shader (see 4.3.5 Varying in
GLSL
> > ES spec
> >
>
https://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf
> > ). If there's no vertex shader, does it mean you must have no input varying
in
> > the fragment shader?
> 
> I think the spec is written in the way that it does not explicitly say that
> there is no vertex shader. E.g. linking rules and such are agnostic to this,
if
> I read this correctly.
> 
> It says that vertex shader is *skipped* when Cover function is run. Then
> somebody might point out that vertex shader fills the fragment shader
varyings,
> and then ask that what fills the varyings if vertex shader is skipped. This
> answer is an answer to this question.

Well... there's a lot more than the vertex shader that is skipped by these
calls, and it's not very explicit. Varyings don't come straight from the vertex
shader anyway (clipping, interpolation etc.). So I think we have to be a little
bit more precise than that. I guess there's 2 parts:

1- How do you define a program that's valid to use with these calls. It sounds
like you need a program with a vertex and a fragment shaders, that are well
defined with respect to each other (with the same linking rules) as if the
program was going to be used on regular GL primitives, even though the vertex
shader will never be called - that's fairly awkward, at least should be called
out?
2- How is the fragment shader called, and with which values for the inputs, both
built-in (gl_FragCoord, gl_FrontFacing) and user-defined. It sounds like you
want to leave the user-defined inputs undefined? Can you mention that
explicitly?

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:14048: GLES2Util::IsNPOT(mask +
1)) {
On 2015/06/24 12:49:02, Kimmo Kinnunen wrote:
> On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> > Did you mean !GLES2Util::IsNPOT(mask + 1) ?
> 
> IsNPOT means "Is Not Power of Two"
> 
> It is error if |mask + 1| is not power of two.
> 
> Thus IsNPOT(mask + 1) is the correct check.
> 
> The error message might have misled you, I'll fix it.

Ah, yes, my bad, this is fine.

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.cc (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:85: }
On 2015/06/24 12:49:02, Kimmo Kinnunen wrote:
> On 2015/06/24 03:27:45, piman (Very slow to review) wrote:
> > I find this all a bit hard to follow. I would like to suggest an
alternative:
> > 1- insert the new range, unconditionally - this actually returns an iterator
> to
> > the new element
> > 2- merge (if possible) with the previous range
> > 3- merge (if possible) with the next range.
> 
> Right..
> Current code has the good property that the expected default case never
inserts,
> so there's no allocations nor destructions. With your proposal, it would
always
> insert. Most of the time it would end up erasing. Sometimes it would erase
> twice.
> 
> > I think the code would be a bit simpler and symmetric, and would allow a
> single
> > map traversal (on insert).
> 
> I don't think there's much difference in the amount of traversals in practice.
> This implementation tries to capture the "fast" path for the expected common
> case..
> 
> Code wise, I think proposal would amount to moving the insert from below to
the
> top, removing lower_bound and replacing "return" above with erase.
> 
> If you still confirm that this is simpler in your eyes, is worth the tradeoff
of
> "slower" operation and thus has to be done, I will course do it...

TBH I spent about an hour yesterday reviewing this file. It's too much, and it
would benefit from simplification. Unless we have data that shows that this is
in the critical path (my suspicion is that it's way dominated by the
PathCommands part), I'd rather opt for readable/auditable - this is security
sensitive.

Part of it is that lower_bound returns something that is close but never quite
what we need, so you have to muck with iterators. Maybe there's another way to
write helper functions that wrap lower_bounds give you e.g. the range that
precedes first_client_id or something.

Or even go all the way and say that maybe it's not that useful to keep the
"range" structure and instead simply use a direct client_id->service_id
hash_map. It might even make GetPath (which is in the critical path) more
efficient. RemovePaths can very easily reconstruct ranges (if we want to save
calls to glDeletePathsNV).

[no sievers, 2015-06-24 23:56:24]

> sievers@: could you please review bindings?

just one nit really

https://codereview.chromium.org/169403005/diff/740001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/169403005/diff/740001/ui/gl/generate_bindings...
ui/gl/generate_bindings.py:240: 'extensions': ['GL_NV_path_rendering'] }],
My bad for not renaming the |extensions| field to something more explicit, but
this should now only really be needed if the GL headers don't specify the
functions in the expected way.

Can you try removing it here and everywhere below and see if the generated
bindings still look good?

[Kimmo Kinnunen, 2015-06-25 12:13:35]

Now uploaded a new patch, for real.

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:155: allow.
On 2015/06/24 22:50:18, piman (Very slow to review) wrote:
> On 2015/06/24 12:49:01, Kimmo Kinnunen wrote:
> > On 2015/06/24 03:27:44, piman (Very slow to review) wrote:
> > > Is there any way to query such limit, or ensure a minimum valid size?
> Forcing
> > a
> > > round trip on every path creation (to check for error) is very
unfortunate.
> > 
> > Just so that I understand: the problem is that correct parameters might fail
> due
> > to implementation details. The expectation of callers would be to implement
> the
> > calling code so that the parameters are always correct by spec, and then
just
> > omit the error check. Is this correct interpretation of the problem?
> > 
> > The limit is sizeof(coordType) * num_coords + num_commands not overflowing
> > 32-bit uint. I could write that in the spec?
> 
> Yes, please. If that's the limitation, then it is known statically and the
> caller doesn't need to round-trip to the service side. The current wording
> suggests that it can fail for arbitrary reasons (from the caller's point of
> view) and so the caller has to call GetError which forces a round-trip because
> it might fail on the service side.
> 
> > 
> > Though: why is it worry here, but not worry in other functions that have the
> > similar problem?
> > 
> > PathCommands is sort of similar to BufferData. BufferData, for example,
> > validates arguments for command buffer limitations and then fails (without
the
> > error documented?). So you can have BufferData call with size == X to
succeed
> > and you can have size == X+1 fail. If I understand correctly from the
callers
> > perspective both should succeed if the first succeeds, and the caller should
> be
> > "allowed" to omit the error check.
> 
> glBufferData is allowed to fail with GL_OUT_OF_MEMORY, like all gl calls, but
> it's not explicitly called in the spec for BufferData (instead mentioned at
the
> top of the spec that every call can fail with that).

Actually, I meant the GL_INVALID_OPERATION generated by BufferData due to
implementation-dependent limit on |size|, similar to INVALID_OPERATION generated
by PathCommands due to implementation-dependent limit on |numCoords|. And I
contrasted that it appeared to be problem for PathCommands but not BufferData. 
Anyhow, it's not important.

https://codereview.chromium.org/169403005/diff/740001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:561: zero these
undefined input varying values.
On 2015/06/24 22:50:18, piman (Very slow to review) wrote:
> 1- How do you define a program that's valid to use with these calls. It sounds
> like you need a program with a vertex and a fragment shaders, that are well
> defined with respect to each other (with the same linking rules) as if the
> program was going to be used on regular GL primitives, even though the vertex
> shader will never be called - that's fairly awkward, at least should be called
> out?
> 2- How is the fragment shader called, and with which values for the inputs,
both
> built-in (gl_FragCoord, gl_FrontFacing) and user-defined. It sounds like you
> want to leave the user-defined inputs undefined? Can you mention that
> explicitly?

I tried to address these in the .txt by adding a hunk related to path covering
rasterization details as well as few related question-answer pairs. I'm no gl
spec writer, though, so..

I also changed to specify that user-defined inputs would be forced zero. I hope
it's ok, and we can trust that current driver zeroes them and we do not need to
check that in the service side?

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.cc (right):

https://codereview.chromium.org/169403005/diff/740001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:85: }
On 2015/06/24 22:50:18, piman (Very slow to review) wrote:
> TBH I spent about an hour yesterday reviewing this file. It's too much, and it
> would benefit from simplification.

Sorry about spending your time.

I tried to simplify the code. See if it is more readable now, even though it
uses the original logic?

> Or even go all the way and say that maybe it's not that useful to keep the
> "range" structure and instead simply use a direct client_id->service_id
> hash_map. It might even make GetPath (which is in the critical path) more
> efficient. RemovePaths can very easily reconstruct ranges (if we want to save
> calls to glDeletePathsNV).

I do not think that will work. When the allocations go past 100k or 1M ids (can
not remember anymore), the per-id hashmap calls never return.

The problem is that the implementation has to support the whole id space in
order for the implementation to gain any sort of trust in quality. This then
necessitates testing the implementation for the whole id space. 

The hashmap does not scale too well, and thus it can not execute many of the
testcases needed to test this implementation. This was already a problem in the
client-side id allocator, which was using per-id mapping. I changed it to use
ranges.

Also in practice Skia will use somewhat big amount of IDs, though of course not
nearly the entire id space.

[Kimmo Kinnunen, 2015-06-25 12:15:23]

https://codereview.chromium.org/169403005/diff/740001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/169403005/diff/740001/ui/gl/generate_bindings...
ui/gl/generate_bindings.py:240: 'extensions': ['GL_NV_path_rendering'] }],
On 2015/06/24 23:56:24, sievers wrote:
> My bad for not renaming the |extensions| field to something more explicit, but
> this should now only really be needed if the GL headers don't specify the
> functions in the expected way.
> 
> Can you try removing it here and everywhere below and see if the generated
> bindings still look good?
> 

Done.

https://codereview.chromium.org/169403005/diff/760001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt (right):

https://codereview.chromium.org/169403005/diff/760001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:99: 32-bit uint.
Also changed this condition to INVALID_OPERATION, for consistency with the other
error.

https://codereview.chromium.org/169403005/diff/760001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_path_rendering.txt:113: 32-bit uint.
..these too.

[no sievers, 2015-06-25 18:53:47]

ui/gl lgtm

[piman, 2015-06-25 21:04:59]

lgtm + nits.

https://codereview.chromium.org/169403005/diff/780001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.cc (right):

https://codereview.chromium.org/169403005/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:62: LastServiceId(range) ==
first_service_id - 1u) {
nit: can you add a DCHECK_EQ(LastClientId(range), first_client_id - 1u)? That
follows from !HasPathsInRange, but it'd be useful to show it's safe to merge.

https://codereview.chromium.org/169403005/diff/780001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:46: // Should be used only by the
implememntation.
nit: implememntation->implementation

[Kimmo Kinnunen, 2015-06-26 08:00:16]

https://codereview.chromium.org/169403005/diff/780001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.cc (right):

https://codereview.chromium.org/169403005/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:62: LastServiceId(range) ==
first_service_id - 1u) {
On 2015/06/25 21:04:59, piman (Very slow to review) wrote:
> nit: can you add a DCHECK_EQ(LastClientId(range), first_client_id - 1u)? That
> follows from !HasPathsInRange, but it'd be useful to show it's safe to merge.

Done.

https://codereview.chromium.org/169403005/diff/780001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/780001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:46: // Should be used only by the
implememntation.
On 2015/06/25 21:04:59, piman (Very slow to review) wrote:
> nit: implememntation->implementation

Done.

[Kimmo Kinnunen, 2015-06-26 08:01:00]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-06-26 08:01:02]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/169403005/#ps800001
(title: "address review comments")

[commit-bot: I haz the power, 2015-06-26 08:01:42]

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

[commit-bot: I haz the power, 2015-06-26 08:01:51]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-06-26 08:03:12]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-06-26 08:24:47]

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

[commit-bot: I haz the power, 2015-06-26 08:24:49]

Exceeded global retry quota

[Kimmo Kinnunen, 2015-06-26 11:53:33]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-06-26 11:53:34]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/169403005/#ps820001
(title: "include <algorithm> for std::min for msvc")

[commit-bot: I haz the power, 2015-06-26 11:53:41]

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

[commit-bot: I haz the power, 2015-06-26 11:53:51]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-06-26 12:07:39]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-06-26 12:13:49]

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

[commit-bot: I haz the power, 2015-06-26 12:13:52]

Exceeded global retry quota

[Kimmo Kinnunen, 2015-06-26 13:39:21]

https://codereview.chromium.org/169403005/diff/860001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/860001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:80: template <typename T>
piman@: so I had to add this kind of complication instead of |it = erase(cit,
cit)|. Is this acceptable? Mac 10.6 failed, the erase works differently in c++98
vs c++11

https://codereview.chromium.org/169403005/diff/860001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/169403005/diff/860001/ui/gl/generate_bindings...
ui/gl/generate_bindings.py:41: the extension string does not change.
piman@, sievers@: so I had to add this.. Do you think this kind of thing can
work? Any idea how this can be checked in feature_info.cc?

[piman, 2015-06-27 01:20:08]

https://codereview.chromium.org/169403005/diff/860001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/860001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:80: template <typename T>
On 2015/06/26 13:39:20, Kimmo Kinnunen wrote:
> piman@: so I had to add this kind of complication instead of |it = erase(cit,
> cit)|. Is this acceptable? Mac 10.6 failed, the erase works differently in
c++98
> vs c++11

Ah, didn't realize erase(const_iterator, const_iterator) was c++11 only - we
currently can't do c++11 library features.

That's fine, but if you're going to make them static and pass the map
explicitly, can you move the GetContainingRange* functions as free functions
into the .cc?

(I'm surprised you didn't have to explicitly instantiate the templates).

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

https://codereview.chromium.org/169403005/diff/860002/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:1014:
gfx::GetGLProcAddress("glStencilThenCoverStrokePathNV");
Why is this needed?

https://codereview.chromium.org/169403005/diff/860002/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.cc (right):

https://codereview.chromium.org/169403005/diff/860002/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.cc:147: typename
IteratorSelector<MapType>::iterator it =
nit: feel free to use auto here.

[Kimmo Kinnunen, 2015-06-29 05:46:05]

https://codereview.chromium.org/169403005/diff/860001/gpu/command_buffer/serv...
File gpu/command_buffer/service/path_manager.h (right):

https://codereview.chromium.org/169403005/diff/860001/gpu/command_buffer/serv...
gpu/command_buffer/service/path_manager.h:80: template <typename T>
On 2015/06/27 01:20:07, piman (Very slow to review) wrote:
> On 2015/06/26 13:39:20, Kimmo Kinnunen wrote:
> > piman@: so I had to add this kind of complication instead of |it =
erase(cit,
> > cit)|. Is this acceptable? Mac 10.6 failed, the erase works differently in
> c++98
> > vs c++11
> 
> Ah, didn't realize erase(const_iterator, const_iterator) was c++11 only - we
> currently can't do c++11 library features.
> 
> That's fine, but if you're going to make them static and pass the map
> explicitly, can you move the GetContainingRange* functions as free functions
> into the .cc?
> 
> (I'm surprised you didn't have to explicitly instantiate the templates).

Done.

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

https://codereview.chromium.org/169403005/diff/860002/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:1014:
gfx::GetGLProcAddress("glStencilThenCoverStrokePathNV");
On 2015/06/27 01:20:07, piman (Very slow to review) wrote:
> Why is this needed?

These required functions are part of the API version 1.2.  For
older drivers, these do not exist, even though the extension
string is the same. In theory these particular functions could be simulated if
missing. In patch #3 of the patchset, texturing, there will be use of API
version 1.3, which can not be simulated. This is why I did not implement the
simulation.

[piman, 2015-06-29 23:31:18]

LGTM because I don't want to delay this further, but see below.

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

https://codereview.chromium.org/169403005/diff/860002/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:1014:
gfx::GetGLProcAddress("glStencilThenCoverStrokePathNV");
On 2015/06/29 05:46:04, Kimmo Kinnunen wrote:
> On 2015/06/27 01:20:07, piman (Very slow to review) wrote:
> > Why is this needed?
> 
> These required functions are part of the API version 1.2.  For
> older drivers, these do not exist, even though the extension
> string is the same. In theory these particular functions could be simulated if
> missing. In patch #3 of the patchset, texturing, there will be use of API
> version 1.3, which can not be simulated. This is why I did not implement the
> simulation.

I don't much like this idea of extension "API version"... It sounds like older
drivers are exposing an extension string without implementing the extension as
documented on the registry
(https://www.khronos.org/registry/gles/extensions/NV/path_rendering.txt), which
is really a driver bug.

I would actually prefer if we blacklisted the extension on older driver versions
using the workarounds mechanism (gpu/config/gpu_driver_bug_list_json.cc). This
is fragile, we mock the GetProcAddress function in some environments.

[Kimmo Kinnunen, 2015-07-01 12:33:01]

On 2015/06/29 23:31:18, piman (Very slow to review) wrote:
> LGTM because I don't want to delay this further, but see below.
...
> I don't much like this idea of extension "API version"... It sounds like older
> drivers are exposing an extension string without implementing the extension as
> documented on the registry
> (https://www.khronos.org/registry/gles/extensions/NV/path_rendering.txt),
which
> is really a driver bug.
> 
> I would actually prefer if we blacklisted the extension on older driver
versions
> using the workarounds mechanism (gpu/config/gpu_driver_bug_list_json.cc). This
> is fragile, we mock the GetProcAddress function in some environments.

Done. However, the blacklist does not work for the gl_tests, if used as
intended. Using "disabled_extensions" fails.

Should I try to fix disabled_extensions or create a new workaround feature?

[piman, 2015-07-01 20:38:30]

lgtm

https://codereview.chromium.org/169403005/diff/930001/gpu/config/gpu_driver_b...
File gpu/config/gpu_driver_bug_list_json.cc (right):

https://codereview.chromium.org/169403005/diff/930001/gpu/config/gpu_driver_b...
gpu/config/gpu_driver_bug_list_json.cc:1475: "description": "NVIDIA drivers 337
and later have useful NV_path_rendering and related extensions",
nit: Typically we describe what the bug is - in this case, drivers before
version 337 expose the extension string but with an incomplete implementation.
See how it appears in about:gpu.

[Kimmo Kinnunen, 2015-07-02 08:09:42]

Sorry to go back and forth with this. Looks like I was a bit optimistic on
getting the amount of design info and feedback on the
disabled_extensions-in-gl_tests problem. It seems I will not be able to solve
the issue in timely manner.

Would it be possible to land this patch with the feature workaround flag
"disable_gl_path_rendering" instead of disabled_extensions? Later on, I would
refactor to remove the feature flag in favor of working disabled_extensions.
There's still other patches left to review, and I feel they may be a bit
demotivating to review until the earlier patches are in the tree.

>
https://codereview.chromium.org/169403005/diff/930001/gpu/config/gpu_driver_b...
> gpu/config/gpu_driver_bug_list_json.cc:1475: "description": "NVIDIA drivers
337
> and later have useful NV_path_rendering and related extensions",


> nit: Typically we describe what the bug is - in this case, drivers before
> version 337 expose the extension string but with an incomplete implementation.

I've conveyed the problems of the unconventional way to update the extension to
the author already. However, the extension does mention to use GetProcAddress to
determine the API version. Thus I here propose not claim that as a bug to the
user. (I am aware of some of the semantic problems of signifying this with
GetProcAddress. Now also I'm aware of the Chromium -specific problems) 

If the above is strictly not ok with you, I can of course still change the
string to blame the driver and the extension.

[piman, 2015-07-07 23:54:16]

LGTM as is, we can fix up later. Thanks for the wording update.

[Kimmo Kinnunen, 2015-07-13 07:48:18]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-07-13 07:48:20]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/169403005/#ps970001
(title: " ")

[commit-bot: I haz the power, 2015-07-13 07:48:27]

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

[commit-bot: I haz the power, 2015-07-13 07:48:39]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[Kimmo Kinnunen, 2015-07-13 07:48:48]

The CQ bit was unchecked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-07-13 07:49:40]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-07-13 07:49:42]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
sievers@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/169403005/#ps990001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-13 07:49:47]

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

[commit-bot: I haz the power, 2015-07-13 07:50:01]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-13 08:02:32]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-13 08:32:04]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-13 08:52:55]

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

[commit-bot: I haz the power, 2015-07-13 08:52:58]

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

[Kimmo Kinnunen, 2015-07-13 12:27:45]

Still one (hopefully) update:
So I had to add the 'is_optional' tag back to the bindings definitions.
I did not understand that no matter how the extension filtering is done, at some
point in the system the bindings are filled before knowing which features to
disable and which extensions to remove. Thus the bindings must support the
non-existing functions. The other option is that the version checks must be done
at bindings -level, before filling the function lists, but it sounds a bit
elaborate..

[piman, 2015-07-13 18:21:14]

lgtm

[Kimmo Kinnunen, 2015-07-14 05:45:19]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-07-14 05:45:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/169403005/#ps1010001
(title: "make bindings capable of using optional extension functions")

[commit-bot: I haz the power, 2015-07-14 05:45:43]

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

[commit-bot: I haz the power, 2015-07-14 05:45:58]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-14 06:02:46]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-14 06:32:26]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-14 06:37:50]

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

[commit-bot: I haz the power, 2015-07-14 06:37:53]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Kimmo Kinnunen, 2015-07-14 09:33:52]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-07-14 09:33:54]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
sievers@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/169403005/#ps1030001 (title: "fix windows
build")

[commit-bot: I haz the power, 2015-07-14 09:34:37]

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

[commit-bot: I haz the power, 2015-07-14 09:34:53]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-14 10:03:14]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-14 10:32:20]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-14 11:01:57]

The author kkinnunen@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[commit-bot: I haz the power, 2015-07-14 11:08:49]

Committed patchset #47 (id:1030001)

[commit-bot: I haz the power, 2015-07-14 11:09:42]

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