1210514 - Fix color inversion when BasicCompositor is used on gonk

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Sotaro Ikeda [:sotaro]]

When BasicCompositor is enabled on gonk, display shows color inverted rendering.

Comment 1

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #0)
> When BasicCompositor is enabled on gonk, display shows color inverted
> rendering.

See Bug 1206583 Comment 15.

Comment 2

[Sotaro Ikeda [:sotaro]]

The problem happens by the following.
 - gecko renders content by BGR color.
 - DrawTargetCairo supports only BGR color.
   When RGB is requested, BGR render target is allocated.
   And even if we could allocate BGR render target, performance might not be good without GPU.

Comment 3

[Sotaro Ikeda [:sotaro]]

On gonk, hwc could expose RB swap capability. Flame-kk's hwc says that hwc supports RB swap, but it seems not work for DisplaySurface's RBSwap. I tried it, but it did not work.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Convert BGR to RGB in nsWindow::EndRemoteDrawing()

Confirmed on master flame-KK with the following preference.
  - layers.acceleration.disabled -> true
  - gfx.canvas.azure.accelerated -> false
  - layers.gralloc.disable       -> true

Comment 5

[Sotaro Ikeda [:sotaro]]

hwc seems not support rbswap of DisplaySurface's layer even when hwc suports rbswap. It seems that DisplaySurface's layer is always set as last hwc layer of hwc layers. From it, the layer seems to be automatically handled as HWC_FRAMEBUFFER_TARGET. It seems to affect to it.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #5)
> hwc seems not support rbswap of DisplaySurface's layer even when hwc suports
> rbswap. It seems that DisplaySurface's layer is always set as last hwc layer
> of hwc layers. From it, the layer seems to be automatically handled as
> HWC_FRAMEBUFFER_TARGET. It seems to affect to it.

normal hwc need one HWC_FRAMEBUFFER_TARGET for composition.

Comment 7

[Nicolas Silva [:nical]]

Looks like with your path we perform the manual rb swap on the framebuffer after each composition. Wouldn't it be much cheaper to do it on layers after they are painted so that we don't have to do a lot of processing during scrolling?

Comment 8

[Sotaro Ikeda [:sotaro]]

nical: can you explain more about it? I do not know well to moz 2d api. DrawTargetCairo seems to support only BGR.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #8)
> nical: can you explain more about it? I do not know well to moz 2d api.
> DrawTargetCairo seems to support only BGR.

Seems to support B8G8R8A8 and B8G8R8X8. But not support R8G8B8A8.

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8668667 [details] [diff] [review]
patch - Convert BGR to RGB in nsWindow::EndRemoteDrawing()

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

R+ since this is a temporary solution. Please add big red-flags comments and performance warnings that this has very slow code paths that should not be used in production.

Comment 11

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct intermittent during PTO] from comment #9)
> (In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #8)
> > nical: can you explain more about it? I do not know well to moz 2d api.
> > DrawTargetCairo seems to support only BGR.
> 
> Seems to support B8G8R8A8 and B8G8R8X8. But not support R8G8B8A8.

This is a problem if we do things that are not plain composite operations, but if all we do is blit semi-transparent pixels, we are better off rb-swapping content that we receive and composite it (even if means an extra copy and lying to moz2d about the format), as long as the alpha channel is in the same place. This way we only do extra work when things are painted rather than every time we present.
Better even, rb-swap on the content side after painting, to avoid the extra copy on the compositor side.

I know what I am suggesting can look worryingly hacky, but rb-swapping the entire screen every time we present is far too expensive to do on the cpu, especially on a platform that is supposedly very cheap because we don't have a gpu to do the composition.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Convert BGR to RGB in nsWindow::EndRemoteDrawing()

Apply the comment.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aebb47a694b

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2aebb47a694b