| Summary: | readPixels must take PACK_ALIGNMENT into account | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||||||
| Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | chinmaya, cmarrin, commit-queue, kbr, oliver | ||||||||||
| Priority: | P3 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Kenneth Russell
2010-02-08 12:56:22 PST
The implementation must also handle the format parameter correctly, and likely perform a flush to work around apparent driver bugs where the most up-to-date rendering results are not read back. Created attachment 52825 [details]
patch
Created attachment 52879 [details]
revised patch: flush before reading pixels
Created attachment 52894 [details]
revised patch: deal with internal glReadPixels situation
Comment on attachment 52894 [details] revised patch: deal with internal glReadPixels situation Overall this looks excellent; a few comments below. > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 57249) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,21 @@ > +2010-04-07 Zhenyao Mo <zmo@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + readPixels must take PACK_ALIGNMENT into account > + https://bugs.webkit.org/show_bug.cgi?id=34718 > + > + Test: fast/canvas/webgl/read-pixels.html > + > + * html/canvas/WebGLRenderingContext.cpp: > + (WebCore::WebGLRenderingContext::WebGLRenderingContext): Init members to support pack_alignment. > + (WebCore::WebGLRenderingContext::pixelStorei): Save pack/unpack_alignment. > + (WebCore::WebGLRenderingContext::readPixels): Validate enum and deal with pack_alignment. > + * html/canvas/WebGLRenderingContext.h: Add members to support pack_alignment. > + * platform/graphics/GraphicsContext3D.h: Refac readPixels. Refac -> Refactor > + * platform/graphics/mac/GraphicsContext3DMac.cpp: > + (WebCore::GraphicsContext3D::readPixels): Move array allocation and alpha fix to WebGLRenderingContext; flush before read pixels. > + > 2010-04-07 Dumitru Daniliuc <dumi@chromium.org> > > Reviewed by Jeremy Orlow. > Index: WebCore/html/canvas/WebGLRenderingContext.cpp > =================================================================== > --- WebCore/html/canvas/WebGLRenderingContext.cpp (revision 57240) > +++ WebCore/html/canvas/WebGLRenderingContext.cpp (working copy) > @@ -38,6 +38,7 @@ > #include "RenderBox.h" > #include "RenderLayer.h" > #include "WebGLActiveInfo.h" > +#include "WebGLUnsignedShortArray.h" > #include "WebGLBuffer.h" > #include "WebGLContextAttributes.h" > #include "WebGLFramebuffer.h" > @@ -85,11 +86,21 @@ WebGLRenderingContext::WebGLRenderingCon > , m_needsUpdate(true) > , m_markedCanvasDirty(false) > , m_activeTextureUnit(0) > + , m_packAlignment(4) > + , m_unpackAlignment(4) > { > ASSERT(m_context); > int numVertexAttribs = 0; > m_context->getIntegerv(GraphicsContext3D::MAX_VERTEX_ATTRIBS, &numVertexAttribs); > m_maxVertexAttribs = numVertexAttribs; > + int implementationColorReadFormat = GraphicsContext3D::RGBA; > + m_context->getIntegerv(GraphicsContext3D::IMPLEMENTATION_COLOR_READ_FORMAT, &implementationColorReadFormat); > + m_implementationColorReadFormat = implementationColorReadFormat; > + int implementationColorReadType = GraphicsContext3D::UNSIGNED_BYTE; > + m_context->getIntegerv(GraphicsContext3D::IMPLEMENTATION_COLOR_READ_TYPE, &implementationColorReadType); > + // FIXME: remove the getError() when IMPLEMENTATION_COLOR_READ_FORMAT/TYPE are supported. > + m_context->getError(); > + m_implementationColorReadType = implementationColorReadType; > m_context->reshape(canvas()->width(), canvas()->height()); > m_context->viewport(0, 0, canvas()->width(), canvas()->height()); > } > @@ -1574,6 +1585,16 @@ void WebGLRenderingContext::linkProgram( > void WebGLRenderingContext::pixelStorei(unsigned long pname, long param) > { > m_context->pixelStorei(pname, param); > + if (param == 1 || param == 2 || param == 4 || param == 8) { > + switch (pname) { > + case GraphicsContext3D::PACK_ALIGNMENT: > + m_packAlignment = static_cast<int>(param); > + break; > + case GraphicsContext3D::UNPACK_ALIGNMENT: > + m_unpackAlignment = static_cast<int>(param); > + break; > + } > + } > cleanupAfterGraphicsCall(false); > } > > @@ -1585,7 +1606,76 @@ void WebGLRenderingContext::polygonOffse > > PassRefPtr<WebGLArray> WebGLRenderingContext::readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type) > { > - RefPtr<WebGLArray> array = m_context->readPixels(x, y, width, height, format, type); > + // Validate enums. > + unsigned long componentsPerPixel = 0; > + switch (format) { > + case GraphicsContext3D::ALPHA: > + componentsPerPixel = 1; > + break; > + case GraphicsContext3D::RGB: > + componentsPerPixel = 3; > + break; > + case GraphicsContext3D::RGBA: > + componentsPerPixel = 4; > + break; This is missing the LUMINANCE and LUMINANCE_ALPHA formats. > + default: > + m_context->synthesizeGLError(GraphicsContext3D::INVALID_ENUM); > + return 0; > + } > + unsigned long bytesPerComponent = 0; > + switch (type) { > + case GraphicsContext3D::UNSIGNED_BYTE: > + bytesPerComponent = sizeof(unsigned char); > + break; > + case GraphicsContext3D::UNSIGNED_SHORT_5_6_5: > + case GraphicsContext3D::UNSIGNED_SHORT_4_4_4_4: > + case GraphicsContext3D::UNSIGNED_SHORT_5_5_5_1: > + componentsPerPixel = 1; > + bytesPerComponent = sizeof(unsigned short); > + break; > + default: > + m_context->synthesizeGLError(GraphicsContext3D::INVALID_ENUM); > + return 0; > + } > + if (!(format == GraphicsContext3D::RGBA && type == GraphicsContext3D::UNSIGNED_BYTE || format == m_implementationColorReadFormat && type == m_implementationColorReadFormat)) { > + m_context->synthesizeGLError(GraphicsContext3D::INVALID_OPERATION); > + return 0; > + } > + // Calculate array size, taking into consideration of PACK_ALIGNMENT. > + unsigned long bytesPerRow = componentsPerPixel * bytesPerComponent * width; > + unsigned long padding = 0; > + unsigned long residuleBytes = bytesPerRow % m_packAlignment; residule -> residual > + if (residuleBytes) { > + padding = m_packAlignment - residuleBytes; > + bytesPerRow += padding; > + } > + // The last row needs no padding. > + unsigned long totalBytes = bytesPerRow * height - padding; > + unsigned long num = totalBytes / bytesPerComponent; > + RefPtr<WebGLArray> array; > + if (type == GraphicsContext3D::UNSIGNED_BYTE) > + array = WebGLUnsignedByteArray::create(num); > + else > + array = WebGLUnsignedShortArray::create(num); > + void* data = array->baseAddress(); > + m_context->readPixels(x, y, width, height, format, type, data); > +#if PLATFORM(CG) > + // FIXME: remove this section when GL driver bug on Mac is fixed, i.e., > + // when alpha is off, readPixels should set alpha to 255 instead of 0. > + if ((format == GraphicsContext3D::ALPHA || format == GraphicsContext3D::RGBA) && !m_context->getContextAttributes().alpha) { > + if (type == GraphicsContext3D::UNSIGNED_BYTE) { > + unsigned char* pixels = reinterpret_cast<unsigned char*>(data); > + for (unsigned long iy = 0; iy < height; ++iy) { > + for (unsigned long ix = 0; ix < width; ++ix) { > + pixels[componentsPerPixel - 1] = 255; > + pixels += componentsPerPixel; > + } > + pixels += padding; > + } > + } > + // FIXME: check whether we need to do the same with UNSIGNED_SHORT. > + } > +#endif > cleanupAfterGraphicsCall(false); > return array; > } > Index: WebCore/html/canvas/WebGLRenderingContext.h > =================================================================== > --- WebCore/html/canvas/WebGLRenderingContext.h (revision 57240) > +++ WebCore/html/canvas/WebGLRenderingContext.h (working copy) > @@ -342,6 +342,11 @@ class WebKitCSSMatrix; > TextureUnitState m_textureUnits[32]; > unsigned long m_activeTextureUnit; > > + int m_packAlignment; > + int m_unpackAlignment; > + unsigned long m_implementationColorReadFormat; > + unsigned long m_implementationColorReadType; > + > // Helpers for getParameter and others > WebGLGetInfo getBooleanParameter(unsigned long pname); > WebGLGetInfo getFloatParameter(unsigned long pname); > Index: WebCore/platform/graphics/GraphicsContext3D.h > =================================================================== > --- WebCore/platform/graphics/GraphicsContext3D.h (revision 57240) > +++ WebCore/platform/graphics/GraphicsContext3D.h (working copy) > @@ -592,7 +592,7 @@ namespace WebCore { > void pixelStorei(unsigned long pname, long param); > void polygonOffset(double factor, double units); > > - PassRefPtr<WebGLArray> readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type); > + void readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type, void* data); > > void releaseShaderCompiler(); > void renderbufferStorage(unsigned long target, unsigned long internalformat, unsigned long width, unsigned long height); > Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp > =================================================================== > --- WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp (revision 57240) > +++ WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp (working copy) > @@ -785,36 +785,20 @@ void GraphicsContext3D::polygonOffset(do > ::glPolygonOffset(static_cast<float>(factor), static_cast<float>(units)); > } > > -PassRefPtr<WebGLArray> GraphicsContext3D::readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type) > +void GraphicsContext3D::readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type, void* data) > { > ensureContext(m_contextObj); > - > - // FIXME: For now we only accept GL_UNSIGNED_BYTE/GL_RGBA. In reality OpenGL ES 2.0 accepts that pair and one other > - // as specified by GL_IMPLEMENTATION_COLOR_READ_FORMAT and GL_IMPLEMENTATION_COLOR_READ_TYPE. But for now we will > - // not accept those. > - // FIXME: Also, we should throw when an unacceptable value is passed The second FIXME should state that we should synthesize a GL error when an unacceptable value is passed. Actually, you should just synthesize an INVALID_OPERATION error and get rid of the FIXME. > - if (type != GL_UNSIGNED_BYTE || format != GL_RGBA) > - return 0; > - > + ::glFlush(); I think it's worth adding a note that the calls to glFlush appear to be necessary because of some OpenGL drivers which seem to not complete all of the previous rendering work before executing the glReadPixels. > if (m_attrs.antialias && m_boundFBO == m_multisampleFBO) { > ::glBindFramebufferEXT(GL_READ_FRAMEBUFFER_EXT, m_multisampleFBO); > ::glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER_EXT, m_fbo); > ::glBlitFramebufferEXT(x, y, x + width, y + height, x, y, x + width, y + height, GL_COLOR_BUFFER_BIT, GL_LINEAR); > ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_fbo); > + ::glFlush(); > } > - RefPtr<WebGLUnsignedByteArray> array = WebGLUnsignedByteArray::create(width * height * 4); > - ::glReadPixels(x, y, width, height, format, type, (GLvoid*) array->data()); > + ::glReadPixels(x, y, width, height, format, type, data); > if (m_attrs.antialias && m_boundFBO == m_multisampleFBO) > ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_multisampleFBO); > - if (!m_attrs.alpha) { > - // If alpha is off, by default glReadPixels should set the alpha to 255 instead of 0. > - // This is a hack until ::glReadPixels fixes its behavior. > - GLubyte* data = reinterpret_cast<GLubyte*>(array->data()); > - unsigned byteLength = array->byteLength(); > - for (unsigned i = 3; i < byteLength; i += 4) > - data[i] = 255; > - } > - return array; > } > > void GraphicsContext3D::releaseShaderCompiler() > Index: WebKit/chromium/ChangeLog > =================================================================== > --- WebKit/chromium/ChangeLog (revision 57249) > +++ WebKit/chromium/ChangeLog (working copy) > @@ -1,3 +1,15 @@ > +2010-04-07 Zhenyao Mo <zmo@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + readPixels must take PACK_ALIGNMENT into account > + https://bugs.webkit.org/show_bug.cgi?id=34718 > + > + * src/GraphicsContext3D.cpp: Refac readPixels. Refac -> Refactor > + * src/WebGraphicsContext3DDefaultImpl.cpp: > + (WebKit::WebGraphicsContext3DDefaultImpl::readBackFramebuffer): Temporarily disable pack alignment for glReadPixels. > + (WebKit::WebGraphicsContext3DDefaultImpl::readPixels): Move array allocation and alpha fix to WebGLRenderingContext; flush before read pixels. > + > 2010-04-07 Pavel Feldman <pfeldman@chromium.org> > > Reviewed by Yury Semikhatsky. > Index: WebKit/chromium/src/GraphicsContext3D.cpp > =================================================================== > --- WebKit/chromium/src/GraphicsContext3D.cpp (revision 57240) > +++ WebKit/chromium/src/GraphicsContext3D.cpp (working copy) > @@ -218,7 +218,7 @@ public: > void pixelStorei(unsigned long pname, long param); > void polygonOffset(double factor, double units); > > - PassRefPtr<WebGLArray> readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type); > + void readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type, void* data); > > void releaseShaderCompiler(); > void renderbufferStorage(unsigned long target, unsigned long internalformat, unsigned long width, unsigned long height); > @@ -590,6 +590,12 @@ rt GraphicsContext3DInternal::name(t1 a1 > return m_impl->name(a1, a2, a3, a4, a5, a6); \ > } > > +#define DELEGATE_TO_IMPL_7(name, t1, t2, t3, t4, t5, t6, t7) \ > +void GraphicsContext3DInternal::name(t1 a1, t2 a2, t3 a3, t4 a4, t5 a5, t6 a6, t7 a7) \ > +{ \ > + m_impl->name(a1, a2, a3, a4, a5, a6, a7); \ > +} > + > #define DELEGATE_TO_IMPL_7R(name, t1, t2, t3, t4, t5, t6, t7, rt) \ > rt GraphicsContext3DInternal::name(t1 a1, t2 a2, t3 a3, t4 a4, t5 a5, t6 a6, t7 a7) \ > { \ > @@ -804,15 +810,7 @@ DELEGATE_TO_IMPL_1(lineWidth, double) > DELEGATE_TO_IMPL_1_X(linkProgram, WebGLProgram*) > DELEGATE_TO_IMPL_2(pixelStorei, unsigned long, long) > DELEGATE_TO_IMPL_2(polygonOffset, double, double) > - > -PassRefPtr<WebGLArray> GraphicsContext3DInternal::readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type) > -{ > - // FIXME: take into account pack alignment. > - RefPtr<WebGLUnsignedByteArray> array = WebGLUnsignedByteArray::create(width * height * 4); > - m_impl->readPixels(x, y, width, height, format, type, array->baseAddress()); > - return array; > -} > - > +DELEGATE_TO_IMPL_7(readPixels, long, long, unsigned long, unsigned long, unsigned long, unsigned long, void*) > DELEGATE_TO_IMPL(releaseShaderCompiler) > DELEGATE_TO_IMPL_4(renderbufferStorage, unsigned long, unsigned long, unsigned long, unsigned long) > DELEGATE_TO_IMPL_2(sampleCoverage, double, bool) > @@ -1026,6 +1024,12 @@ rt GraphicsContext3D::name(t1 a1, t2 a2, > return m_internal->name(a1, a2, a3, a4, a5, a6); \ > } > > +#define DELEGATE_TO_INTERNAL_7(name, t1, t2, t3, t4, t5, t6, t7) \ > +void GraphicsContext3D::name(t1 a1, t2 a2, t3 a3, t4 a4, t5 a5, t6 a6, t7 a7) \ > +{ \ > + m_internal->name(a1, a2, a3, a4, a5, a6, a7); \ > +} > + > #define DELEGATE_TO_INTERNAL_7R(name, t1, t2, t3, t4, t5, t6, t7, rt) \ > rt GraphicsContext3D::name(t1 a1, t2 a2, t3 a3, t4 a4, t5 a5, t6 a6, t7 a7) \ > { \ > @@ -1183,7 +1187,7 @@ DELEGATE_TO_INTERNAL_1(linkProgram, WebG > DELEGATE_TO_INTERNAL_2(pixelStorei, unsigned long, long) > DELEGATE_TO_INTERNAL_2(polygonOffset, double, double) > > -DELEGATE_TO_INTERNAL_6R(readPixels, long, long, unsigned long, unsigned long, unsigned long, unsigned long, PassRefPtr<WebGLArray>) > +DELEGATE_TO_INTERNAL_7(readPixels, long, long, unsigned long, unsigned long, unsigned long, unsigned long, void*) > > DELEGATE_TO_INTERNAL(releaseShaderCompiler) > DELEGATE_TO_INTERNAL_4(renderbufferStorage, unsigned long, unsigned long, unsigned long, unsigned long) > Index: WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp > =================================================================== > --- WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp (revision 57240) > +++ WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp (working copy) > @@ -661,6 +661,15 @@ bool WebGraphicsContext3DDefaultImpl::re > glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_fbo); > } > } > + > + GLint packAlignment = 4; > + bool mustRestorePackAlignment = false; > + glGetIntegerv(GL_PACK_ALIGNMENT, &packAlignment); > + if (packAlignment > 4) { > + glPixelStorei(GL_PACK_ALIGNMENT, 4); > + mustRestorePackAlignment = true; > + } > + > #if PLATFORM(SKIA) > glReadPixels(0, 0, m_cachedWidth, m_cachedHeight, GL_BGRA, GL_UNSIGNED_BYTE, pixels); > #elif PLATFORM(CG) > @@ -669,6 +678,9 @@ bool WebGraphicsContext3DDefaultImpl::re > #error Must port to your platform > #endif > > + if (mustRestorePackAlignment) > + glPixelStorei(GL_PACK_ALIGNMENT, packAlignment); > + > if (mustRestoreFBO) > glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_boundFBO); > > @@ -1118,27 +1130,17 @@ DELEGATE_TO_GL_2(polygonOffset, PolygonO > > void WebGraphicsContext3DDefaultImpl::readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type, void* pixels) > { I think we should add a comment about the need for the glFlush calls here too. > + glFlush(); > #ifndef RENDER_TO_DEBUGGING_WINDOW > if (m_attributes.antialias && m_boundFBO == m_multisampleFBO) { > glBindFramebufferEXT(GL_READ_FRAMEBUFFER_EXT, m_multisampleFBO); > glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER_EXT, m_fbo); > glBlitFramebufferEXT(x, y, x + width, y + height, x, y, x + width, y + height, GL_COLOR_BUFFER_BIT, GL_LINEAR); > glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_fbo); > + glFlush(); > } > #endif > glReadPixels(x, y, width, height, format, type, pixels); > -#if PLATFORM(CG) > - if (!m_attributes.alpha) { > - // If alpha is off, by default glReadPixels should set the alpha to 255 instead of 0. > - // This is a hack until glReadPixels fixes its behavior. > - // Pixels are stored in WebGLUnsignedByteArray, which is unsigned char array. > - unsigned char* data = reinterpret_cast<unsigned char*>(pixels); > - // FIXME: take into account pack alignment. > - unsigned long byteLength = width * height * 4 * sizeof(unsigned char); > - for (unsigned long i = 3; i < byteLength; i += 4) > - data[i] = 255; > - } > -#endif > #ifndef RENDER_TO_DEBUGGING_WINDOW > if (m_attributes.antialias && m_boundFBO == m_multisampleFBO) > glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_boundFBO); > Index: LayoutTests/ChangeLog > =================================================================== > --- LayoutTests/ChangeLog (revision 57249) > +++ LayoutTests/ChangeLog (working copy) > @@ -1,3 +1,13 @@ > +2010-04-07 Zhenyao Mo <zmo@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + readPixels must take PACK_ALIGNMENT into account > + https://bugs.webkit.org/show_bug.cgi?id=34718 > + > + * fast/canvas/webgl/read-pixels-expected.txt: Verify pixelStorei and readPixels. > + * fast/canvas/webgl/read-pixels.html: Ditto. > + > 2010-04-07 Eric Seidel <eric@webkit.org> > > Reviewed by Adam Barth. > Index: LayoutTests/fast/canvas/webgl/read-pixels-expected.txt > =================================================================== > --- LayoutTests/fast/canvas/webgl/read-pixels-expected.txt (revision 0) > +++ LayoutTests/fast/canvas/webgl/read-pixels-expected.txt (revision 0) > @@ -0,0 +1,56 @@ > +Verify readPixels() works fine with various PACK_ALIGNMENT values. > + > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > + > +PASS gl = initWebGL('example', 'vshader', 'fshader', [ 'pos', 'colorIn' ], [ 0, 0, 0, 1 ], 1) is non-null. > +Testing PACK_ALIGNMENT = 1, width = 1, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 2, width = 1, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 4, width = 1, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 8, width = 1, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 4, width = 2, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 8, width = 2, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 4, width = 3, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 8, width = 3, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 4, width = 4, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 8, width = 4, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 8, width = 5, height = 1 > +PASS array.length is expectedSize > +Testing PACK_ALIGNMENT = 4, width = 5, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 8, width = 5, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 8, width = 6, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 8, width = 7, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +Testing PACK_ALIGNMENT = 8, width = 8, height = 2 > +PASS array.length is expectedSize > +PASS pixel is [ 51, 102, 204, 255] > +PASS successfullyParsed is true > + > +TEST COMPLETE > + > Index: LayoutTests/fast/canvas/webgl/read-pixels.html > =================================================================== > --- LayoutTests/fast/canvas/webgl/read-pixels.html (revision 0) > +++ LayoutTests/fast/canvas/webgl/read-pixels.html (revision 0) > @@ -0,0 +1,112 @@ > +<html> > +<head> > +<link rel="stylesheet" href="../../js/resources/js-test-style.css"/> > +<script src="../../js/resources/js-test-pre.js"></script> > +<script src="resources/webgl-test.js"></script> > +<script src="resources/utils3d.js"></script> > +<script id="vshader" type="x-shader/x-vertex"> > +attribute vec3 pos; > +attribute vec4 colorIn; > +varying vec4 color; > + > +void main() > +{ > + color = colorIn; > + gl_Position = vec4(pos.xyz, 1.0); > +} > +</script> > + > +<script id="fshader" type="x-shader/x-fragment"> > +varying vec4 color; > + > +void main() > +{ > + gl_FragColor = color; > +} > +</script> > + > +<script> > +var successfullyParsed = false; > + > +// These four 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 ]; > + > +function init() > +{ > + if (window.layoutTestController) { > + layoutTestController.overridePreference("WebKitWebGLEnabled", "1"); > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); > + } > + > + description('Verify readPixels() works fine with various PACK_ALIGNMENT values.'); > + > + runTest(); > +} > + > +function runTestIteration(packAlignment, width, height, size) > +{ > + debug("Testing PACK_ALIGNMENT = " + packAlignment + ", width = " + width + ", height = " + height); > + gl.clearColor(0.2, 0.4, 0.8, 1); It may be risky to rely on the precise rounding of these values. Perhaps specify them as e.g. (63.0 / 255.0) and check for 63 from readPixels. > + gl.clear(gl.COLOR_BUFFER_BIT); > + gl.pixelStorei(gl.PACK_ALIGNMENT, packAlignment); > + array = gl.readPixels(0, 0, width, height, gl.RGBA, gl.UNSIGNED_BYTE); > + expectedSize = size; > + shouldBe("array.length", "expectedSize"); > + // Check the first pixel of the second row. > + if (height > 1) { > + var padding = (size - width * height * 4) / (height - 1); > + var bytesPerRow = width * 4 + padding; > + pixel[0] = array[bytesPerRow]; > + pixel[1] = array[bytesPerRow + 1]; > + pixel[2] = array[bytesPerRow + 2]; > + pixel[3] = array[bytesPerRow + 3]; > + shouldBe("pixel", "[ 51, 102, 204, 255]"); See above. > + } > +} > + > +function runTest() > +{ > + shouldBeNonNull("gl = initWebGL('example', 'vshader', 'fshader', [ 'pos', 'colorIn' ], [ 0, 0, 0, 1 ], 1)"); > + gl.disable(gl.BLEND); > + > + runTestIteration(1, 1, 2, 8); > + runTestIteration(2, 1, 2, 8); > + runTestIteration(4, 1, 2, 8); > + runTestIteration(8, 1, 2, 12); > + runTestIteration(4, 2, 2, 16); > + runTestIteration(8, 2, 2, 16); > + runTestIteration(4, 3, 2, 24); > + runTestIteration(8, 3, 2, 28); > + runTestIteration(4, 4, 2, 32); > + runTestIteration(8, 4, 2, 32); > + runTestIteration(8, 5, 1, 20); > + runTestIteration(4, 5, 2, 40); > + runTestIteration(8, 5, 2, 44); > + runTestIteration(8, 6, 2, 48); > + runTestIteration(8, 7, 2, 60); > + runTestIteration(8, 8, 2, 64); > + > + successfullyParsed = true; > + var epilogue = document.createElement("script"); > + epilogue.onload = finish; > + epilogue.src = "../../js/resources/js-test-post.js"; > + document.body.appendChild(epilogue); > +} > + > +function finish() { > + if (window.layoutTestController) { > + layoutTestController.notifyDone(); > + } > +} > +</script> > +</head> > +<body onload="init()"> > +<canvas id="example" width="32px" height="32px"></canvas> > +<div id="description"></div> > +<div id="console"></div> > +</body> > +</html> (In reply to comment #5) > This is missing the LUMINANCE and LUMINANCE_ALPHA formats. Sorry, as we discussed offline, I'm wrong about this. > The second FIXME should state that we should synthesize a GL error when an > unacceptable value is passed. Actually, you should just synthesize an > INVALID_OPERATION error and get rid of the FIXME. Sorry, I failed to realize you deleted these FIXMEs. Ignore me. Created attachment 52905 [details]
revised patch: responding to Ken Russell's review
Also, modified the test to check the last pixel in the last row because that pixel is more PACK_ALIGNMENT sensitive.
Comment on attachment 52905 [details]
revised patch: responding to Ken Russell's review
Looks good to me.
Comment on attachment 52905 [details]
revised patch: responding to Ken Russell's review
if kbr says kbr, I say rs=me.
Comment on attachment 52905 [details] revised patch: responding to Ken Russell's review Clearing flags on attachment: 52905 Committed r57574: <http://trac.webkit.org/changeset/57574> All reviewed patches have been landed. Closing bug. |