735932 - toDataURL uses incorrect alpha on Mac

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[conor]

toDataURL is using an alpha value of 0 on Mac if the WebGL context is created with the context creation attribute "alpha" set to false.  If it is set to true then it (correctly) uses the alpha values from the canvas, but when alpha is set to false then the toDataURL needs to use alpha values of 1 like it does when compositing.  On Windows toDataURL works correctly in both cases.

To reproduce, navigate to http://www.railgun3d.com/screenshot/demo.htm and click the Screenshot button.  On Mac the resulting image is black.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Interesting. I'll take a look.

Comment 2

[conor]

Any update on this? It is affecting our product which is in beta.  Many users are experiencing this problem.

Comment 3

[Kelsey Gilbert [:jgilbert]]

I think I know what this is, but I won't be able to test on my mac until later today.

We may or may not have a bug with toDataURL in a related case on windows, as well, from some testing I just did.

Comment 4

[Kelsey Gilbert [:jgilbert]]

What is the about:support for a machine this reproduces on? I cannot reproduce it on my macmini.

Comment 5

[Sam Thompson]

Reproduced on my 13" Macbook from 2008, with an Nvidia 9400M: 



  Application Basics

        Name
        Firefox

        Version
        11.0

        User Agent
        Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0

        Profile Directory

          Show in Finder

        Enabled Plugins

          about:plugins

        Build Configuration

          about:buildconfig

        Crash Reports

          about:crashes

        Memory Use

          about:memory

  Extensions

        Name

        Version

        Enabled

        ID

        Firebug
        1.9.1
        true
        [e-mail]

        Test Pilot
        1.2.1
        true
        [e-mail]

  Modified Preferences

      Name

      Value

        accessibility.typeaheadfind.flashBar
        0

        browser.places.smartBookmarksVersion
        3

        browser.startup.homepage_override.buildID
        20120312181643

        browser.startup.homepage_override.mstone
        rv:11.0

        extensions.lastAppVersion
        11.0

        network.cookie.prefsMigrated
        true

        places.database.lastMaintenance
        1333999869

        places.history.expiration.transient_current_max_pages
        100664

        places.history.expiration.transient_optimal_database_size
        161061272

        print.macosx.pagesetup-2
        PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0iVVRGLTgiPz4KPCFET0NUWVBFIHBsaXN0IFBVQkxJQyAiLS8vQXBwbGUvL0RURCBQTElTVCAxLjAvL0VO…

        print.print_bgcolor
        false

        print.print_bgimages
        false

        print.print_command

        print.print_downloadfonts
        false

        print.print_evenpages
        true

        print.print_in_color
        true

        print.print_margin_bottom
        0.5

        print.print_margin_left
        0.5

        print.print_margin_right
        0.5

        print.print_margin_top
        0.5

        print.print_oddpages
        true

        print.print_orientation
        0

        print.print_page_delay
        50

        print.print_paper_data
        0

        print.print_paper_height
        11.00

        print.print_paper_size_type
        1

        print.print_paper_size_unit
        0

        print.print_paper_width
        8.50

        print.print_printer

        print.print_reversed
        false

        print.print_scaling
        1.00

        print.print_shrink_to_fit
        true

        print.print_to_file
        false

        print.print_unwriteable_margin_bottom
        56

        print.print_unwriteable_margin_left
        25

        print.print_unwriteable_margin_right
        25

        print.print_unwriteable_margin_top
        25

        privacy.sanitize.migrateFx3Prefs
        true

        privacy.sanitize.timeSpan
        0

        security.disable_button.openCertManager
        false

        security.warn_viewing_mixed
        false

  Graphics

        Vendor ID
        10de

        Device ID
        0863

        WebGL Renderer
        NVIDIA Corporation -- NVIDIA GeForce 9400M OpenGL Engine -- 2.1 NVIDIA-1.6.36

        GPU Accelerated Windows
        1/1 OpenGL

        AzureBackend
        skia

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: Possible patch

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=5f31128c01ac

If this runs green, let's try to reproduce on it.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: Possible testcase

Here's a possible testcase.
I get [127,127,0,255] when I run it, which is the expected result.
Let me know what you get. If my hunch is right, you'll get [127,127,0,0] or similar. Also, the canvas should be yellow-ish.

Comment 8

[Sam Thompson]

Hmmm, I also get [127,127,0,255]. I then retried my original test case in another tab to make sure and I am still getting a black screen shot. The canvas is a yellowish color.

Comment 9

[Kelsey Gilbert [:jgilbert]]

It looks like we correct for this case already in WebGL. Looks like we'll need to wait on the try builds.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Hah, I'm bad at things; that try build is garbage. Spinning up a better one now.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Working try run:
https://tbpl.mozilla.org/?tree=Try&rev=fa4477e0a310

Comment 12

[Kelsey Gilbert [:jgilbert]]

Builds for that are available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fa4477e0a310/

You probably want Mac x64 Opt, which is here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fa4477e0a310/try-macosx64/firefox-14.0a1.en-US.mac.dmg

Comment 13

[Sam Thompson]

Awesome! That absolutely worked, on both our test case and our actual application.

Thank you for looking into this, I'm sure our firefox users will appreciate it.

Any idea in what kind of time frame we might see a fix like this in the live version? Alternatively, I don't suppose there's some trick we could do on our side to work around it in the meantime or anything like that, is there?

Thanks again!

Comment 14

[Sam Thompson]

Actually, I just tried it with PNG instead of JPG in toDataURL, and that still fails.

Comment 15

[Kelsey Gilbert [:jgilbert]]

(In reply to Sam Thompson from comment #14)
> Actually, I just tried it with PNG instead of JPG in toDataURL, and that
> still fails.

Did PNG already work before, or did this just only fix PNG but not JPG?

Comment 16

[Sam Thompson]

Neither PNG nor JPG worked before. After this fix JPG works but PNG does not.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Should have a fix for this too, but to do it right is a little more involved. If we end up backporting this, I'll do a minimal fix, but I'd like to get this right the right way. Patch is forthcoming.

It's possible that you may be able to work around this. I'm going to emulate the issue locally, and see what I can find.

I think that if all else fails, you can probably emulate alpha:false on an alpha:true canvas with something like:
gl.enable(gl.BLEND);
gl.blendFuncSeparate(srcFuncRGB, dstFuncRGB, gl.ZERO, gl.ONE);
gl.clearColor(0.0, 0.0, 0.0, 1.0);
gl.clear(gl.COLOR_BUFFER_BIT);

If you don't do any blending, use the defaults for srcFuncRGB and dstFuncRGB:
// Take source color, and keep dest alpha (always 1.0).
gl.blendFuncSeparate(gl.ONE, gl.ZERO, gl.ZERO, gl.ONE);

Comment 18

[Kelsey Gilbert [:jgilbert]]

How does the new build for https://tbpl.mozilla.org/?tree=Try&rev=07cde548f5ec look?

It should be at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-07cde548f5ec/

Comment 19

[Sam Thompson]

Looks good on both PNG and JPG! Thanks!

Comment 20

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 1: Work around drivers returning non-1.0 alpha values when lacking an alpha channel.

From both GL 3.0(+?) and GLES 2.0 Section 4.3.1:
If the framebuffer does not support alpha values then the A that is
obtained is 1.0.

Looks like part of this, at least, is a driver bug.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 2: Support (X)RGB surfaces for rendering alphaless canvases

I believe this is the correct path for this fix, though we'll need to be careful, since we're adding a new format.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 617106 [details] [diff] [review]
Patch 1: Work around drivers returning non-1.0 alpha values when lacking an alpha channel.

Review of attachment 617106 [details] [diff] [review]:
-----------------------------------------------------------------

I have just enough nits to warrant a r-.

::: gfx/gl/GLContext.cpp
@@ +2051,5 @@
> +        aDest->Format() == gfxASurface::ImageFormatARGB32 &&
> +        width && height)
> +    {
> +        GLint alphaBits = 0;
> +        fGetIntegerv(LOCAL_GL_ALPHA_BITS, &alphaBits);

Should we cache this?

@@ +2053,5 @@
> +    {
> +        GLint alphaBits = 0;
> +        fGetIntegerv(LOCAL_GL_ALPHA_BITS, &alphaBits);
> +        if (!alphaBits) {
> +            const PRUint32 alphaMask = 0xff000000;

Don't you need to check endianness here? OTOH, I can see existing code here that doesn't seem to. (The 0xff00ff00 below)

@@ +2063,5 @@
> +                PRUint32* itrEnd = itr + width*height;  // Stride is guaranteed to be width*4.
> +
> +                for (; itr != itrEnd; itr++) {
> +                    *itr |= alphaMask;
> +                }

That's expensive as we're iterating over all pixels. Can't we at least try to do this only on known-buggy drivers?

@@ +2078,2 @@
>                  *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
>                  row++;

The redundant index increment hurts my eyes. Please remove it? (even if it was already here).

Also, I wonder if all this masking business is really faster than manually swapping bytes? But that can be studied in a separate bug.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 617108 [details] [diff] [review]
Patch 2: Support (X)RGB surfaces for rendering alphaless canvases

Review of attachment 617108 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but only on the WebGLContext.cpp parts. Didn't look at the other files.

::: content/canvas/src/WebGLContext.cpp
@@ +645,5 @@
>  
> +    int inputFormat = imgIEncoder::INPUT_FORMAT_HOSTARGB;
> +    if (!mOptions.alpha) {
> +        inputFormat = imgIEncoder::INPUT_FORMAT_HOSTXRGB;
> +    } else if (!mOptions.premultipliedAlpha) {

I'm sure you can do this with fewer negations :-)

Let's see:

int inputFormat;
if (mOptions.alpha) {
  if (mOptions.premultipliedAlpha) {
    inputFormat = ...;
  } else {
    inputFormat = ...;
  }
} else {
  inputFormat = ...;
}

Comment 24

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #22)
> Comment on attachment 617106 [details] [diff] [review]
> Patch 1: Work around drivers returning non-1.0 alpha values when lacking an
> alpha channel.
> 
> Review of attachment 617106 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have just enough nits to warrant a r-.
> 
> ::: gfx/gl/GLContext.cpp
> @@ +2051,5 @@
> > +        aDest->Format() == gfxASurface::ImageFormatARGB32 &&
> > +        width && height)
> > +    {
> > +        GLint alphaBits = 0;
> > +        fGetIntegerv(LOCAL_GL_ALPHA_BITS, &alphaBits);
> 
> Should we cache this?
This is not that simple to cache, since it depends on the current bound read framebuffer.

> 
> @@ +2053,5 @@
> > +    {
> > +        GLint alphaBits = 0;
> > +        fGetIntegerv(LOCAL_GL_ALPHA_BITS, &alphaBits);
> > +        if (!alphaBits) {
> > +            const PRUint32 alphaMask = 0xff000000;
> 
> Don't you need to check endianness here? OTOH, I can see existing code here
> that doesn't seem to. (The 0xff00ff00 below)
Yes, we probably should, since we're in HOSTARGB.

> @@ +2063,5 @@
> > +                PRUint32* itrEnd = itr + width*height;  // Stride is guaranteed to be width*4.
> > +
> > +                for (; itr != itrEnd; itr++) {
> > +                    *itr |= alphaMask;
> > +                }
> 
> That's expensive as we're iterating over all pixels. Can't we at least try
> to do this only on known-buggy drivers?
Yes. I need some help getting up-to-speed on blocklisting though.

> 
> @@ +2078,2 @@
> >                  *row = (*row & 0xff00ff00) | ((*row & 0xff) << 16) | ((*row & 0xff0000) >> 16);
> >                  row++;
> 
> The redundant index increment hurts my eyes. Please remove it? (even if it
> was already here).
> 
> Also, I wonder if all this masking business is really faster than manually
> swapping bytes? But that can be studied in a separate bug.

Shift-and-mask appears to be faster than swapping bytes in my work on pixel format conversions, but it should be more deliberately tested.

Comment 25

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #23)
> Comment on attachment 617108 [details] [diff] [review]
> Patch 2: Support (X)RGB surfaces for rendering alphaless canvases
> 
> Review of attachment 617108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ but only on the WebGLContext.cpp parts. Didn't look at the other files.
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +645,5 @@
> >  
> > +    int inputFormat = imgIEncoder::INPUT_FORMAT_HOSTARGB;
> > +    if (!mOptions.alpha) {
> > +        inputFormat = imgIEncoder::INPUT_FORMAT_HOSTXRGB;
> > +    } else if (!mOptions.premultipliedAlpha) {
> 
> I'm sure you can do this with fewer negations :-)
> 
> Let's see:
> 
> int inputFormat;
> if (mOptions.alpha) {
>   if (mOptions.premultipliedAlpha) {
>     inputFormat = ...;
>   } else {
>     inputFormat = ...;
>   }
> } else {
>   inputFormat = ...;
> }

Sure, we can do that.

Comment 26

[Joe Drew (not getting mail)]

Comment on attachment 617108 [details] [diff] [review]
Patch 2: Support (X)RGB surfaces for rendering alphaless canvases

Review of attachment 617108 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/encoders/bmp/nsBMPEncoder.cpp
@@ +444,5 @@
>  //    an output with no alpha in machine-independent byte order.
>  //
>  void
>  nsBMPEncoder::ConvertHostARGBRow(const PRUint8* aSrc, PRUint8* aDest,
> +                                 PRUint32 aPixelWidth, bool aUseAlpha)

I'd prefer an enum to a boolean for this argument.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: testcase

This should show all four panels with bright green.
On an affected system, one or more panels should be either yellow or black.

Comment 28

[Kelsey Gilbert [:jgilbert]]

Conor: Can you test whether this testcase shows all-bright-green for your affected system?

Comment 29

[Sam Thompson]

Sorry for the delay. I tested this in 12.0 and Aurora (13.02) on a mac, and in both cases the upper left is bright green and other 3 are black. In chrome they are all bright green.

Did you want me to test with a specific version?

Comment 30

[Kelsey Gilbert [:jgilbert]]

I need to spin up another build with this fix in it, since the previous one expired.

Comment 31

[Kelsey Gilbert [:jgilbert]]

New try-build spinning up at: https://tbpl.mozilla.org/?tree=Try&rev=618ec4503ec9

Comment 32

[Sam Thompson]

That build works for me. I see all four panels as bright green, and screenshotting works for me as expected.

Comment 33

[Kelsey Gilbert [:jgilbert]]

We need an approach for this. It seems like we need this on nVidia hardware:
Try's Mac >=10.8 (Intel): Works
My Mac 10.7 (ATI): Works
Try's mac <10.8 (nVidia): Busted

It could also be that 10.8 fixes it, and Intel is broken on <10.8.
10.8 could also have fixed nVidia.

Working machine's about:support:
Vendor ID: 0x1002
Device ID: 0x6741
WebGL Renderer: ATI [...] ATI Radeon HD 6630M [...] 2.1 ATI-7.18.18

Try 10.8: (from log)
Chipset Model: Intel HD Graphics 3000
Vendor: Intel (0x8086)
Device ID: 0x0116

What's broken:
Try <10.8: (pulled from log)
Chipset Model: NVIDIA GeForce 320M
Vendor: NVIDIA (0x10de)
Device ID: 0x08a4

Sam Thompson's machine: (about:support dump from above)
Vendor ID: 0x10de
Device ID: 0x0863
WebGL Renderer: NVIDIA Corporation -- NVIDIA GeForce 9400M OpenGL Engine -- 2.1 NVIDIA-1.6.36

Comment 34

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Force 0xFF alpha on ReadPixels from no-alpha surfaces on Mac+nVidia

Comment 35

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7f57250ac7d1

Comment 36

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 656329 [details] [diff] [review]
patch 1: Force 0xFF alpha on ReadPixels from no-alpha surfaces on Mac+nVidia

This was the inbound link I just posted.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f57250ac7d1

Comment 38

[Kelsey Gilbert [:jgilbert]]

I think I used the wrong keep-open flag.

Comment 39

[Joe Drew (not getting mail)]

yup

Comment 40

[Kelsey Gilbert [:jgilbert]]

We need to get tests before we should land these fixes which we have no tests for.

Comment 41

[Kelsey Gilbert [:jgilbert]]

Evidently setting 'blocks: webgl-reftests' would be a circular dependency. Regardless, consider webgl-reftests as blocking this bug.

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jgilbert, maybe it's time to close this bug?