1210182 - Implement GrallocTextureHostBasic

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

On b2g, there is a case that we do not want to use GPU. See Bug 1206583. If we still want to use gralloc, we need GrallocTextureHostBasic like MacIOSurfaceTextureHostBasic.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Disable GPU composition and gralloc

By applying the patch, master flame-kk was booted, but it had the following problems.
 - colors are reverted
 - video playback / camera preview causes system reboot.
    (They are heavily depends on gralloc implementation).

Comment 2

[:garvan]

Hi, the component=Geolocation is wrong

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Cc Cervantes. This might also help we run emulator without GL support.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: temporary patch - Disable GPU composition and use BasicCompositor

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Add GrallocTextureHostBasic

It lacks yub support and necessary fence handling.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Add GrallocTextureHostBasic

Enable video on full hwc composition case. It seems to have a lot of flickering. It seems to be caused by lacking fence handling.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #6)
> Created attachment 8669154 [details] [diff] [review]
> wip patch - Add GrallocTextureHostBasic
> 
> Enable video on full hwc composition case. It seems to have a lot of
> flickering. It seems to be caused by lacking fence handling.

I understand that it is not come from fence. On basic compositor, DisplaySurface::QueueBuffer() is always called even when hwc full composition case. When hwc full composition case, the QueueBuffer() should not be called.

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #7)
> 
> I understand that it is not come from fence. On basic compositor,
> DisplaySurface::QueueBuffer() is always called even when hwc full
> composition case. When hwc full composition case, the QueueBuffer() should
> not be called.

Sorry, it is wrong. DisplaySurface::QueueBuffer() is not called in full hwc composition. I am going to investigate more.

Comment 9

[Sotaro Ikeda [:sotaro]]

The flicker was caused by blitComposite handling code. Current implementation expects EGLSurface. It is GonkDisplayJB::UpdateDispSurface(). By fixing it, the flicker was fixed.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Add GrallocTextureHostBasic

Fixed flicker during full hwc composition.
Video rendering by BasicCompositor is not supported yet.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GrallocTextureHostBasic

Add video rendering with BasicCompositor.
By this patch, all necessary capabilities seems to be implemented.
I tested a little bit on master flame-kk. From it, BasicCompositor seems to have some problem around invalidation.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GrallocTextureHostBasic

Correct patch.

Comment 13

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8669369 [details] [diff] [review]
patch - Add GrallocTextureHostBasic

:nical, I use GrallocImage to render video with BasicCompositor. What is a preferred way?

Comment 14

[Nicolas Silva [:nical]]

Comment on attachment 8669369 [details] [diff] [review]
patch - Add GrallocTextureHostBasic

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

I would prefer to have GrallocTextureHostBasic handle locking and unlocking the graphic buffer and create a generic WrappingBasicTextureSource (or something like that) which can be created around a pointer and that is not gralloc-specific (we could also use it with BufferTextureHost for instance).
I am not super found of the idea that this TextureHost/Source pair can only be used with video (since the underlying buffer is only unlocked when the TextureHost is destroyed, it can't be used for painting and you can only know that by carefully looking at its code).
Since compositing is happening synchronously, you should be able to lock the graphic buffer before DrawQuad and unlock it after it (so in TextureHost::Lock and TextureHost::Unlock) which is a less surprising behavior and would make it possible to use this kind of texture for painting.
Let's say f- as a first impression, but since this is only for a not-very-well-defined (experimental?) platform, it's possible that you'll convince me to change my mind.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GrallocTextureHostBasic

Updated as to use DataTextureSourceBasic and removed dairy hack to GrallocImage.

At first, I tried to implement lock/unlock semantics clearly. But TextureHost does not implement it clearly and TextureHost seems to assume that buffer mapping is actually bound to TexureSource. Therefore the patch implements it to similar level.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GrallocTextureHostBasic

Update nits.

Comment 17

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8678748 [details] [diff] [review]
patch - Add GrallocTextureHostBasic

nical, how about this patch.

Comment 18

[Nicolas Silva [:nical]]

Comment on attachment 8678748 [details] [diff] [review]
patch - Add GrallocTextureHostBasic

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

::: gfx/layers/basic/GrallocTextureHostBasic.cpp
@@ +99,5 @@
> +                               aFlags & TextureFlags::RB_SWAPPED);
> +    mSize = gfx::IntSize(grallocBuffer->getWidth(), grallocBuffer->getHeight());
> +    mCropSize = mSize;
> +  } else {
> +    printf_stderr("gralloc buffer is nullptr");

nit: you should add a \n at the end or use one of our warning macros, otherwise the log won't show up when you expect it.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GrallocTextureHostBasic

Apply the comment.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add GrallocTextureHostBasic

Fix build failure on linux. Carry "r=nical".

Comment 21

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=031700c2e760

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cab34e0b0a7b

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/cab34e0b0a7b