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.

Configure gfxASurface with correct color format.

paste try server link. https://tbpl.mozilla.org/?tree=Try&rev=1874f33bb7dd

(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+.

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.

(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

add reviewer

This fixed a bunch of visual artifacts on my device. Thanks!