Bug 37281

Summary:

Emulate GL_IMPLEMENTATION_COLOR_READ_FORMAT/TYPE for glGet

Product:

WebKit

Reporter:

Zhenyao Mo <zmo>

Component:

WebGL

Assignee:

Zhenyao Mo <zmo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarrin, commit-queue, kbr, oliver, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

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	Details Formatted Diff Diff
revised patch: remove two extra whitespace (27.55 KB, patch) 2010-04-14 18:49 PDT, Zhenyao Mo	no flags	Details Formatted Diff Diff
revised patch : responding to Ken Russell's review (27.25 KB, patch) 2010-04-15 15:38 PDT, Zhenyao Mo	no flags	Details Formatted Diff Diff
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	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

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.