command_buffer: Implement glGetStringi for GL ES 3.0

command_buffer: Implement glGetStringi for GL ES 3.0

Implement glGetStringi for GL ES 3.0.
It is available for GL ES 2.0 contexts, too.

Skia uses the function to get extensions on ES 3.0.

BUG=581634
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/5d3785ff3a2292370414522838cac6f15470d635
Cr-Commit-Position: refs/heads/master@{#376462}

[Kimmo Kinnunen, 2016-02-11 12:27:50]

Description was changed from

==========
command_buffer: Implement glGetStringi for GL ES 3.0

Implement glGetStringi for GL ES 3.0.
It is available for GL ES 2.0 contexts, too.

Skia uses the function to get extensions on ES 3.0.

BUG=581634
==========

to

==========
command_buffer: Implement glGetStringi for GL ES 3.0

Implement glGetStringi for GL ES 3.0.
It is available for GL ES 2.0 contexts, too.

Skia uses the function to get extensions on ES 3.0.

BUG=581634
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Kimmo Kinnunen, 2016-02-11 12:28:14]

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

[Kimmo Kinnunen, 2016-02-11 12:28:39]

The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-11 12:28:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1688163002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1688163002/1

[commit-bot: I haz the power, 2016-02-11 13:33:42]

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

[commit-bot: I haz the power, 2016-02-11 13:33:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kimmo Kinnunen, 2016-02-11 15:24:43]

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

[Kimmo Kinnunen, 2016-02-11 15:24:43]

piman@ for the autogen/gyp/gn files zmo does not own

[Kimmo Kinnunen, 2016-02-11 15:24:53]

The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-11 15:25:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1688163002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1688163002/20001

[piman, 2016-02-11 18:50:38]

https://chromiumcodereview.appspot.com/1688163002/diff/60001/gpu/command_buff...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://chromiumcodereview.appspot.com/1688163002/diff/60001/gpu/command_buff...
gpu/command_buffer/client/gles2_implementation.cc:3326: const std::string& cache
= *gl_strings_.insert(str).first;
Is it really advantageous to change gl_strings_ from a map to a set?
strings are more expensive to compare, and we would logically leak extension
strings when we call RequestExtensionCHROMIUM.

We could instead make the key in gl_strings_ a (name, index) pair, using e.g.
UINT32_MAX for unindexed ones. It would also remove the need for a separate
cached_extensions_ map.

[Kimmo Kinnunen, 2016-02-11 18:55:42]

https://chromiumcodereview.appspot.com/1688163002/diff/60001/gpu/command_buff...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://chromiumcodereview.appspot.com/1688163002/diff/60001/gpu/command_buff...
gpu/command_buffer/client/gles2_implementation.cc:3326: const std::string& cache
= *gl_strings_.insert(str).first;
On 2016/02/11 18:50:38, piman wrote:
> Is it really advantageous to change gl_strings_ from a map to a set?

It is changed from many sets to sets.
map of sets -> set

> strings are more expensive to compare, and we would logically leak extension
> strings when we call RequestExtensionCHROMIUM.

The exact idea is to leak those for eternity? I'm not following now...

> We could instead make the key in gl_strings_ a (name, index) pair, using e.g.
> UINT32_MAX for unindexed ones. It would also remove the need for a separate
> cached_extensions_ map.

I'm not following now at all.

Set is pretty optimal -- it contains each possible string that needs to be
retained for eternity, just once.

[piman, 2016-02-11 19:15:27]

https://chromiumcodereview.appspot.com/1688163002/diff/60001/gpu/command_buff...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://chromiumcodereview.appspot.com/1688163002/diff/60001/gpu/command_buff...
gpu/command_buffer/client/gles2_implementation.cc:3326: const std::string& cache
= *gl_strings_.insert(str).first;
On 2016/02/11 18:55:42, Kimmo Kinnunen wrote:
> On 2016/02/11 18:50:38, piman wrote:
> > Is it really advantageous to change gl_strings_ from a map to a set?
> 
> It is changed from many sets to sets.
> map of sets -> set
> 
> > strings are more expensive to compare, and we would logically leak extension
> > strings when we call RequestExtensionCHROMIUM.
> 
> The exact idea is to leak those for eternity? I'm not following now...
> 
> > We could instead make the key in gl_strings_ a (name, index) pair, using
e.g.
> > UINT32_MAX for unindexed ones. It would also remove the need for a separate
> > cached_extensions_ map.
> 
> I'm not following now at all.
> 
> Set is pretty optimal -- it contains each possible string that needs to be
> retained for eternity, just once.
> 

Mmh, ok, I misread the former code. I was under the impression that
RequestExtensionCHROMIUM would cause the extension strings to be reset, but I
see that it's not the case. We can define RequestExtensionCHROMIUM however we
want, and I really wouldn't mind defining it as invalidating all the previously
returned GL_EXTENSIONS strings so that we don't need to leak anything. Extension
lists can be large, and this could become a lot of data when multiplied by every
combination of requested extensions. It's also only called by WebGL, and we can
certainly make sure WebGL knows how to deal with that.

[Kimmo Kinnunen, 2016-02-11 19:45:00]

On 2016/02/11 19:15:27, piman wrote:
> We can define RequestExtensionCHROMIUM however we
> want, and I really wouldn't mind defining it as invalidating all the
previously
> returned GL_EXTENSIONS strings so that we don't need to leak anything.

Maybe -- I hope this discussion is a matter of a different commit.

This change just changes multiple redundant sets to one set, for sake of
simplicity of the code.
I would "handwave" it also decreases memory usage and preserving perf. If you
don't think it simplifies the code, I can of course try to revert this part of
the change.


> Extension
> lists can be large, and this could become a lot of data when multiplied by
every
> combination of requested extensions. It's also only called by WebGL, and we
can
> certainly make sure WebGL knows how to deal with that.

(Personally it sounds a bit extreme to trade off few strings for a surprising
hard-to-debug bug. They are largeish strings, though)

[piman, 2016-02-11 20:11:48]

LGTM for now, maybe add a TODO to revisit.

[piman, 2016-02-11 20:11:48]

On Thu, Feb 11, 2016 at 11:44 AM, <kkinnunen@nvidia.com> wrote:

> On 2016/02/11 19:15:27, piman wrote:
> > We can define RequestExtensionCHROMIUM however we
> > want, and I really wouldn't mind defining it as invalidating all the
> previously
> > returned GL_EXTENSIONS strings so that we don't need to leak anything.
>
> Maybe -- I hope this discussion is a matter of a different commit.
>
> This change just changes multiple redundant sets to one set, for sake of
> simplicity of the code.
> I would "handwave" it also decreases memory usage and preserving perf. If you
> don't think it simplifies the code, I can of course try to revert this part
of
> the change.
>
>
> > Extension
> > lists can be large, and this could become a lot of data when multiplied by
> every
> > combination of requested extensions. It's also only called by WebGL, and we
> can
> > certainly make sure WebGL knows how to deal with that.
>
> (Personally it sounds a bit extreme to trade off few strings for a surprising
> hard-to-debug bug. They are largeish strings, though)
>
>
By the time the string makes it to webgl, it has already been copied, so
this is really belts+suspenders. It seems unfortunate to waste several kbs
just for that.

> https://chromiumcodereview.appspot.com/1688163002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kimmo Kinnunen, 2016-02-19 13:37:32]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2016-02-19 13:37:32]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1688163002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-02-19 13:38:24]

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

[commit-bot: I haz the power, 2016-02-19 16:59:45]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-19 17:00:58]

Description was changed from

==========
command_buffer: Implement glGetStringi for GL ES 3.0

Implement glGetStringi for GL ES 3.0.
It is available for GL ES 2.0 contexts, too.

Skia uses the function to get extensions on ES 3.0.

BUG=581634
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
command_buffer: Implement glGetStringi for GL ES 3.0

Implement glGetStringi for GL ES 3.0.
It is available for GL ES 2.0 contexts, too.

Skia uses the function to get extensions on ES 3.0.

BUG=581634
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/5d3785ff3a2292370414522838cac6f15470d635
Cr-Commit-Position: refs/heads/master@{#376462}
==========

[commit-bot: I haz the power, 2016-02-19 17:00:59]

Patchset 7 (id:??) landed as
https://crrev.com/5d3785ff3a2292370414522838cac6f15470d635
Cr-Commit-Position: refs/heads/master@{#376462}