Bug 782785 - Use temp surfaces to ReadPixels with correct stride - r=bjacob

content/canvas/src/WebGLContext.cpp

@@ -610,7 +610,7 @@ WebGLContext::Render(gfxContext *ctx, gfxPattern::GraphicsFilter f, PRUint32 aFl
     if (surf->CairoStatus() != 0)
         return NS_ERROR_FAILURE;
 
-    gl->ReadPixelsIntoImageSurface(0, 0, mWidth, mHeight, surf);
+    gl->ReadPixelsIntoImageSurface(surf);
 
     bool srcPremultAlpha = mOptions.premultipliedAlpha;
     bool dstPremultAlpha = aFlags & RenderFlagPremultAlpha;

gfx/gl/GLContext.cpp

@@ -1769,16 +1769,15 @@ GLContext::MarkDestroyed()
     mSymbols.Zero();
 }
 
-static void SwapRAndBComponents(gfxImageSurface* aSurf)
+static void SwapRAndBComponents(gfxImageSurface* surf)
 {
-  gfxIntSize size = aSurf->GetSize();
+    for (int j = 0; j < surf->Height(); ++j) {
-  for (int j = 0; j < size.height; ++j) {
+        uint32_t* row = (uint32_t*)(surf->Data() + surf->Stride() * j);
-    PRUint32 *row = (PRUint32*) (aSurf->Data() + aSurf->Stride() * j);
+        for (int i = 0; i < surf->Width(); ++i) {
-    for (int i = 0; i < size.width; ++i) {
+            *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
-      *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
+            row++;
-      row++;
+        }
     }
-  }
 }
 
 static already_AddRefed<gfxImageSurface> YInvertImageSurface(gfxImageSurface* aSurf)
@@ -2005,33 +2004,25 @@ GLContext::ReadScreenIntoImageSurface(gfxImageSurface* dest)
     fGetIntegerv(LOCAL_GL_FRAMEBUFFER_BINDING, (GLint*)&boundFB);
     fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);
 
-    ReadPixelsIntoImageSurface(0, 0, dest->Width(), dest->Height(), dest);
+    ReadPixelsIntoImageSurface(dest);
 
     fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, boundFB);
 }
 
 void
-GLContext::ReadPixelsIntoImageSurface(GLint aX, GLint aY,
+GLContext::ReadPixelsIntoImageSurface(gfxImageSurface* dest)
-                                      GLsizei aWidth, GLsizei aHeight,
-                                      gfxImageSurface *aDest)
 {
+    MOZ_ASSERT(dest->Format() == gfxASurface::ImageFormatARGB32 ||
+               dest->Format() == gfxASurface::ImageFormatRGB24);
+
+    MOZ_ASSERT(dest->Stride() == dest->Width() * 4);
+    MOZ_ASSERT(dest->Format() == gfxASurface::ImageFormatARGB32 ||
+               dest->Format() == gfxASurface::ImageFormatRGB24);
+
+    MOZ_ASSERT(dest->Stride() == dest->Width() * 4);
+
     MakeCurrent();
 
-    if (aDest->Format() != gfxASurface::ImageFormatARGB32 &&
-        aDest->Format() != gfxASurface::ImageFormatRGB24)
-    {
-        NS_WARNING("ReadPixelsIntoImageSurface called with invalid image format");
-        return;
-    }
-
-    if (aDest->Width() != aWidth ||
-        aDest->Height() != aHeight ||
-        aDest->Stride() != aWidth * 4)
-    {
-        NS_WARNING("ReadPixelsIntoImageSurface called with wrong size or stride surface");
-        return;
-    }
-
     GLint currentPackAlignment = 0;
     fGetIntegerv(LOCAL_GL_PACK_ALIGNMENT, &currentPackAlignment);
 
@@ -2043,20 +2034,14 @@ GLContext::ReadPixelsIntoImageSurface(GLint aX, GLint aY,
 
     GetOptimalReadFormats(this, format, datatype);
 
-    fReadPixels(0, 0, aWidth, aHeight,
+    fReadPixels(0, 0,
+                dest->Width(), dest->Height(),
                 format, datatype,
-                aDest->Data());
+                dest->Data());
 
-    // Output should be in BGRA, so swap if RGBA
+    // Output should be in BGRA, so swap if RGBA.
     if (format == LOCAL_GL_RGBA) {
-        // swap B and R bytes
+        SwapRAndBComponents(dest);
-        for (int j = 0; j < aHeight; ++j) {
-            PRUint32 *row = (PRUint32*) (aDest->Data() + aDest->Stride() * j);
-            for (int i = 0; i < aWidth; ++i) {
-                *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
-                row++;
-            }
-        }
     }
 
     if (currentPackAlignment != 4)

gfx/gl/GLContext.h

@@ -1385,15 +1385,16 @@ public:
     already_AddRefed<gfxImageSurface> GetTexImage(GLuint aTexture, bool aYInvert, ShaderProgramType aShader);
 
     /**
-     * Call ReadPixels into an existing gfxImageSurface for the given bounds.
+     * Call ReadPixels into an existing gfxImageSurface.
-     * The image surface must be using image format RGBA32 or RGB24.
+     * The image surface must be using image format RGBA32 or RGB24,
+     * and must have stride == width*4.
+     * Note that neither ReadPixelsIntoImageSurface nor
+     * ReadScreenIntoImageSurface call dest->Flush/MarkDirty.
      */
-    void THEBES_API ReadPixelsIntoImageSurface(GLint aX, GLint aY,
+    void THEBES_API ReadPixelsIntoImageSurface(gfxImageSurface* dest);
-                                    GLsizei aWidth, GLsizei aHeight,
-                                    gfxImageSurface *aDest);
 
     // Similar to ReadPixelsIntoImageSurface, but pulls from the screen
-    // instead of the currenly bound framebuffer.
+    // instead of the currently bound framebuffer.
     void ReadScreenIntoImageSurface(gfxImageSurface* dest);
 
     /**

gfx/layers/basic/BasicCanvasLayer.cpp

@@ -77,6 +77,7 @@ protected:
       mCachedFormat = aFormat;
     }
 
+    MOZ_ASSERT(mCachedTempSurface->Stride() == mCachedTempSurface->Width() * 4);
     return mCachedTempSurface;
   }
 
@@ -155,41 +156,67 @@ BasicCanvasLayer::UpdateSurface(gfxASurface* aDestSurface, Layer* aMaskLayer)
         return;
     }
 #endif
-    nsRefPtr<gfxImageSurface> isurf;
+
+    gfxIntSize readSize(mBounds.width, mBounds.height);
+    gfxImageFormat format = (GetContentFlags() & CONTENT_OPAQUE)
+                              ? gfxASurface::ImageFormatRGB24
+                              : gfxASurface::ImageFormatARGB32;
+
+    nsRefPtr<gfxImageSurface> readSurf;
+    nsRefPtr<gfxImageSurface> resultSurf;
+
+    bool usingTempSurface = false;
+
     if (aDestSurface) {
-      DiscardTempSurface();
+      resultSurf = static_cast<gfxImageSurface*>(aDestSurface);
-      isurf = static_cast<gfxImageSurface*>(aDestSurface);
+
+      if (resultSurf->GetSize() != readSize ||
+          resultSurf->Stride() != resultSurf->Width() * 4)
+      {
+        readSurf = GetTempSurface(readSize, format);
+        usingTempSurface = true;
+      }
     } else {
-      nsIntSize size(mBounds.width, mBounds.height);
+      resultSurf = GetTempSurface(readSize, format);
-      gfxImageFormat format = (GetContentFlags() & CONTENT_OPAQUE)
+      usingTempSurface = true;
-                                ? gfxASurface::ImageFormatRGB24
-                                : gfxASurface::ImageFormatARGB32;
-
-      isurf = GetTempSurface(size, format);
     }
 
+    if (!usingTempSurface)
+      DiscardTempSurface();
 
-    if (!isurf || isurf->CairoStatus() != 0) {
+    if (!readSurf)
+      readSurf = resultSurf;
+
+    if (!resultSurf || resultSurf->CairoStatus() != 0)
       return;
-    }
 
-    NS_ASSERTION(isurf->Stride() == mBounds.width * 4, "gfxImageSurface stride isn't what we expect!");
+    MOZ_ASSERT(readSurf);
+    MOZ_ASSERT(readSurf->Stride() == mBounds.width * 4, "gfxImageSurface stride isn't what we expect!");
 
     // We need to Flush() the surface before modifying it outside of cairo.
-    isurf->Flush();
+    readSurf->Flush();
-    mGLContext->ReadScreenIntoImageSurface(isurf);
+    mGLContext->ReadScreenIntoImageSurface(readSurf);
-    isurf->MarkDirty();
+    readSurf->MarkDirty();
 
     // If the underlying GLContext doesn't have a framebuffer into which
     // premultiplied values were written, we have to do this ourselves here.
     // Note that this is a WebGL attribute; GL itself has no knowledge of
     // premultiplied or unpremultiplied alpha.
     if (!mGLBufferIsPremultiplied)
-      gfxUtils::PremultiplyImageSurface(isurf);
+      gfxUtils::PremultiplyImageSurface(readSurf);
+
+    if (readSurf != resultSurf) {
+      MOZ_ASSERT(resultSurf->Width() >= readSurf->Width());
+      MOZ_ASSERT(resultSurf->Height() >= readSurf->Height());
+
+      resultSurf->Flush();
+      resultSurf->CopyFrom(readSurf);
+      resultSurf->MarkDirty();
+    }
 
     // stick our surface into mSurface, so that the Paint() path is the same
     if (!aDestSurface) {
-      mSurface = isurf;
+      mSurface = resultSurf;
     }
   }
 }

gfx/layers/opengl/LayerManagerOGL.cpp

@@ -1073,7 +1073,7 @@ LayerManagerOGL::CopyToTarget(gfxContext *aTarget)
   NS_ASSERTION(imageSurface->Stride() == width * 4,
                "Image Surfaces being created with weird stride!");
 
-  mGLContext->ReadPixelsIntoImageSurface(0, 0, width, height, imageSurface);
+  mGLContext->ReadPixelsIntoImageSurface(imageSurface);
 
   aTarget->SetOperator(gfxContext::OPERATOR_SOURCE);
   aTarget->Scale(1.0, -1.0);