Closed Bug 1210182 Opened 4 years ago Closed 4 years ago

Implement GrallocTextureHostBasic

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 9 obsolete files)

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.
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).
Assignee: nobody → sotaro.ikeda.g
Hi, the component=Geolocation is wrong
Depends on: 1210189
Cc Cervantes. This might also help we run emulator without GL support.
Component: Geolocation → Graphics: Layers
It lacks yub support and necessary fence handling.
Attachment #8669127 - Attachment is obsolete: true
Enable video on full hwc composition case. It seems to have a lot of flickering. It seems to be caused by lacking fence handling.
Attachment #8669130 - Attachment is obsolete: true
(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.
(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.
The flicker was caused by blitComposite handling code. Current implementation expects EGLSurface. It is GonkDisplayJB::UpdateDispSurface(). By fixing it, the flicker was fixed.
Fixed flicker during full hwc composition.
Video rendering by BasicCompositor is not supported yet.
Attachment #8669154 - Attachment is obsolete: true
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.
Attachment #8669249 - Attachment is obsolete: true
Correct patch.
Attachment #8669365 - Attachment is obsolete: true
Comment on attachment 8669369 [details] [diff] [review]
patch - Add GrallocTextureHostBasic

:nical, I use GrallocImage to render video with BasicCompositor. What is a preferred way?
Attachment #8669369 - Flags: feedback?(nical.bugzilla)
Attachment #8669127 - Attachment is obsolete: false
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.
Attachment #8669369 - Flags: feedback?(nical.bugzilla) → feedback-
Depends on: 1215027
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.
Attachment #8669369 - Attachment is obsolete: true
Update nits.
Attachment #8678747 - Attachment is obsolete: true
Comment on attachment 8678748 [details] [diff] [review]
patch - Add GrallocTextureHostBasic

nical, how about this patch.
Attachment #8678748 - Flags: review?(nical.bugzilla)
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.
Attachment #8678748 - Flags: review?(nical.bugzilla) → review+
Apply the comment.
Attachment #8678748 - Attachment is obsolete: true
Depends on: 1218617
Fix build failure on linux. Carry "r=nical".
Attachment #8679189 - Attachment is obsolete: true
Attachment #8679198 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/cab34e0b0a7b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.