Closed Bug 988704 Opened 6 years ago Closed 6 years ago

[dolphin][camera]camera preview is obviously slower than android

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.4+, firefox30 wontfix, firefox31 wontfix, firefox32 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S2 (23may)
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: ying.xu, Assigned: vliu)

References

()

Details

(Whiteboard: [c=handeye p= s= u=1.4][Dolphin_1.4])

Attachments

(1 file, 4 obsolete files)

dolphin camera preview is very slower.
People can detect obvious delay when move the camera direction.

For a preliminary analyze , it's caused by preview size.
The preview size is smaller, the speed is lower.

Now the size is 352X288.
If I change the preview size into 720X480, the preview speed is much better than 352X288
gaia/apps/camera/js/app.js 

some of preview size are removed.

 recorderProfiles: {
    title: 'Video Resolution',
    icon: 'icon-video-size',
    options: [

      // NOTE: Disabled due to Helix crashing
      // when trying to record at these resolutions.

      // {
      //   key: '720p',
      //   title: '720p 1040X720'
      // },
      // {
      //   key: '480p',
      //   title: '480p 720X480'
      // },
      {
        key: 'cif',
        title: 'CIF 352X288'
      },
      {
        key: 'qcif',
        title: 'QCIF 176X144'
      }
    ],
    persistent: true,
    menu: 4
  },
b2g request more buffer than android. In b2g, need to forward buffers to Compositor(in b2g proces), it seems to affected. It might be better to try increase the number of preview buffers.
Blocks: dolphin
Duplicate of this bug: 988669
(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> b2g request more buffer than android. In b2g, need to forward buffers to
> Compositor(in b2g proces), it seems to affected. It might be better to try
> increase the number of preview buffers.

When the preview is slower , I can find the state of waiting buffer of camera hardware in kernel log.

Now the preview buffer queue has 8 items. 
And the preview speed may decrease to 10~20FPS automatically ,according to the scene.
If we increased the buffer queue, the display delay would be very obvious.
The number of prevew buffer queue in Nexus-5 is 9. As for dolphin, I saw 8 items as ying.xu saw. Besides this, Another interesting thing I found is that tarako/Nexus-5 creates 3 threads to deal with QUEUE_BUFFER in IGraphicBufferProducer.cpp but dolphin only creates 1 thread to deal with. I am not sure if it causes the preview slowly and still figure it out.
>I found is
> that tarako/Nexus-5 creates 3 threads to deal with QUEUE_BUFFER in
> IGraphicBufferProducer.cpp but dolphin only creates 1 thread to deal with. I
> am not sure if it causes the preview slowly and still figure it out.

It could affect the performance. In b2g, GonkBufferQueue::dequeueBuffer() could take longer time than android. camera hal needs to be resistant to GonkBufferQueue::dequeueBuffer()'s long time locking.
(In reply to Sotaro Ikeda [:sotaro] from comment #6)

> It could affect the performance. In b2g, GonkBufferQueue::dequeueBuffer()
> could take longer time than android. camera hal needs to be resistant to
> GonkBufferQueue::dequeueBuffer()'s long time locking.

In dolphin, |mDequeueCondition.wait(mMutex)| in GonkBufferQueue::dequeueBuffer() consumed about 20~30ms for waiting a buffer to be released. But in Nexus-5, the waiting never happens.
Can you give us a video of the delay in the screen update?
Flags: needinfo?(ying.xu)
Keywords: perf
Priority: -- → P3
Whiteboard: [c=handeye p= s= u=]
Number of buffer the camera hw allocate can be increase by changing MIN_UNDEQUEUED_BUFFERS or by calling BufferQueue::setMaxAcquiredBufferCount(). But some hardware might not handle correctly this change. For example hamachi case, the phone crashed. But it can be used for checking the effect of increasing buffers.
Attached video Preview of dolphin camera (obsolete) —
We can easily see the delay in the screen update.
(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> Number of buffer the camera hw allocate can be increase by changing
> MIN_UNDEQUEUED_BUFFERS or by calling
> BufferQueue::setMaxAcquiredBufferCount(). But some hardware might not handle
> correctly this change. For example hamachi case, the phone crashed. But it
> can be used for checking the effect of increasing buffers.

As for your suggestion, I hacked to write MIN_UNDEQUEUED_BUFFERS to 4. After that modification, the preview can have a great improvement. Some questions are raised for the next move.

diff --git a/widget/gonk/nativewindow/GonkBufferQueueKK.h b/widget/gonk/nativewindow/GonkBufferQueueKK.h
index 412028f..b492a79 100644
--- a/widget/gonk/nativewindow/GonkBufferQueueKK.h
+++ b/widget/gonk/nativewindow/GonkBufferQueueKK.h
@@ -41,7 +41,7 @@ class GonkBufferQueue : public BnGraphicBufferProducer,
     typedef mozilla::layers::SurfaceDescriptor SurfaceDescriptor;
 
 public:
-    enum { MIN_UNDEQUEUED_BUFFERS = 2 };
+    enum { MIN_UNDEQUEUED_BUFFERS = 4 };
     enum { NUM_BUFFER_SLOTS = 32 };
     enum { NO_CONNECTED_API = 0 };
     enum { INVALID_BUFFER_SLOT = -1 };

Q1: In Comment 2, you had ever mentioned that b2g request more buffer than android. Does the case we adjusted MIN_UNDEQUEUED_BUFFERS just like what you mean? If No, Can you please have a more description about what you said in Comment 2? I am really interested in the difference you said between b2g and Android. 

Q2: Enlarge MIN_UNDEQUEUED_BUFFERS means consumer need acquire more buffer counts to consume data. Does it means the speed the producer produces data is faster than the consumer cousumes? If so, does it means we have to check why display driver consumes slowly?

Q3: When I obsered |mDequeueCondition.wait(mMutex)| in dequeue(), it always took about 20~30ms for waiting a buffer to be released. Does it means also point that the consumer consumes data slowly?
Flags: needinfo?(sotaro.ikeda.g)
> 
> Q1: In Comment 2, you had ever mentioned that b2g request more buffer than
> android. Does the case we adjusted MIN_UNDEQUEUED_BUFFERS just like what you
> mean? If No, Can you please have a more description about what you said in
> Comment 2? I am really interested in the difference you said between b2g and
> Android. 

In android camera preview case, GLConsumer does actual rendering. And beween GLConsumer and BufferQueue are in same process and very close. Beween GLConsumer and BufferQueue, 1-2 gralloc buffer are present.
https://github.com/sotaroikeda/android-diagrams/blob/master/graphics/Layer_4.3.pdf?raw=true

In b2g case, ImageHost does actual rendering. Between CompositorOGL and GonkBufferQueue are in different process. Between ImageHost and GonkBufferQueue, more than 3 gralloc buffers are present.
https://github.com/sotaroikeda/firefox-diagrams/blob/master/gfx/gfx_ImageBridge_FirefoxOS_1_2.pdf?raw=true

> Q2: Enlarge MIN_UNDEQUEUED_BUFFERS means consumer need acquire more buffer
> counts to consume data. Does it means the speed the producer produces data
> is faster than the consumer cousumes? If so, does it means we have to check
> why display driver consumes slowly?

As already written to Q1 comment, Between ImageHost and GonkBufferQueue, more than 3 gralloc buffers are present. The number is more than MIN_UNDEQUEUED_BUFFERS(2). Therefore, GonkBufferQueue always faces intermittent buffer starvation. GonkBufferQueue::dequeueBuffer() needs to wait until buffer is returned from ImageHost to GonkBufferQueue. I do not think display driver's consumption is low. It is just happen because ImageHost is far away from GonkBufferQueue.

> Q3: When I obsered |mDequeueCondition.wait(mMutex)| in dequeue(), it always
> took about 20~30ms for waiting a buffer to be released. Does it means also
> point that the consumer consumes data slowly?

It just says that more than MIN_UNDEQUEUED_BUFFERS(2) buffers are between ImageHost and GonkBufferQueue.
Flags: needinfo?(sotaro.ikeda.g)
We already tried to increase MIN_UNDEQUEUED_BUFFERS(2) in b2g ics phones. But inari, hamachi, leo devices crashed when MIN_UNDEQUEUED_BUFFERS is increased more than 2.
"MIN_UNDEQUEUED_BUFFERS = 4" is also used in SurfaceMediaSource. It is also present in ICS, but inari, hamachi, leo do not support this. Almost all JB devices seems to support the setting.
http://androidxref.com/4.3_r2.1/xref/frameworks/av/include/media/stagefright/SurfaceMediaSource.h#61
In the past there is a bug like Bug 844248. About camera preview handling, current b2g seems to works as soon as possible. Just ImageHost is far way from GonkBufferQueue.
(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> I do not
> think display driver's consumption is low. It is just happen because
> ImageHost is far away from GonkBufferQueue.

From the test what I did. it does.

preview with size 352X288 is much slower than size 720X480, while the screen size is 854X480
Flags: needinfo?(ying.xu)
Whiteboard: [c=handeye p= s= u=] → [c=handeye p= s= u=][Dolphin_1.4]
Assignee: nobody → vliu
blocking-b2g: --- → 1.4?
1.4?
Flags: needinfo?(itsay)
According to Comment 13 and Comment 14, should I propose a patch also increasing MIN_UNDEQUEUED_BUFFERS in GonkBufferQueueKK.h to 4?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Vincent Liu[:vliu] from comment #18)
> According to Comment 13 and Comment 14, should I propose a patch also
> increasing MIN_UNDEQUEUED_BUFFERS in GonkBufferQueueKK.h to 4?

The above change could affect all use cases that use GonkBufferQueueKK. For example, Video playback and WebRTC. It could affect all b2g devices since KK. For the time being, it seems better to change MaxAcquiredBufferCount by calling setMaxAcquiredBufferCount() only on camera use case.

And before changing the code, confirmation to almost all b2g kk devices seems necessary. And it seems better to get feedback from codeaurora and spreadtrum.


By the way, GonkNativeWindow set MaxAcquiredBufferCount by the following code.
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkNativeWindowKK.cpp#40
Flags: needinfo?(sotaro.ikeda.g)
On mobile, kernel is configured to allocate memory as minimum as possible. This change could cause out of memory in unintended use cases and unintended devices. Therefore we need to be very very careful this kind of changes.
Hi Sotaro,

Thanks for the good suggestion on Comment 19. I will do the following.
blocking-b2g: 1.4? → 1.4+
Priority: P3 → P1
Status: NEW → ASSIGNED
Flags: needinfo?(itsay)
Attached patch bug-988704-WIP-v1.patch (obsolete) — Splinter Review
Hi Sotaro,

Here is my proposed patch for this issue. Basically I changed GonkNativeWindow constructor by adding a default parameter GonkBufferQueue::MIN_UNDEQUEUED_BUFFERS which was used by anyone who wants to new this object. GonkCameraHardware::MIN_UNDEQUEUED_BUFFERS was passed by the argument of constructor for Camera specific usage.

This WIP was test on both dolphin and Nexus-4-kk and they both worked fine for Preview/taking-picture/video-recording. If you feel the WIP is fine to you, I will than needinfo codeaurora and spreadtrum. Thanks.
Attachment #8419314 - Flags: feedback?(sotaro.ikeda.g)
Comment on attachment 8419314 [details] [diff] [review]
bug-988704-WIP-v1.patch

It seem better to change GonkCameraHwMgr only only on KK. The patch extended GonkNativeWindow only on KK. And we are going to change the preview's buffer count only on KK.

> mNativeWindow = new GonkNativeWindow(GonkCameraHardware::MIN_UNDEQUEUED_BUFFERS);

Except that looks good.
Attachment #8419314 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Attached patch bug-988704-WIP-v2.patch (obsolete) — Splinter Review
(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> Comment on attachment 8419314 [details] [diff] [review]
> bug-988704-WIP-v1.patch
> 
> It seem better to change GonkCameraHwMgr only only on KK. The patch extended
> GonkNativeWindow only on KK. And we are going to change the preview's buffer
> count only on KK.
> 
> > mNativeWindow = new GonkNativeWindow(GonkCameraHardware::MIN_UNDEQUEUED_BUFFERS);
> 
> Except that looks good.

The attached file is WIP-V2. MIN_UNDEQUEUED_BUFFERS set to 4 only on Android KK base.
Attachment #8419314 - Attachment is obsolete: true
Attachment #8420485 - Flags: feedback?(sotaro.ikeda.g)
Comment on attachment 8420485 [details] [diff] [review]
bug-988704-WIP-v2.patch

Looks good!
Attachment #8420485 - Flags: feedback?(sotaro.ikeda.g) → feedback+
There are two reasons I proposed attachment 8420485 [details] [diff] [review] to increase MIN_UNDEQUEUED_BUFFERS.

1. In android camera preview case, GLConsumer does actual rendering. And beween GLConsumer and BufferQueue are in same process and very close. Beween GLConsumer and BufferQueue, 1-2 gralloc buffer are present. But in b2g case, ImageHost does actual rendering. Between CompositorOGL and GonkBufferQueue are in different process. Between ImageHost and GonkBufferQueue, more than 3 gralloc buffers are present. Please see Comment 12 to know the detail.

2. In Android, "MIN_UNDEQUEUED_BUFFERS = 4" has been used in SurfaceMediaSource since JB base. Please see Comment 14 to know the detail.

To keep the stability for older platform, the patch only affected in Android KK base. This patch may affect the driver layer so I need feedback from codeaurora and spreadtrum.
Flags: needinfo?(ying.xu)
Flags: needinfo?(mvines)
Comment on attachment 8420485 [details] [diff] [review]
bug-988704-WIP-v2.patch

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

You need to update the code where omxdecorder.cpp call new GonkNativeWindow too.
(In reply to ying.xu from comment #27)
> Comment on attachment 8420485 [details] [diff] [review]
> bug-988704-WIP-v2.patch
> 
> Review of attachment 8420485 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You need to update the code where omxdecorder.cpp call new GonkNativeWindow
> too.

The patch intends to increase the # of buffers only for Camera on Android KK base. For omxdecorder, we keep its original value defined in GonkNativeWindow.h.
Whiteboard: [c=handeye p= s= u=][Dolphin_1.4] → [c=handeye p= s= u=1.4][Dolphin_1.4]
Comment on attachment 8420485 [details] [diff] [review]
bug-988704-WIP-v2.patch

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

It's OK.
Clear need info since I'v confirmed with caf and SPRD from different channels.
Flags: needinfo?(ying.xu)
Flags: needinfo?(mvines)
Attached patch bug-988704-fix-v1.patch (obsolete) — Splinter Review
Hi Sotaro,

Can you have a review for the patch? Thanks.
Attachment #8401059 - Attachment is obsolete: true
Attachment #8420485 - Attachment is obsolete: true
Attachment #8422964 - Flags: review?(sotaro.ikeda.g)
Attachment #8422964 - Flags: review?(sotaro.ikeda.g) → review+
Hi Sotaro,

I think it better the patch also land into master branch. How do you think? Please comment it if you have any concern. Thanks.
Flags: needinfo?(sotaro.ikeda.g)
The patch is for v1.4 branch.
Keywords: perfcheckin-needed
More to the point, if master is affected, the tree rules clearly state that it *must* land on master first.
https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_5
Keywords: checkin-needed
(In reply to Vincent Liu[:vliu] from comment #32)
> Hi Sotaro,
> 
> I think it better the patch also land into master branch. How do you think?
> Please comment it if you have any concern. Thanks.

As in Comment 34, at first you need to commit to master.
Flags: needinfo?(sotaro.ikeda.g)
As the suggestions on Comment 34 and Comment 35, I attached the review+ patch for master branch to land. Also attached the result for try server.

https://tbpl.mozilla.org/?tree=Try&rev=1822b8dfd966
https://tbpl.mozilla.org/?tree=Try&rev=c0872f05d614
Attachment #8422964 - Attachment is obsolete: true
The patch on Comment 36 is for master branch.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/96bd829e7272
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S2 (23may)
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/59b45630a463

Keep in mind for the future that your commit message should be summarizing what the patch is actually doing, not restating the problem it's solving :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Not enough information with current STR to write test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
Test case added in moztrap:

https://moztrap.mozilla.org/manage/case/14388/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.