1187364 - [Gecko][Camera] Pause and Resume support during video recording

Status: RESOLVED FIXED

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

Description

[Andrew Osmond [:aosmond] (he/him)]

See bug 1182949 for details. This bug tracks the Gecko patch needed to expose pause/resume functionality for video recording.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Add ability for camera to pause/resume recording -- v1

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8638608 [details] [diff] [review]
Add ability for camera to pause/resume recording -- v1

bz: Can you review the WebIDL? Thanks!

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8638608 [details] [diff] [review]
Add ability for camera to pause/resume recording -- v1

Should probably document in the IDL what happens if resumeRecording() is called while not paused or pauseRecording() called while paused.

r=me with that

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Add ability for camera to pause/resume recording -- v2 [r=bz]

Updated based on bz's feedback.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4dc721a572e

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 8638669 [details] [diff] [review]
Add ability for camera to pause/resume recording -- v2 [r=bz]

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

Looks reasonable to me,.

Comment 6

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

In addition to pause / restart MPEG4Writer, we also need to ensure the first coming frame is I-frame after resume, otherwise there will be broken frames observed. There are two options:

1) request a I-frame after resume and drop the first couple P/B frame in MPEG4Writer. --> Fewer change but codec is still running during pause.

2) pause camerasource & audiosource & videoencoder & audioencoder --> This involve more changes in Gonk but lead to better power consumption saving during pause.

Comment 7

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: request_I_frame_after_resume.patch

I come out a first draft which demands I frame after resume.
Sotaro, could you kindly help review if this is feasible?

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Munro Chiang [:munro_chiang] from comment #7)
> Created attachment 8643653 [details] [diff] [review]
> request_I_frame_after_resume.patch
> 
> I come out a first draft which demands I frame after resume.
> Sotaro, could you kindly help review if this is feasible?

It might be better to handle this problem as a different bug to make clear the problem's fix and discussions. Logically this fix is necessary. But I am not sure how much the fix is necessary. android AOSP and android caf's latest code does not care about this problem. Is there a already a bug or info that show seriousness of the problem?

I am not fun of modifying the OMXCodec. Android does not use OMXCodec for recording since lollipop. Modifying the OMXCodec seems not good for code maintenance point of view. If we want to manually request iframe, it seems better to use MediaCodec. The MediaCodec is wrapped in MediaCodecSource.
  https://github.com/sotaroikeda/android-diagrams/blob/master/media/MediaCodecSource_5.1.pdf
Using MediaCodec request more changes than attachment 8643653 [details] [diff] [review]. But it could reduce source code maintenance cost.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Munro Chiang [:munro_chiang] from comment #7)
> Created attachment 8643653 [details] [diff] [review]
> request_I_frame_after_resume.patch

GonkRecorder requests encoder "1 sec i-frame-interval". Therefore it is not clear how much we could get from requestIframe().

Comment 10

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: VID_0010.3gp

Comment 11

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Is there a already a bug or info that show seriousness of the problem?

Yes, I apply attachment 8638669 [details] [diff] [review] to Flame and recorded the broken frame issue. Please check attachment 8644733 [details].

> 
> I am not fun of modifying the OMXCodec. Android does not use OMXCodec for
> recording since lollipop. Modifying the OMXCodec seems not good for code
> maintenance point of view. If we want to manually request iframe, it seems
> better to use MediaCodec. The MediaCodec is wrapped in MediaCodecSource.
>  
> https://github.com/sotaroikeda/android-diagrams/blob/master/media/
> MediaCodecSource_5.1.pdf
> Using MediaCodec request more changes than attachment 8643653 [details] [diff] [review]
> [diff] [review]. But it could reduce source code maintenance cost.
Agree. we can try to replace OMXCodec with mediacodec. This feature ETA is around 9/25 according to

Comment 12

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Is there a already a bug or info that show seriousness of the problem?

Yes, I apply attachment 8638669 [details] [diff] [review] to Flame and recorded the broken frame issue. Please check attachment 8644733 [details].

> 
> I am not fun of modifying the OMXCodec. Android does not use OMXCodec for
> recording since lollipop. Modifying the OMXCodec seems not good for code
> maintenance point of view. If we want to manually request iframe, it seems
> better to use MediaCodec. The MediaCodec is wrapped in MediaCodecSource.
>  
> https://github.com/sotaroikeda/android-diagrams/blob/master/media/
> MediaCodecSource_5.1.pdf
> Using MediaCodec request more changes than attachment 8643653 [details] [diff] [review]
> [diff] [review]. But it could reduce source code maintenance cost.
Agree. we can try to replace OMXCodec with mediacodec.

Comment 13

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

After comment 6, it seems there are more modifications to add.
This feature is targeted for red-tai so we would need it to complete (on 2.2r) before Sep-End.
If necessary, I think we should consider having a smaller patch for 2.2r and a more profound solution on m-c in order to meet the timeline.
Anyways, it depends on the ETA. Maybe we can adopt same solution to both 2.2r and m-c. 
Can you update the "Target milestone"?

Comment 14

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> (In reply to Munro Chiang [:munro_chiang] from comment #7)
> > Created attachment 8643653 [details] [diff] [review]
> > request_I_frame_after_resume.patch
> 
> GonkRecorder requests encoder "1 sec i-frame-interval". Therefore it is not
> clear how much we could get from requestIframe().

The command OMX_IndexConfigVideoIntraVOPRefresh would force codec to output a I-frame (ignore I-frame interval setting). In my experiment log, it take around 3 frames to get one I-frame. Without this patch, it may take 23~29 frames to get one I-frame in worst case depending on camera FPS.

Comment 15

[Sotaro Ikeda [:sotaro]]

Created Bug 119231 for using MediaCodecSource for camera recording.

Comment 16

[Sotaro Ikeda [:sotaro]]

(In reply to Munro Chiang [:munro_chiang] from comment #14)
> (In reply to Sotaro Ikeda [:sotaro] from comment #9)
> > (In reply to Munro Chiang [:munro_chiang] from comment #7)
> > > Created attachment 8643653 [details] [diff] [review]
> > > request_I_frame_after_resume.patch
> > 
> > GonkRecorder requests encoder "1 sec i-frame-interval". Therefore it is not
> > clear how much we could get from requestIframe().
> 
> The command OMX_IndexConfigVideoIntraVOPRefresh would force codec to output
> a I-frame (ignore I-frame interval setting). In my experiment log, it take
> around 3 frames to get one I-frame. Without this patch, it may take 23~29
> frames to get one I-frame in worst case depending on camera FPS.

Thanks for checking! For a new b2g phone, explicit iframe request seems necessary.

Comment 17

[Sotaro Ikeda [:sotaro]]

diego, do you know why MPEG4Writer does not resume from pause with i-frame on both android aosp and android caf?

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Munro Chiang [:munro_chiang] from comment #10)
> Created attachment 8644733 [details]
> VID_0010.3gp

Thanks for the video. Yeah, it looks bad.

Comment 19

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Ensure recording resumed with key frame -- v1

Even if doing this on L is acceptable (I'm not sure, we probably need to ship this on KK?), I don't see anything in MediaCodecSource will ensures the key frames come in on pause / resume (via ::pause() and ::start() with seek time/mode).

I already have a pending change to MPEG4Writer from over a year ago that has yet to be acknowledged upstream, and I'd like to stay out of that boat if possible given out tight deadline :). Are we certain we can't resolve this acceptably without touching the Android frameworks?

I've put together something which does everything in Gecko. By wrapping the encoder MediaSource object, I can intercept the ::read calls to ensure that a key frame is read after resuming. We could possibly enhance this to avoid the encoding as well. The "resume" signal from Gecko to the app isn't sent until the key frame is read, so there won't be an early UI transition.

If you want minimal app support, you can test with the attached gaia pull request in the parent bug; touch to focus during recording to pause/resume.

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Munro Chiang [:munro_chiang] from comment #14)
> (In reply to Sotaro Ikeda [:sotaro] from comment #9)
> > (In reply to Munro Chiang [:munro_chiang] from comment #7)
> > > Created attachment 8643653 [details] [diff] [review]
> > > request_I_frame_after_resume.patch
> > 
> > GonkRecorder requests encoder "1 sec i-frame-interval". Therefore it is not
> > clear how much we could get from requestIframe().
> 
> The command OMX_IndexConfigVideoIntraVOPRefresh would force codec to output
> a I-frame (ignore I-frame interval setting). In my experiment log, it take
> around 3 frames to get one I-frame. Without this patch, it may take 23~29
> frames to get one I-frame in worst case depending on camera FPS.

If 1 second is the upper bound, it would be nice to reduce that time, but I don't believe that is a blocking condition. I do agree it looks much better if we wait for a sync frame when resuming and we should block on that :).

Comment 21

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8648127 [details] [diff] [review]
Part 2. Ensure recording resumed with key frame -- v1

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

The whole patch is good to me except that, do we really need to pass cameraSource as the parameter in WrappedMediaSource constructor?
I didn't see mCameraSource used anywhere.

> Are we certain we can't resolve this acceptably without touching the Android frameworks?
Currently Gonk ACodec / OMXCodec doesn't use this command OMX_IndexConfigVideoIntraVOPRefresh.
There is also no interface which allows GonkRecorder to control mOMX directly.
I am afraid touching Android source code is the only way....

Comment 22

[Diego Wilson [:diego]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> diego, do you know why MPEG4Writer does not resume from pause with i-frame
> on both android aosp and android caf?

AFAICT you can't pause while recording a video on Android. My guess is this is just an untested case.

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Add ability for camera to pause/resume recording -- v2.1 [carries r=dhylands,bz]

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Ensure recording resumed with key frame -- v2 [carries r=mchiang]

(In reply to Munro Chiang [:munro_chiang] from comment #21)
> Comment on attachment 8648127 [details] [diff] [review]
> Part 2. Ensure recording resumed with key frame -- v1
> 
> Review of attachment 8648127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The whole patch is good to me except that, do we really need to pass
> cameraSource as the parameter in WrappedMediaSource constructor?
> I didn't see mCameraSource used anywhere.
> 

Ah thanks, I had a different approach initially but forgot to take that bit out. Removed.

> > Are we certain we can't resolve this acceptably without touching the Android frameworks?
> Currently Gonk ACodec / OMXCodec doesn't use this command
> OMX_IndexConfigVideoIntraVOPRefresh.
> There is also no interface which allows GonkRecorder to control mOMX
> directly.
> I am afraid touching Android source code is the only way....

Yes, that's to force the key frame out earlier to shorten the wait time. I see that as an optimization, and certainly a desirable one, but not a blocker. We should track that in a new bug and land these two parts.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/5aa09eb27b71
https://hg.mozilla.org/integration/b2g-inbound/rev/93a9f980f5ba

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5aa09eb27b71
https://hg.mozilla.org/mozilla-central/rev/93a9f980f5ba

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.2r uplift.

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [b2g37_v2_2r] Part 1. Add ability for camera to pause/resume recording [carries r=dhylands,bz]

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [b2g37_v2_2r] Part 2. Ensure recording resumed with key frame [carries r=mchiang]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a0d2161cddd

Comment 30

[Jean-Yves Perrier [:teoli]]

Andrew, I wonder if patch 1 has not unexpectedly removed onrecorderstatechange because of a missing end of comment (*/)
/* the event dispatched when the recorder changes state, either because
248      the recording process encountered an error, or because one of the
249      recording limits (see CameraStartRecordingOptions) was reached.
…
263              'MediaRecorderFailed' if failed due to local error
264              'MediaServerFailed' if failed due to media server
265   attribute EventHandler    onrecorderstatechange;

Comment 31

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Fix missing end comment in WebIDL

I'm not entirely sure how it works with that end comment missing. The camera recording doesn't seem broken in the latest build! (Which is how I missed it.)

Comment 32

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [b2g37_v2_2r] Part 1. Add ability for camera to pause/resume recording [carries r=dhylands,bz]

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/070bbe2d7cf7

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/70e839cda5c0
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/5f4343ab8b28

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/070bbe2d7cf7

Comment 36

[Chris Mills [:cmills]]

I have added ref pages covering these new methods:

https://developer.mozilla.org/en-US/docs/Web/API/CameraControl/pauseRecording

And put an entry in the latest release notes for FxOS:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Releases/2.5#Web_API_changes

A quick tech review would be nice - cheers!

Comment 37

[Chris Mills [:cmills]]

Forgot one:

https://developer.mozilla.org/en-US/docs/Web/API/CameraControl/resumeRecording