1197669 - Enable test_mediarecorder_record_gum_video_timeslice.html on B2G platform.

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect)

Comment 1

[Benjamin Chen [:bechen]]

Now I'm fixing the testcase for b2g, during the fix, I found a timeout issue that the the js is waiting for the onstop event. It might be the same failure at bug 1149842 comment 42.

Comment 2

[Benjamin Chen [:bechen]]

After discussed with John, we have confirmed that there is a bug in OMXTrackEncoderccpp. The EOS signal may not send into MediaCodec because the dequeue/enqueue buffer may be timeout. I'll fix it too.

Comment 3

[Benjamin Chen [:bechen]]

Attachment: bug-1197669-part1.v01.patch

1. enable the testcase on b2g/android platform.
2. check the mimetype if the blob is not empty.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: bug-1197669-part2.v01.patch

1. Ensure the EOS is sent to MediaCodec. Change the |OMXAudioEncoder::Encode| and |OMXVideoEncoder::Encode|
2. mEndOfStream should be protected by monitor.

Comment 5

[Benjamin Chen [:bechen]]

Attachment: bug-1197669-part3.v01.patch

In MediaRecorder, ensure the start event comes before any blobs.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8652745 [details] [diff] [review]
bug-1197669-part1.v01.patch

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

::: dom/media/test/test_mediarecorder_record_gum_video_timeslice.html
@@ +41,5 @@
>           'Event type should dataavailable');
>        ok(evt.data.size >= 0,
> +         'Blob data size ' + evt.data.size + ' received is greater than or equal to zero');
> +      if (evt.data.size > 0) {
> +        is(mediaRecorder.mimeType, evt.data.type,

Why do we check mediaRecorder.mimeType only when evt.data.size > 0?

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8652749 [details] [diff] [review]
bug-1197669-part3.v01.patch

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

::: dom/media/MediaRecorder.cpp
@@ +498,3 @@
>            NS_DispatchToMainThread(
>              new DispatchStartEventRunnable(this, NS_LITERAL_STRING("start")));
> +          mIsStartEventFired = true;

It is a data race for mIsStartEventFired is accessed on multiple threads.

Comment 8

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #7)
> Comment on attachment 8652749 [details] [diff] [review]
> bug-1197669-part3.v01.patch
> 
> Review of attachment 8652749 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaRecorder.cpp
> @@ +498,3 @@
> >            NS_DispatchToMainThread(
> >              new DispatchStartEventRunnable(this, NS_LITERAL_STRING("start")));
> > +          mIsStartEventFired = true;
> 
> It is a data race for mIsStartEventFired is accessed on multiple threads.

It won't have data race, because |DoSessionEndTask| on mian thread and |Session::Extract| on encoder thread wont exist at the same time. |DoSessionEndTask| only be called when something wrong so there is no |ExtractRunnable| running.

Comment 9

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #6)
> Comment on attachment 8652745 [details] [diff] [review]
> bug-1197669-part1.v01.patch
> 
> Review of attachment 8652745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/test/test_mediarecorder_record_gum_video_timeslice.html
> @@ +41,5 @@
> >           'Event type should dataavailable');
> >        ok(evt.data.size >= 0,
> > +         'Blob data size ' + evt.data.size + ' received is greater than or equal to zero');
> > +      if (evt.data.size > 0) {
> > +        is(mediaRecorder.mimeType, evt.data.type,
> 
> Why do we check mediaRecorder.mimeType only when evt.data.size > 0?

Good question, since we ensure the start event comes before blob, we can also ensure the mediaRecorder.mimeType won't be empty because the mediaRecorder.mimeType was set in |DispatchStartEventRunnable|. So we should able to check mediaRecorder.mimeType even the blob size is 0.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Benjamin Chen [:bechen] from comment #8)
> It won't have data race, because |DoSessionEndTask| on mian thread and
> |Session::Extract| on encoder thread wont exist at the same time.
> |DoSessionEndTask| only be called when something wrong so there is no
> |ExtractRunnable| running.

Data races are also about memory visibility which is how a change in a memory location in a thread becomes visible to another thread.

Comment 11

[Benjamin Chen [:bechen]]

Attachment: bug-1197669-part1.v01.patch

Check the blob's mimetype even it is an empty blob.

Comment 12

[Benjamin Chen [:bechen]]

Attachment: bug-1197669-part2.v01.patch

Add comments.

Comment 13

[Benjamin Chen [:bechen]]

Attachment: bug-1197669-part3.v01.patch

Address comment 7, 8, 10.

See comment 8, since the |DoSessionEndTask| and |ExtractRunnable| are exclusive, so I can remove |mIsStartEventFired| checking on main thread.

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8653317 [details] [diff] [review]
bug-1197669-part2.v01.patch

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

Not familiar with omx code. Maybe Sotaro should review it.

Comment 15

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8653317 [details] [diff] [review]
bug-1197669-part2.v01.patch

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

Review+ as a short term fix. For a long term, it seems better to fix TrackEncoder's implementation and MediaCodec handling. About MediaCodec handling, it might be better to use kWhatEncoderActivity message to get free input buffer like MediaCodecSource.

FYI: MediaCodecSource diagram.
  https://github.com/sotaroikeda/android-diagrams/blob/master/media/MediaCodecSource_5.1.pdf

Comment 16

[Benjamin Chen [:bechen]]

Attachment: bug-1197669-part2.v01.patch

r=sotaro.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba9e1d699bba

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59abc20bd941
https://hg.mozilla.org/integration/mozilla-inbound/rev/1201c1eb56ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac96586283d6

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/59abc20bd941
https://hg.mozilla.org/mozilla-central/rev/1201c1eb56ea
https://hg.mozilla.org/mozilla-central/rev/ac96586283d6