Closed Bug 844248 Opened 11 years ago Closed 11 years ago

MediaStreamGraph adds lag to CameraPreview drawing update

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(3 files, 16 obsolete files)

DOMCameraPreview diagram
110.41 KB, application/pdf
Details
patch v7 - add a custom media stream for camera preview
12.92 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
patch v7 for b2g18 v3 - add a custom media stream for camera preview
12.91 KB, patch
sotaro
: review+
Details | Diff | Splinter Review 
Camera preview has a noticeable lag. It is very slow than android's preivew screen.
And preview update is jaggy, not smooth. It seems that a camera hal regularly drops preview frames. The camera hal expects following.
 - preview images are rendered as soon as possible
 - rendered images are returned to ANativeWindow as soon as possible

But MediaStreamGraph add some delay(about 30ms) to preview images and they are not released soon. MediaStreamGraph is not the only place of the camera preview's lag. But it is one of the major place to do it.
Camera preview images are put into MediaStreamGraph in DOMCameraPreview::ReceiveFrame(). And they come out from MediaStreamGraph in MediaStreamGraphImpl::PlayVideo() by calling VideoFrameContainer::SetCurrentFrame().

http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraPreview.cpp#202

http://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp#818
The patch add logout with timestamp by ms resolution.
Total camera Preview delay seems more than 100ms. I am going to measure a delay around MediaStreamGraph more correctly.
A diagram to help understanding around DOMCameraPreview.
I found following problems around using MediaStreamGraph for camera preview.

[1] MediaStreamGraph is a framework for synchronized audio/video processing and playback.But camera preview's requirements are totally opposite. Render images as soon as possible and release them as soon as possible. But we need to use MediaStreamGraph to render camera preview to video tag.

[2] DOMCameraPreview set the TrackTick's resolution to preview FPS. The FPS of otoro device is 30. Then each frame becomes 33.3 ms. It is too low resolution. MediaStreamGraph controls video out and buffers free depend on the TrackTick. Images rendering and release could delay by 33.3 ms resolution.

[3] camera preview FPS is best effort. The FPS is not guaranteed. MediaStreamGraph expects FPS is guranteed. On otoro device, it seems that an autual FPS is more fast. More Images are going to stored within MediaStreamGraph. But because of this, camera hal becomes starved of Images and stop to post new preview image. After when MediaStreamGraph renders and releases images, camera hal can receive new image for preview and re-start to post new preview image.

[4] MediaStreamGraph expects video track buffers images until 30ms future rendering. If it is not, the track is decided as blocked.

[5] StreamBuffer release Images by 50ms resolution. It could delay release Images.
    
[6] Media graph thread runs by 10ms resolution.
add log of actual delay of Image within MediaStreamGraph.
Attachment #717266 - Attachment is obsolete: true
Image get delay about 30ms and 70ms each.
Attachment #717267 - Attachment is obsolete: true
Attached patch Temporary patch - add timestamp (obsolete) — — Splinter Review 
Just a temporary patch to use Timestamp.
But by applying the patch, frame delay and jitter becomes more significant. It happens because GraphcBuffers are not returned to GonkNativeWindow soon, then camera hal starves GraphcBuffer and slowdown preview frames update.
Blocks: 848627
Flag this bug tef? since bug 848627 is tef+.
blocking-b2g: --- → tef?
(tef+, blocks a blocker)
blocking-b2g: tef? → tef+
(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> Created attachment 723605 [details] [diff] [review]
> Temporary patch - add timestamp
> 
> Just a temporary patch to use Timestamp.
> But by applying the patch, frame delay and jitter becomes more significant.
> It happens because GraphcBuffers are not returned to GonkNativeWindow soon,
> then camera hal starves GraphcBuffer and slowdown preview frames update.

To address timestamp problem, it is also nedessary to solve the buffer starvation problem.
As I wrote in Bug 848627 comment #6 . There are two ways to solve the buffer starvation problem.

> There are 2 ways to solve this problem. Both ways are very risky and need
> more gralloc buffers.
> [1] Increment MIN_UNDEQUEUED_BUFFERS from 2 to some value. Ensure the max
> number of gralloc buffers between GonkNativeWindow and Compositor. Modify
> camera hal to accept MIN_UNDEQUEUED_BUFFERS more than 2.
> 
> [2] Copy preview frames for rendering and GonkNativeWindow run in async
> mode.  The copied frames are throwed out of GonkNativeWindow for rendering.
> Original frames stay within GonkNativeWindow.
[1] needs to fix camera hal. I do not have a way to do it. Then I am going to write temporary patch of [2].
What about modifying MediaStream to not hold onto so many frames?  Even if we solve the other problems, MediaStream hanging onto three frames means the preview will lag reality by at least ~100ms (and I typically measure more like ~200ms).
Implement [2] and add timestamp.

I confirmed the patch on unagi. Preview update seems work. It becomes much better. On recording mode without recording, preview FPS becomes about 29-30. But in camera mode, after when I took a picture. camera app abort because of out of memory(gralloc buffer).

As a side effect, mode change between "video mode" and "camera mode" becomes very fast. Current implementation is very slow of the change, it seems that dequeueBuffer() slowness make it happens.
(In reply to Mike Habicher [:mikeh] from comment #15)
> What about modifying MediaStream to not hold onto so many frames?  Even if
> we solve the other problems, MediaStream hanging onto three frames means the
> preview will lag reality by at least ~100ms (and I typically measure more
> like ~200ms).

Yeah, it is necessary. [1] is a correct way to fix buffer starvation problem. To do it, it is necessary.
(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> Created attachment 724585 [details] [diff] [review]
> Temporary patch - add timestamp, copy video frames and GonkNativeWindow run
> in async mode
> 
> Implement [2] and add timestamp.
> 
> I confirmed the patch on unagi. Preview update seems work. It becomes much
> better. On recording mode without recording, preview FPS becomes about
> 29-30. But in camera mode, after when I took a picture. camera app abort
> because of out of memory(gralloc buffer).

In camera mode, preview video size is 480*320, it is larger than record mode(352*288). And the copy takes about 10ms-25ms. It is a very very long time. Then [1] is a way to correct fix.
(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> I found following problems around using MediaStreamGraph for camera preview.
> 
> [3] camera preview FPS is best effort. The FPS is not guaranteed.
> MediaStreamGraph expects FPS is guranteed. On otoro device, it seems that an
> autual FPS is more fast. More Images are going to stored within
> MediaStreamGraph. But because of this, camera hal becomes starved of Images
> and stop to post new preview image. After when MediaStreamGraph renders and
> releases images, camera hal can receive new image for preview and re-start
> to post new preview image.

Could we config the graph to render as soon as possible? Maybe roc could give some input about media graph.
unbitrot
Attachment #721762 - Attachment is obsolete: true
checked the delay within MediaStreamGraph by applying the following patches
 - attachment 724585 [details] [diff] [review]
 - attachment 725046 [details] [diff] [review]

Camera mode: ~120ms delay
Video mode: ~90ms delay
Add a custom MediaStream "CameraPreviewMediaStream". The class directly send Image to VideoFrameContainer without delay and buffering. 

The patch is a temporary ugly hack patch. It is just to confirm "no delay" and  "no buffering" at MediaStream on unagi device. The patch cause crash when changing camera/video mode in camera app.
In previous patch, I forgot to add CameraPreviewMediaStream...
Attachment #725475 - Attachment is obsolete: true
Attachment #721763 - Attachment is obsolete: true
Comment on attachment 725478 [details] [diff] [review]
Temporary patch v2 - add a custom media stream for camera preview

roc, can you give me an advice about how to solve the delay and jitter problem? I am thinking to create the custom MediaStream. Is it a way to go?
Attachment #725478 - Flags: feedback?(roc)
fix crash after video/camera mode change.
Attachment #725478 - Attachment is obsolete: true
Attachment #725478 - Flags: feedback?(roc)
Attachment #726207 - Flags: feedback?(roc)
Confirmed on unagi built against inbound-src.  That's a noticeable and significant improvement to the viewfinder.
Comment on attachment 726207 [details] [diff] [review]
Temporary patch v3 - add a custom media stream for camera preview

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

I think this is a reasonable temporary fix.

::: dom/camera/CameraPreviewMediaStream.cpp
@@ +35,5 @@
> +  mIsConsumed = true;
> +  for (uint32_t j = 0; j < mListeners.Length(); ++j) {
> +    MediaStreamListener* l = mListeners[j];
> +    l->NotifyConsumptionChanged(gm, MediaStreamListener::CONSUMED);
> +  }

You're not handling the case where there are multiple video outputs for the same stream.
Attachment #726207 - Flags: feedback?(roc) → feedback+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Comment on attachment 726207 [details] [diff] [review]
> Temporary patch v3 - add a custom media stream for camera preview
> 
> Review of attachment 726207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is a reasonable temporary fix.
> 
> ::: dom/camera/CameraPreviewMediaStream.cpp
> @@ +35,5 @@
> > +  mIsConsumed = true;
> > +  for (uint32_t j = 0; j < mListeners.Length(); ++j) {
> > +    MediaStreamListener* l = mListeners[j];
> > +    l->NotifyConsumptionChanged(gm, MediaStreamListener::CONSUMED);
> > +  }
> 
> You're not handling the case where there are multiple video outputs for the
> same stream.

Thank you for the comment. I will fix it in a next patch.
Attachment #723605 - Attachment is obsolete: true
Attachment #724585 - Attachment is obsolete: true
Attachment #725046 - Attachment is obsolete: true
address the comment.
Attachment #726207 - Attachment is obsolete: true
update a nit.
Attachment #726929 - Attachment is obsolete: true
Comment on attachment 726939 [details] [diff] [review]
patch v5 - add a custom media stream for camera preview

roc, can you review the patch?
Attachment #726939 - Flags: review?(roc)
commitable patch. Carry "roc: review+".
Attachment #726939 - Attachment is obsolete: true
Attachment #726993 - Flags: review+
unbitrot. Carry "roc: review+".
Attachment #726993 - Attachment is obsolete: true
Attachment #727167 - Flags: review+
(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> https://tbpl.mozilla.org/?tree=Try&rev=2a5148ee5563

There are some oranges in some platforms than B2G. They are not related to the patch. Camera API is enabled only on b2g right now.
Attachment #726242 - Attachment is obsolete: true
Keywords: checkin-needed
A patch for b2g 18. attachment 727167 [details] [diff] [review] is not applicable to b2g18.
Confirmed the patch on unagi phone.
Comment on attachment 727189 [details] [diff] [review]
patch v7 for b2g18 - add a custom media stream for camera preview

Carry "roc: review+".
Attachment #727189 - Flags: review+
Add header to the patch. Carry "roc: review+".
Attachment #727189 - Attachment is obsolete: true
Attachment #727311 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/34a9d115d704
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Backed out on v1_0_1 for warnings-as-errors bustage. I didn't feel comfortable removing the line myself not knowing if the variable not being used was on purpose or an oversight.
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/832054c341d1

https://tbpl.mozilla.org/php/getParsedLog.php?id=20932196&tree=Mozilla-B2g18_v1_0_1

../../../../dom/camera/CameraPreviewMediaStream.cpp:83:21: error: unused variable 'gm' [-Werror,-Wunused-variable]
  MediaStreamGraph* gm = MediaStreamGraph::GetInstance();
                    ^
1 error generated.
make[7]: *** [CameraPreviewMediaStream.o] Error 1
status-b2g18-v1.0.1: fixed → affected
(In reply to Ryan VanderMeulen [:RyanVM] from comment #44)
> Backed out on v1_0_1 for warnings-as-errors bustage. I didn't feel
> comfortable removing the line myself not knowing if the variable not being
> used was on purpose or an oversight.
> https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/832054c341d1
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=20932196&tree=Mozilla-
> B2g18_v1_0_1
> 
> ../../../../dom/camera/CameraPreviewMediaStream.cpp:83:21: error: unused
> variable 'gm' [-Werror,-Wunused-variable]
>   MediaStreamGraph* gm = MediaStreamGraph::GetInstance();
>                     ^
> 1 error generated.
> make[7]: *** [CameraPreviewMediaStream.o] Error 1

sorry for the bustage.
fix "unused variable". Carry "roc: review+".
Attachment #727311 - Attachment is obsolete: true
Attachment #727829 - Flags: review+
confirm fix for b2g18 on try
https://tbpl.mozilla.org/?tree=Try&rev=ecb514d9053f
See Also: → 1124630
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: