color: media/color-profile-video-poster-image.html has blank image with --enable-color-correct-rendering and GPU raster |
||||
Issue description
To reproduce:
1. Next to the other PhysicalTestSuite lines of base.py add
PhysicalTestSuite('media/color-profile', [
'--enable-color-correct-rendering',
'--force-color-profile=srgb',
'--force-gpu-rasterization']),
2. Run the layout test media/color-profile-video-poster-image.html
The main image should be visible, but it is blank.
,
May 17 2017
I've dug a bit more into this. The blank images only happen in the layout test framework -- the issue consistently fails to reproduce outside of layout tests.
FWICT, this is an issue with the shader generation in GrGLNonlinearColorSpaceXformEffect::emitCode. In particular, if I apply the following patch, then the images re-appear:
diff --git a/src/gpu/effects/GrNonlinearColorSpaceXformEffect.cpp b/src/gpu/effects/GrNonlinearColorSpaceXformEffect.cpp
index c6c0c26..108af5f 100644
--- a/src/gpu/effects/GrNonlinearColorSpaceXformEffect.cpp
+++ b/src/gpu/effects/GrNonlinearColorSpaceXformEffect.cpp
@@ -53,13 +53,15 @@ public:
};
SkString transferFnBody;
// Temporaries to make evaluation line readable
- transferFnBody.printf("float A = coeffs[0];");
- transferFnBody.append("float B = coeffs[1];");
- transferFnBody.append("float C = coeffs[2];");
- transferFnBody.append("float D = coeffs[3];");
- transferFnBody.append("float E = coeffs[4];");
- transferFnBody.append("float F = coeffs[5];");
- transferFnBody.append("float G = coeffs[6];");
+ // If we change this line to actually read the coeffs array, then
+ // we get no output
+ transferFnBody.printf("float A = 1;"); // coeffs[0]
+ transferFnBody.append("float B = 0;"); // coeffs[1]
+ transferFnBody.append("float C = 1;"); // coeffs[2]
+ transferFnBody.append("float D = 0;"); // coeffs[3]
+ transferFnBody.append("float E = 0;"); // coeffs[4]
+ transferFnBody.append("float F = 0;"); // coeffs[5]
+ transferFnBody.append("float G = 1;"); // coeffs[6]
transferFnBody.appendf("return (x < D) ? (C * x) + F : pow(A * x + B, G) + E;");
fragBuilder->emitFunction(kFloat_GrSLType, "transfer_fn",
SK_ARRAY_COUNT(gTransferFnFuncArgs), gTransferFnFuncArgs,
The following patch is necessary to run the layout test
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
index cd7b662..2d64ac0 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
@@ -1472,6 +1472,8 @@ class Port(object):
# For example, to turn on force-compositing-mode in the svg/ directory:
# PhysicalTestSuite('svg', ['--force-compositing-mode']),
PhysicalTestSuite('fast/text', ['--enable-direct-write', '--enable-font-antialiasing']),
+ PhysicalTestSuite('images/color-profile', ['--enable-color-correct-rendering', '--force-gpu-rasterization']),
+ PhysicalTestSuite('media/color-profile', ['--enable-color-correct-rendering', '--force-color-profile=srgb', '--force-gpu-rasterization']),
]
def virtual_test_suites(self):
And the actual command to run the layout test is
python blink/tools/run_layout_tests.py --debug images/color-profile-image.html
The r
,
May 17 2017
It made me feel a bit scared to be passing a uniform array as an argument to a function in GLSL. It should be fine, but who knows what could go wrong.
If I apply the following patch, which passes the arguments as individual floats, then things seem to work fine.
I'm vaguely terrified that this is a GLSL bug, particular to my machine. I'll try on my Linux box when I'm back in office.
Also, if this shader is built for every src+dst pair, maybe it would make sense to hard-code the parameters.
--- a/src/gpu/effects/GrNonlinearColorSpaceXformEffect.cpp
+++ b/src/gpu/effects/GrNonlinearColorSpaceXformEffect.cpp
@@ -48,19 +48,20 @@ public:
SkString tfFuncNameString;
static const GrShaderVar gTransferFnFuncArgs[] = {
GrShaderVar("x", kFloat_GrSLType),
- GrShaderVar("coeffs", kFloat_GrSLType,
- GrNonlinearColorSpaceXformEffect::kNumTransferFnCoeffs),
+ GrShaderVar("A", kFloat_GrSLType),
+ GrShaderVar("B", kFloat_GrSLType),
+ GrShaderVar("C", kFloat_GrSLType),
+ GrShaderVar("D", kFloat_GrSLType),
+ GrShaderVar("E", kFloat_GrSLType),
+ GrShaderVar("F", kFloat_GrSLType),
+ GrShaderVar("G", kFloat_GrSLType),
};
SkString transferFnBody;
// Temporaries to make evaluation line readable
- transferFnBody.printf("float A = coeffs[0];");
- transferFnBody.append("float B = coeffs[1];");
- transferFnBody.append("float C = coeffs[2];");
- transferFnBody.append("float D = coeffs[3];");
- transferFnBody.append("float E = coeffs[4];");
- transferFnBody.append("float F = coeffs[5];");
- transferFnBody.append("float G = coeffs[6];");
+ // If we change this line to actually read the coeffs array, then
+ // we get no output
transferFnBody.appendf("return (x < D) ? (C * x) + F : pow(A * x + B, G) + E;");
+ fprintf(stderr, "%s\n", transferFnBody.c_str());
fragBuilder->emitFunction(kFloat_GrSLType, "transfer_fn",
SK_ARRAY_COUNT(gTransferFnFuncArgs), gTransferFnFuncArgs,
transferFnBody.c_str(), &tfFuncNameString);
@@ -77,9 +78,9 @@ public:
// 2: Apply src transfer function (to get to linear RGB)
if (srcCoeffsName) {
- fragBuilder->codeAppendf("color.r = %s(color.r, %s);", tfFuncName, srcCoeffsName);
- fragBuilder->codeAppendf("color.g = %s(color.g, %s);", tfFuncName, srcCoeffsName);
- fragBuilder->codeAppendf("color.b = %s(color.b, %s);", tfFuncName, srcCoeffsName);
+ fragBuilder->codeAppendf("color.r = %s(color.r, %s[0], %s[1], %s[2], %s[3], %s[4], %s[5], %s[6]);", tfFuncName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName);
+ fragBuilder->codeAppendf("color.g = %s(color.g, %s[0], %s[1], %s[2], %s[3], %s[4], %s[5], %s[6]);", tfFuncName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName);
+ fragBuilder->codeAppendf("color.b = %s(color.b, %s[0], %s[1], %s[2], %s[3], %s[4], %s[5], %s[6]);", tfFuncName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName, srcCoeffsName);
}
// 3: Apply gamut matrix
@@ -91,9 +92,9 @@ public:
// 4: Apply dst transfer fn
if (dstCoeffsName) {
- fragBuilder->codeAppendf("color.r = %s(color.r, %s);", tfFuncName, dstCoeffsName);
- fragBuilder->codeAppendf("color.g = %s(color.g, %s);", tfFuncName, dstCoeffsName);
- fragBuilder->codeAppendf("color.b = %s(color.b, %s);", tfFuncName, dstCoeffsName);
+ fragBuilder->codeAppendf("color.r = %s(color.r, %s[0], %s[1], %s[2], %s[3], %s[4], %s[5], %s[6]);", tfFuncName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName);
+ fragBuilder->codeAppendf("color.g = %s(color.g, %s[0], %s[1], %s[2], %s[3], %s[4], %s[5], %s[6]);", tfFuncName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName);
+ fragBuilder->codeAppendf("color.b = %s(color.b, %s[0], %s[1], %s[2], %s[3], %s[4], %s[5], %s[6]);", tfFuncName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName, dstCoeffsName);
}
// 5: Premultiply again
,
May 17 2017
I'm happy to switch to the c#3 version - it should be functionally identical.
,
May 17 2017
I think I'll go for that. This looks like a GL driver bug (since it works perfectly fine on Linux, and only fails on Mac in some very particular circumstances). I'll get a shader dump before I make the patch.
,
May 17 2017
Chris, can you send me the shader that goes wrong on Mac with gpu info, so I can add it to WebGL conformance suite?
,
May 17 2017
Shader source below. I checked this on Linux, and it fails in the same way there.
What I find suspicious is passing "uniform float uSrcTransferFn_Stage2[7]" as "float coeffs[7]" to transfer_fn_Stage2.
----
Vertex Shader:
----
#version 100
precision highp float;
uniform highp vec4 urtAdjustment_Stage0;
uniform highp mat3 uCoordTransformMatrix_0_Stage0;
attribute highp vec2 inPosition;
attribute mediump vec4 inColor;
attribute highp vec2 inLocalCoord;
varying mediump vec4 vcolor_Stage0;
varying highp vec2 vTransformedCoords_0_Stage0;
void main() {
vcolor_Stage0 = inColor;
vec2 pos2 = inPosition;
vTransformedCoords_0_Stage0 = (uCoordTransformMatrix_0_Stage0 * vec3(inLocalCoord, 1.0)).xy;
gl_Position = vec4(pos2.x * urtAdjustment_Stage0.x + urtAdjustment_Stage0.y, pos2.y * urtAdjustment_Stage0.z + urtAdjustment_Stage0.w, 0.0, 1.0);
}
----
Fragment Shader:
----
#version 100
precision mediump float;
uniform float uSrcTransferFn_Stage2[7];
uniform float uDstTransferFn_Stage2[7];
uniform mat4 uGamutXform_Stage2;
uniform highp sampler2D uTextureSampler_0_Stage1;
varying mediump vec4 vcolor_Stage0;
varying highp vec2 vTransformedCoords_0_Stage0;
float transfer_fn_Stage2(float x, float coeffs[7]) {
float A = coeffs[0];
float B = coeffs[1];
float C = coeffs[2];
float D = coeffs[3];
float E = coeffs[4];
float F = coeffs[5];
float G = coeffs[6];
return x < D ? C * x + F : pow(A * x + B, G) + E;
}
void main() {
vec4 outputColor_Stage0;
{
outputColor_Stage0 = vcolor_Stage0;
}
vec4 output_Stage1;
{
output_Stage1 = outputColor_Stage0 * texture2D(uTextureSampler_0_Stage1, vTransformedCoords_0_Stage0);
}
vec4 output_Stage2;
{
vec4 color = output_Stage1;
float nonZeroAlpha = max(color.w, 1.0000000000000001e-05);
color = vec4(color.xyz / nonZeroAlpha, nonZeroAlpha);
color.x = transfer_fn_Stage2(color.x, uSrcTransferFn_Stage2);
color.y = transfer_fn_Stage2(color.y, uSrcTransferFn_Stage2);
color.z = transfer_fn_Stage2(color.z, uSrcTransferFn_Stage2);
color.xyz = clamp((uGamutXform_Stage2 * vec4(color.xyz, 1.0)).xyz, 0.0, 1.0);
color.x = transfer_fn_Stage2(color.x, uDstTransferFn_Stage2);
color.y = transfer_fn_Stage2(color.y, uDstTransferFn_Stage2);
color.z = transfer_fn_Stage2(color.z, uDstTransferFn_Stage2);
output_Stage2 = vec4(color.xyz * color.w, color.w);
}
{
gl_FragColor = output_Stage2;
}
}
----
,
May 23 2017
The following revision refers to this bug: https://skia.googlesource.com/skia/+/ccb46aaef838e1bd9d2feb249fb65aa935d7dc2c commit ccb46aaef838e1bd9d2feb249fb65aa935d7dc2c Author: Christopher Cameron <ccameron@chromium.org> Date: Tue May 23 17:34:32 2017 Don't pass uniform arrays in GrGLNonlinearColorSpaceXformEffect This avoids a problem where passing a uniform array as a function array sometimes gets lost. Instead of passing the uniform array as an argument to the transfer function evaluation function, create two separate transfer function evaluation functions that read the uniforms directly. BUG= 723133 Change-Id: Ib46b99efdbc04ce0201644ebbc1dfd4cb27e9276 Reviewed-on: https://skia-review.googlesource.com/17293 Commit-Queue: Brian Osman <brianosman@google.com> Reviewed-by: Brian Osman <brianosman@google.com> [modify] https://crrev.com/ccb46aaef838e1bd9d2feb249fb65aa935d7dc2c/src/gpu/effects/GrNonlinearColorSpaceXformEffect.cpp
,
May 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4df202e0b38733d30c04f43408dae4a8c42608fc commit 4df202e0b38733d30c04f43408dae4a8c42608fc Author: skia-deps-roller@chromium.org <skia-deps-roller@chromium.org> Date: Tue May 23 21:35:29 2017 Roll src/third_party/skia/ 5373609d9..ee92f131b (11 commits) https://skia.googlesource.com/skia.git/+log/5373609d90d8..ee92f131b8bb $ git log 5373609d9..ee92f131b --date=short --no-merges --format='%ad %ae %s' 2017-05-23 scroggo Fix alpha in webp bug introduced with animation 2017-05-23 egdaniel Really disable srgb on nexus player vulkan 2017-05-19 mtklein Dither copies when decreasing precision below 32-bit. 2017-05-23 bsalomon Revert "Add a flag to GrSurfaceFlags that requires the texture to be cleared upon creation. " 2017-05-23 reed add default arg so android can bulid 2017-05-23 mtklein Revert "add knob to turn off fancy SkJumper features" 2017-05-23 egdaniel Disable srgb on vulkan nexus player 2017-05-23 jvanverth Add Material Design shadow reference sample 2017-05-23 bsalomon Add a flag to GrSurfaceFlags that requires the texture to be cleared upon creation. 2017-05-22 ccameron Don't pass uniform arrays in GrGLNonlinearColorSpaceXformEffect 2017-05-23 mtklein move sk_memset?? to SkOpts Created with: roll-dep src/third_party/skia BUG= 720105 , 723133 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, see: http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel TBR=brianosman@chromium.org Change-Id: I1607acbbbd2f34a68ae36c38cbb046465c1924db Reviewed-on: https://chromium-review.googlesource.com/513045 Reviewed-by: Skia Deps Roller <skia-deps-roller@chromium.org> Commit-Queue: Skia Deps Roller <skia-deps-roller@chromium.org> Cr-Commit-Position: refs/heads/master@{#474065} [modify] https://crrev.com/4df202e0b38733d30c04f43408dae4a8c42608fc/DEPS
,
May 31 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by ccameron@chromium.org
, May 17 2017