Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1245703004:
Fix tile drop-out on S4 for texture decal mode. (Closed)
|
Created:
5 years, 5 months ago by jvanverth1 Modified:
5 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionFix tile drop-out on S4 for texture decal mode.
Switch to use highp on interpolants.
Also removes some unnecessary formatting.
BUG=skia:3381
Committed: https://skia.googlesource.com/skia/+/3fc656006c8bcbdca069b1b0f500f3c7311e5e5a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix y denominator #Messages
Total messages: 13 (4 generated)
jvanverth@google.com changed reviewers: + bsalomon@google.com
https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... File src/gpu/effects/GrTextureDomain.cpp (right): https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... src/gpu/effects/GrTextureDomain.cpp:102: builder->codeAppend(GrGLShaderVar::PrecisionString(kHigh_GrSLPrecision, Is inCoords already highp in the cases that this causes an error? I wonder if we could know that here and only make the intermediate variables highp if necessary.
robertphillips@google.com changed reviewers: + robertphillips@google.com
https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... File src/gpu/effects/GrTextureDomain.cpp (right): https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... src/gpu/effects/GrTextureDomain.cpp:110: domain, domain, domain); This w -> z & y -> x swap surprises me.
https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... File src/gpu/effects/GrTextureDomain.cpp (right): https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... src/gpu/effects/GrTextureDomain.cpp:102: builder->codeAppend(GrGLShaderVar::PrecisionString(kHigh_GrSLPrecision, On 2015/07/21 21:10:31, bsalomon wrote: > Is inCoords already highp in the cases that this causes an error? I wonder if we > could know that here and only make the intermediate variables highp if > necessary. Looks like it's mediump. I could try forcing the uv-coordinate varying to be highp instead. https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... src/gpu/effects/GrTextureDomain.cpp:110: domain, domain, domain); On 2015/07/22 12:21:47, robertphillips wrote: > This w -> z & y -> x swap surprises me. Whoops, that's a copy-paste error.
https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... File src/gpu/effects/GrTextureDomain.cpp (right): https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... src/gpu/effects/GrTextureDomain.cpp:102: builder->codeAppend(GrGLShaderVar::PrecisionString(kHigh_GrSLPrecision, On 2015/07/22 13:56:18, jvanverth1 wrote: > On 2015/07/21 21:10:31, bsalomon wrote: > > Is inCoords already highp in the cases that this causes an error? I wonder if > we > > could know that here and only make the intermediate variables highp if > > necessary. > > Looks like it's mediump. I could try forcing the uv-coordinate varying to be > highp instead. Can you try that first? We already have a mechanism to do so that could be extended. I don't really have a problem with this CL, but I don't want to develop a pattern of "just bump stuff to highp unconditionally" whenever we see a precision problem on one device for one case.
On 2015/07/22 15:13:38, bsalomon wrote: > https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... > File src/gpu/effects/GrTextureDomain.cpp (right): > > https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... > src/gpu/effects/GrTextureDomain.cpp:102: > builder->codeAppend(GrGLShaderVar::PrecisionString(kHigh_GrSLPrecision, > On 2015/07/22 13:56:18, jvanverth1 wrote: > > On 2015/07/21 21:10:31, bsalomon wrote: > > > Is inCoords already highp in the cases that this causes an error? I wonder > if > > we > > > could know that here and only make the intermediate variables highp if > > > necessary. > > > > Looks like it's mediump. I could try forcing the uv-coordinate varying to be > > highp instead. > > Can you try that first? We already have a mechanism to do so that could be > extended. I don't really have a problem with this CL, but I don't want to > develop a pattern of "just bump stuff to highp unconditionally" whenever we see > a precision problem on one device for one case. After in person chat, lgtm
https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... File src/gpu/effects/GrTextureDomain.cpp (right): https://codereview.chromium.org/1245703004/diff/1/src/gpu/effects/GrTextureDo... src/gpu/effects/GrTextureDomain.cpp:110: domain, domain, domain); On 2015/07/22 13:56:18, jvanverth1 wrote: > On 2015/07/22 12:21:47, robertphillips wrote: > > This w -> z & y -> x swap surprises me. > > Whoops, that's a copy-paste error. Fixed
The CQ bit was checked by jvanverth@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1245703004/#ps20001 (title: "Fix y denominator")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245703004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/3fc656006c8bcbdca069b1b0f500f3c7311e5e5a |
