Bug 37281

Summary: Emulate GL_IMPLEMENTATION_COLOR_READ_FORMAT/TYPE for glGet
Product: WebKit Reporter: Zhenyao Mo <zmo>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, kbr, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
revised patch: remove two extra whitespace
none
revised patch : responding to Ken Russell's review
none
revised patch: no longer print out the other supported format/type in the test - they are not constant for ES ports. none

Zhenyao Mo
Reported 2010-04-08 11:03:49 PDT
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.
Attachments
patch (27.55 KB, patch)
2010-04-14 18:42 PDT, Zhenyao Mo
no flags
revised patch: remove two extra whitespace (27.55 KB, patch)
2010-04-14 18:49 PDT, Zhenyao Mo
no flags
revised patch : responding to Ken Russell's review (27.25 KB, patch)
2010-04-15 15:38 PDT, Zhenyao Mo
no flags
revised patch: no longer print out the other supported format/type in the test - they are not constant for ES ports. (24.19 KB, patch)
2010-04-15 15:45 PDT, Zhenyao Mo
no flags
Zhenyao Mo
Comment 1 2010-04-14 18:42:07 PDT
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.
WebKit Review Bot
Comment 2 2010-04-14 18:45:23 PDT
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.
Zhenyao Mo
Comment 3 2010-04-14 18:49:46 PDT
Created attachment 53397 [details] revised patch: remove two extra whitespace
Kenneth Russell
Comment 4 2010-04-15 11:51:51 PDT
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.
Zhenyao Mo
Comment 5 2010-04-15 15:38:23 PDT
Created attachment 53484 [details] revised patch : responding to Ken Russell's review
Zhenyao Mo
Comment 6 2010-04-15 15:45:37 PDT
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.
Kenneth Russell
Comment 7 2010-04-21 18:59:32 PDT
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.
Dimitri Glazkov (Google)
Comment 8 2010-04-22 09:24:22 PDT
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! :)
WebKit Commit Bot
Comment 9 2010-04-22 09:54:12 PDT
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>
WebKit Commit Bot
Comment 10 2010-04-22 09:54:17 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.