toDataURL uses incorrect alpha on Mac

ASSIGNED
Assigned to

Status

()

ASSIGNED
7 years ago
3 months ago

People

(Reporter: conor, Assigned: jgilbert)

Tracking

({leave-open})

11 Branch
x86_64
Mac OS X
leave-open
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rplus] webgl-driver, URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Assignee)

Comment 1

7 years ago
Interesting. I'll take a look.
Component: Canvas: 2D → Canvas: WebGL
QA Contact: canvas.2d → canvas.webgl
(Reporter)

Comment 2

7 years ago
Any update on this? It is affecting our product which is in beta.  Many users are experiencing this problem.
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

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

Comment 5

7 years ago
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
(Assignee)

Comment 6

7 years ago
Created attachment 616375 [details] [diff] [review]
Possible patch

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

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

Comment 7

7 years ago
Created attachment 616379 [details]
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

7 years ago
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.
(Assignee)

Comment 9

7 years ago
It looks like we correct for this case already in WebGL. Looks like we'll need to wait on the try builds.
(Assignee)

Updated

7 years ago
Attachment #616379 - Attachment is obsolete: true
(Assignee)

Comment 10

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

Comment 13

7 years ago
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

7 years ago
Actually, I just tried it with PNG instead of JPG in toDataURL, and that still fails.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 15

7 years ago
(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

7 years ago
Neither PNG nor JPG worked before. After this fix JPG works but PNG does not.
(Assignee)

Comment 17

7 years ago
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 19

7 years ago
Looks good on both PNG and JPG! Thanks!
(Assignee)

Comment 20

7 years ago
Created attachment 617106 [details] [diff] [review]
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.
Assignee: nobody → jgilbert
Attachment #616375 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #617106 - Flags: review?(bjacob)
(Assignee)

Comment 21

7 years ago
Created attachment 617108 [details] [diff] [review]
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.
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+
(Assignee)

Comment 24

7 years ago
(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.
(Assignee)

Comment 25

7 years ago
(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+
(Assignee)

Comment 27

7 years ago
Created attachment 628521 [details]
testcase

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

Comment 28

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

Comment 29

7 years ago
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?
(Assignee)

Comment 30

7 years ago
I need to spin up another build with this fix in it, since the previous one expired.
(Assignee)

Comment 31

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

Comment 32

7 years ago
That build works for me. I see all four panels as bright green, and screenshotting works for me as expected.
(Assignee)

Updated

7 years ago
Whiteboard: [rplus]
(Assignee)

Updated

6 years ago
Blocks: 784925
(Assignee)

Comment 33

6 years ago
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
(Assignee)

Comment 34

6 years ago
Created attachment 656329 [details] [diff] [review]
patch 1: Force 0xFF alpha on ReadPixels from no-alpha surfaces on Mac+nVidia
Attachment #617106 - Attachment is obsolete: true
Attachment #656329 - Flags: review?(bjacob)
(Assignee)

Updated

6 years ago
Blocks: 782860
Attachment #656329 - Flags: review?(bjacob) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [rplus] → [rplus] [keep-open]
(Assignee)

Comment 36

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Comment 38

6 years ago
I think I used the wrong keep-open flag.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
yup
Whiteboard: [rplus] [keep-open] → [rplus] [leave-open]
(Assignee)

Updated

6 years ago
No longer blocks: 782860
Status: REOPENED → ASSIGNED
Target Milestone: mozilla18 → ---
(Assignee)

Comment 40

6 years ago
We need to get tests before we should land these fixes which we have no tests for.
Blocks: 782860
(Assignee)

Comment 41

6 years ago
Evidently setting 'blocks: webgl-reftests' would be a circular dependency. Regardless, consider webgl-reftests as blocking this bug.
No longer blocks: 782860
(Assignee)

Updated

5 years ago
Whiteboard: [rplus] [leave-open] → [rplus] [leave-open] webgl-driver
Keywords: leave-open
Whiteboard: [rplus] [leave-open] webgl-driver → [rplus] webgl-driver
You need to log in before you can comment on or make changes to this bug.