Bug 54023 – [chromium] Three texture-related webgl tests failing in DEBUG due to skia change

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

[Zhenyao Mo]

Put them to test expectations for now (http://trac.webkit.org/changeset/77959)

Need to investigate.

fast/canvas/webgl/gl-teximage.html
fast/canvas/webgl/tex-image-with-format-and-type.html
fast/canvas/webgl/texture-transparent-pixels-initialized.html

[Zhenyao Mo]

This happened after http://crrev.com/74103 by [e-mail]

[Zhenyao Mo]

Sorry blamed the wrong guy and wrong CL.  It is skia in r74130. Cc'ed [e-mail]

[Kenneth Russell]

(In reply to comment #2)
> Sorry blamed the wrong guy and wrong CL.  It is skia in r74130. Cc'ed [e-mail]

That sounds like the wrong revision.

[Kenneth Russell]

(In reply to comment #3)
> (In reply to comment #2)
> > Sorry blamed the wrong guy and wrong CL.  It is skia in r74130. Cc'ed [e-mail]
> 
> That sounds like the wrong revision.

To be clear, that's Chromium revision 74130.
http://src.chromium.org/viewvc/chrome?view=rev&revision=74130

[Zhenyao Mo]

[31664:31664:2059838021045:FATAL:SkColorPriv.h(216)] third_party/skia/include/core/SkColorPriv.h:216: failed assertion "r <= a"

Backtrace:
	base::debug::StackTrace::StackTrace() [0x127580e]
	logging::LogMessage::~LogMessage() [0x128a4e2]
	SkDebugf_FileLine() [0x15724ff]
	SkPackARGB32() [0x269036d]
	WebCore::ImageFrame::setRGBA() [0x2692d5e]
	WebCore::ImageFrame::setRGBA() [0x2692bee]
	WebCore::PNGImageDecoder::rowAvailable() [0x269d71d]
	WebCore::rowAvailable() [0x269c9fa]
	0x7f90db042383
	0x7f90db04287c
	0x7f90db042af3
	0x7f90db043c6b
	WebCore::PNGImageReader::decode() [0x269dbab]
	WebCore::PNGImageDecoder::decode() [0x269d88c]
	WebCore::PNGImageDecoder::frameBufferAtIndex() [0x269ccfd]
	WebCore::ImageSource::frameIsCompleteAtIndex() [0x2683591]
	WebCore::GraphicsContext3D::getImageData() [0x26e6bcd]
	WebCore::GraphicsContext3D::extractImageData() [0x26449da]
	WebCore::WebGLRenderingContext::texImage2DImpl() [0x2395969]
	WebCore::WebGLRenderingContext::texImage2D() [0x2395f6f]
	WebCore::WebGLRenderingContextInternal::texImage2D3Callback() [0x24c2d1c]
	WebCore::WebGLRenderingContextInternal::texImage2DCallback() [0x24c38df]
	v8::internal::HandleApiCallHelper<>() [0x18b47c3]
	v8::internal::Builtin_Impl_HandleApiCall() [0x18b2280]
	v8::internal::Builtin_HandleApiCall() [0x18b2259]
	0x7f90af5ce24a

[Mike Reed]

In debug mode (SK_DEBUG defined for Skia), SkPackARGB32() will assert that the specified color is, in fact, legal premultiplied. i.e.

a >= r
a >= g
a >= b

I presume that this is the assert that is firing in the stack-trace.

The call site is (I think) this:


        inline void setRGBA(PixelData* dest, unsigned r, unsigned g, unsigned b, unsigned a)
        {
            if (m_premultiplyAlpha && !a)
                *dest = 0;
            else {
                if (m_premultiplyAlpha && a < 255) {
                    float alphaPercent = a / 255.0f;
                    r = static_cast<unsigned>(r * alphaPercent);
                    g = static_cast<unsigned>(g * alphaPercent);
                    b = static_cast<unsigned>(b * alphaPercent);
                }
#if PLATFORM(SKIA)
                *dest = SkPackARGB32(a, r, g, b);
#else
                *dest = (a << 24 | r << 16 | g << 8 | b);
#endif
            }
        }

The caller will apply the alpha before calling, but only if m_premultiplyAlpha is true. I don't know when this is not true (maybe it is always true), but if that is ever false, then we will skip the multiplies, which could make the assert fire.

Another cause could be that one of a,r,g,b is just out of range (doubtful), since SkPackARGB32() also asserts that each one is <= 0xFF

[Zhenyao Mo]

Yes in WebGL sometimes we have m_premultiplyAlpha == false.  It is a spec requirement.

So how should we fix this?

[Mike Reed]

I am testing a roll to Skia 796, which has a version of that macro that skips the premultiply check. When that lands, I will create a webkit patch to use it.

[Zhenyao Mo]

(In reply to comment #8)
> I am testing a roll to Skia 796, which has a version of that macro that skips the premultiply check. When that lands, I will create a webkit patch to use it.

Great!  Thank you!

[Mike Reed]

Created attachment 82622 [details]
call non-asserting version of SkPackARGB32

[Mike Reed]

(more notes on patch)

The standard way to create a 32bit pixel in skia is to call SkPackARGB32(a,r,g,b). This puts the components in the correct (compile-time) order, and also (in the Debug build) asserts that the components are legal for premultiplied. i.e. r <= a, g <= a, b <= a. This test makes sense for Skia in general, as it only supports premultiplied bitmaps today. However, WebGL is using the same decoder and bitmap to store images that will be passed directly to opengl, and opengl supports premultiplied and non-premultiplied.

This change calls a new version of the pack macro: SkPackARGB32NoCheck(a,r,g,b) which skips the asserts on the component values (since any values can be valid for opengl).

[Mike Reed]

*** Bug 54550 has been marked as a duplicate of this bug. ***

[Zhenyao Mo]

Looks good.  Thanks for the quick fix.

When this lands, I will remove the three tests out of test_expectations.txt.

[Kenneth Russell]

Comment on attachment 82622 [details]
call non-asserting version of SkPackARGB32

Looks fine. I patched my workspace locally to still call SkPackARGB32 if m_premultiplyAlpha is true, but it's another if-test in an inner loop that isn't really necessary. r=me

[Kenneth Russell]

BTW, is the chromium_rev in Source/WebKit/chromium/DEPS at or later than the revision where you incorporated the Skia roll? If not, that is going to need to be updated. I have an unfortunate feeling that the commit queue won't catch this error, so please file another bug ASAP updating DEPS if not.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 82622 [details]:

animations/suspend-resume-animation-events.html bug 51002 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 82622 [details]
call non-asserting version of SkPackARGB32

Clearing flags on attachment: 82622

Committed r78753: <http://trac.webkit.org/changeset/78753>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78753 might have broken GTK Linux 64-bit Debug

[Zhenyao Mo]

Updated the test expectations for these three tests.

Committed r78836: <http://trac.webkit.org/changeset/78836>

Note that fast/canvas/webgl/tex-image-with-format-and-type.html still fail from time to time in Linux Debug, but I think that's another issue.