gl.readPixels with type gl.FLOAT does not work

https://bugs.webkit.org/show_bug.cgi?id=171432
<rdar://problem/31905150>

Reviewed by Antoine Quint.

Source/WebCore:

Our validation code was identifying readPixels of
type FLOAT as invalid, for three reasons:
- we didn't support the FLOAT type at all.
- we only allowed the combination of RGBA and
UNSIGNED_BYTE in WebGL 1 [*].
- if we had a framebuffer of format RGBA, we assumed
we could only read into a Uint8 ArrayBuffer.

[*] This bug isn't completely fixed, so I opened
https://bugs.webkit.org/show_bug.cgi?id=196418

Test: fast/canvas/webgl/readPixels-float.html

* html/canvas/WebGLRenderingContextBase.cpp:
(WebCore::WebGLRenderingContextBase::readPixels):
- flip the logic in a conditional that was clearly wrong yet
  thankfully had no impact.
- support type FLOAT when the relevant extension is enabled.
- allow FLOAT as a valid type (see new bug above)
- create a new macro for CHECK_COMPONENT_COUNT
- update the existing macros to not be case statements,
  so that we can put logic in the switch.

LayoutTests:

New test that exercises reading a framebuffer object
with a floating point texture attached.

* platform/ios/TestExpectations: Skip this test on iOS, where floating-point
FBOs are not supported.
* fast/canvas/webgl/readPixels-float-expected.txt: Added.
* fast/canvas/webgl/readPixels-float.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@243681 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index dacf9dc..87c564d 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,19 @@
+2019-03-29  Dean Jackson  <dino@apple.com>
+
+        gl.readPixels with type gl.FLOAT does not work
+        https://bugs.webkit.org/show_bug.cgi?id=171432
+        <rdar://problem/31905150>
+
+        Reviewed by Antoine Quint.
+
+        New test that exercises reading a framebuffer object
+        with a floating point texture attached.
+
+        * platform/ios/TestExpectations: Skip this test on iOS, where floating-point
+        FBOs are not supported.
+        * fast/canvas/webgl/readPixels-float-expected.txt: Added.
+        * fast/canvas/webgl/readPixels-float.html: Added.
+
 2019-03-30  Zalan Bujtas  <zalan@apple.com>
 
         [ContentChangeObserver] Add iFrame elements to the list of "considered clickable" elements.

diff --git a/LayoutTests/fast/canvas/webgl/readPixels-float-expected.txt b/LayoutTests/fast/canvas/webgl/readPixels-float-expected.txt
new file mode 100644 (file)
index 0000000..0debef5
--- /dev/null
+++ b/LayoutTests/fast/canvas/webgl/readPixels-float-expected.txt
@@ -0,0 +1,7 @@
+Create WebGL context
+Clear to black
+Create a floating point texture
+Create and bind framebuffer
+Clear framebuffer to red
+PASS: Framebuffer is floating-point red.
+

diff --git a/LayoutTests/fast/canvas/webgl/readPixels-float.html b/LayoutTests/fast/canvas/webgl/readPixels-float.html
new file mode 100644 (file)
index 0000000..17bc960
--- /dev/null
+++ b/LayoutTests/fast/canvas/webgl/readPixels-float.html
@@ -0,0 +1,80 @@
+<style>
+canvas {
+    width: 2px;
+    height: 2px;
+}
+</style>
+</head>
+<script>
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+const width = 2;
+const height = 2;
+let canvas;
+let gl;
+
+function output(msg) {
+    const div = document.getElementById("output");
+    div.innerHTML += `${msg}<br>`;
+}
+
+function isRed(buffer, x, y) {
+    let offset = (y * width + x) * 4;
+    return buffer[offset] == 1.0 && buffer[offset+1] == 0.0 && buffer[offset+2] == 0.0 && buffer[offset+3] == 1.0;
+}
+
+function runTest() {
+
+    canvas = document.querySelector("canvas");
+    canvas.width = width;
+    canvas.height = height;
+
+    output("Create WebGL context");
+
+    gl = canvas.getContext("webgl");
+    gl.getExtension("OES_texture_float");
+
+    output("Clear to black");
+
+    gl.clearColor(0, 0, 0, 1);
+    gl.clear(gl.COLOR_BUFFER_BIT);
+
+    output("Create a floating point texture");
+
+    const texture = gl.createTexture();
+    gl.bindTexture(gl.TEXTURE_2D, texture);
+    gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.NEAREST);
+    gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.NEAREST);
+    gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_S, gl.CLAMP_TO_EDGE);
+    gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_T, gl.CLAMP_TO_EDGE);
+    gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, width, height, 0, gl.RGBA, gl.FLOAT, null);
+
+    output("Create and bind framebuffer");
+
+    const framebuffer = gl.createFramebuffer();
+    gl.bindFramebuffer(gl.FRAMEBUFFER, framebuffer);
+    gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.TEXTURE_2D, texture, 0);
+
+    output("Clear framebuffer to red");
+
+    gl.clearColor(1.0, 0.0, 0.0, 1.0);
+    gl.clear(gl.COLOR_BUFFER_BIT);
+
+    let pixels = new Float32Array(width * height * 4);
+    gl.readPixels(0, 0, width, height, gl.RGBA, gl.FLOAT, pixels);
+
+    if (isRed(pixels, 1, 1))
+        output("PASS: Framebuffer is floating-point red.");
+    else
+        output("FAIL: Framebuffer is not floating-point red.");
+}
+
+window.addEventListener("load", runTest, false);
+</script>
+<body>
+    <canvas></canvas>
+    <div id="output">
+    </div>
+</body>

diff --git a/LayoutTests/platform/ios/TestExpectations b/LayoutTests/platform/ios/TestExpectations
index 1692edb..ecb07d5 100644 (file)
--- a/LayoutTests/platform/ios/TestExpectations
+++ b/LayoutTests/platform/ios/TestExpectations
@@ -2765,6 +2765,9 @@ fast/canvas/webgl/texImage2D-mse-flipY-true.html [ Skip ]
 fast/canvas/webgl/texImage2D-video-flipY-false.html [ Skip ]
 fast/canvas/webgl/texImage2D-video-flipY-true.html [ Skip ]
 
+# rendering to floating point FBOs not supported on iOS
+fast/canvas/webgl/readPixels-float.html [ Skip ]
+
 # Skipped on iOS since UIHelper.activateAt() doesn't produce a user gesture that ITP captures on iOS
 webkit.org/b/174120 http/tests/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html [ Skip ]
 http/tests/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html [ Skip ]

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 99baa40..b54de56 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,34 @@
+2019-03-29  Dean Jackson  <dino@apple.com>
+
+        gl.readPixels with type gl.FLOAT does not work
+        https://bugs.webkit.org/show_bug.cgi?id=171432
+        <rdar://problem/31905150>
+
+        Reviewed by Antoine Quint.
+
+        Our validation code was identifying readPixels of
+        type FLOAT as invalid, for three reasons:
+        - we didn't support the FLOAT type at all.
+        - we only allowed the combination of RGBA and
+        UNSIGNED_BYTE in WebGL 1 [*].
+        - if we had a framebuffer of format RGBA, we assumed
+        we could only read into a Uint8 ArrayBuffer.
+
+        [*] This bug isn't completely fixed, so I opened
+        https://bugs.webkit.org/show_bug.cgi?id=196418
+
+        Test: fast/canvas/webgl/readPixels-float.html
+
+        * html/canvas/WebGLRenderingContextBase.cpp:
+        (WebCore::WebGLRenderingContextBase::readPixels):
+        - flip the logic in a conditional that was clearly wrong yet
+          thankfully had no impact.
+        - support type FLOAT when the relevant extension is enabled.
+        - allow FLOAT as a valid type (see new bug above)
+        - create a new macro for CHECK_COMPONENT_COUNT
+        - update the existing macros to not be case statements,
+          so that we can put logic in the switch.
+
 2019-03-30  Antti Koivisto  <antti@apple.com>
 
         Try to fix Windows build.

diff --git a/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp b/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp
index c882417..d64a974 100644 (file)
--- a/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp
+++ b/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp
@@ -3449,9 +3449,9 @@ void WebGLRenderingContextBase::readPixels(GC3Dint x, GC3Dint y, GC3Dsizei width
         internalFormat = m_framebufferBinding->getColorBufferFormat();
     } else {
         if (m_attributes.alpha)
-            internalFormat = GraphicsContext3D::RGB8;
-        else
             internalFormat = GraphicsContext3D::RGBA8;
+        else
+            internalFormat = GraphicsContext3D::RGB8;
     }
 
     if (!internalFormat) {
@@ -3475,12 +3475,18 @@ void WebGLRenderingContextBase::readPixels(GC3Dint x, GC3Dint y, GC3Dsizei width
         case GraphicsContext3D::UNSIGNED_SHORT_4_4_4_4:
         case GraphicsContext3D::UNSIGNED_SHORT_5_5_5_1:
             break;
+        case GraphicsContext3D::FLOAT:
+            if (!m_oesTextureFloat && !m_oesTextureHalfFloat) {
+                synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "invalid type");
+                return;
+            }
+            break;
         default:
             synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "invalid type");
             return;
         }
-        if (format != GraphicsContext3D::RGBA || type != GraphicsContext3D::UNSIGNED_BYTE) {
-            synthesizeGLError(GraphicsContext3D::INVALID_OPERATION, "readPixels", "format not RGBA or type not UNSIGNED_BYTE");
+        if (format != GraphicsContext3D::RGBA || (type != GraphicsContext3D::UNSIGNED_BYTE && type != GraphicsContext3D::FLOAT)) {
+            synthesizeGLError(GraphicsContext3D::INVALID_OPERATION, "readPixels", "format not RGBA or type not UNSIGNED_BYTE|FLOAT");
             return;
         }
     }
@@ -3492,71 +3498,84 @@ void WebGLRenderingContextBase::readPixels(GC3Dint x, GC3Dint y, GC3Dsizei width
         return;
     }
 
-#define INTERNAL_FORMAT_CHECK(themeMacro, typeMacro, pixelTypeMacro) case InternalFormatTheme::themeMacro: \
-        if (type != GraphicsContext3D::typeMacro || pixels.getType() != JSC::pixelTypeMacro) { \
-            synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
-            return; \
-        } \
-        if (format != GraphicsContext3D::RED && format != GraphicsContext3D::RG && format != GraphicsContext3D::RGB && format != GraphicsContext3D::RGBA) { \
-            synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Unknown format"); \
-            return; \
-        } \
-        if (numberOfComponentsForFormat(format) < internalFormatComponentCount) { \
-            synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Not enough components in format"); \
-            return; \
-        } \
-        break;
+#define CHECK_COMPONENT_COUNT \
+    if (numberOfComponentsForFormat(format) < internalFormatComponentCount) { \
+        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Not enough components in format"); \
+        return; \
+    }
 
-#define INTERNAL_FORMAT_INTEGER_CHECK(themeMacro, typeMacro, pixelTypeMacro) case InternalFormatTheme::themeMacro: \
-        if (type != GraphicsContext3D::typeMacro || pixels.getType() != JSC::pixelTypeMacro) { \
-            synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
-            return; \
-        } \
-        if (format != GraphicsContext3D::RED_INTEGER && format != GraphicsContext3D::RG_INTEGER && format != GraphicsContext3D::RGB_INTEGER && format != GraphicsContext3D::RGBA_INTEGER) { \
-            synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Unknown format"); \
-            return; \
-        } \
-        if (numberOfComponentsForFormat(format) < internalFormatComponentCount) { \
-            synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Not enough components in format"); \
-            return; \
-        } \
-        break;
+#define INTERNAL_FORMAT_CHECK(typeMacro, pixelTypeMacro) \
+    if (type != GraphicsContext3D::typeMacro || pixels.getType() != JSC::pixelTypeMacro) { \
+        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
+        return; \
+    } \
+    if (format != GraphicsContext3D::RED && format != GraphicsContext3D::RG && format != GraphicsContext3D::RGB && format != GraphicsContext3D::RGBA) { \
+        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Unknown format"); \
+        return; \
+    } \
+    CHECK_COMPONENT_COUNT
 
-#define PACKED_INTERNAL_FORMAT_CHECK(internalFormatMacro, formatMacro, type0Macro, pixelType0Macro, type1Macro, pixelType1Macro) case GraphicsContext3D::internalFormatMacro: \
-        if (!(type == GraphicsContext3D::type0Macro && pixels.getType() == JSC::pixelType0Macro) \
-            && !(type == GraphicsContext3D::type1Macro && pixels.getType() == JSC::pixelType1Macro)) { \
-            synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
-            return; \
-        } \
-        if (format != GraphicsContext3D::formatMacro) { \
-            synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Invalid format"); \
-            return; \
-        } \
-        break;
+#define INTERNAL_FORMAT_INTEGER_CHECK(typeMacro, pixelTypeMacro) \
+    if (type != GraphicsContext3D::typeMacro || pixels.getType() != JSC::pixelTypeMacro) { \
+        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
+        return; \
+    } \
+    if (format != GraphicsContext3D::RED_INTEGER && format != GraphicsContext3D::RG_INTEGER && format != GraphicsContext3D::RGB_INTEGER && format != GraphicsContext3D::RGBA_INTEGER) { \
+        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Unknown format"); \
+        return; \
+    } \
+    CHECK_COMPONENT_COUNT
+
+#define CASE_PACKED_INTERNAL_FORMAT_CHECK(internalFormatMacro, formatMacro, type0Macro, pixelType0Macro, type1Macro, pixelType1Macro) case GraphicsContext3D::internalFormatMacro: \
+    if (!(type == GraphicsContext3D::type0Macro && pixels.getType() == JSC::pixelType0Macro) \
+        && !(type == GraphicsContext3D::type1Macro && pixels.getType() == JSC::pixelType1Macro)) { \
+        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
+        return; \
+    } \
+    if (format != GraphicsContext3D::formatMacro) { \
+        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Invalid format"); \
+        return; \
+    } \
+    break;
 
     switch (internalFormatTheme) {
-    INTERNAL_FORMAT_CHECK        (NormalizedFixedPoint      , UNSIGNED_BYTE, TypeUint8  );
-    INTERNAL_FORMAT_CHECK        (SignedNormalizedFixedPoint, BYTE         , TypeInt8   );
-    INTERNAL_FORMAT_CHECK        (FloatingPoint             , FLOAT        , TypeFloat32);
-    INTERNAL_FORMAT_INTEGER_CHECK(SignedInteger             , INT          , TypeInt32  );
-    INTERNAL_FORMAT_INTEGER_CHECK(UnsignedInteger           , UNSIGNED_INT , TypeUint32 );
+    case InternalFormatTheme::NormalizedFixedPoint:
+        if (type == GraphicsContext3D::FLOAT) {
+            INTERNAL_FORMAT_CHECK(FLOAT, TypeFloat32);
+        } else {
+            INTERNAL_FORMAT_CHECK(UNSIGNED_BYTE, TypeUint8);
+        }
+        break;
+    case InternalFormatTheme::SignedNormalizedFixedPoint:
+        INTERNAL_FORMAT_CHECK(BYTE, TypeInt8);
+        break;
+    case InternalFormatTheme::FloatingPoint:
+        INTERNAL_FORMAT_CHECK(FLOAT, TypeFloat32);
+        break;
+    case InternalFormatTheme::SignedInteger:
+        INTERNAL_FORMAT_INTEGER_CHECK(INT, TypeInt32);
+        break;
+    case InternalFormatTheme::UnsignedInteger:
+        INTERNAL_FORMAT_INTEGER_CHECK(UNSIGNED_INT, TypeUint32);
+        break;
     case InternalFormatTheme::Packed:
         switch (internalFormat) {
-        PACKED_INTERNAL_FORMAT_CHECK(RGB565        , RGB         , UNSIGNED_SHORT_5_6_5        , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
-        PACKED_INTERNAL_FORMAT_CHECK(RGB5_A1       , RGBA        , UNSIGNED_SHORT_5_5_5_1      , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
-        PACKED_INTERNAL_FORMAT_CHECK(RGBA4         , RGBA        , UNSIGNED_SHORT_4_4_4_4      , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
-        PACKED_INTERNAL_FORMAT_CHECK(RGB9_E5       , RGB         , UNSIGNED_INT_5_9_9_9_REV    , TypeUint32, UNSIGNED_INT_5_9_9_9_REV   , TypeUint32 );
-        PACKED_INTERNAL_FORMAT_CHECK(RGB10_A2      , RGBA        , UNSIGNED_INT_2_10_10_10_REV , TypeUint32, UNSIGNED_INT_2_10_10_10_REV, TypeUint32 );
-        PACKED_INTERNAL_FORMAT_CHECK(R11F_G11F_B10F, RGB         , UNSIGNED_INT_10F_11F_11F_REV, TypeUint32, FLOAT                      , TypeFloat32);
-        PACKED_INTERNAL_FORMAT_CHECK(RGB10_A2UI    , RGBA_INTEGER, UNSIGNED_INT_2_10_10_10_REV , TypeUint32, UNSIGNED_INT_2_10_10_10_REV, TypeUint32 );
+            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB565        , RGB         , UNSIGNED_SHORT_5_6_5        , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
+            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB5_A1       , RGBA        , UNSIGNED_SHORT_5_5_5_1      , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
+            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGBA4         , RGBA        , UNSIGNED_SHORT_4_4_4_4      , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
+            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB9_E5       , RGB         , UNSIGNED_INT_5_9_9_9_REV    , TypeUint32, UNSIGNED_INT_5_9_9_9_REV   , TypeUint32 );
+            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB10_A2      , RGBA        , UNSIGNED_INT_2_10_10_10_REV , TypeUint32, UNSIGNED_INT_2_10_10_10_REV, TypeUint32 );
+            CASE_PACKED_INTERNAL_FORMAT_CHECK(R11F_G11F_B10F, RGB         , UNSIGNED_INT_10F_11F_11F_REV, TypeUint32, FLOAT                      , TypeFloat32);
+            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB10_A2UI    , RGBA_INTEGER, UNSIGNED_INT_2_10_10_10_REV , TypeUint32, UNSIGNED_INT_2_10_10_10_REV, TypeUint32 );
         }
         break;
     case InternalFormatTheme::None:
         ASSERT_NOT_REACHED();
     }
+#undef CHECK_COMPONENT_COUNT
 #undef INTERNAL_FORMAT_CHECK
 #undef INTERNAL_FORMAT_INTEGER_CHECK
-#undef PACKED_INTERNAL_FORMAT_CHECK
+#undef CASE_PACKED_INTERNAL_FORMAT_CHECK
 
     // Calculate array size, taking into consideration of PACK_ALIGNMENT.
     unsigned totalBytesRequired = 0;