New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 723133 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 713891



Sign in to add a comment

color: media/color-profile-video-poster-image.html has blank image with --enable-color-correct-rendering and GPU raster

Project Member Reported by ccameron@chromium.org, May 17 2017

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.

 
This is also true for almost all layout tests in image/color-profile*
Cc: msarett@chromium.org brianosman@chromium.org
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
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
I'm happy to switch to the c#3 version - it should be functionally identical.
Cc: zmo@chromium.org
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.

Comment 6 by zmo@chromium.org, 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?
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;
    }
}
----
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment