vasanth Reporter
	Description • 10 years ago

Pre requisite
-------------
This issue is only present in msm8610 (dual core version)
Use flame devices after disabling 3rd and 4th core.
adb shell stop mpdecision
adb shell "echo 0 > /sys/devices/system/cpu/cpu2/online"
adb shell "echo 0 > /sys/devices/system/cpu/cpu3/online"

Steps to reproduce
------------------
1.open Camcorder.
2.Enable below setprop command to observe the FPS flow.
adb shell setprop persist.debug.sf.showfps 1
3.Then run below command on command prompt to see FPS low.
adb logcat | grep "Video Frames Per Second"
4.start recording.
5.Zoom in/out while recording is going on.
6. Huge frames drops observed while zoom in/out during Video recording this leads to jerkiness on the preview as well as on recorded clips. FPS goes below 10 
7. Not reproducible in Android

FPS flow
--------
E/QCamera2HWI(  214): Video Frames Per Second: 0.8207
E/QCamera2HWI(  214): Video Frames Per Second: 1.9317
E/QCamera2HWI(  214): Video Frames Per Second: 9.8785
E/QCamera2HWI(  214): Video Frames Per Second: 3.5233
E/QCamera2HWI(  214): Video Frames Per Second: 1.3770
E/QCamera2HWI(  214): Video Frames Per Second: 2.4637

Analysis
--------
From Camera folks
-----------------
No free buffer left at camera HAL or framework side and thus we see video frame drops like below till we get buffer from encoder.

From encoder folks
------------------
The number of buffers given to encoder itself is less. So the middle part like Camera source might be holding the other buffers.

I see we have GonkCameraSource.cpp instead of stage fright's CameraSource.cpp
Could you guys try to reproduce this issue and debug recording zoom in/out from 
GonkCameraSource perspective?

	Preeti Raghunath(:Preeti)
	Comment 1 • 10 years ago

Vasanth

Can we please provide numbers in comparison with 1.4?

	Hema Koka [:hema]
	Comment 2 • 10 years ago

Andrew,

Can you try reproducing this and investigate it on flame?

copying mikeh and sotaro too.

Thanks
Hema

	vasanth Reporter
	Comment 3 • 10 years ago

(In reply to Preeti Raghunath(:Preeti) from comment #1)
> Vasanth
> 
> Can we please provide numbers in comparison with 1.4?

Same issue happens in 1.4 as well. FPS goes below 10 leads to jerkiness on the preview as well as on recorded clips
Without Zoom In/Out FPS is around ~30

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 4 • 10 years ago

I was able to reproduce last night on Flame. We have architectural differences in how we handle the preview display compared to Android, which may be causing additional overhead. As found empirically, it appears there are up to 9 media buffers outstanding from the camera driver and we stop receiving new data until old is processed; during the zoom, it looks like encoding falls behind (presumably due to lack of CPU cycles) which is why we start dropping frames at the camera driver layer. Testing is complicated by yesterday's build crashing when attempting to watch a recorded video, and today's build's zoom does nothing. Exploring some changes in gecko to see if a workaround is possible (at least for the actual recording, preview may be harder).

	David Flanagan [:djf]
	Comment 5 • 10 years ago

Andrew: if you think you can continue to make progress on this, please assign it to yourself. Otherwise, let Hema know so we can find an assignee. As a blocker, we don't want to let it go without an owner.

Vasanth: does this bug go away if you just use a smaller video size? We default to recording very large videos, and it may simply be a matter of not having enough CPU power to handle that on the device you are using.

Try uncommenting these lines in apps/camera/js/config/config.js:

      // {
      //   key: 'recorderProfiles'
      // },

That should give you a choice of video resolutions in the camera settings menu. Then, empirically you can determine what the maximum size is that works well enough for your device. This may simply be a matter of doing device-specific build-time configuration so that we record at a size that devices can comfortably handle.

Setting needinfo for Diego, too, because he knows more about how to configure the video resolution.

	vasanth Reporter
	Comment 6 • 10 years ago

(In reply to David Flanagan [:djf] from comment #5)
> Vasanth: does this bug go away if you just use a smaller video size? 
Nope. Able to reproduce with lowest resolution as well. (qcif 176x144)

	Hema Koka [:hema]
	Comment 7 • 10 years ago

Andrew/Mike any update on this?

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 8 • 10 years ago

Preliminary testing on a change I made shows that by slowing down pushing frames into the preview when it is flooded helps with the buffers running out observed in the lower levels of the camera by the vendor. However although even we do not appear to run out of buffers, I do not perceive the recorded video to be significantly smoother than before.

We do appear to be much more aggressive in the degree to which one gesture can change the zoom as compared to BB/iPhone (I assume Android is similar, no reference phone on hand). Pinch to zoom in/out appears to have a maximum increment on those devices, where we can jump from the minimum to the maximum with a sufficiently quick gesture. I am now exploring rate limiting this to see if that has the desired effect on the fluidness of the video.

Vasanth: Would it be possible to get a brief video showing the zoom in/out behavior you expect to make sure I am reproducing/visualizing the issue properly? And if at all possible, a debug log showing the zoom parameter changes over time?

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 9 • 10 years ago

Vasanth: To clarify, I was looking for the debug log / actual recorded video produced by Android, not by Firefox OS (although a recorded video from the latter couldn't hurt ;)).

	Justin D'Arcangelo [:justindarc]
	Comment 10 • 10 years ago

(In reply to Andrew Osmond [:aosmond] from comment #8)
> We do appear to be much more aggressive in the degree to which one gesture
> can change the zoom as compared to BB/iPhone (I assume Android is similar,
> no reference phone on hand). Pinch to zoom in/out appears to have a maximum
> increment on those devices, where we can jump from the minimum to the
> maximum with a sufficiently quick gesture. I am now exploring rate limiting
> this to see if that has the desired effect on the fluidness of the video.

Andrew: Let me know if you think this would help mitigate the problem. I can probably throw together a gaia patch for this relatively quickly.

	Justin D'Arcangelo [:justindarc]
	Comment 12 • 10 years ago

Andrew: This is a patch for throttling the repetitive speed in which we call the `zoom` setter attribute on the Camera Control API. This should only allow for 1 call every 150ms (roughly 6 calls per second).

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 13 • 10 years ago

There will have to be a follow up fix to this one resolving an unrelated crash which appears to be caused by slightly different timing resulting in this change (unless somebody sees a flaw breaking the OmxDecoder destructor :)).

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 14 • 10 years ago

Comment on attachment 8449495 [details]
pull-request (master)

jdarcangelo, looks good and works well; this will need to be committed along with my fix when ready although obviously they can be put in separately safely.

	Inder
	Comment 15 • 10 years ago

Vasanth -- can you please try out this patch and see if it helps.

	Justin D'Arcangelo [:justindarc]
	Comment 16 • 10 years ago

Comment on attachment 8449495 [details]
pull-request (master)

Flagging Diego for formal review.

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 17 • 10 years ago

Fix aforementioned crash after stopping recording, fix how we did not reset the rate limiting after recording was done.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 18 • 10 years ago

Comment on attachment 8449706 [details] [diff] [review]
Throttle preview frame, v1

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

r- largely due to nits and style issues. Also, a question about holding mMutex before reading mInvalidatePending.

::: dom/camera/CameraControlImpl.cpp
@@ +254,5 @@
> +  // This function runs on neither the Main Thread nor the Camera Thread.
> +  RwLockAutoEnterRead lock(mListenerLock);
> +
> +  DOM_CAMERA_LOGI("OnRateLimitPreview: %d\n",
> +    aLimit);

This can all go on one line.

::: dom/camera/CameraPreviewMediaStream.cpp
@@ +118,5 @@
> +CameraPreviewMediaStream::Invalidate()
> +{
> +  MutexAutoLock lock(mMutex);
> +  --mInvalidatePending;
> +  for (uint32_t i = 0; i < mVideoOutputs.Length(); ++i) {

Instead of uint32_t, you should use <mVideoOutputs type>::size_type.

@@ +123,5 @@
> +    VideoFrameContainer* output = mVideoOutputs[i];
> +    output->Invalidate();
> +  }
> +}
> + 

nit: extra whitespace.

@@ +135,3 @@
>  CameraPreviewMediaStream::SetCurrentFrame(const gfxIntSize& aIntrinsicSize, Image* aImage)
>  {
> +  if (mInvalidatePending > 0) {

Should this function hold mMutex before reading mInvalidatePending et al?

@@ +135,5 @@
>  CameraPreviewMediaStream::SetCurrentFrame(const gfxIntSize& aIntrinsicSize, Image* aImage)
>  {
> +  if (mInvalidatePending > 0) {
> +    if (mRateLimit || mInvalidatePending > MAX_INVALIDATE_PENDING) {
> +      DOM_CAMERA_LOGW("Discard preview frame %d, invalidates pending %d", ++mDiscardedFrames, mInvalidatePending);

nit: wrap this line at some suitable place--it's getting a bit long.

Also, put the increment of mDiscardedFrames on its own line, instead of in the macro.

@@ +138,5 @@
> +    if (mRateLimit || mInvalidatePending > MAX_INVALIDATE_PENDING) {
> +      DOM_CAMERA_LOGW("Discard preview frame %d, invalidates pending %d", ++mDiscardedFrames, mInvalidatePending);
> +      return;
> +    } else {
> +      DOM_CAMERA_LOGI("Update preview frame, invalidates pending %d", mInvalidatePending);

"Update preview frame, %d invalidation(s) pending\n"

@@ +147,5 @@
> +  {
> +    MutexAutoLock lock(mMutex);
> +
> +    TimeStamp now = TimeStamp::Now();
> +    for (uint32_t i = 0; i < mVideoOutputs.Length(); ++i) {

Please make this use ::size_type as well.

@@ +154,5 @@
> +    }
> +
> +    if (mFrameCallback) {
> +      mFrameCallback->OnNewFrame(aIntrinsicSize, aImage);
> +    }

This conditional block is a hook for WebRTC. You might want to ping jesup to let him know the behaviour is going to change in this patch.

::: dom/camera/CameraPreviewMediaStream.h
@@ +16,5 @@
>    virtual void OnNewFrame(const gfxIntSize& aIntrinsicSize, layers::Image* aImage) = 0;
>  };
>  
>  /**
> + * This is a stream for camera preview.

Nice. :)

::: dom/camera/DOMCameraControl.h
@@ +213,5 @@
>    //  with it.
>    DOMCameraControlListener* mListener;
>  
>    // our viewfinder stream
> +  nsRefPtr<CameraPreviewMediaStream> mInput;

Did this used to leak?

::: dom/camera/GonkCameraControl.h
@@ +56,4 @@
>    void OnNewPreviewFrame(layers::TextureClient* aBuffer);
>    void OnRecorderEvent(int msg, int ext1, int ext2);
>    void OnSystemError(CameraControlListener::SystemContext aWhere, nsresult aError);
>   

nit: extra whitespace

::: dom/camera/GonkCameraHwMgr.cpp
@@ +46,5 @@
>  
>  void
> +GonkCameraHardware::OnRateLimitPreview(bool aLimit)
> +{
> +  ::OnRateLimitPreview(mTarget, aLimit);

:: not required.

::: dom/camera/GonkCameraSource.cpp
@@ +688,5 @@
>  }
>  
>  void GonkCameraSource::dataCallbackTimestamp(int64_t timestampUs,
>          int32_t msgType, const sp<IMemory> &data) {
> +    bool rateLimit, prevRateLimit;

Please put these declarations on separate lines.

@@ +729,5 @@
> +        }
> +        ++mNumFramesReceived;
> +
> +        // If a backlog is building up in the receive queue, we are likely
> +        // resource constrained and we need to throttle 

nit: trailing whitespace.

@@ +731,5 @@
> +
> +        // If a backlog is building up in the receive queue, we are likely
> +        // resource constrained and we need to throttle 
> +        prevRateLimit = mRateLimit;
> +        rateLimit = mRateLimit = mFramesReceived.empty();

Ugh, separate lines, please!

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 19 • 10 years ago

(In reply to Andrew Osmond [:aosmond] from comment #14)

> jdarcangelo, looks good and works well; this will need to be committed along
> with my fix when ready although obviously they can be put in separately
> safely.

Justin, your change is to rate-limit zoom calls, right? That's probably a good approach to get this fixed, but I think long-term it makes more sense to make the WebAPI do the rate-limiting.

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 20 • 10 years ago

Fix nits, save the not-needing-:: since it gives a compiler error without. Made additional changes requested offline, namely removing the frame callback since it is no longer used.

	Justin D'Arcangelo [:justindarc]
	Comment 21 • 10 years ago

(In reply to Mike Habicher [:mikeh] from comment #19)
> (In reply to Andrew Osmond [:aosmond] from comment #14)
> 
> > jdarcangelo, looks good and works well; this will need to be committed along
> > with my fix when ready although obviously they can be put in separately
> > safely.
> 
> Justin, your change is to rate-limit zoom calls, right? That's probably a
> good approach to get this fixed, but I think long-term it makes more sense
> to make the WebAPI do the rate-limiting.

Yes, all this patch is doing is rate-limiting the `zoom` setter attribute. It would probably be good to handle it from the WebAPI if possible.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 22 • 10 years ago

Comment on attachment 8449789 [details] [diff] [review]
Throttle preview frame, v3

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

r+ from me with nits below fixed; but I'd like cpearce to okay the MediaOmxReader change.

::: content/media/omx/MediaOmxReader.cpp
@@ +78,5 @@
> +{
> +  ReleaseMediaResources();
> +  nsCOMPtr<nsIRunnable> event =
> +    NS_NewRunnableMethod(this, &MediaOmxReader::ReleaseDecoder);
> +  NS_DispatchToMainThread(event);

cpearce, is it okay that this might change the order in which ReleaseDecoder() is called?

::: dom/camera/CameraPreviewMediaStream.cpp
@@ +122,5 @@
> +    VideoFrameContainer* output = mVideoOutputs[i];
> +    output->Invalidate();
> +  }
> +}
> + 

nit: still an extra space on line 126.

@@ +141,5 @@
> +        ++mDiscardedFrames;
> +        DOM_CAMERA_LOGW("Discard preview frame %d, %d invalidation(s) pending",
> +          mDiscardedFrames, mInvalidatePending);
> +        return;
> +      } else {

Drop the else case, please, since the block above returns.

::: dom/camera/GonkCameraControl.cpp
@@ +1680,5 @@
>    nsRefPtr<RecorderProfileManager> profileMgr = GetGonkRecorderProfileManager();
>    return profileMgr.forget();
>  }
>  
> +void nsGonkCameraControl::OnRateLimitPreview(bool aLimit)

nit:

void
nsGonkCamera...
{
  ...
}

::: dom/camera/GonkCameraSource.cpp
@@ +587,5 @@
>      }
>      stopCameraRecording();
> +    if (mRateLimit) {
> +      mRateLimit = false;
> +      mCameraHw->OnRateLimitPreview(mRateLimit);

nit: just make this ->OnRateLimitPreview(false).

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 23 • 10 years ago

(In reply to Mike Habicher [:mikeh] from comment #22)
> Comment on attachment 8449789 [details] [diff] [review]
> Throttle preview frame, v3
> 
> Review of attachment 8449789 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ from me with nits below fixed; but I'd like cpearce to okay the
> MediaOmxReader change.
> 
> ::: content/media/omx/MediaOmxReader.cpp
> @@ +78,5 @@
> > +{
> > +  ReleaseMediaResources();
> > +  nsCOMPtr<nsIRunnable> event =
> > +    NS_NewRunnableMethod(this, &MediaOmxReader::ReleaseDecoder);
> > +  NS_DispatchToMainThread(event);
> 
> cpearce, is it okay that this might change the order in which
> ReleaseDecoder() is called?

That won't change the order in which ReleaseMediaResources() and OMXDecoder::ReleaseDecoder() are called, it will just make ReleaseDecoder be called later, on the main thread. Why is this change needed? Why can't we shutdown OMXDecoder on the decode thread?

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 24 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #23)
> That won't change the order in which ReleaseMediaResources() and
> OMXDecoder::ReleaseDecoder() are called, it will just make ReleaseDecoder be
> called later, on the main thread. Why is this change needed? Why can't we
> shutdown OMXDecoder on the decode thread?

Chris, with my change I end up hitting an assert failure a lot, which expected OMXDecoder to be freed on the main thread...since I don't believe I've made any changes directly, I assumed I've changed the thread scheduling enough to uncover a latent race condition.

I've attached a stack trace of the assert; hopefully that helps in evaluating if what I did is correct for all cases and not just my tests :).

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 25 • 10 years ago

Comment on attachment 8449789 [details] [diff] [review]
Throttle preview frame, v3

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

Guess I'd better r+ it then. Thanks for the stack trace.

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 26 • 10 years ago

Fix final nits requested.

try: https://tbpl.mozilla.org/?tree=Try&rev=43bc73c35e78

	vasanth Reporter
	Comment 27 • 10 years ago

(In reply to Andrew Osmond [:aosmond] from comment #26)
> Created attachment 8449939 [details] [diff] [review]
> Throttle preview frame, v3.1

Thanks Andrew & Justin!
Could you please provide a patch for mozilla/v2.0 branch which I can test?
Also did you guys verify fps is increased in Flame devices with 2 core as mentioned in description?

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 28 • 10 years ago

Removed the OMXDecoder changes as it does not look like the bits that impacted me were promoted.

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 29 • 10 years ago

Second try to actually run the emulator tests: https://tbpl.mozilla.org/?tree=Try&rev=7024b592ca44

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 30 • 10 years ago

(In reply to vasanth from comment #27)
> (In reply to Andrew Osmond [:aosmond] from comment #26)
> > Created attachment 8449939 [details] [diff] [review]
> > Throttle preview frame, v3.1
> 
> Thanks Andrew & Justin!
> Could you please provide a patch for mozilla/v2.0 branch which I can test?
> Also did you guys verify fps is increased in Flame devices with 2 core as
> mentioned in description?

Vasanth, my flame only had two cores in the first place, but I did verify the FPS and run "stop mpdecision".

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 31 • 10 years ago

Attach the correct patch this time. The previously attached patch came from bug 1030821 which is also required on v2.0 if you want to actually zoom in and out, if it hasn't been pushed into that branch yet when you test.

	Diego Marcos [:dmarcos]
	Comment 32 • 10 years ago

Comment on attachment 8449495 [details]
pull-request (master)

I put it on hold until further clarification:

1. I see some duplication of functionality: Two timers created to solve the same issue. I might be missing something.
2. We should unit these changes.

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 33 • 10 years ago

Checkin requested for throttle preview frame patch.

	vasanth Reporter
	Comment 34 • 10 years ago

(In reply to Andrew Osmond [:aosmond] from comment #31)
> Created attachment 8450354 [details] [diff] [review]
> Throttle preview frame, v1.1 for v2.0

Hi Andrew,

This patch has improved the FPS during zoom in/out(~13 now, ~7 before), 
however still the preview/recorded video is not as smooth as Android

Some reasons could be,
1. Average FPS (without Zoom in/out) it self has degraded from 1.4 to 2.0. (~25 now, ~30 before)
Do you guys seeing this?
2. Android uses Fixed FPS mode whereas FFOS uses Auto FPS mode, where FPS varies more?
3. Getting better FPS when hardware composer is disabled. Something to do with HwC? (~15)

	Justin D'Arcangelo [:justindarc]
	Comment 35 • 10 years ago

Comment on attachment 8449495 [details]
pull-request (master)

Diego: I've updated the patch so that we're only throttling in one place. Not sure what I was thinking the first time around, this makes much more sense! :-/

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 10 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/387df8864e0d

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 37 • 10 years ago

Oops updating missing tracking flags to ensure my 2.0 targeted patch (Throttle preview frame, v1.1 for v2.0) is marked for landing.

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 10 years ago

Per https://wiki.mozilla.org/Release_Management/B2G_Landing, the standard procedure is for fixes to land on mozilla-central first, then get uplifted. Also, the bug queries on that page cover uplifts, so please don't set checkin-needed additionally as it just clutters up other bug queries.

	Justin D'Arcangelo [:justindarc]
	Comment 39 • 10 years ago

Comment on attachment 8449495 [details]
pull-request (master)

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/5ddc778a7eb4deaf139471c9518a7065c1a774d4

I think we need to also land on v2.0?

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 40 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/387df8864e0d

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/c80b2f2af26e
v2.0: https://github.com/mozilla-b2g/gaia/commit/4f23d04e43135071bb0fcfd1c42059f796b6cfff

	Brogan Zumwalt [Inactive]
	Comment 42 • 10 years ago

New test case needs to be written to address bug.

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 43 • 10 years ago

Test case added in moztrap:

https://moztrap.mozilla.org/manage/case/14355/