Assertion failure running Mozilla's WebGL performance regression tests
authorkbr@google.com <kbr@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 May 2012 00:00:52 +0000 (00:00 +0000)
committerkbr@google.com <kbr@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 May 2012 00:00:52 +0000 (00:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=85942

Reviewed by Stephen White.

Fixed incorrect assumptions about source formats and buffer sizes
when uploading to floating-point textures. Added code paths
supporting the necessary conversions.

Tests have been added to the WebGL conformance suite which cover
these new code paths; they verify uploads of HTMLCanvasElement,
HTMLImageElement, HTMLVideoElement, and ImageData to
floating-point textures. However, because floating-point texture
support is optional, and generally only supported on bots which
run with real GPUs and not in virtual machines, it isn't feasible
to incorporate these tests as layout tests.

Ran the new WebGL conformance tests in Chromium on Linux; all
pass.

* platform/graphics/GraphicsContext3D.cpp:
(WebCore::GraphicsContext3D::extractImageData):
Properly compute size of destination buffer.

(WebCore):
Add pack/unpack routines for converting RGBA8/BGRA8 to floating point.

(WebCore::doFloatingPointPacking):
Support RGBA8 and BGRA8 source formats.

(WebCore::isFloatingPointSource):
Factored out logic for assertions.

(WebCore::GraphicsContext3D::packPixels):
Generalized assertions and logic.

* platform/graphics/cairo/GraphicsContext3DCairo.cpp:
(WebCore::GraphicsContext3D::getImageData):
Properly compute size of destination buffer.

* platform/graphics/cg/GraphicsContext3DCG.cpp:
(WebCore::GraphicsContext3D::getImageData):
Properly compute size of destination buffer.

* platform/graphics/qt/GraphicsContext3DQt.cpp:
(WebCore::GraphicsContext3D::getImageData):
Properly compute size of destination buffer.

* platform/graphics/skia/GraphicsContext3DSkia.cpp:
(WebCore::GraphicsContext3D::getImageData):
Properly compute size of destination buffer.

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/GraphicsContext3D.cpp
Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp
Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp
Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp
Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp

index 1574775..167046a 100644 (file)
@@ -1,3 +1,57 @@
+2012-05-15  Kenneth Russell  <kbr@google.com>
+
+        Assertion failure running Mozilla's WebGL performance regression tests
+        https://bugs.webkit.org/show_bug.cgi?id=85942
+
+        Reviewed by Stephen White.
+
+        Fixed incorrect assumptions about source formats and buffer sizes
+        when uploading to floating-point textures. Added code paths
+        supporting the necessary conversions.
+
+        Tests have been added to the WebGL conformance suite which cover
+        these new code paths; they verify uploads of HTMLCanvasElement,
+        HTMLImageElement, HTMLVideoElement, and ImageData to
+        floating-point textures. However, because floating-point texture
+        support is optional, and generally only supported on bots which
+        run with real GPUs and not in virtual machines, it isn't feasible
+        to incorporate these tests as layout tests.
+
+        Ran the new WebGL conformance tests in Chromium on Linux; all
+        pass.
+
+        * platform/graphics/GraphicsContext3D.cpp:
+        (WebCore::GraphicsContext3D::extractImageData):
+        Properly compute size of destination buffer.
+
+        (WebCore):
+        Add pack/unpack routines for converting RGBA8/BGRA8 to floating point.
+
+        (WebCore::doFloatingPointPacking):
+        Support RGBA8 and BGRA8 source formats.
+
+        (WebCore::isFloatingPointSource):
+        Factored out logic for assertions.
+
+        (WebCore::GraphicsContext3D::packPixels):
+        Generalized assertions and logic.
+
+        * platform/graphics/cairo/GraphicsContext3DCairo.cpp:
+        (WebCore::GraphicsContext3D::getImageData):
+        Properly compute size of destination buffer.
+
+        * platform/graphics/cg/GraphicsContext3DCG.cpp:
+        (WebCore::GraphicsContext3D::getImageData):
+        Properly compute size of destination buffer.
+
+        * platform/graphics/qt/GraphicsContext3DQt.cpp:
+        (WebCore::GraphicsContext3D::getImageData):
+        Properly compute size of destination buffer.
+
+        * platform/graphics/skia/GraphicsContext3DSkia.cpp:
+        (WebCore::GraphicsContext3D::getImageData):
+        Properly compute size of destination buffer.
+
 2012-05-15  James Robinson  <jamesr@chromium.org>
 
         [chromium] Chromium port never sets USE(CG) so code behind it is dead
index c1b36f6..f01556b 100644 (file)
@@ -200,8 +200,13 @@ bool GraphicsContext3D::extractImageData(ImageData* imageData,
         return false;
     int width = imageData->width();
     int height = imageData->height();
-    int dataBytes = width * height * 4;
-    data.resize(dataBytes);
+
+    unsigned int packedSize;
+    // Output data is tightly packed (alignment == 1).
+    if (computeImageSizeInBytes(format, type, width, height, 1, &packedSize, 0) != GraphicsContext3D::NO_ERROR)
+        return false;
+    data.resize(packedSize);
+
     if (!packPixels(imageData->data()->data(),
                     SourceFormatRGBA8,
                     width,
@@ -704,6 +709,32 @@ void unpackOneRowOfA16BigToRGBA8(const uint16_t* source, uint8_t* destination, u
     }
 }
 
+void unpackOneRowOfRGBA8ToRGBA32F(const uint8_t* source, float* destination, unsigned int pixelsPerRow)
+{
+    const float scaleFactor = 1.0f / 255.0f;
+    for (unsigned int i = 0; i < pixelsPerRow; ++i) {
+        destination[0] = source[0] * scaleFactor;
+        destination[1] = source[1] * scaleFactor;
+        destination[2] = source[2] * scaleFactor;
+        destination[3] = source[3] * scaleFactor;
+        source += 4;
+        destination += 4;
+    }
+}
+
+void unpackOneRowOfBGRA8ToRGBA32F(const uint8_t* source, float* destination, unsigned int pixelsPerRow)
+{
+    const float scaleFactor = 1.0f / 255.0f;
+    for (unsigned int i = 0; i < pixelsPerRow; ++i) {
+        destination[0] = source[2] * scaleFactor;
+        destination[1] = source[1] * scaleFactor;
+        destination[2] = source[0] * scaleFactor;
+        destination[3] = source[3] * scaleFactor;
+        source += 4;
+        destination += 4;
+    }
+}
+
 void unpackOneRowOfRGB32FToRGBA32F(const float* source, float* destination, unsigned int pixelsPerRow)
 {
     for (unsigned int i = 0; i < pixelsPerRow; ++i) {
@@ -1062,6 +1093,31 @@ void packOneRowOfRGBA32FToRGB32FPremultiply(const float* source, float* destinat
     }
 }
 
+void packOneRowOfRGBA32FToRGB32FUnmultiply(const float* source, float* destination, unsigned int pixelsPerRow)
+{
+    for (unsigned int i = 0; i < pixelsPerRow; ++i) {
+        float scaleFactor = source[3] ? 1.0f / source[3] : 1.0f;
+        destination[0] = source[0] * scaleFactor;
+        destination[1] = source[1] * scaleFactor;
+        destination[2] = source[2] * scaleFactor;
+        source += 4;
+        destination += 3;
+    }
+}
+
+// Used only during RGBA8 or BGRA8 -> floating-point uploads.
+void packOneRowOfRGBA32FToRGBA32F(const float* source, float* destination, unsigned int pixelsPerRow)
+{
+    for (unsigned int i = 0; i < pixelsPerRow; ++i) {
+        destination[0] = source[0];
+        destination[1] = source[1];
+        destination[2] = source[2];
+        destination[3] = source[3];
+        source += 4;
+        destination += 4;
+    }
+}
+
 void packOneRowOfRGBA32FToRGBA32FPremultiply(const float* source, float* destination, unsigned int pixelsPerRow)
 {
     for (unsigned int i = 0; i < pixelsPerRow; ++i) {
@@ -1075,6 +1131,19 @@ void packOneRowOfRGBA32FToRGBA32FPremultiply(const float* source, float* destina
     }
 }
 
+void packOneRowOfRGBA32FToRGBA32FUnmultiply(const float* source, float* destination, unsigned int pixelsPerRow)
+{
+    for (unsigned int i = 0; i < pixelsPerRow; ++i) {
+        float scaleFactor = source[3] ? 1.0f / source[3] : 1.0f;
+        destination[0] = source[0] * scaleFactor;
+        destination[1] = source[1] * scaleFactor;
+        destination[2] = source[2] * scaleFactor;
+        destination[3] = source[3];
+        source += 4;
+        destination += 4;
+    }
+}
+
 void packOneRowOfRGBA32FToA32F(const float* source, float* destination, unsigned int pixelsPerRow)
 {
     for (unsigned int i = 0; i < pixelsPerRow; ++i) {
@@ -1103,6 +1172,15 @@ void packOneRowOfRGBA32FToR32FPremultiply(const float* source, float* destinatio
     }
 }
 
+void packOneRowOfRGBA32FToR32FUnmultiply(const float* source, float* destination, unsigned int pixelsPerRow)
+{
+    for (unsigned int i = 0; i < pixelsPerRow; ++i) {
+        float scaleFactor = source[3] ? 1.0f / source[3] : 1.0f;
+        destination[0] = source[0] * scaleFactor;
+        source += 4;
+        destination += 1;
+    }
+}
 
 void packOneRowOfRGBA32FToRA32F(const float* source, float* destination, unsigned int pixelsPerRow)
 {
@@ -1119,7 +1197,18 @@ void packOneRowOfRGBA32FToRA32FPremultiply(const float* source, float* destinati
     for (unsigned int i = 0; i < pixelsPerRow; ++i) {
         float scaleFactor = source[3];
         destination[0] = source[0] * scaleFactor;
-        destination[1] = scaleFactor;
+        destination[1] = source[3];
+        source += 4;
+        destination += 2;
+    }
+}
+
+void packOneRowOfRGBA32FToRA32FUnmultiply(const float* source, float* destination, unsigned int pixelsPerRow)
+{
+    for (unsigned int i = 0; i < pixelsPerRow; ++i) {
+        float scaleFactor = source[3] ? 1.0f / source[3] : 1.0f;
+        destination[0] = source[0] * scaleFactor;
+        destination[1] = source[3];
         source += 4;
         destination += 2;
     }
@@ -1367,6 +1456,16 @@ static void doFloatingPointPacking(const void* sourceData,
 {
     switch (sourceDataFormat) {
     case GraphicsContext3D::SourceFormatRGBA8: {
+        unsigned int sourceElementsPerRow = computeSourceElementsPerRow<uint8_t>(width, 4, sourceUnpackAlignment);
+        doUnpackingAndPacking<uint8_t, float, float>(static_cast<const uint8_t*>(sourceData), unpackOneRowOfRGBA8ToRGBA32F, width, height, sourceElementsPerRow, destinationData, rowPackingFunc, destinationElementsPerPixel);
+        break;
+    }
+    case GraphicsContext3D::SourceFormatBGRA8: {
+        unsigned int sourceElementsPerRow = computeSourceElementsPerRow<uint8_t>(width, 4, sourceUnpackAlignment);
+        doUnpackingAndPacking<uint8_t, float, float>(static_cast<const uint8_t*>(sourceData), unpackOneRowOfBGRA8ToRGBA32F, width, height, sourceElementsPerRow, destinationData, rowPackingFunc, destinationElementsPerPixel);
+        break;
+    }
+    case GraphicsContext3D::SourceFormatRGBA32F: {
         unsigned int sourceElementsPerRow = computeSourceElementsPerRow<float>(width, 4, sourceUnpackAlignment);
         const float* source = static_cast<const float*>(sourceData);
         const float* endPointer = source + height * sourceElementsPerRow;
@@ -1403,6 +1502,23 @@ static void doFloatingPointPacking(const void* sourceData,
     }
 }
 
+
+#if !ASSERT_DISABLED
+static bool isFloatingPointSource(GraphicsContext3D::SourceDataFormat format)
+{
+    switch (format) {
+    case GraphicsContext3D::SourceFormatRGBA32F:
+    case GraphicsContext3D::SourceFormatRGB32F:
+    case GraphicsContext3D::SourceFormatRA32F:
+    case GraphicsContext3D::SourceFormatR32F:
+    case GraphicsContext3D::SourceFormatA32F:
+        return true;
+    default:
+        return false;
+    }
+}
+#endif
+
 bool GraphicsContext3D::packPixels(const uint8_t* sourceData,
                                    GraphicsContext3D::SourceDataFormat sourceDataFormat,
                                    unsigned int width,
@@ -1539,16 +1655,21 @@ bool GraphicsContext3D::packPixels(const uint8_t* sourceData,
     }
     case FLOAT: {
         // OpenGL ES, and therefore WebGL, require that the format and
-        // internalformat be identical, which implies that the source and
-        // destination formats will both be floating-point in this branch -- at
-        // least, until WebKit supports floating-point image formats natively.
-        ASSERT(sourceDataFormat == SourceFormatRGBA32F || sourceDataFormat == SourceFormatRGB32F
-               || sourceDataFormat == SourceFormatRA32F || sourceDataFormat == SourceFormatR32F
-               || sourceDataFormat == SourceFormatA32F);
-        // Because WebKit doesn't use floating-point color channels for anything
-        // internally, there's no chance we have to do a (lossy) unmultiply
-        // operation.
-        ASSERT(alphaOp == AlphaDoNothing || alphaOp == AlphaDoPremultiply);
+        // internalformat be identical. This means that whenever the
+        // developer supplies an ArrayBufferView on this code path,
+        // the source data will be in a floating-point format.
+        //
+        // The only time the source data will not be floating-point is
+        // when uploading a DOM element or ImageData as a
+        // floating-point texture. Only RGBA8 and BGRA8 are handled in
+        // this case.
+        ASSERT(isFloatingPointSource(sourceDataFormat)
+               || sourceDataFormat == SourceFormatRGBA8
+               || sourceDataFormat == SourceFormatBGRA8);
+        // When uploading a canvas into a floating-point texture,
+        // unmultiplication may be necessary.
+        ASSERT((alphaOp == AlphaDoNothing || alphaOp == AlphaDoPremultiply)
+               || !isFloatingPointSource(sourceDataFormat));
         // For the source formats with an even number of channels (RGBA32F,
         // RA32F) it is guaranteed that the pixel data is tightly packed because
         // unpack alignment <= sizeof(float) * number of channels.
@@ -1570,14 +1691,25 @@ bool GraphicsContext3D::packPixels(const uint8_t* sourceData,
             case AlphaDoPremultiply:
                 doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToRGB32FPremultiply, 3);
                 break;
-            default:
-                ASSERT_NOT_REACHED();
+            case AlphaDoUnmultiply:
+                doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToRGB32FUnmultiply, 3);
+                break;
             }
             break;
         case RGBA:
-            // AlphaDoNothing is handled above with fast path.
-            ASSERT(alphaOp == AlphaDoPremultiply);
-            doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToRGBA32FPremultiply, 4);
+            // AlphaDoNothing for RGBA32F -> RGBA is handled above with fast path.
+            ASSERT(alphaOp != AlphaDoNothing || sourceDataFormat != SourceFormatRGBA32F);
+            switch (alphaOp) {
+            case AlphaDoNothing:
+                doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToRGBA32F, 4);
+                break;
+            case AlphaDoPremultiply:
+                doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToRGBA32FPremultiply, 4);
+                break;
+            case AlphaDoUnmultiply:
+                doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToRGBA32FUnmultiply, 4);
+                break;
+            }
             break;
         case ALPHA:
             // From the desktop OpenGL conversion rules (OpenGL 2.1
@@ -1596,8 +1728,9 @@ bool GraphicsContext3D::packPixels(const uint8_t* sourceData,
             case AlphaDoPremultiply:
                 doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToR32FPremultiply, 1);
                 break;
-            default:
-                ASSERT_NOT_REACHED();
+            case AlphaDoUnmultiply:
+                doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToR32FUnmultiply, 1);
+                break;
             }
             break;
         case LUMINANCE_ALPHA:
@@ -1611,8 +1744,9 @@ bool GraphicsContext3D::packPixels(const uint8_t* sourceData,
             case AlphaDoPremultiply:
                 doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToRA32FPremultiply, 2);
                 break;
-            default:
-                ASSERT_NOT_REACHED();
+            case AlphaDoUnmultiply:
+                doFloatingPointPacking(sourceData, sourceDataFormat, width, height, sourceUnpackAlignment, destination, packOneRowOfRGBA32FToRA32FUnmultiply, 2);
+                break;
             }
             break;
         }
index 8d39692..0fa2745 100644 (file)
@@ -185,7 +185,12 @@ bool GraphicsContext3D::getImageData(Image* image, unsigned int format, unsigned
             ++srcUnpackAlignment;
     }
 
-    outputVector.resize(width * height * 4);
+    unsigned int packedSize;
+    // Output data is tightly packed (alignment == 1).
+    if (computeImageSizeInBytes(format, type, width, height, 1, &packedSize, 0) != GraphicsContext3D::NO_ERROR)
+        return false;
+    outputVector.resize(packedSize);
+
     return packPixels(cairo_image_surface_get_data(imageSurface.get()), SourceFormatBGRA8,
                       width, height, srcUnpackAlignment, format, type, alphaOp, outputVector.data());
 }
index 57abe71..e044b8d 100644 (file)
@@ -240,7 +240,13 @@ bool GraphicsContext3D::getImageData(Image* image,
     if (!pixelData)
         return false;
     const UInt8* rgba = CFDataGetBytePtr(pixelData.get());
-    outputVector.resize(width * height * 4);
+
+    unsigned int packedSize;
+    // Output data is tightly packed (alignment == 1).
+    if (computeImageSizeInBytes(format, type, width, height, 1, &packedSize, 0) != GraphicsContext3D::NO_ERROR)
+        return false;
+    outputVector.resize(packedSize);
+
     unsigned int srcUnpackAlignment = 0;
     size_t bytesPerRow = CGImageGetBytesPerRow(cgImage);
     unsigned int padding = bytesPerRow - bitsPerPixel / 8 * width;
index e1c2b49..af95e4f 100644 (file)
@@ -1614,7 +1614,13 @@ bool GraphicsContext3D::getImageData(Image* image,
     AlphaOp neededAlphaOp = AlphaDoNothing;
     if (premultiplyAlpha)
         neededAlphaOp = AlphaDoPremultiply;
-    outputVector.resize(nativeImage.byteCount());
+
+    unsigned int packedSize;
+    // Output data is tightly packed (alignment == 1).
+    if (computeImageSizeInBytes(format, type, image->width(), image->height(), 1, &packedSize, 0) != GraphicsContext3D::NO_ERROR)
+        return false;
+    outputVector.resize(packedSize);
+
     return packPixels(nativeImage.bits(), SourceFormatBGRA8, image->width(), image->height(), 0, format, type, neededAlphaOp, outputVector.data());
 }
 
index f341152..019037c 100644 (file)
@@ -79,7 +79,11 @@ bool GraphicsContext3D::getImageData(Image* image,
     const SkBitmap& skiaImageRef = skiaImage->bitmap();
     SkAutoLockPixels lock(skiaImageRef);
     ASSERT(skiaImageRef.rowBytes() == skiaImageRef.width() * 4);
-    outputVector.resize(skiaImageRef.rowBytes() * skiaImageRef.height());
+    unsigned int packedSize;
+    // Output data is tightly packed (alignment == 1).
+    if (computeImageSizeInBytes(format, type, skiaImageRef.width(), skiaImageRef.height(), 1, &packedSize, 0) != GraphicsContext3D::NO_ERROR)
+        return false;
+    outputVector.resize(packedSize);
     return packPixels(reinterpret_cast<const uint8_t*>(skiaImageRef.getPixels()),
                       SK_B32_SHIFT ? SourceFormatRGBA8 : SourceFormatBGRA8,
                       skiaImageRef.width(), skiaImageRef.height(), 0,