835367 - [camera][Meta] Improve performace of the camera

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Comment 1

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

Attachment: Camera library API timing

Spreadsheet with some data on how long it takes the various picture-taking APIs to execute.  This is the time spent in those synchronous functions, not necessarily the time it takes for the asynchronous processes (of i.e. autoFocus() and takePicture()) to complete.

The worst API by far is startPreview(), which (interestingly) takes only 202ms to run when the camera first starts, but 500 to 600ms (and in one instance, up to 850ms) on subsequent post-takePiture() calls.

Mean execution times for other functions is:
- getParameters() 2052ms
- setParameters() 25546ms
- autoFocus() 348ms
- takePicture() 76836ms
- stopPreview() 1.1s

See spreadsheet for full details, including standard deviations.

Comment 2

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

(Also, the above numbers were obtained on Unagi, with B2G_DEBUG=0.)

Comment 3

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

Attachment: WIP - profiler patch for camera library APIs

Comment 4

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

Speculation: part of the long execution time for startPreview() may be having to reallocate 9 graphics buffers each time.  It may be possible to reuse these buffers between photos.

Comment 5

[Sotaro Ikeda [:sotaro]]

Android camera hal do not require graphic buffer reallocation between photos. I confirmed it on some android phone running FirefoxOS. But unagi phones's camera hal seems not work correctly in current setting...

Comment 6

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

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
>
> Android camera hal do not require graphic buffer reallocation between
> photos. I confirmed it on some android phone running FirefoxOS. But unagi
> phones's camera hal seems not work correctly in current setting...

Do you have a patch?  :)  Is there a bug in the Unagi camera driver?

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #6)
> (In reply to Sotaro Ikeda [:sotaro] from comment #5)
>
> Do you have a patch?  :)  Is there a bug in the Unagi camera driver?

I do not have a patch right now. It is implemented as part of Bug 803471. I am going to create the patch by referencing Bug 803471. 

I am not sure that I can say it is a bug for unagi. How ANativeWindow works is still different from android original implementation. But it worked on qcom's ics_strawberry. ungi phone uses qcom's ics_chocolate. There might be other workaround for ics_chocolate.

Comment 8

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

More data:
- when startPreview() takes ~600ms, ~250ms of that is in the calls to abandon() and dequeueBuffer()
- the first call to dequeueBuffer() in startPreview() takes ~60 to 80ms
- the second call takes ~10 to 20ms
- the third through ninth calls take ~5 to 10ms

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> (In reply to Mike Habicher [:mikeh] from comment #6)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #5)
> >
> > Do you have a patch?  :)  Is there a bug in the Unagi camera driver?
> 
> I do not have a patch right now. It is implemented as part of Bug 803471. I
> am going to create the patch by referencing Bug 803471. 

I remind what I had done about preview start. I just removed abandon() call in GonkCameraHardware::StartPreview().

The hardware using ics_strawberry did not require all GraphicBuffers when startPreview after camera capture. But unagi using ics_chocolate always requires all GraphicBuffers when startPreview :-(
It is implemented in QualcommCameraHardware::getBuffersAndStartPreview().

Comment 10

[Sotaro Ikeda [:sotaro]]

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

- forcibly free graphic buffers in rendering state in StartPreview()
- reallocate graphic buffers when graphic buffers attributes are different than requested

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> Created attachment 707604 [details] [diff] [review]
> WIP patch - reduce graphic buffer reallocation in StartPreview()
> 
> - forcibly free graphic buffers in rendering state in StartPreview()
> - reallocate graphic buffers when graphic buffers attributes are different
> than requested

I confirmed that the patch works on unagi phone.

Comment 12

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

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

I can confirm that this works on unagi as well, and get getBuffersAndStartPreview(), most of the dequeueBuffer() calls drop to >30us (below the resolution of CLOCK_MONOTONIC).

Furthermore, startPreview() drops from ~500 to 600ms, to ~450 to 500ms, with no outliers above ~570ms.  It's not a big difference, but the shorter cycle time does make the camera feel more responsive.

Comment 13

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

Jumping the gun on nom-ing what looks like a meta bug.  Apologies.

Comment 14

[[:fabrice] Fabrice Desré]

Guys, either we keep that as a meta and move the patch to a dependant bug, or we change this one to not be a meta. In any case, this patch looks worth nominating for b2g-18

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to Fabrice Desré [:fabrice] from comment #14)
> Guys, either we keep that as a meta and move the patch to a dependant bug,
> or we change this one to not be a meta. In any case, this patch looks worth
> nominating for b2g-18

I am going to move attachment 707604 [details] [diff] [review] to but 836782 and will nominate for b2g-18. Before nominating it, I am going to check Camera hw hal code.

Comment 16

[Sotaro Ikeda [:sotaro]]

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

the patch move to Bug 836782.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Marking fixed since this is a meta bug and all its dependencies are closed.