Closed Bug 1304291 Opened 3 years ago Closed 3 years ago

Assertion failure: [GFX1]: RGBX corner pixel at (0,0) in 1280x868 surface is not opaque: 0,0,0,0

Categories

(Core :: Graphics, defect, P3)

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: cleu, Assigned: cleu)

Details

(Keywords: assertion, Whiteboard: [gfx-noted])

Attachments

(3 files, 5 obsolete files)

Attached file stacktrace.txt
Test Environment:
Macbook pro 15 inch (late 2013)
Mac OSX 10.11.6
Firefox Nightly debug build

Step to Reproduce:
Go to this website
https://artsandculture.withgoogle.com/en-us/national-parks-service/hawaii-volcanoes/jagger-museum/hawaii-volcanoes-helicopter-360

The assertion failed on both intel iris and discrete gfx(nvidia GT750M)

The stack trace is in the attatchment.

I've tested it on Macbook pro 15-inch late-2013, mid-2014,
and Macbook pro 13-inch early-2015.

It seems that only the Macbook pro 15 inch late-2013 model has this problem.

I found it when I am investigating bug1300549, it seems to be a different issue, so I file a new bug for it.
(In reply to Michael Leu[:lenzak800](UTC+8) from comment #1)
> Found some weird code when debugging, why using OR between 2 identical
> booleans?
> 
> https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLReadTexImageHelper.
> cpp?q=ReadPixelsIntoDataSurface&redirect_type=direct#290

They're not identical. One is RGBA while the other is BGRA.
(In reply to Lee Salzman [:lsalzman] from comment #3)
> (In reply to Michael Leu[:lenzak800](UTC+8) from comment #1)
> > Found some weird code when debugging, why using OR between 2 identical
> > booleans?
> > 
> > https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLReadTexImageHelper.
> > cpp?q=ReadPixelsIntoDataSurface&redirect_type=direct#290
> 
> They're not identical. One is RGBA while the other is BGRA.

Aw.. It's my fault, sorry about that. :(
I can't reproduce this one at all on Linux at least with SkiaGL. Mason or Markus, can you see if it reproduces for you with the test site on Mac?
Has STR: --- → yes
Flags: needinfo?(mstange)
Flags: needinfo?(mchang)
Keywords: assertion
Priority: -- → P3
Whiteboard: [gfx-noted]
Yes, this reproduces on Mac for me.

[GFX3-]: Surface width or height <= 0!
Crash Annotation GraphicsCriticalError: |[C0][GFX1]: RGBX corner pixel at (0,0) in 2160x1216 surface is not opaque: 0,0,0,0 (t=56.3963) [GFX1]: RGBX corner pixel at (0,0) in 2160x1216 surface is not opaque: 0,0,0,0
Assertion failure: [GFX1]: RGBX corner pixel at (0,0) in 2160x1216 surface is not opaque: 0,0,0,0, at /builds/slave/m-cen-m64-d-000000000000000000/build/src/gfx/2d/Logging.h:513
Flags: needinfo?(mstange)
Flags: needinfo?(mchang)
Hi Markus, 
What is the model of your Mac?
It seems that not all Mac has this problem.

I have tested several different Macbook Pros, only the late-2013 model has this issue.
Flags: needinfo?(mstange)
Assignee: nobody → cleu
The surface's alpha channel become 0 after a ReadBack in BasicCanvasLayer::UpdateSurface().
It seems that something going wrong here.

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCanvasLayer.cpp?q=UpdateSurface&redirect_type=direct#71
Ok, I found where change the alpha channel to 0, it is gl->fReadPixels inside ReadPixelsIntoDataSurface()

https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLReadTexImageHelper.cpp?q=ReadPixelsIntoDataSurface&redirect_type=direct#403

I am now wonder why only certain Mac models have this problem
(In reply to Michael Leu[:lenzak800](UTC+8) from comment #7)
> Hi Markus, 
> What is the model of your Mac?

Macbook Pro Retina Early 2013, using Intel HD Graphics 4000 1536 MB
Flags: needinfo?(mstange)
(In reply to Michael Leu[:lenzak800](UTC+8) from comment #8)
> The surface's alpha channel become 0 after a ReadBack in
> BasicCanvasLayer::UpdateSurface().
> It seems that something going wrong here.
> 
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/
> BasicCanvasLayer.cpp?q=UpdateSurface&redirect_type=direct#71

It looks like just below that there is some bug workaround code for filling in the alpha in BGRA formats. I wonder if we might need to use that for BGRX as well?

The question is just if this is because of a bug in certain drivers that the alpha channel does not get filled, or this is a general case we need to enable on all hardware with BGRX under Skia at this point in the code...

Requires more scrutiny from one of you guys who can reproduce. It would be appreciated if this can be investigated more deeply.
I've tried using that workaround code to fill alpha channel manually.

It did pass the original assertion point.

But it results in another assertion failure in compositor side, the error message is almost identical.

Assertion failure: [GFX1]: RGBX corner pixel at (0,0) in 2560x1588 surface is not opaque: 0,0,0,0

I think maybe the alpha channel has been incorrect before we call gl->fReadPixels.
And here is the stacktrace after using that workaround code.
A workaround for malformed RGBX/BGRX bitmap under some Mac models
by manually fill the X channels 0xFF
Attachment #8797922 - Attachment is obsolete: true
Attachment #8797929 - Flags: review?(mstange)
Attachment #8797929 - Attachment is obsolete: true
Attachment #8797929 - Flags: review?(mstange)
Attachment #8797931 - Flags: review?(mstange)
Attachment #8797931 - Flags: review?(mstange) → review?(lsalzman)
Comment on attachment 8797931 [details] [diff] [review]
Add workaround for malformed RGBX/BGRX channels

This is not the right kind of fix. Ultimately we want to to avoid expensive and unsafe modification of all source surface inputs to Skia. We should rather identify those places where the faulty data is being produced, and fix those places, instead of imposing a cost at the Skia bottleneck itself.

We should find out if ReadPixels is supposed to be filling the alpha, why it is not if it is not, and further on which configurations it is behaving like that so we can do a spot fix at the production site.

If this turns out to be a bug, the right place to put the fix I think would be where the existing fix of Nvidia hardware is underneath the ReadPixels.
Attachment #8797931 - Flags: review?(lsalzman) → review-
Comment on attachment 8805040 [details] [diff] [review]
Move driver workaround code into fReadPixels and remove alpha check

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

It seems that it is a Mac-only driver issue.
Some Mac models returns malformed alpha channel when calling glReadPixels.
This patch moves the workaround code from GLReadTexImageHelper to GLContext right after fReadPixels.
Since both RGBA/BGRA and RGBX/BGRX will be affected by this issue, I also remove the hasAlpha check.
Attachment #8805040 - Flags: review?(jgilbert)
Comment on attachment 8805040 [details] [diff] [review]
Move driver workaround code into fReadPixels and remove alpha check

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

::: gfx/gl/GLContext.cpp
@@ +2867,5 @@
> +#ifdef XP_MACOSX
> +    if (WorkAroundDriverBugs() &&
> +        Vendor() == gl::GLVendor::NVIDIA &&
> +        !IsCoreProfile() &&
> +        width && height)

&& format == LOCAL_GL_RGBA && type == LOCAL_GL_UNSIGNED_BYTE
Attachment #8805040 - Flags: review?(jgilbert) → review-
Add checks for type and format.
Attachment #8805040 - Attachment is obsolete: true
Attachment #8805442 - Flags: review?(jgilbert)
Comment on attachment 8805442 [details] [diff] [review]
Move driver workaround code into fReadPixels and remove alpha check

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

Sorry for the delay!
Attachment #8805442 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c04a775a0e69
Move driver workaround code into fReadPixels and remove alpha check. r=jgilbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c04a775a0e69
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8805442 [details] [diff] [review]
Move driver workaround code into fReadPixels and remove alpha check

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8805442 - Flags: approval-mozilla-aurora?
Comment on attachment 8805442 [details] [diff] [review]
Move driver workaround code into fReadPixels and remove alpha check

WebGL 2 is the feature we want to ship in 51. Aurora51+.
Attachment #8805442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
applying https://bug1304291.bmoattachments.org/attachment.cgi?id=8805442
patching file gfx/gl/GLReadTexImageHelper.cpp
Hunk #1 FAILED at 410
1 out of 1 hunks FAILED -- saving rejects to file gfx/gl/GLReadTexImageHelper.cpp.rej
abort: patch failed to apply
Attached patch patch for aurora (obsolete) — Splinter Review
Rebase to aurora.
Comment on attachment 8810317 [details] [diff] [review]
patch for aurora

Approval Request Comment
[Feature/regressing bug #]: webgl2
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]: Tested locally and try server.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8810317 - Flags: approval-mozilla-aurora?
Attachment #8810317 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8810317 [details] [diff] [review]
patch for aurora

The original patch was uplifted to aurora.
Attachment #8810317 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.