836782 - [camera] Reduce gralloc buffer reallocation in StartPreview()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #835367 +++

Comment 1

[Sotaro Ikeda [:sotaro]]

Everytime taking a photo, camera preview is automatically stopped by camera hw. When taking picture completes, camera app request to re-start preview again.

Duraing re-starting the preview, all gralloc buffers are freed and re-allocated. It takes a longtime and delays preview restart.

Comment 2

[Sotaro Ikeda [:sotaro]]

After camera app take a photo, the app try to re-start preview again.

During re-starting preview, camera hal in unagi try to dequeue all GraphicBuffer's from ANativeWindow. The camera hal expect that ANativeWindow(GonkNativeWindow) is gralloc buffers final destination. In android, it is possible to do it. But in FirefoxOS, there is a problem. Some GraphicBuffers are posted to Compositor and they are out of GonkNativeWindow. Then the camera hal wait forever to get all gralloc buffers.

To prevent the problem, all gralloc buffers are re-allocated during preview re-start.

Comment 3

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

Move the patch here as well?

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - reduce graphic buffer reallocation in StartPreview()

Move attachment 707604 [details] [diff] [review] from Bug 835367. The bug is meta bug.

Comment 5

[Sotaro Ikeda [:sotaro]]

camera hal library is outside out built. We need to use prebuilt "camera.msm7627a.so". But the source code of the library are fethed from codeaurora.org and in source tree. Then we can read the source code.

From the log, QualcommCameraHardware::getBuffersAndStartPreview() try to get all gralloc buffers. It is implemented in QualcommCameraHardware.cpp.

https://www.codeaurora.org/gitweb/quic/la/?p=platform/hardware/qcom/camera.git;a=blob;f=QualcommCameraHardware.cpp;h=7ba1b2936410e76f358c5dad165a72491c5bbd00;hb=refs/heads/ics_chocolate#l4830

There are other source code that do not try to get all gralloc buffer if it is restart from camera capture. It is implemented in QCameraHWI_Preview.cpp. But it is not used for unagi.

https://www.codeaurora.org/gitweb/quic/la/?p=platform/hardware/qcom/camera.git;a=blob;f=QCameraHWI_Preview.cpp;h=857ab1fbfb1edcfbf88b5d1313f7d54f6591487d;hb=refs/heads/ics_chocolate#l1161

https://www.codeaurora.org/gitweb/quic/la/?p=platform/hardware/qcom/camera.git;a=blob;f=QCameraHWI_Preview.cpp;h=857ab1fbfb1edcfbf88b5d1313f7d54f6591487d;hb=refs/heads/ics_chocolate#l542

Comment 6

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

Note that the code you can see in codeaurora.org isn't necessarily the code that was used to build the library.  The CA.org code is the basis for the unagi library, but it has been modified.

Comment 7

[Sotaro Ikeda [:sotaro]]

QCameraHWI_Preview.cpp is used when V4L2_BASED_LIBCAM is true. It is enabled only when platform is msm8960. related source is following. It's restriction might depend on hw limitation.

https://www.codeaurora.org/gitweb/quic/la/?p=platform/hardware/qcom/camera.git;a=blob;f=Android.mk;h=809fa503c47e568a276e38c23dbfeb3c233ee440;hb=refs/heads/ics_chocolate#l4

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #6)
> Note that the code you can see in codeaurora.org isn't necessarily the code
> that was used to build the library.  The CA.org code is the basis for the
> unagi library, but it has been modified.

Yeah, I know. But cant't we read the source code, can we?

Comment 9

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

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> 
> Yeah, I know. But cant't we read the source code, can we?

We certainly can.  I just wanted to make sure you were aware that the code running on unagi isn't necessarily reflected in the source code you're looking at.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #9)
> (In reply to Sotaro Ikeda [:sotaro] from comment #8)
> > 
> > Yeah, I know. But cant't we read the source code, can we?
> 
> We certainly can.  I just wanted to make sure you were aware that the code
> running on unagi isn't necessarily reflected in the source code you're
> looking at.

Thanks for the notification.

Comment 11

[Sotaro Ikeda [:sotaro]]

Lat year, I confirmed that "try to get all gralloc buffer" did not happen on msm8967.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #9)
> (In reply to Sotaro Ikeda [:sotaro] from comment #8)
> > 
> > Yeah, I know. But cant't we read the source code, can we?
> 
> We certainly can.  I just wanted to make sure you were aware that the code
> running on unagi isn't necessarily reflected in the source code you're
> looking at.

How can we read the source?

Comment 13

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

Is this patch ready to land?

Comment 14

[Sotaro Ikeda [:sotaro]]

The patch needs to update.

Also need to make clear why current implementation needs to call abandon() during re-starting. It is not necessary if GonkNativeWindow is implemented more similar to android SurfaceTexture. And current GonkNativeWindow's implementation works incorrectly on msm8960(more correct camera hal implementation).

Comment 15

[Sotaro Ikeda [:sotaro]]

GonkNativeWindow try to optimize gralloc buffer reallocation than SurfaceTexture by optimizing setBufferCount(int bufferCount).

In original implementation, setBufferCount() frees all gralloc buffers and also rendering Texture. The function is also used to reset SurfaceTexture to initial state.

In GonkNativeWindow, it does not frees all gralloc buffers. If a requested buffer number is same or greater than the current one, try to keep gralloc buffers and exit the functions. By this change, the function does not work as "reset to initial state".

http://mxr.mozilla.org/mozilla-central/source/dom/camera/GonkNativeWindow.cpp#177

http://androidxref.com/4.0.4/xref/frameworks/base/libs/gui/SurfaceTexture.cpp#197

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: Temporary patch - just to confirm Comment 15

This patch is just to confirm that it is not necessary to call GonkNativeWindow::abandon() when restarting camera preview. I confirmed it works on Unagi device. Camera preview re-started after taking photos.

Comment 17

[Sotaro Ikeda [:sotaro]]

GonkNativeWindow tries to optimize gralloc buffers reallocation. But it achieve no performance win. It tries to prevent buffers reallocation as minimun. But it becomes to need to call abandon() from GonkCameraHardware to free all buffers in StartPreview().

In SurfaceTexture, abandon() is used to block any operation during the object's destruction. The function is called only once just before the object's destruction.

In GonkNativeWindow, abandon() is changed to be call anytime from GonkCameraHardware. And the function does not work as operation blocker. The change could introduce timing related defetct.

Comment 18

[Sotaro Ikeda [:sotaro]]

From camera hal point of view, GonkNativeWindow(ANativeWindow) is both buffers allocator and video renderer. The camera hal is a client of ANativeWindow API.

Call abandon() in GonkCameraHardware::StartPreview() and free all buffers means that an allocator frees all buffers without notice to the client. It should not be done.

It works luckily only on qcom ics_chocolate device. Current camera hal on unagi/otoro device luckily concurrently frees references to the buffers. It is camera hal implementation dependent thing. Basically it should not do it.

Actually, on msm8960, camera hal implementation is more efficient. It do not try to reallocate buffers when restarting camera preview after taking photoes. It tries to keep reference to the buffers. If GonkNativeWindow frees the buffers, camera preview do not work correctly. I wached this case last year on msm8960 device. 

Therefore applying attachment 708669 [details] [diff] [review] , should be limited to specific device. And need to change abandon() work as original.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch rev2 - reduce graphic buffer reallocation in StartPreview()

- remove clearRenderingStateBuffers() from GonkCameraHardware::StartPreview()
  it is moved within GonkNativeWindow::setBufferCount()
- change abandon() like as in SurfaceTexture

Clear buffers in RENDERING state is done in setBufferCount(). By this change, the buffers are safely cleared and should work also in other hardwares like msm8960.

Solve all problems in the bug and get more performance than SurfaceTexture.

Comment 20

[Sotaro Ikeda [:sotaro]]

Comment on attachment 709305 [details] [diff] [review]
patch rev2 - reduce graphic buffer reallocation in StartPreview()

kanru, can you review the patch?

Comment 21

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

Comment on attachment 709305 [details] [diff] [review]
patch rev2 - reduce graphic buffer reallocation in StartPreview()

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

::: dom/camera/GonkNativeWindow.cpp
@@ +220,5 @@
>  
>          // Error out if the user has dequeued buffers or sent buffers to
>          // video stream
>          for (int i=0 ; i<mBufferCount ; i++) {
> +            if (mSlots[i].mBufferState == BufferSlot::DEQUEUED) {

Nit: change the comments above too.

@@ +315,5 @@
>          mSlots[buf].mBufferState = BufferSlot::DEQUEUED;
>  
>          const sp<GraphicBuffer>& gbuf(mSlots[buf].mGraphicBuffer);
>          alloc = (gbuf == NULL);
> +        if ( (gbuf!=NULL) && 

Nit: trailing whitespace

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch rev3 - reduce graphic buffer reallocation in StartPreview()

fix comments. carry "kchen: review+".

Comment 23

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=7d1b0e5bb33a

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: Patch rev4 - reduce graphic buffer reallocation in StartPreview()

Set the title (commit message) of the patch.
carry "kchen: review+"

Comment 25

[Alex Keybl [:akeybl]]

Please nominate for approval once landed on m-c, but we won't block v1.0 on this bug.

Comment 26

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dece0c29d00

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7dece0c29d00

Comment 28

[Sotaro Ikeda [:sotaro]]

Comment on attachment 710035 [details] [diff] [review]
Patch rev4 - reduce graphic buffer reallocation in StartPreview()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: After taking a photo, camera preview re-start takes long time.
Testing completed: Tested in Unagui phone.
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 710035 [details] [diff] [review]
Patch rev4 - reduce graphic buffer reallocation in StartPreview()

Please go ahead with uplift to the tip of mozilla-b2g18 branch for v1.0.1

Comment 30

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

Landed!
https://hg.mozilla.org/releases/mozilla-b2g18/rev/693d15ae6dad

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1