GL_IMPLEMENTATION_COLOR_READ_FORMAT and GL_IMPLEMENTATION_COLOR_READ_TYPE are part of ES but not GL. Need to emulate them in GraphicsContext3DMac and WebGraphicsContext3DDefaultImpl. Could just return any of the supported pair, but for desktop users, GL_RGB/GL_UNSIGNED_BYTE might be the most useful format.
Created attachment 53396 [details] patch Although currently we return RGB/UNSIGNED_BYTE as the other supported combination for readPixels(), all possible valid combinations have been tested.
Attachment 53396 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1059: One space before end of line comments [whitespace/comments] [5] WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1062: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 53397 [details] revised patch: remove two extra whitespace
Comment on attachment 53397 [details] revised patch: remove two extra whitespace Generally looks good; a few comments. > Index: WebCore/html/canvas/WebGLRenderingContext.cpp > =================================================================== > --- WebCore/html/canvas/WebGLRenderingContext.cpp (revision 57587) > +++ WebCore/html/canvas/WebGLRenderingContext.cpp (working copy) > @@ -1646,7 +1648,7 @@ PassRefPtr<WebGLArray> WebGLRenderingCon > if (type == GraphicsContext3D::UNSIGNED_BYTE) > array = WebGLUnsignedByteArray::create(num); > else > - array = WebGLUnsignedShortArray::create(num); > + array = WebGLUnsignedByteArray::create(num * sizeof(unsigned short)); This change is incorrect. readPixels is supposed to return a WebGLUnsignedShortArray when the type is UNSIGNED_SHORT. > Index: LayoutTests/fast/canvas/webgl/read-pixels.html > =================================================================== > --- LayoutTests/fast/canvas/webgl/read-pixels.html (revision 57587) > +++ LayoutTests/fast/canvas/webgl/read-pixels.html (working copy) > @@ -24,90 +24,250 @@ void main() > gl_FragColor = color; > } > </script> > - > +</head> > +<body> > +<canvas id="example" width="32px" height="32px"></canvas> > +<div id="description"></div> > +<div id="console"></div> > <script> > var successfullyParsed = false; > > -// These four declarations need to be global for "shouldBe" to see them > +// The below declarations need to be global for "shouldBe" to see them > var gl = null; > var array = null; > var expectedSize = 0; > -var pixel = [ 0, 0, 0, 1 ]; > +var pixel = [ 0, 0, 0, 0 ]; > +var expectedColor = [ 0, 0, 0, 0 ]; > +var pixelFormat = 0; > +var pixelType = 0; > + > +function getPixelFormatText(format) > +{ > + switch (format) { > + case gl.ALPHA: > + return "ALPHA"; > + case gl.RGB: > + return "RGB"; > + case gl.RGBA: > + return "RGBA"; > + default: > + return "UNKNOWN"; > + } > +} > > -function init() > +function getPixelTypeText(type) > { > - if (window.layoutTestController) { > - layoutTestController.overridePreference("WebKitWebGLEnabled", "1"); > - layoutTestController.dumpAsText(); > - layoutTestController.waitUntilDone(); > + switch (type) { > + case gl.UNSIGNED_BYTE: > + return "UNSIGNED_BYTE"; > + case gl.UNSIGNED_SHORT_5_6_5: > + return "UNSIGNED_SHORT_5_6_5"; > + case gl.UNSIGNED_SHORT_4_4_4_4: > + return "UNSIGNED_SHORT_4_4_4_4"; > + case gl.UNSIGNED_SHORT_5_5_5_1: > + return "UNSIGNED_SHORT_5_5_5_1"; > + default: > + return "UNKNOWN"; > } > +} > > - description('Verify readPixels() works fine with various PACK_ALIGNMENT values.'); > +function calculatePixelBytes(format, type) > +{ > + var size = 0; > + switch (format) { > + case gl.ALPHA: > + size = 1; > + break; > + case gl.RGB: > + size = 3; > + break; > + case gl.RGBA: > + size = 4; > + break; > + default: > + return -1; > + } > + switch (type) { > + case gl.UNSIGNED_BYTE: > + break; > + case gl.UNSIGNED_SHORT_5_6_5: > + if (format != gl.RGB) > + return -1; > + size = 2; > + break; > + case gl.UNSIGNED_SHORT_4_4_4_4: > + case gl.UNSIGNED_SHORT_5_5_5_1: > + if (format != gl.RGBA) > + return -1; > + size = 2; > + break; > + default: > + return -1; > + } > + return size; > +} > > - runTest(); > +function calculatePaddingBytes(bytesPerPixel, packAlignment, width) > +{ > + var padding = 0; > + switch (packAlignment) { > + case 1: > + case 2: > + case 4: > + case 8: > + padding = (bytesPerPixel * width) % packAlignment; > + if (padding > 0) > + padding = packAlignment - padding; > + break; > + default: > + return -1; > + } > + return padding; > } > > -function runTestIteration(packAlignment, width, height, size) > +function packColor(format, type, r, g, b, a) > { > - debug("Testing PACK_ALIGNMENT = " + packAlignment + ", width = " + width + ", height = " + height); > - gl.clearColor(1, 1, 0, 1); > + // FIXME: not sure if the color packing is correct for UNSIGNED_SHORT_*. The packing below is in fact incorrect. See below. > + var color = [ 0, 0, 0, 0 ]; > + var short = 0; > + switch (type) { > + case gl.UNSIGNED_BYTE: > + switch (format) { > + case gl.ALPHA: > + color[0] = a; > + break; > + case gl.RGB: > + color[0] = r; > + color[1] = g; > + color[2] = b; > + break; > + case gl.RGBA: > + color[0] = r; > + color[1] = g; > + color[2] = b; > + color[3] = a; > + break; > + default: > + return null; > + } > + break; > + case gl.UNSIGNED_SHORT_5_6_5: > + if (format != gl.RGB) > + return null; > + r >>= 3; > + g >>= 2; > + b >>= 3; > + short = (r << 11) + (g << 5) + b; > + color[0] = short % 256; > + color[1] = short >> 8; If the colors were treated as unsigned bytes, then this splitting would need to be dependent on the host platform's byte order. The splitting here would work for little-endian architectures, but not big-endian ones. However, per above, readPixels is supposed to return a WebGLUnsignedShortArray in this case. Therefore you can just compare the short value from the WebGLUnsignedShortArray with the value you assembled here, without worrying about splitting it across two unsigned bytes. That should simplify the tests.
Created attachment 53484 [details] revised patch : responding to Ken Russell's review
Created attachment 53485 [details] revised patch: no longer print out the other supported format/type in the test - they are not constant for ES ports.
Comment on attachment 53485 [details] revised patch: no longer print out the other supported format/type in the test - they are not constant for ES ports. Looks good.
Comment on attachment 53485 [details] revised patch: no longer print out the other supported format/type in the test - they are not constant for ES ports. rs=me. I even somewhat understand what it does! :)
Comment on attachment 53485 [details] revised patch: no longer print out the other supported format/type in the test - they are not constant for ES ports. Clearing flags on attachment: 53485 Committed r58102: <http://trac.webkit.org/changeset/58102>
All reviewed patches have been landed. Closing bug.