Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. - r=jrmuizel
authorJeff Gilbert <jgilbert@mozilla.com>
Thu, 21 Jul 2016 23:05:52 -0700
changeset 306517 9e90e83343b6d2c380b9e08ffaf05c7e343083fc
parent 306516 eaf7778fef47cfe476e1b9f7da88d29de398b05b
child 306518 19b827d6c48923485376a258dbbdbcb2355ccb28
push id79870
push userjgilbert@mozilla.com
push dateMon, 25 Jul 2016 20:55:31 +0000
treeherdermozilla-inbound@19b827d6c489 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1280499
milestone50.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. - r=jrmuizel Top-of-tree test is green now. MozReview-Commit-ID: IbCTHK62qGT
dom/canvas/TexUnpackBlob.cpp
dom/canvas/WebGLTextureUpload.cpp
--- a/dom/canvas/TexUnpackBlob.cpp
+++ b/dom/canvas/TexUnpackBlob.cpp
@@ -128,31 +128,39 @@ FormatForPackingInfo(const PackingInfo& 
     }
 
     return WebGLTexelFormat::FormatNotSupportingAnyConversion;
 }
 
 ////////////////////
 
 static uint32_t
+ZeroOn2D(TexImageTarget target, uint32_t val)
+{
+    return (IsTarget3D(target) ? val : 0);
+}
+
+static uint32_t
 FallbackOnZero(uint32_t val, uint32_t fallback)
 {
     return (val ? val : fallback);
 }
 
 TexUnpackBlob::TexUnpackBlob(const WebGLContext* webgl, TexImageTarget target,
                              uint32_t rowLength, uint32_t width, uint32_t height,
                              uint32_t depth, bool isSrcPremult)
     : mAlignment(webgl->mPixelStore_UnpackAlignment)
     , mRowLength(rowLength)
-    , mImageHeight(FallbackOnZero(webgl->mPixelStore_UnpackImageHeight, height))
+    , mImageHeight(FallbackOnZero(ZeroOn2D(target,
+                                           webgl->mPixelStore_UnpackImageHeight),
+                                  height))
 
     , mSkipPixels(webgl->mPixelStore_UnpackSkipPixels)
     , mSkipRows(webgl->mPixelStore_UnpackSkipRows)
-    , mSkipImages(IsTarget3D(target) ? webgl->mPixelStore_UnpackSkipImages : 0)
+    , mSkipImages(ZeroOn2D(target, webgl->mPixelStore_UnpackSkipImages))
 
     , mWidth(width)
     , mHeight(height)
     , mDepth(depth)
 
     , mIsSrcPremult(isSrcPremult)
 
     , mNeedsExactUpload(false)
@@ -388,51 +396,64 @@ TexUnpackBytes::TexOrSubImage(bool isSub
 
     if (!isSubImage) {
         // Alloc first to catch OOMs.
         gl->fBindBuffer(LOCAL_GL_PIXEL_UNPACK_BUFFER, 0);
         *out_error = DoTexOrSubImage(false, gl, target, level, dui, xOffset, yOffset,
                                      zOffset, mWidth, mHeight, mDepth, nullptr);
         gl->fBindBuffer(LOCAL_GL_PIXEL_UNPACK_BUFFER,
                         webgl->mBoundPixelUnpackBuffer->mGLName);
+        if (*out_error)
+            return false;
     }
 
     //////
 
+    // Make our sometimes-implicit values explicit. Also this keeps them constant when we
+    // ask for height=mHeight-1 and such.
+    gl->fPixelStorei(LOCAL_GL_UNPACK_ROW_LENGTH, mRowLength);
+    gl->fPixelStorei(LOCAL_GL_UNPACK_IMAGE_HEIGHT, mImageHeight);
+
     if (mDepth > 1) {
         *out_error = DoTexOrSubImage(true, gl, target, level, dui, xOffset, yOffset,
                                      zOffset, mWidth, mHeight, mDepth-1, uploadPtr);
     }
 
+    // Skip the images we uploaded.
+    gl->fPixelStorei(LOCAL_GL_UNPACK_SKIP_IMAGES, mSkipImages + mDepth - 1);
+
     if (mHeight > 1) {
         *out_error = DoTexOrSubImage(true, gl, target, level, dui, xOffset, yOffset,
                                      zOffset+mDepth-1, mWidth, mHeight-1, 1, uploadPtr);
     }
 
-    const uint32_t imageStride = rowStride.value() * mImageHeight;
+    const auto totalSkipRows = CheckedUint32(mSkipImages) * mImageHeight + mSkipRows;
+    const auto totalFullRows = CheckedUint32(mDepth - 1) * mImageHeight + mHeight - 1;
+    const auto tailOffsetRows = totalSkipRows + totalFullRows;
 
-    const uint32_t usedImages = mSkipImages + mDepth;
-    const uint32_t usedRows = mSkipRows + mHeight;
+    const auto tailOffsetBytes = tailOffsetRows * rowStride;
 
-    uploadPtr += (usedImages - 1) * imageStride;
-    uploadPtr += (usedRows - 1) * rowStride.value();
+    uploadPtr += tailOffsetBytes.value();
 
     //////
 
-    gl->fPixelStorei(LOCAL_GL_UNPACK_ALIGNMENT, 1);
-    gl->fPixelStorei(LOCAL_GL_UNPACK_IMAGE_HEIGHT, 0);
-    gl->fPixelStorei(LOCAL_GL_UNPACK_SKIP_IMAGES, 0);
-    gl->fPixelStorei(LOCAL_GL_UNPACK_SKIP_ROWS, 0);
+    gl->fPixelStorei(LOCAL_GL_UNPACK_ALIGNMENT, 1);   // No stride padding.
+    gl->fPixelStorei(LOCAL_GL_UNPACK_ROW_LENGTH, 0);  // No padding in general.
+    gl->fPixelStorei(LOCAL_GL_UNPACK_SKIP_IMAGES, 0); // Don't skip images,
+    gl->fPixelStorei(LOCAL_GL_UNPACK_SKIP_ROWS, 0);   // or rows.
+                                                      // Keep skipping pixels though!
 
     *out_error = DoTexOrSubImage(true, gl, target, level, dui, xOffset,
                                  yOffset+mHeight-1, zOffset+mDepth-1, mWidth, 1, 1,
                                  uploadPtr);
 
+    // Reset all our modified state.
     gl->fPixelStorei(LOCAL_GL_UNPACK_ALIGNMENT, webgl->mPixelStore_UnpackAlignment);
     gl->fPixelStorei(LOCAL_GL_UNPACK_IMAGE_HEIGHT, webgl->mPixelStore_UnpackImageHeight);
+    gl->fPixelStorei(LOCAL_GL_UNPACK_ROW_LENGTH, webgl->mPixelStore_UnpackRowLength);
     gl->fPixelStorei(LOCAL_GL_UNPACK_SKIP_IMAGES, webgl->mPixelStore_UnpackSkipImages);
     gl->fPixelStorei(LOCAL_GL_UNPACK_SKIP_ROWS, webgl->mPixelStore_UnpackSkipRows);
 
     return true;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////////////
--- a/dom/canvas/WebGLTextureUpload.cpp
+++ b/dom/canvas/WebGLTextureUpload.cpp
@@ -124,44 +124,43 @@ DoesJSTypeMatchUnpackType(GLenum unpackT
         return false;
     }
 }
 
 bool
 WebGLContext::ValidateUnpackPixels(const char* funcName, uint32_t fullRows,
                                    uint32_t tailPixels, webgl::TexUnpackBlob* blob)
 {
-    const auto usedPixelsPerRow = CheckedUint32(blob->mSkipPixels) + blob->mWidth;
-    const auto usedRowsPerImage = CheckedUint32(blob->mSkipRows) + blob->mHeight;
-    const auto usedImages = CheckedUint32(blob->mSkipImages) + blob->mDepth;
+    auto skipPixels = CheckedUint32(blob->mSkipPixels);
+    skipPixels += CheckedUint32(blob->mSkipRows);
 
-    if (!usedPixelsPerRow.isValid() ||
-        !usedRowsPerImage.isValid() ||
-        !usedImages.isValid())
-    {
-        ErrorOutOfMemory("%s: Invalid calculation for e.g. UNPACK_SKIP_PIXELS + width.",
-                         funcName);
+    const auto usedPixelsPerRow = CheckedUint32(blob->mSkipPixels) + blob->mWidth;
+    if (!usedPixelsPerRow.isValid() || usedPixelsPerRow.value() > blob->mRowLength) {
+        ErrorInvalidOperation("%s: UNPACK_SKIP_PIXELS + height > UNPACK_ROW_LENGTH.",
+                              funcName);
+        return false;
+    }
+
+    if (blob->mHeight > blob->mImageHeight) {
+        ErrorInvalidOperation("%s: height > UNPACK_IMAGE_HEIGHT.", funcName);
         return false;
     }
 
     //////
 
-    if (usedPixelsPerRow.value() > blob->mRowLength ||
-        usedRowsPerImage.value() > blob->mImageHeight)
-    {
-        ErrorInvalidOperation("%s: UNPACK_ROW_LENGTH or UNPACK_IMAGE_HEIGHT too small.",
-                              funcName);
-        return false;
-    }
+    // The spec doesn't bound SKIP_ROWS + height <= IMAGE_HEIGHT, unfortunately.
+    auto skipFullRows = CheckedUint32(blob->mSkipImages) * blob->mImageHeight;
+    skipFullRows += blob->mSkipRows;
 
-    //////
+    MOZ_ASSERT(blob->mDepth >= 1);
+    MOZ_ASSERT(blob->mHeight >= 1);
+    auto usedFullRows = CheckedUint32(blob->mDepth - 1) * blob->mImageHeight;
+    usedFullRows += blob->mHeight - 1; // Full rows in the final image, excluding the tail.
 
-    auto fullRowsNeeded = (usedImages - 1) * blob->mImageHeight;
-    fullRowsNeeded += usedRowsPerImage - 1;
-
+    const auto fullRowsNeeded = skipFullRows + usedFullRows;
     if (!fullRowsNeeded.isValid()) {
         ErrorOutOfMemory("%s: Invalid calculation for required row count.",
                          funcName);
         return false;
     }
 
     if (fullRows > fullRowsNeeded.value())
         return true;