929973 - Implement android::IGraphicBufferAlloc in B2G

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sotaro Ikeda [:sotaro]]

New nexus7(2013 version) implement camera hal ver 3.0. If camera hal version is more than 2, Camera2Client is created in CameraService in mediaserver. The Camera2Client instantiates BufferQueue. The BufferQueue uses android::IGraphicBufferAlloc as Buffer allocator.

If we want to use CameraService without modifying it, we need to provide android::IGraphicBufferAlloc.

Comment 1

[Vincent Lin[:vilin]]

Very interesting idea.
Are you going to do that similar to what we've done in widget / gonk / libdisplay / GonkDisplayJB.cpp

#include <gui/GraphicBufferAlloc.h>
mAlloc = new GraphicBufferAlloc();

Here Gonk is just to allocate buffer for showing BootAnimation only.

But I think it's much more complicated for your objective(use CameraService without modifying it).

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - add IGraphicBufferAlloc support to gonk

Confirmed camera preview and taking a photo on nexus-7(flo).

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - add IGraphicBufferAlloc support to gonk

Fix nsAppShell::Init().

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add FakeSurfaceComposer

Implement ISurfaceComposer(SurfaceFlinger) just to provide ISurfaceComposer::createGraphicBufferAlloc().

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Updata nativewindow/moz.build

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Instantiate and register FakeSurfaceComposer

Register FakeSurfaceComposer as SurfaceFlinger.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Remove MaxAcquiredBufferCount limitation from GonkBufferQueue

On b2g, rendering target is far wary from BufferQueue. Consumer needs to dequeue more buffers than Android one.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 - Add null check to GonkCameraHardware::OnNewFrame()

Comment 9

[Sotaro Ikeda [:sotaro]]

Video recording are still failing on nexus-7. For the recording, at least Bug 910498 needs to be fixed.

Comment 10

[Sotaro Ikeda [:sotaro]]

Until android 4.2, the graphic buffers for taking pictures were allocated in camera hal by platform specific ways. Since android 4.3, new camera hal is going to allocate the buffers by using IGraphicBufferProducer interface. Camera preview's graphic buffers are allocated same way as before.

Comment 11

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 829525 [details] [diff] [review]
patch part 5 - Add null check to GonkCameraHardware::OnNewFrame()

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

Looks simple enough. Under what circumstances do you expect us to get an OnNewFrame() event, but we're unable to get a 'buffer'?

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #11)
> 
> Looks simple enough. Under what circumstances do you expect us to get an
> OnNewFrame() event, but we're unable to get a 'buffer'?

Current master gecko's GonkBufferQueue limits the number of buffers got by Consumer side. This causes the nullptr. The crash was happened often after taking a photo or attach a GDB. This limitation is going to be removed by attachment 829524 [details] [diff] [review].

Comment 13

[Michael Wu [:mwu]]

Comment on attachment 829520 [details] [diff] [review]
patch part 3 - Instantiate and register FakeSurfaceComposer

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

::: widget/gonk/nsAppShell.cpp
@@ +745,5 @@
>  #endif
> +#if ANDROID_VERSION >= 18
> +    if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +      android::FakeSurfaceComposer::instantiate();
> +    }

Can you clean this up a little bit while you're at it? Combine this if block into the one above it, and make sure they both use 4 space indents instead of the broken indentation we currently have.

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 829517 [details] [diff] [review]
patch part 1 - Add FakeSurfaceComposer

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

::: widget/gonk/nativewindow/FakeSurfaceComposer.cpp
@@ +58,5 @@
> +}
> +
> +/* static */
> +void FakeSurfaceComposer::instantiate() {
> +  waitServiceManager();

Do we need this? I don't really see examples of code spinning in a loop waiting for the service manager.

@@ +59,5 @@
> +
> +/* static */
> +void FakeSurfaceComposer::instantiate() {
> +  waitServiceManager();
> +  waitBeforeAdding( android::String16("SurfaceFlinger") );

On a normal B2G phone, the only process that would claim to be SurfaceFlinger should be this process, since the surfaceflinger binary shouldn't be installed. Does this cover the possibility that the service manager doesn't remove the service quickly enough if we crash?

Comment 15

[Michael Wu [:mwu]]

m1, do you know if this should be a 1.3 blocker? This basically lets us use newer camera hals. We still work with camera hals that use the older api.

Comment 16

[Michael Vines [:m1] [:evilmachines]]

Not a v1.3 blocker over here.

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #13)
> Comment on attachment 829520 [details] [diff] [review]
> patch part 3 - Instantiate and register FakeSurfaceComposer
> 
> Review of attachment 829520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nsAppShell.cpp
> @@ +745,5 @@
> >  #endif
> > +#if ANDROID_VERSION >= 18
> > +    if (XRE_GetProcessType() == GeckoProcessType_Default) {
> > +      android::FakeSurfaceComposer::instantiate();
> > +    }
> 
> Can you clean this up a little bit while you're at it? Combine this if block
> into the one above it, and make sure they both use 4 space indents instead
> of the broken indentation we currently have.

I am going to apply it to next patch.

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #14)
> Comment on attachment 829517 [details] [diff] [review]
> patch part 1 - Add FakeSurfaceComposer
> 
> Review of attachment 829517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/FakeSurfaceComposer.cpp
> @@ +58,5 @@
> > +}
> > +
> > +/* static */
> > +void FakeSurfaceComposer::instantiate() {
> > +  waitServiceManager();
> 
> Do we need this? I don't really see examples of code spinning in a loop
> waiting for the service manager.

It is not necessary. I am going to remove it in next patch. It is copied from MediaResourceManagerService's implementation. When I wrote it, I just added it for protection. But service manager always start as a first service. So, it is not necessary. I am going to remove it in next patch and also remove MediaResourceManagerService's one in a new bug.

> 
> @@ +59,5 @@
> > +
> > +/* static */
> > +void FakeSurfaceComposer::instantiate() {
> > +  waitServiceManager();
> > +  waitBeforeAdding( android::String16("SurfaceFlinger") );
> 
> On a normal B2G phone, the only process that would claim to be
> SurfaceFlinger should be this process, since the surfaceflinger binary
> shouldn't be installed. Does this cover the possibility that the service
> manager doesn't remove the service quickly enough if we crash?

Yes, I intended for it. But it seems not necessary. I see it only on ice codeaurora's code. I am going to remove it in next patch.

https://www.codeaurora.org/cgit/external/gigabyte/platform/frameworks/base/tree/media/mediaserver/main_mediaserver.cpp?h=caf/b2g_ics_1.2#n38

Comment 19

[Sotaro Ikeda [:sotaro]]

On nexus-4, the services were started like following order.

-> starting 'servicemanager'
-> starting 'vold'
-> starting 'kickstart'
-> starting 'netd'
-> starting 'debuggerd'
-> starting 'ril-daemon'
-> starting 'drm'
-> starting 'media'
-> starting 'installd'
-> starting 'keystore'
-> starting 'fakeperm'
-> starting 'fakesched'
-> starting 'fakeappops'
-> starting 'b2g'
-> starting 'rilproxy'
-> starting 'nfcd'
-> starting 'OOMLogger'
-> starting 'rmt_storage'
-> starting 'bridgemgrd'
-> starting 'qmuxd'
-> starting 'netmgrd'
-> starting 'thermald'
-> starting 'mpdecision'
-> starting 'bdAddrLoader'

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 v2 - Instantiate and register FakeSurfaceComposer

Apply the comment. Carry 'mwu=review+'.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v2 - Add FakeSurfaceComposer

Apply the comment. Confirmed the patch works on nexus7(flo).

Comment 22

[Michael Wu [:mwu]]

Comment on attachment 8333658 [details] [diff] [review]
patch part 1 v2 - Add FakeSurfaceComposer

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

Looks good - thanks!

::: widget/gonk/nativewindow/FakeSurfaceComposer.cpp
@@ +29,5 @@
> +
> +/* static */
> +void FakeSurfaceComposer::instantiate() {
> +  defaultServiceManager()->addService(
> +            String16("SurfaceFlinger"), new FakeSurfaceComposer());

nit: fix indentation here.

@@ +43,5 @@
> +}
> +
> +sp<ISurfaceComposerClient> FakeSurfaceComposer::createConnection()
> +{
> +    return NULL;

nit: use nullptr, assuming it works.

Comment 23

[Michael Wu [:mwu]]

Comment on attachment 829524 [details] [diff] [review]
patch part 4 - Remove MaxAcquiredBufferCount limitation from GonkBufferQueue

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

Can we change http://hg.mozilla.org/mozilla-central/file/c335eaa4494a/widget/gonk/nativewindow/GonkNativeWindowJB.cpp#l39 from MIN_UNDEQUEUED_BUFFERS to MAX_MAX_ACQUIRED_BUFFERS instead?

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #23)
> Comment on attachment 829524 [details] [diff] [review]
> patch part 4 - Remove MaxAcquiredBufferCount limitation from GonkBufferQueue
> 
> Review of attachment 829524 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we change
> http://hg.mozilla.org/mozilla-central/file/c335eaa4494a/widget/gonk/
> nativewindow/GonkNativeWindowJB.cpp#l39 from MIN_UNDEQUEUED_BUFFERS to
> MAX_MAX_ACQUIRED_BUFFERS instead?

Yeah, it is better solution. It should work. I am going to update the patch.

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 ver2 - Remove MaxAcquiredBufferCount limitation from GonkBufferQueue

Apply the comment.

Comment 26

[Michael Wu [:mwu]]

Comment on attachment 8335107 [details] [diff] [review]
patch part 4 ver2 - Remove MaxAcquiredBufferCount limitation from GonkBufferQueue

r=me, assuming you want this reviewed and the patch worked for you.

Comment 27

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #26)
> Comment on attachment 8335107 [details] [diff] [review]
> patch part 4 ver2 - Remove MaxAcquiredBufferCount limitation from
> GonkBufferQueue
> 
> r=me, assuming you want this reviewed and the patch worked for you.

Thanks! I just thought to confirm the patches before asking to review.

Comment 28

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #23)
> 
> Can we change
> http://hg.mozilla.org/mozilla-central/file/c335eaa4494a/widget/gonk/
> nativewindow/GonkNativeWindowJB.cpp#l39 from MIN_UNDEQUEUED_BUFFERS to
> MAX_MAX_ACQUIRED_BUFFERS instead?

Oh, I misunderstood about it. The value set by GonkBufferQueue::setMaxAcquiredBufferCount()is used as MinUndequeuedBufferCount. GonkBufferQueue is allocate 'MinUndequeuedBufferCount + "number of buffers requested from ANativeWindow's client"' number of buffers. So, change to MAX_MAX_ACQUIRED_BUFFERS means to allocate more than 30 buffers. For the time being, there is no big necessity to increase MinUndequeuedBufferCount. I am going to drop part 4 patch.

Comment 29

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8335107 [details] [diff] [review]
patch part 4 ver2 - Remove MaxAcquiredBufferCount limitation from GonkBufferQueue

part 4 became not ncecessary.

Comment 30

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=1f523d4c2557

Comment 31

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch - Implement android::IGraphicBufferAlloc in B2G

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch ver2 - Implement android::IGraphicBufferAlloc in B2G

Fix nit.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8c1cc3f587a7

Comment 34

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

https://hg.mozilla.org/mozilla-central/rev/8c1cc3f587a7

Comment 35

[Preeti Raghunath(:Preeti)]

will be in 1.3 automatically