Bug 54023 - [chromium] Three texture-related webgl tests failing in DEBUG due to skia change
Summary: [chromium] Three texture-related webgl tests failing in DEBUG due to skia change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Mike Reed
URL:
Keywords:
: 54550 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-02-08 11:16 PST by Zhenyao Mo
Modified: 2011-02-17 09:08 PST (History)
8 users (show)

See Also:


Attachments
call non-asserting version of SkPackARGB32 (1.98 KB, patch)
2011-02-16 06:11 PST, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenyao Mo 2011-02-08 11:16:58 PST
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
Comment 1 Zhenyao Mo 2011-02-08 11:18:47 PST
This happened after http://crrev.com/74103 by hbono@chromium.org
Comment 2 Zhenyao Mo 2011-02-08 11:34:46 PST
Sorry blamed the wrong guy and wrong CL.  It is skia in r74130. Cc'ed reed@chromium.org
Comment 3 Kenneth Russell 2011-02-08 14:40:29 PST
(In reply to comment #2)
> Sorry blamed the wrong guy and wrong CL.  It is skia in r74130. Cc'ed reed@chromium.org

That sounds like the wrong revision.
Comment 4 Kenneth Russell 2011-02-08 14:41:35 PST
(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 reed@chromium.org
> 
> That sounds like the wrong revision.

To be clear, that's Chromium revision 74130.
http://src.chromium.org/viewvc/chrome?view=rev&revision=74130
Comment 5 Zhenyao Mo 2011-02-14 14:09:42 PST
[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
Comment 6 Mike Reed 2011-02-14 14:27:34 PST
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
Comment 7 Zhenyao Mo 2011-02-14 14:37:25 PST
Yes in WebGL sometimes we have m_premultiplyAlpha == false.  It is a spec requirement.

So how should we fix this?
Comment 8 Mike Reed 2011-02-15 07:54:02 PST
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.
Comment 9 Zhenyao Mo 2011-02-15 09:05:48 PST
(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!
Comment 10 Mike Reed 2011-02-16 06:11:31 PST
Created attachment 82622 [details]
call non-asserting version of SkPackARGB32
Comment 11 Mike Reed 2011-02-16 06:12:01 PST
(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).
Comment 12 Mike Reed 2011-02-16 06:12:33 PST
*** Bug 54550 has been marked as a duplicate of this bug. ***
Comment 13 Zhenyao Mo 2011-02-16 09:22:11 PST
Looks good.  Thanks for the quick fix.

When this lands, I will remove the three tests out of test_expectations.txt.
Comment 14 Kenneth Russell 2011-02-16 12:29:55 PST
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
Comment 15 Kenneth Russell 2011-02-16 12:35:03 PST
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.
Comment 16 WebKit Commit Bot 2011-02-16 17:07:53 PST
The commit-queue encountered the following flaky tests while processing attachment 82622 [details]:

animations/suspend-resume-animation-events.html bug 51002 (author: cmarrin@apple.com)
The commit-queue is continuing to process your patch.
Comment 17 WebKit Commit Bot 2011-02-16 17:09:11 PST
Comment on attachment 82622 [details]
call non-asserting version of SkPackARGB32

Clearing flags on attachment: 82622

Committed r78753: <http://trac.webkit.org/changeset/78753>
Comment 18 WebKit Commit Bot 2011-02-16 17:09:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Review Bot 2011-02-16 18:50:17 PST
http://trac.webkit.org/changeset/78753 might have broken GTK Linux 64-bit Debug
Comment 20 Zhenyao Mo 2011-02-17 09:08:02 PST
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.