gpu: Workaround invalid values from GetShaderPrecisionFormat

gpu: Workaround invalid values from GetShaderPrecisionFormat

N4 reports negative numbers in the range when they should be positive,
so we take the absolute value of the range before sending it to the
client.

Some devices report highp precisions when they can't actually be used,
so we force highp precisions to 0 (usupported) in those cases before
sending it to the client.

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

[Ken Russell (switch to Gerrit), 2013-06-11 20:01:27]

This LGTM but I am not an owner; Gregg should review this.

https://codereview.chromium.org/16409014/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/16409014/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:134: (range[0] < 62 || range[1]
< 62 || *precision < 16)) {
Can the duplicate logic here and in GLES2DecoderImpl::InitializeShaderTranslator
be refactored?

[Ken Russell (switch to Gerrit), 2013-06-11 21:45:09]

Actually, it would be really good if this workaround could be controlled by the
GPU driver bug workarounds so it can be disabled at run time. See
gpu_driver_bug_workaround_type.h and gpu_driver_bug_list_json.cc. If you plan to
merge back this fix, it's OK if the first commit doesn't contain the control for
it (to keep the patch small) but on top of tree please add it to the list of
known bugs.

[greggman, 2013-06-11 21:58:50]

https://codereview.chromium.org/16409014/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/16409014/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2526: GLint range[2] = { 0, 0 };
What kbr said, should this code have the same workaround? Should call some
function so the workaround is in one place?

[brianderson, 2013-06-11 23:30:26]

On 2013/06/11 21:45:09, kbr wrote:
> Actually, it would be really good if this workaround could be controlled by
the
> GPU driver bug workarounds so it can be disabled at run time. See
> gpu_driver_bug_workaround_type.h and gpu_driver_bug_list_json.cc. If you plan
to
> merge back this fix, it's OK if the first commit doesn't contain the control
for
> it (to keep the patch small) but on top of tree please add it to the list of
> known bugs.

The call to this function is cached client-side, so we would have to invalidate
the client's cache in order to really be able to control this at runtime. I will
defer this to a separate patch.

[Ken Russell (switch to Gerrit), 2013-06-11 23:35:34]

On 2013/06/11 23:30:26, Brian Anderson wrote:
> On 2013/06/11 21:45:09, kbr wrote:
> > Actually, it would be really good if this workaround could be controlled by
> the
> > GPU driver bug workarounds so it can be disabled at run time. See
> > gpu_driver_bug_workaround_type.h and gpu_driver_bug_list_json.cc. If you
plan
> to
> > merge back this fix, it's OK if the first commit doesn't contain the control
> for
> > it (to keep the patch small) but on top of tree please add it to the list of
> > known bugs.
> 
> The call to this function is cached client-side, so we would have to
invalidate
> the client's cache in order to really be able to control this at runtime. I
will
> defer this to a separate patch.

Fine with me to defer the run-time switch to a separate patch, but the client's
cache shouldn't need to be invalidated. The GPU driver bug workarounds are
initialized at GPU process launch, so the first and only call each client makes
to the service side either will or will not have the workaround applied.

[brianderson, 2013-06-11 23:35:46]

ptal

https://codereview.chromium.org/16409014/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/16409014/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:134: (range[0] < 62 || range[1]
< 62 || *precision < 16)) {
On 2013/06/11 20:01:27, kbr wrote:
> Can the duplicate logic here and in
GLES2DecoderImpl::InitializeShaderTranslator
> be refactored?

Done.

https://codereview.chromium.org/16409014/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2526: GLint range[2] = { 0, 0 };
On 2013/06/11 21:58:50, greggman wrote:
> What kbr said, should this code have the same workaround? Should call some
> function so the workaround is in one place?

The workaround is in GetShaderPrecisionFormatImpl so it is also used here,
however in the second patch I have put the magic numbers 62,62,16 into a common
function.

[Ken Russell (switch to Gerrit), 2013-06-11 23:36:48]

lgtm

[greggman, 2013-06-12 00:32:43]

lgtm

[commit-bot: I haz the power, 2013-06-12 00:33:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/16409014/11001

[commit-bot: I haz the power, 2013-06-12 17:00:46]

Change committed as 205832