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

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

({dev-doc-complete})

unspecified
FxOS-S5 (21Aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2r+, firefox43 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

(Whiteboard: [red-tai])

Attachments

(7 attachments, 4 obsolete attachments)

3.29 KB, patch
Details | Diff | Splinter Review
2.58 MB, video/3gpp
Details
22.79 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
9.45 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
9.51 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
1.49 KB, patch
Details | Diff | Splinter Review
18.13 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
See bug 1182949 for details. This bug tracks the Gecko patch needed to expose pause/resume functionality for video recording.
Assignee: nobody → aosmond
Blocks: 1182949
Group: tai-confidential
Status: NEW → ASSIGNED
Whiteboard: [red-tai]
Attachment #8638608 - Flags: review?(dhylands)
Target Milestone: --- → FxOS-S4 (07Aug)
Comment on attachment 8638608 [details] [diff] [review]
Add ability for camera to pause/resume recording -- v1

bz: Can you review the WebIDL? Thanks!
Attachment #8638608 - Flags: review?(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
Attachment #8638608 - Flags: review?(dhylands) → review+
Attachment #8638608 - Flags: review?(bzbarsky) → review?(dhylands)
Updated based on bz's feedback.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4dc721a572e
Attachment #8638608 - Attachment is obsolete: true
Attachment #8638608 - Flags: review?(dhylands)
Attachment #8638669 - Flags: review?(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,.
Attachment #8638669 - Flags: review?(dhylands) → review+
feature-b2g: --- → 2.2r?
feature-b2g: 2.2r? → 2.2r+
Group: tai-confidential
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.
I come out a first draft which demands I frame after resume.
Sotaro, could you kindly help review if this is feasible?
Flags: needinfo?(sotaro.ikeda.g)
(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.
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(mchiang)
(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().
(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
Flags: needinfo?(mchiang)
(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.
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"?
(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.
Depends on: 1192312
Created Bug 119231 for using MediaCodecSource for camera recording.
(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.
diego, do you know why MPEG4Writer does not resume from pause with i-frame on both android aosp and android caf?
Flags: needinfo?(dwilson)
(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.
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.
Attachment #8648127 - Flags: review?(mchiang)
(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 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....
Attachment #8648127 - Flags: review?(mchiang) → review+
(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.
Flags: needinfo?(dwilson)
(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.
Attachment #8648127 - Attachment is obsolete: true
Attachment #8648890 - Flags: review+
Target Milestone: FxOS-S4 (07Aug) → FxOS-S5 (21Aug)
Needs rebasing for v2.2r uplift.
Flags: needinfo?(aosmond)
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;
Flags: needinfo?(aosmond)
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.)
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!
You need to log in before you can comment on or make changes to this bug.