Bug 1280499 - Allow SKIP_ROWS+height>IMAGE_HEIGHT and fix paranoid uploading. - r=jrmuizel draft
authorJeff Gilbert <jgilbert@mozilla.com>
Thu, 21 Jul 2016 23:05:52 -0700
changeset 391148 bafed731ed734adb72de4c85f5d7b14bbf37e8b3
parent 391147 4256744f0ee521f2afd955467f7e46e8debb20e4
child 526142 b7aa827a2a10b51b01f807da128163ce07fd5979
push id23821
push userbmo:jgilbert@mozilla.com
push dateFri, 22 Jul 2016 06:06:29 +0000
reviewersjrmuizel
bugs1280499
milestone50.0a1
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;