1286309 - Support patience diff in diff viewer

Status: RESOLVED INVALID

(MozReview Graveyard :: Review Board: User Interface, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Patience diff tends to do a better job of figuring out what actually changed compared to the more traditional diff.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: regular-diff

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8770210 [details] [diff] [review]
regular-diff

> bool
> WebGLContext::ValidatePackSize(const char* funcName, uint32_t width, uint32_t height,
>                                uint8_t bytesPerPixel, uint32_t* const out_rowStride,
>                                uint32_t* const out_endOffset)
> {
>     if (!width || !height) {
>         *out_rowStride = 0;
>@@ -1438,238 +1510,267 @@ WebGLContext::ValidatePackSize(const char* funcName, uint32_t width, uint32_t he
> }
> 
> void
> WebGLContext::ReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format,
>                          GLenum type,
>                          const dom::Nullable<dom::ArrayBufferView>& pixels,
>                          ErrorResult& out_error)
> {
>-    const char funcName[] = "readPixels";
>-    if (IsContextLost())
>+    if (!ReadPixels_SharedPrecheck(&out_error))
>         return;
> 
>-    if (mCanvasElement &&
>-        mCanvasElement->IsWriteOnly() &&
>-        !nsContentUtils::IsCallerChrome())
>-    {
>-        GenerateWarning("readPixels: Not allowed");
>-        out_error.Throw(NS_ERROR_DOM_SECURITY_ERR);
>+    if (mBoundPixelPackBuffer) {
>+        ErrorInvalidOperation("readPixels: PIXEL_PACK_BUFFER must be null.");
>         return;
>     }
> 
>-    if (width < 0 || height < 0)
>-        return ErrorInvalidValue("readPixels: negative size passed");
>+    //////
> 
>-    if (pixels.IsNull())
>-        return ErrorInvalidValue("readPixels: null destination buffer");
>+    if (pixels.IsNull()) {
>+        ErrorInvalidValue("readPixels: null destination buffer");
>+        return;
>+    }
> 
>-    if (!(IsWebGL2() && IsIntegerFormatAndTypeUnpackable(format, type)) &&
>-        !IsFormatAndTypeUnpackable(format, type, IsWebGL2())) {
>-        return ErrorInvalidEnum("readPixels: Bad format or type.");
>+    const auto& view = pixels.Value();
>+
>+    //////
>+
>+    js::Scalar::Type reqScalarType;
>+    if (!GetJSScalarFromGLType(type, &reqScalarType)) {
>+        ErrorInvalidEnum("readPixels: Bad `type`.");
>+        return;
>     }
> 
>-    int channels = 0;
>+    const js::Scalar::Type dataScalarType = JS_GetArrayBufferViewType(view.Obj());
>+    if (dataScalarType != reqScalarType) {
>+        ErrorInvalidOperation("readPixels: `pixels` type does not match `type`.");
>+        return;
>+    }
> 
>-    // Check the format param
>-    switch (format) {
>-    case LOCAL_GL_ALPHA:
>-    case LOCAL_GL_LUMINANCE:
>-    case LOCAL_GL_RED:
>-    case LOCAL_GL_RED_INTEGER:
>-        channels = 1;
>-        break;
>-    case LOCAL_GL_LUMINANCE_ALPHA:
>-    case LOCAL_GL_RG:
>-    case LOCAL_GL_RG_INTEGER:
>-        channels = 2;
>-        break;
>-    case LOCAL_GL_RGB:
>-    case LOCAL_GL_RGB_INTEGER:
>-        channels = 3;
>-        break;
>-    case LOCAL_GL_RGBA:
>-    case LOCAL_GL_RGBA_INTEGER:
>-        channels = 4;
>-        break;
>-    default:
>-        MOZ_CRASH("GFX: bad `format`");
>+    //////
>+
>+    // Compute length and data.  Don't reenter after this point, lest the
>+    // precomputed go out of sync with the instant length/data.
>+    view.ComputeLengthAndData();
>+    void* data = view.DataAllowShared();
>+    const auto dataLen = view.LengthAllowShared();
>+
>+    if (!data) {
>+        ErrorOutOfMemory("readPixels: buffer storage is null. Did we run out of memory?");
>+        out_error.Throw(NS_ERROR_OUT_OF_MEMORY);
>+        return;
>     }
> 
>+    ReadPixelsImpl(x, y, width, height, format, type, data, dataLen);
>+}
> 
>-    // Check the type param
>-    int bytesPerPixel;
>-    int requiredDataType;
>-    switch (type) {
>-    case LOCAL_GL_BYTE:
>-        bytesPerPixel = 1*channels;
>-        requiredDataType = js::Scalar::Int8;
>-        break;
>+void
>+WebGL2Context::ReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format,
>+                          GLenum type, WebGLsizeiptr offset, ErrorResult& out_error)
>+{
>+    if (!ReadPixels_SharedPrecheck(&out_error))
>+        return;
> 
>-    case LOCAL_GL_UNSIGNED_BYTE:
>-        bytesPerPixel = 1*channels;
>-        requiredDataType = js::Scalar::Uint8;
>-        break;
>+    if (!mBoundPixelPackBuffer) {
>+        ErrorInvalidOperation("readPixels: PIXEL_PACK_BUFFER must not be null.");
>+        return;
>+    }
> 
>-    case LOCAL_GL_SHORT:
>-        bytesPerPixel = 2*channels;
>-        requiredDataType = js::Scalar::Int16;
>-        break;
>+    //////
> 
>-    case LOCAL_GL_UNSIGNED_SHORT:
>-    case LOCAL_GL_UNSIGNED_SHORT_4_4_4_4:
>-    case LOCAL_GL_UNSIGNED_SHORT_5_5_5_1:
>-    case LOCAL_GL_UNSIGNED_SHORT_5_6_5:
>-        bytesPerPixel = 2;
>-        requiredDataType = js::Scalar::Uint16;
>-        break;
>+    if (offset < 0) {
>+        ErrorInvalidValue("readPixels: offset must not be negative.");
>+        return;
>+    }
> 
>-    case LOCAL_GL_UNSIGNED_INT_2_10_10_10_REV:
>-    case LOCAL_GL_UNSIGNED_INT_5_9_9_9_REV:
>-    case LOCAL_GL_UNSIGNED_INT_10F_11F_11F_REV:
>-    case LOCAL_GL_UNSIGNED_INT_24_8:
>-        bytesPerPixel = 4;
>-        requiredDataType = js::Scalar::Uint32;
>-        break;
>+    {
>+        const auto bytesPerType = webgl::BytesPerPixel({LOCAL_GL_RED, type});
> 
>-    case LOCAL_GL_UNSIGNED_INT:
>-        bytesPerPixel = 4*channels;
>-        requiredDataType = js::Scalar::Uint32;
>+        if (offset % bytesPerType != 0) {
>+            ErrorInvalidOperation("readPixels: `offset` must be divisible by the size"
>+                                  " a `type` in bytes.");
>+            return;
>+        }
>+    }
>+
>+    //////
>+
>+    const auto bytesAvailable = mBoundPixelPackBuffer->ByteLength();
>+    const auto checkedBytesAfterOffset = CheckedUint32(bytesAvailable) - offset;
>+
>+    uint32_t bytesAfterOffset = 0;
>+    if (checkedBytesAfterOffset.isValid()) {
>+        bytesAfterOffset = checkedBytesAfterOffset.value();
>+    }
>+
>+    ReadPixelsImpl(x, y, width, height, format, type, (void*)offset, bytesAfterOffset);
>+}
>+
>+static bool
>+ValidateReadPixelsFormatAndType(const webgl::FormatInfo* srcFormat,
>+                                const webgl::PackingInfo& pi, gl::GLContext* gl,
>+                                WebGLContext* webgl)
>+{
>+    // Check the format and type params to assure they are an acceptable pair (as per spec)
>+    GLenum mainFormat;
>+    GLenum mainType;
>+
>+    switch (srcFormat->componentType) {
>+    case webgl::ComponentType::Float:
>+        mainFormat = LOCAL_GL_RGBA;
>+        mainType = LOCAL_GL_FLOAT;
>         break;
> 
>-    case LOCAL_GL_INT:
>-        bytesPerPixel = 4*channels;
>-        requiredDataType = js::Scalar::Int32;
>+    case webgl::ComponentType::UInt:
>+        mainFormat = LOCAL_GL_RGBA_INTEGER;
>+        mainType = LOCAL_GL_UNSIGNED_INT;
>         break;
> 
>-    case LOCAL_GL_FLOAT:
>-        bytesPerPixel = 4*channels;
>-        requiredDataType = js::Scalar::Float32;
>+    case webgl::ComponentType::Int:
>+        mainFormat = LOCAL_GL_RGBA_INTEGER;
>+        mainType = LOCAL_GL_INT;
>         break;
> 
>-    case LOCAL_GL_HALF_FLOAT:
>-    case LOCAL_GL_HALF_FLOAT_OES:
>-        bytesPerPixel = 2*channels;
>-        requiredDataType = js::Scalar::Uint16;
>+    case webgl::ComponentType::NormInt:
>+    case webgl::ComponentType::NormUInt:
>+        mainFormat = LOCAL_GL_RGBA;
>+        mainType = LOCAL_GL_UNSIGNED_BYTE;
>         break;
> 
>     default:
>-        MOZ_CRASH("GFX: bad `type`");
>+        gfxCriticalNote << "Unhandled srcFormat->componentType: "
>+                        << (uint32_t)srcFormat->componentType;
>+        webgl->ErrorInvalidOperation("readPixels: Unhandled srcFormat->componentType."
>+                                     " Please file a bug!");
>+        return false;
>     }
> 
>+    if (pi.format == mainFormat && pi.type == mainType)
>+        return true;
>+
>     //////
>+    // OpenGL ES 3.0.4 p194 - When the internal format of the rendering surface is
>+    // RGB10_A2, a third combination of format RGBA and type UNSIGNED_INT_2_10_10_10_REV
>+    // is accepted.
> 
>-    const auto& view = pixels.Value();
>+    if (webgl->IsWebGL2() &&
>+        srcFormat->effectiveFormat == webgl::EffectiveFormat::RGB10_A2 &&
>+        pi.format == LOCAL_GL_RGBA &&
>+        pi.type == LOCAL_GL_UNSIGNED_INT_2_10_10_10_REV)
>+    {
>+        return true;
>+    }
> 
>-    // Compute length and data.  Don't reenter after this point, lest the
>-    // precomputed go out of sync with the instant length/data.
>-    view.ComputeLengthAndData();
>-    void* data = view.DataAllowShared();
>-    size_t bytesAvailable = view.LengthAllowShared();
>-    js::Scalar::Type dataType = JS_GetArrayBufferViewType(view.Obj());
>+    //////
>+    // OpenGL ES 2.0 $4.3.1 - IMPLEMENTATION_COLOR_READ_{TYPE/FORMAT} is a valid
>+    // combination for glReadPixels()...
> 
>-    // Check the pixels param type
>-    if (dataType != requiredDataType)
>-        return ErrorInvalidOperation("readPixels: Mismatched type/pixels types");
>+    // So yeah, we are actually checking that these are valid as /unpack/ formats, instead
>+    // of /pack/ formats here, but it should cover the INVALID_ENUM cases.
>+    if (!webgl->mFormatUsage->AreUnpackEnumsValid(pi.format, pi.type)) {
>+        webgl->ErrorInvalidEnum("readPixels: Bad format and/or type.");
>+        return false;
>+    }
> 
>-    if (!data) {
>-        ErrorOutOfMemory("readPixels: buffer storage is null. Did we run out of memory?");
>-        out_error.Throw(NS_ERROR_OUT_OF_MEMORY);
>-        return;
>+    // Only valid when pulled from:
>+    // * GLES 2.0.25 p105:
>+    //   "table 3.4, excluding formats LUMINANCE and LUMINANCE_ALPHA."
>+    // * GLES 3.0.4 p193:
>+    //   "table 3.2, excluding formats DEPTH_COMPONENT and DEPTH_STENCIL."
>+    switch (pi.format) {
>+    case LOCAL_GL_LUMINANCE:
>+    case LOCAL_GL_LUMINANCE_ALPHA:
>+    case LOCAL_GL_DEPTH_COMPONENT:
>+    case LOCAL_GL_DEPTH_STENCIL:
>+        webgl->ErrorInvalidEnum("readPixels: Invalid format: 0x%04x", pi.format);
>+        return false;
>     }
> 
>-    //////
>+    MOZ_ASSERT(gl->IsCurrent());
>+    if (gl->IsSupported(gl::GLFeature::ES2_compatibility)) {
>+        const auto auxFormat = gl->GetIntAs<GLenum>(LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT);
>+        const auto auxType = gl->GetIntAs<GLenum>(LOCAL_GL_IMPLEMENTATION_COLOR_READ_TYPE);
> 
>-    uint32_t rowStride;
>-    uint32_t bytesNeeded;
>-    if (!ValidatePackSize(funcName, width, height, bytesPerPixel, &rowStride,
>-                          &bytesNeeded))
>-    {
>-        return;
>+        if (auxFormat && auxType &&
>+            pi.format == auxFormat && pi.type == auxType)
>+        {
>+            return true;
>+        }
>     }
> 
>-    if (bytesNeeded > bytesAvailable) {
>-        ErrorInvalidOperation("readPixels: buffer too small");
>+    //////
>+
>+    webgl->ErrorInvalidOperation("readPixels: Invalid format or type.");
>+    return false;
>+}
>+

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: patience-diff

Comment 4

[Mike Conley (:mconley) (:⚙️)]

I'm pretty sure if Review Board sees a valid diff (and a Patience Diff is valid), it shouldn't, like, restructure the diff or regenerate it or anything. What I'm trying to say, is that I'm pretty certain that Review Board can display Patience Diffs.

I think maybe what we need to do is make it simpler for our tools to create those diffs. I believe git makes this pretty easy[1], but Mercurial doesn't. jlebar wrote a Patience Diff extension for Mercurial way back in the day[2] which might still work, but I don't think it's maintained.

[1]: Ex: git diff --patience
[2]: https://bitbucket.org/troter/hg-patience - see also https://github.com/jlebar/blog/blob/master/drafts/hg-patience.txt

Comment 5

[Steven MacLeod [:smacleod]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #4)
> I'm pretty sure if Review Board sees a valid diff (and a Patience Diff is
> valid), it shouldn't, like, restructure the diff or regenerate it or
> anything. What I'm trying to say, is that I'm pretty certain that Review
> Board can display Patience Diffs.

This is incorrect. The algorithm used by Review Board is the following:
 - Take the provided diff, parse it into sections by file.
 - Fetch the files listed in the diff from the repository (at the parent revision).
 - Apply the provided diff to these files, so Review Board has pre and post patch versions.
 - Run RBs built-in diffing algorithm[1] to generate its side-by-side diff.

So no matter what diff you feed into RB, if it results in the same original vs patched files you will get the same output in the Review Board diff viewer.

[1] The diff algorithm used by Review Board is a modified version of the Myers' diff (with some extra features I believe). For the code Review Board uses see https://github.com/reviewboard/reviewboard/blob/280069e31a306fae11aba1a616d52904c49c5299/reviewboard/diffviewer/myersdiff.py - also note that some of the extra features in RB's diff viewer may come from code after the diff algorithm is run.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Aw hell.

Comment 7

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.