Closed Bug 836782 Opened 7 years ago Closed 7 years ago

[camera] Reduce gralloc buffer reallocation in StartPreview()

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g1819+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-b2g -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 19+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: c=performance)

Attachments

(2 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #835367 +++
Assignee: nobody → sotaro.ikeda.g
QA Contact: jhammink
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.
Blocks: 812527
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.
Move the patch here as well?
blocking-b2g: --- → tef?
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
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.
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
(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?
(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.
(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.
Lat year, I confirmed that "try to get all gralloc buffer" did not happen on msm8967.
(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?
Component: Gaia::Camera → General
Is this patch ready to land?
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).
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
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.
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.
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.
- 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.
Attachment #708669 - Attachment is obsolete: true
Comment on attachment 709305 [details] [diff] [review]
patch rev2 - reduce graphic buffer reallocation in StartPreview()

kanru, can you review the patch?
Attachment #709305 - Flags: review?(kchen)
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
Attachment #709305 - Flags: review?(kchen) → review+
fix comments. carry "kchen: review+".
Attachment #709305 - Attachment is obsolete: true
Attachment #709726 - Flags: review+
Set the title (commit message) of the patch.
carry "kchen: review+"
Attachment #709726 - Attachment is obsolete: true
Attachment #710035 - Flags: review+
Please nominate for approval once landed on m-c, but we won't block v1.0 on this bug.
blocking-b2g: tef? → -
Keywords: checkin-needed
Attachment #710035 - Attachment description: pPatch rev4 - reduce graphic buffer reallocation in StartPreview() → Patch rev4 - reduce graphic buffer reallocation in StartPreview()
Blocks: 803471
https://hg.mozilla.org/mozilla-central/rev/7dece0c29d00
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #710035 - Flags: approval-mozilla-b2g18?
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
Attachment #710035 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Target Milestone: --- → B2G C4 (2jan on)
Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1
Whiteboard: c=performance
You need to log in before you can comment on or make changes to this bug.