Closed Bug 934886 Opened 8 years ago Closed 8 years ago

Fix incorrect color format of GetThebesSurfaceForDrawTarget when use SKIA as backend

Categories

(Core :: Graphics, defect)

26 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: pchang, Assigned: pchang)

References

Details

Attachments

(1 file, 1 obsolete file)

Inside gfxPlatform::GetThebesSurfaceForDrawTarget, it tries to create the snapshot form the drawtarget.

If the device's color depth is 16(leo/buri), it will always map CONTENT_COLOR type to RGB565.

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#920
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#1985

And it caused bug 915010 when we switch backend to skia which uses RGBA or RGBX by default.
Assignee: nobody → pchang
Attached patch patch (obsolete) — Splinter Review
Configure gfxASurface with correct color format.
Blocks: 915010
(In reply to peter chang[:pchang] from comment #0)
> Inside gfxPlatform::GetThebesSurfaceForDrawTarget, it tries to create the
> snapshot form the drawtarget.
> 
> If the device's color depth is 16(leo/buri), it will always map
> CONTENT_COLOR type to RGB565.
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#920
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#1985
> 
> And it caused bug 915010 when we switch backend to skia which uses RGBA or
> RGBX by default.

gfxPlatform::OptimalFormatForContent will return offscreenforamt with CONTENT_COLOR type.

And the offscreenformat is based on framebuffer color depth.
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxAndroidPlatform.cpp#109
Blocking koi+.
blocking-b2g: --- → koi+
So the root cause for the bug, afaik, is that SkiaGL doesn't support RGB565 read pixels calls on a GL-backed SkGpuDevice, so when we call Snapshot() on the DrawTargetSkia object and then doing the readback we are getting back an 8888 buffer from Skia.

I wonder then why data->GetFormat() is returning something that ContentForFormat thinks is CONTENT_COLOR instead of CONTENT_COLOR_ALPHA?

I think the bug lies elsewhere. I'm going to investigate this further.
(In reply to George Wright (:gw280) from comment #5)
> So the root cause for the bug, afaik, is that SkiaGL doesn't support RGB565
> read pixels calls on a GL-backed SkGpuDevice, so when we call Snapshot() on
> the DrawTargetSkia object and then doing the readback we are getting back an
> 8888 buffer from Skia.
Actually this issue also happened when skia was enabled(acceleration disable). 
As you said we configure RGBX8888 format for skia and then force read back as RGB565 format by using gfxImageSurface.

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#924

> 
> I wonder then why data->GetFormat() is returning something that
> ContentForFormat thinks is CONTENT_COLOR instead of CONTENT_COLOR_ALPHA?
> 
> I think the bug lies elsewhere. I'm going to investigate this further.

It is configured as RGBX because mOpaque is false.
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasRenderingContext2D.cpp#834

In my opinion, I think we should map correct color format when we uses non-cairo as backend in the following line.

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#920
(In reply to peter chang[:pchang] from comment #2)
> paste try server link.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=1874f33bb7dd

Got mochitest test 2 error on  Android 2.2 Armv6 Opt.

Re-run patch that limited the changes only for b2g to see any difference.

https://tbpl.mozilla.org/?tree=Try&rev=01dbadee50e8
That M2 test failure looks completely unrelated. I can r+ this if you want to land it, as I think this is the correct fix now.
This code is really nastily tangled up.  We have the information we need (format is 565 or rgbx or rgba or bgrx or bgra.  We then lose that information by calling ContentForFormat, collapsing things like 565+rgbx+bgrx into GFX_CONTENT_COLOR.  We then use the system/platform information as a part of OptimalFormatForContent, by using GetOffscreenFormat call.  It may very well work right now, but it is fragile, and we have pollution of external information.  It seems wrong to do this.
Right, Peter's patch gets rid of that loss of precision and converts the format directly to an image format.
Attachment #827247 - Flags: review?(gwright)
(In reply to George Wright (:gw280) from comment #8)
> That M2 test failure looks completely unrelated. I can r+ this if you want
> to land it, as I think this is the correct fix now.

If I limited changes on b2g only and it got green and the organ for M2.
I think they are unrelated too.

https://tbpl.mozilla.org/?tree=Try&rev=01dbadee50e8
Attachment #827247 - Flags: review?(gwright) → review+
Attached patch patchSplinter Review
add reviewer
Attachment #827247 - Attachment is obsolete: true
Attachment #828314 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/73061d362809
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Duplicate of this bug: 915010
This fixed a bunch of visual artifacts on my device. Thanks!
You need to log in before you can comment on or make changes to this bug.