Closed Bug 735932 Opened 8 years ago Closed 9 months ago

toDataURL uses incorrect alpha on Mac

Categories

(Core :: Canvas: WebGL, defect)

11 Branch
x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: conor, Assigned: jgilbert)

References

()

Details

(Whiteboard: [rplus] webgl-driver)

Attachments

(3 files, 3 obsolete files)

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.
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
QA Contact: untriaged → canvas.2d
Interesting. I'll take a look.
Component: Canvas: 2D → Canvas: WebGL
QA Contact: canvas.2d → canvas.webgl
Any update on this? It is affecting our product which is in beta.  Many users are experiencing this problem.
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.
What is the about:support for a machine this reproduces on? I cannot reproduce it on my macmini.
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
        firebug@software.joehewitt.com

        Test Pilot
        1.2.1
        true
        testpilot@labs.mozilla.com

  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
Attached patch Possible patch (obsolete) — Splinter Review
Try run:
https://tbpl.mozilla.org/?tree=Try&rev=5f31128c01ac

If this runs green, let's try to reproduce on it.
Attached file Possible testcase (obsolete) —
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.
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.
It looks like we correct for this case already in WebGL. Looks like we'll need to wait on the try builds.
Attachment #616379 - Attachment is obsolete: true
Hah, I'm bad at things; that try build is garbage. Spinning up a better one now.
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!
Actually, I just tried it with PNG instead of JPG in toDataURL, and that still fails.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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?
Neither PNG nor JPG worked before. After this fix JPG works but PNG does not.
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);
Looks good on both PNG and JPG! Thanks!
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.
Assignee: nobody → jgilbert
Attachment #616375 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #617106 - Flags: review?(bjacob)
I believe this is the correct path for this fix, though we'll need to be careful, since we're adding a new format.
Attachment #617108 - Flags: review?(joe)
Attachment #617108 - Flags: review?(bjacob)
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.
Attachment #617106 - Flags: review?(bjacob) → review-
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 = ...;
}
Attachment #617108 - Flags: review?(bjacob) → review+
(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.
(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 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.
Attachment #617108 - Flags: review?(joe) → review+
Attached file testcase
This should show all four panels with bright green.
On an affected system, one or more panels should be either yellow or black.
Conor: Can you test whether this testcase shows all-bright-green for your affected system?
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?
I need to spin up another build with this fix in it, since the previous one expired.
That build works for me. I see all four panels as bright green, and screenshotting works for me as expected.
Whiteboard: [rplus]
Blocks: 784925
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
Attachment #656329 - Flags: review?(bjacob) → review+
Whiteboard: [rplus] → [rplus] [keep-open]
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.
Attachment #656329 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/7f57250ac7d1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
I think I used the wrong keep-open flag.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
yup
Whiteboard: [rplus] [keep-open] → [rplus] [leave-open]
No longer blocks: webgl-reftests
Status: REOPENED → ASSIGNED
Target Milestone: mozilla18 → ---
We need to get tests before we should land these fixes which we have no tests for.
Evidently setting 'blocks: webgl-reftests' would be a circular dependency. Regardless, consider webgl-reftests as blocking this bug.
No longer blocks: webgl-reftests
Whiteboard: [rplus] [leave-open] → [rplus] [leave-open] webgl-driver
Keywords: leave-open
Whiteboard: [rplus] [leave-open] webgl-driver → [rplus] webgl-driver

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

Flags: needinfo?(jgilbert)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago9 months ago
Flags: needinfo?(jgilbert)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.