Bug 34718 – readPixels must take PACK_ALIGNMENT into account

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

The implementation of readPixels must take the PACK_ALIGNMENT into account, in particular the case where it is set to 8. This must be done for all calls to readPixels, including any done internally (for example, in the Chromium port).

[Kenneth Russell]

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.

[Zhenyao Mo]

Created attachment 52825 [details]
patch

[Zhenyao Mo]

Created attachment 52879 [details]
revised patch: flush before reading pixels

[Zhenyao Mo]

Created attachment 52894 [details]
revised patch: deal with internal glReadPixels situation

[Kenneth Russell]

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 @@
> [phone]  Zhenyao Mo  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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 @@
> [phone]  Zhenyao Mo  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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 @@
> [phone]  Zhenyao Mo  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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>

[Kenneth Russell]

(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.

[Zhenyao Mo]

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.

[Kenneth Russell]

Comment on attachment 52905 [details]
revised patch: responding to Ken Russell's review

Looks good to me.

[Dimitri Glazkov (Google)]

Comment on attachment 52905 [details]
revised patch: responding to Ken Russell's review

if kbr says kbr, I say rs=me.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.