Bug 44566 – Passing premultiplyAlpha=false to tex{Sub}Image2D loses information (cg)

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

[Zhenyao Mo]

skia port is fixed in https://bugs.webkit.org/show_bug.cgi?id=38282

[Zhenyao Mo]

Created attachment 65521 [details]
patch

[Zhenyao Mo]

Don't review this patch.  There is some issue I need to fix first.

[Zhenyao Mo]

Created attachment 65590 [details]
revised patch

[Kenneth Russell]

Comment on attachment 65590 [details]
revised patch

This basically looks good but there is one issue that needs addressing which is why I'm r-'ing it.

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 66078)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> [phone]  Zhenyao Mo  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Passing premultiplyAlpha=false to tex{Sub}Image2D loses information (cg)
> +        https://bugs.webkit.org/show_bug.cgi?id=44566
> +
> +        * platform/graphics/cg/GraphicsContext3DCG.cpp:
> +        (WebCore::GraphicsContext3D::getImageData): Fix the premultiplyAlpha issue for cg.
> +
>  2010-08-25  Mark Rowe  <[e-mail]>
>  
>          Reviewed by Dan Bernstein.
> Index: WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp
> ===================================================================
> --- WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp	(revision 66042)
> +++ WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp	(working copy)
> @@ -34,6 +34,7 @@
>  
>  #include <CoreGraphics/CGBitmapContext.h>
>  #include <CoreGraphics/CGContext.h>
> +#include <CoreGraphics/CGDataProvider.h>
>  #include <CoreGraphics/CGImage.h>
>  
>  #include <wtf/RetainPtr.h>
> @@ -49,61 +50,98 @@ bool GraphicsContext3D::getImageData(Ima
>      if (!image)
>          return false;
>      CGImageRef cgImage = image->nativeImageForCurrentFrame();

We don't want to call this all the time, only if image->data() is 0. Since you'd be assigning cgImage in both arms of the if, you wouldn't need to initialize it upon declaration.

> +    RetainPtr<CGImageRef> decodedImage;
> +    if (image->data()) {
> +        ImageSource decoder(false);
> +        decoder.setData(image->data(), true);
> +        if (!decoder.frameCount())
> +            return false;
> +        decodedImage = decoder.createFrameAtIndex(0);
> +        cgImage = decodedImage.get();
> +    }
>      if (!cgImage)
>          return false;
> -    int width = CGImageGetWidth(cgImage);
> -    int height = CGImageGetHeight(cgImage);
> -    // FIXME: we should get rid of this temporary copy where possible.
> -    int tempRowBytes = width * 4;
> -    Vector<uint8_t> tempVector;
> -    tempVector.resize(height * tempRowBytes);
> -    // Try to reuse the color space from the image to preserve its colors.
> -    // Some images use a color space (such as indexed) unsupported by the bitmap context.
> -    CGColorSpaceRef colorSpace = CGImageGetColorSpace(cgImage);
> -    bool releaseColorSpace = false;
> -    CGColorSpaceModel colorSpaceModel = CGColorSpaceGetModel(colorSpace);
> -    switch (colorSpaceModel) {
> -    case kCGColorSpaceModelMonochrome:
> -    case kCGColorSpaceModelRGB:
> -    case kCGColorSpaceModelCMYK:
> -    case kCGColorSpaceModelLab:
> -    case kCGColorSpaceModelDeviceN:
> +    size_t width = CGImageGetWidth(cgImage);
> +    size_t height = CGImageGetHeight(cgImage);
> +    if (!width || !height || CGImageGetBitsPerComponent(cgImage) != 8)
> +        return false;
> +    size_t componentsPerPixel = CGImageGetBitsPerPixel(cgImage) / 8;
> +    SourceDataFormat srcDataFormat = kSourceFormatRGBA8;
> +    AlphaOp neededAlphaOp = kAlphaDoNothing;
> +    switch (CGImageGetAlphaInfo(cgImage)) {
> +    case kCGImageAlphaPremultipliedFirst:
> +    case kCGImageAlphaFirst:
> +    case kCGImageAlphaNoneSkipFirst:
> +        return false;
> +    case kCGImageAlphaPremultipliedLast:
> +        // This is a special case for texImage2D with HTMLCanvasElement input,
> +        // in which case image->data() should be null.
> +        ASSERT(!image->data());
> +        if (!premultiplyAlpha)
> +            neededAlphaOp = kAlphaDoUnmultiply;
> +        switch (componentsPerPixel) {
> +        case 2:
> +            srcDataFormat = kSourceFormatRA8;
> +            break;
> +        case 4:
> +            srcDataFormat = kSourceFormatRGBA8;
> +            break;
> +        default:
> +            return false;
> +        }
>          break;
> -    default:
> -        colorSpace = CGColorSpaceCreateWithName(kCGColorSpaceGenericRGBLinear);
> -        releaseColorSpace = true;
> +    case kCGImageAlphaLast:
> +        if (premultiplyAlpha)
> +            neededAlphaOp = kAlphaDoPremultiply;
> +        switch (componentsPerPixel) {
> +        case 1:
> +            srcDataFormat = kSourceFormatA8;
> +            break;
> +        case 2:
> +            srcDataFormat = kSourceFormatRA8;
> +            break;
> +        case 4:
> +            srcDataFormat = kSourceFormatRGBA8;
> +            break;
> +        default:
> +            return false;
> +        }
>          break;
> +    case kCGImageAlphaNoneSkipLast:
> +        switch (componentsPerPixel) {
> +        case 2:
> +            srcDataFormat = kSourceFormatRA8;
> +            break;
> +        case 4:
> +            srcDataFormat = kSourceFormatRGBA8;
> +            break;
> +        default:
> +            return false;
> +        }
> +    case kCGImageAlphaNone:
> +        switch (componentsPerPixel) {
> +        case 1:
> +            srcDataFormat = kSourceFormatR8;
> +            break;
> +        case 3:
> +            srcDataFormat = kSourceFormatRGB8;
> +            break;
> +        default:
> +            return false;
> +        }
> +        break;
> +    default:
> +        return false;
>      }
> -    CGContextRef tempContext = CGBitmapContextCreate(tempVector.data(),
> -                                                     width, height, 8, tempRowBytes,
> -                                                     colorSpace,
> -                                                     // FIXME: change this!
> -                                                     kCGImageAlphaPremultipliedLast);
> -    if (releaseColorSpace)
> -        CGColorSpaceRelease(colorSpace);
> -    if (!tempContext)
> -        return false;
> -    CGContextSetBlendMode(tempContext, kCGBlendModeCopy);
> -    CGContextDrawImage(tempContext,
> -                       CGRectMake(0, 0, static_cast<CGFloat>(width), static_cast<CGFloat>(height)),
> -                       cgImage);
> -    CGContextRelease(tempContext);
> -    // Pack the pixel data into the output vector.
> -    unsigned long componentsPerPixel, bytesPerComponent;
> -    if (!computeFormatAndTypeParameters(format, type, &componentsPerPixel, &bytesPerComponent))
> -        return false;
> -    int rowBytes = width * componentsPerPixel * bytesPerComponent;
> -    outputVector.resize(height * rowBytes);
> -    CGImageAlphaInfo info = CGImageGetAlphaInfo(cgImage);
> -    bool hasAlphaChannel = (info != kCGImageAlphaNone
> -                            && info != kCGImageAlphaNoneSkipLast
> -                            && info != kCGImageAlphaNoneSkipFirst);
> -    AlphaOp neededAlphaOp = kAlphaDoNothing;
> -    if (!premultiplyAlpha && hasAlphaChannel)
> -        // FIXME: must fetch the image data before the premultiplication step.
> -        neededAlphaOp = kAlphaDoUnmultiply;
> -    return packPixels(tempVector.data(), kSourceFormatRGBA8, width, height, 0,
> -                      format, type, neededAlphaOp, outputVector.data());
> +    CFDataRef pixelData = CGDataProviderCopyData(CGImageGetDataProvider(cgImage));

Does this work even if we didn't take the code path for "if (image->data())"? Also, you should use RetainPtr<CFDataRef> here, and initialize it with adoptCF(...).

> +    if (!pixelData)
> +        return false;
> +    const UInt8* rgba = CFDataGetBytePtr(pixelData);
> +    outputVector.resize(width * height * 4);
> +    bool rt = packPixels(rgba, srcDataFormat, width, height, 0,
> +                         format, type, neededAlphaOp, outputVector.data());
> +    CFRelease(pixelData);
> +    return rt;
>  }
>  
>  void GraphicsContext3D::paintToCanvas(const unsigned char* imagePixels, int imageWidth, int imageHeight, int canvasWidth, int canvasHeight, CGContextRef context)
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 66078)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> [phone]  Zhenyao Mo  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Passing premultiplyAlpha=false to tex{Sub}Image2D loses information (cg)
> +        https://bugs.webkit.org/show_bug.cgi?id=44566
> +
> +        * fast/canvas/webgl/gl-teximage-expected.txt: Fix a typo in the file.
> +        * platform/chromium/test_expectations.txt: Re-enable gl-teximage.html test.
> +        * platform/mac/Skipped: Ditto.
> +
>  2010-08-25  Michael Saboff  <[e-mail]>
>  
>          Reviewed by Oliver Hunt.
> Index: LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt
> ===================================================================
> --- LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt	(revision 66042)
> +++ LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt	(working copy)
> @@ -7,7 +7,7 @@ PASS getError was expected value: NO_ERR
>  check pixels are NOT pre-multiplied
>  PASS getError was expected value: NO_ERROR : Should be no errors from setup
>  PASS pixel 0, 15 should be 0, 0, 0, 255 was 0, 0, 0, 255
> -PASS pixel 128, 15 should be 255, 0, 255, 255 was 255, 0, 0, 255
> +PASS pixel 128, 15 should be 255, 0, 255, 255 was 255, 0, 255, 255
>  PASS pixel 255, 15 should be 0, 0, 255, 255 was 0, 0, 255, 255
>  PASS pixel 0, 8 should be 128, 128, 128, 255 was 128, 128, 128, 255
>  PASS pixel 128, 8 should be 255, 255, 255, 255 was 255, 255, 255, 255
> Index: LayoutTests/platform/chromium/test_expectations.txt
> ===================================================================
> --- LayoutTests/platform/chromium/test_expectations.txt	(revision 66042)
> +++ LayoutTests/platform/chromium/test_expectations.txt	(working copy)
> @@ -2565,9 +2565,6 @@ BUGWK37297 : fast/history/sibling-visite
>  BUGWK36983 MAC : fast/canvas/webgl/null-object-behaviour.html = TEXT
>  BUGWK36983 MAC : fast/canvas/webgl/uniform-location.html = TEXT
>  
> -// Caused by premultiplying alphas in CG image decoding.
> -BUG44566 MAC : fast/canvas/webgl/gl-teximage.html = TEXT
> -
>  // Added in http://trac.webkit.org/changeset/57476. Fails in Chromium because
>  // LayoutTestController::computedStyleWithVisitedInfo() is missing.
>  BUG41206 : fast/history/multiple-classes-visited.html = TEXT TIMEOUT
> Index: LayoutTests/platform/mac/Skipped
> ===================================================================
> --- LayoutTests/platform/mac/Skipped	(revision 66042)
> +++ LayoutTests/platform/mac/Skipped	(working copy)
> @@ -292,6 +292,3 @@ animations/play-state.html
>  
>  # https://bugs.webkit.org/show_bug.cgi?id=43332
>  inspector/dom-breakpoints.html
> -
> -# https://bugs.webkit.org/show_bug.cgi?id=44566
> -fast/canvas/webgl/gl-teximage.html

[Zhenyao Mo]

(In reply to comment #4)
> (From update of attachment 65590 [details])
> This basically looks good but there is one issue that needs addressing which is why I'm r-'ing it.
> 
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog	(revision 66078)
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,13 @@
> > [phone]  Zhenyao Mo  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Passing premultiplyAlpha=false to tex{Sub}Image2D loses information (cg)
> > +        https://bugs.webkit.org/show_bug.cgi?id=44566
> > +
> > +        * platform/graphics/cg/GraphicsContext3DCG.cpp:
> > +        (WebCore::GraphicsContext3D::getImageData): Fix the premultiplyAlpha issue for cg.
> > +
> >  2010-08-25  Mark Rowe  <[e-mail]>
> >  
> >          Reviewed by Dan Bernstein.
> > Index: WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp
> > ===================================================================
> > --- WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp	(revision 66042)
> > +++ WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp	(working copy)
> > @@ -34,6 +34,7 @@
> >  
> >  #include <CoreGraphics/CGBitmapContext.h>
> >  #include <CoreGraphics/CGContext.h>
> > +#include <CoreGraphics/CGDataProvider.h>
> >  #include <CoreGraphics/CGImage.h>
> >  
> >  #include <wtf/RetainPtr.h>
> > @@ -49,61 +50,98 @@ bool GraphicsContext3D::getImageData(Ima
> >      if (!image)
> >          return false;
> >      CGImageRef cgImage = image->nativeImageForCurrentFrame();
> 
> We don't want to call this all the time, only if image->data() is 0. Since you'd be assigning cgImage in both arms of the if, you wouldn't need to initialize it upon declaration.
> 
> > +    RetainPtr<CGImageRef> decodedImage;
> > +    if (image->data()) {
> > +        ImageSource decoder(false);
> > +        decoder.setData(image->data(), true);
> > +        if (!decoder.frameCount())
> > +            return false;
> > +        decodedImage = decoder.createFrameAtIndex(0);
> > +        cgImage = decodedImage.get();
> > +    }
> >      if (!cgImage)
> >          return false;
> > -    int width = CGImageGetWidth(cgImage);
> > -    int height = CGImageGetHeight(cgImage);
> > -    // FIXME: we should get rid of this temporary copy where possible.
> > -    int tempRowBytes = width * 4;
> > -    Vector<uint8_t> tempVector;
> > -    tempVector.resize(height * tempRowBytes);
> > -    // Try to reuse the color space from the image to preserve its colors.
> > -    // Some images use a color space (such as indexed) unsupported by the bitmap context.
> > -    CGColorSpaceRef colorSpace = CGImageGetColorSpace(cgImage);
> > -    bool releaseColorSpace = false;
> > -    CGColorSpaceModel colorSpaceModel = CGColorSpaceGetModel(colorSpace);
> > -    switch (colorSpaceModel) {
> > -    case kCGColorSpaceModelMonochrome:
> > -    case kCGColorSpaceModelRGB:
> > -    case kCGColorSpaceModelCMYK:
> > -    case kCGColorSpaceModelLab:
> > -    case kCGColorSpaceModelDeviceN:
> > +    size_t width = CGImageGetWidth(cgImage);
> > +    size_t height = CGImageGetHeight(cgImage);
> > +    if (!width || !height || CGImageGetBitsPerComponent(cgImage) != 8)
> > +        return false;
> > +    size_t componentsPerPixel = CGImageGetBitsPerPixel(cgImage) / 8;
> > +    SourceDataFormat srcDataFormat = kSourceFormatRGBA8;
> > +    AlphaOp neededAlphaOp = kAlphaDoNothing;
> > +    switch (CGImageGetAlphaInfo(cgImage)) {
> > +    case kCGImageAlphaPremultipliedFirst:
> > +    case kCGImageAlphaFirst:
> > +    case kCGImageAlphaNoneSkipFirst:
> > +        return false;
> > +    case kCGImageAlphaPremultipliedLast:
> > +        // This is a special case for texImage2D with HTMLCanvasElement input,
> > +        // in which case image->data() should be null.
> > +        ASSERT(!image->data());
> > +        if (!premultiplyAlpha)
> > +            neededAlphaOp = kAlphaDoUnmultiply;
> > +        switch (componentsPerPixel) {
> > +        case 2:
> > +            srcDataFormat = kSourceFormatRA8;
> > +            break;
> > +        case 4:
> > +            srcDataFormat = kSourceFormatRGBA8;
> > +            break;
> > +        default:
> > +            return false;
> > +        }
> >          break;
> > -    default:
> > -        colorSpace = CGColorSpaceCreateWithName(kCGColorSpaceGenericRGBLinear);
> > -        releaseColorSpace = true;
> > +    case kCGImageAlphaLast:
> > +        if (premultiplyAlpha)
> > +            neededAlphaOp = kAlphaDoPremultiply;
> > +        switch (componentsPerPixel) {
> > +        case 1:
> > +            srcDataFormat = kSourceFormatA8;
> > +            break;
> > +        case 2:
> > +            srcDataFormat = kSourceFormatRA8;
> > +            break;
> > +        case 4:
> > +            srcDataFormat = kSourceFormatRGBA8;
> > +            break;
> > +        default:
> > +            return false;
> > +        }
> >          break;
> > +    case kCGImageAlphaNoneSkipLast:
> > +        switch (componentsPerPixel) {
> > +        case 2:
> > +            srcDataFormat = kSourceFormatRA8;
> > +            break;
> > +        case 4:
> > +            srcDataFormat = kSourceFormatRGBA8;
> > +            break;
> > +        default:
> > +            return false;
> > +        }
> > +    case kCGImageAlphaNone:
> > +        switch (componentsPerPixel) {
> > +        case 1:
> > +            srcDataFormat = kSourceFormatR8;
> > +            break;
> > +        case 3:
> > +            srcDataFormat = kSourceFormatRGB8;
> > +            break;
> > +        default:
> > +            return false;
> > +        }
> > +        break;
> > +    default:
> > +        return false;
> >      }
> > -    CGContextRef tempContext = CGBitmapContextCreate(tempVector.data(),
> > -                                                     width, height, 8, tempRowBytes,
> > -                                                     colorSpace,
> > -                                                     // FIXME: change this!
> > -                                                     kCGImageAlphaPremultipliedLast);
> > -    if (releaseColorSpace)
> > -        CGColorSpaceRelease(colorSpace);
> > -    if (!tempContext)
> > -        return false;
> > -    CGContextSetBlendMode(tempContext, kCGBlendModeCopy);
> > -    CGContextDrawImage(tempContext,
> > -                       CGRectMake(0, 0, static_cast<CGFloat>(width), static_cast<CGFloat>(height)),
> > -                       cgImage);
> > -    CGContextRelease(tempContext);
> > -    // Pack the pixel data into the output vector.
> > -    unsigned long componentsPerPixel, bytesPerComponent;
> > -    if (!computeFormatAndTypeParameters(format, type, &componentsPerPixel, &bytesPerComponent))
> > -        return false;
> > -    int rowBytes = width * componentsPerPixel * bytesPerComponent;
> > -    outputVector.resize(height * rowBytes);
> > -    CGImageAlphaInfo info = CGImageGetAlphaInfo(cgImage);
> > -    bool hasAlphaChannel = (info != kCGImageAlphaNone
> > -                            && info != kCGImageAlphaNoneSkipLast
> > -                            && info != kCGImageAlphaNoneSkipFirst);
> > -    AlphaOp neededAlphaOp = kAlphaDoNothing;
> > -    if (!premultiplyAlpha && hasAlphaChannel)
> > -        // FIXME: must fetch the image data before the premultiplication step.
> > -        neededAlphaOp = kAlphaDoUnmultiply;
> > -    return packPixels(tempVector.data(), kSourceFormatRGBA8, width, height, 0,
> > -                      format, type, neededAlphaOp, outputVector.data());
> > +    CFDataRef pixelData = CGDataProviderCopyData(CGImageGetDataProvider(cgImage));
> 
> Does this work even if we didn't take the code path for "if (image->data())"? Also, you should use RetainPtr<CFDataRef> here, and initialize it with adoptCF(...).

Yes, it works for both cases.  I'll fix the mentioned issues.

> 
> > +    if (!pixelData)
> > +        return false;
> > +    const UInt8* rgba = CFDataGetBytePtr(pixelData);
> > +    outputVector.resize(width * height * 4);
> > +    bool rt = packPixels(rgba, srcDataFormat, width, height, 0,
> > +                         format, type, neededAlphaOp, outputVector.data());
> > +    CFRelease(pixelData);
> > +    return rt;
> >  }
> >  
> >  void GraphicsContext3D::paintToCanvas(const unsigned char* imagePixels, int imageWidth, int imageHeight, int canvasWidth, int canvasHeight, CGContextRef context)
> > Index: LayoutTests/ChangeLog
> > ===================================================================
> > --- LayoutTests/ChangeLog	(revision 66078)
> > +++ LayoutTests/ChangeLog	(working copy)
> > @@ -1,3 +1,14 @@
> > [phone]  Zhenyao Mo  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Passing premultiplyAlpha=false to tex{Sub}Image2D loses information (cg)
> > +        https://bugs.webkit.org/show_bug.cgi?id=44566
> > +
> > +        * fast/canvas/webgl/gl-teximage-expected.txt: Fix a typo in the file.
> > +        * platform/chromium/test_expectations.txt: Re-enable gl-teximage.html test.
> > +        * platform/mac/Skipped: Ditto.
> > +
> >  2010-08-25  Michael Saboff  <[e-mail]>
> >  
> >          Reviewed by Oliver Hunt.
> > Index: LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt
> > ===================================================================
> > --- LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt	(revision 66042)
> > +++ LayoutTests/fast/canvas/webgl/gl-teximage-expected.txt	(working copy)
> > @@ -7,7 +7,7 @@ PASS getError was expected value: NO_ERR
> >  check pixels are NOT pre-multiplied
> >  PASS getError was expected value: NO_ERROR : Should be no errors from setup
> >  PASS pixel 0, 15 should be 0, 0, 0, 255 was 0, 0, 0, 255
> > -PASS pixel 128, 15 should be 255, 0, 255, 255 was 255, 0, 0, 255
> > +PASS pixel 128, 15 should be 255, 0, 255, 255 was 255, 0, 255, 255
> >  PASS pixel 255, 15 should be 0, 0, 255, 255 was 0, 0, 255, 255
> >  PASS pixel 0, 8 should be 128, 128, 128, 255 was 128, 128, 128, 255
> >  PASS pixel 128, 8 should be 255, 255, 255, 255 was 255, 255, 255, 255
> > Index: LayoutTests/platform/chromium/test_expectations.txt
> > ===================================================================
> > --- LayoutTests/platform/chromium/test_expectations.txt	(revision 66042)
> > +++ LayoutTests/platform/chromium/test_expectations.txt	(working copy)
> > @@ -2565,9 +2565,6 @@ BUGWK37297 : fast/history/sibling-visite
> >  BUGWK36983 MAC : fast/canvas/webgl/null-object-behaviour.html = TEXT
> >  BUGWK36983 MAC : fast/canvas/webgl/uniform-location.html = TEXT
> >  
> > -// Caused by premultiplying alphas in CG image decoding.
> > -BUG44566 MAC : fast/canvas/webgl/gl-teximage.html = TEXT
> > -
> >  // Added in http://trac.webkit.org/changeset/57476. Fails in Chromium because
> >  // LayoutTestController::computedStyleWithVisitedInfo() is missing.
> >  BUG41206 : fast/history/multiple-classes-visited.html = TEXT TIMEOUT
> > Index: LayoutTests/platform/mac/Skipped
> > ===================================================================
> > --- LayoutTests/platform/mac/Skipped	(revision 66042)
> > +++ LayoutTests/platform/mac/Skipped	(working copy)
> > @@ -292,6 +292,3 @@ animations/play-state.html
> >  
> >  # https://bugs.webkit.org/show_bug.cgi?id=43332
> >  inspector/dom-breakpoints.html
> > -
> > -# https://bugs.webkit.org/show_bug.cgi?id=44566
> > -fast/canvas/webgl/gl-teximage.html

[Zhenyao Mo]

Created attachment 66002 [details]
revised patch: responding to kbr's review

[Kenneth Russell]

Comment on attachment 66002 [details]
revised patch: responding to kbr's review

Looks good; r=me. It would be nice to common up the nested switch statements at some point.

[Zhenyao Mo]

Committed r66494: <http://trac.webkit.org/changeset/66494>

[WebKit Review Bot]

http://trac.webkit.org/changeset/66494 might have broken Leopard Intel Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/66493
http://trac.webkit.org/changeset/66494
http://trac.webkit.org/changeset/66495