Closed Bug 1079616 Opened 10 years ago Closed 10 years ago

Intermittent test_mediarecorder_record_getdata_afterstart.html | check blob has data

Categories

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

34 Branch
All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: KWierso, Assigned: bechen)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 2 obsolete files)

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=841783&repo=fx-team
builder 	b2g_emulator_vm fx-team opt test mochitest-3
buildid 	20141007121318
builduid 	2516fd3c98a4447b9978ccad65a83fc4
results 	warnings (1)
revision 	fda842d4d3729a1d40eb6fa02c4307fd321c938a
slave 	tst-linux64-spot-983
starttime 	Tue Oct 07 2014 13:56:38 GMT-0700 (Pacific Standard Time)



14:32:49 INFO - 305 INFO TEST-START | /tests/content/media/test/test_mediarecorder_record_getdata_afterstart.html
14:32:57 INFO - 306 INFO Started Tue Oct 07 2014 21:32:52 GMT+0000 (UTC) (1412717572.834s)
14:32:57 INFO - 307 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_getdata_afterstart.html | [started detodos.opus-0] Length of array should match number of running tests
14:32:57 INFO - 308 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_getdata_afterstart.html | Media recorder should be recording
14:32:57 INFO - 309 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_getdata_afterstart.html | Media recorder stream = element stream at the start of recording
14:32:57 INFO - 310 INFO onstart fired successfully
14:32:57 INFO - 311 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_getdata_afterstart.html | check the record mimetype return audio/ogg
14:32:57 INFO - 312 INFO ondataavailable fired successfully
14:32:57 INFO - 313 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_getdata_afterstart.html | should has onstart event first
14:32:57 INFO - 314 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_getdata_afterstart.html | check blob has data - expected PASS
14:32:58 INFO - 315 INFO TEST-OK | /tests/content/media/test/test_mediarecorder_record_getdata_afterstart.html | took 8957ms
14:32:59 INFO - 316 INFO TEST-START | /tests/content/media/test/test_mediarecorder_record_immediate_stop.html
14:33:09 INFO - 317 INFO TEST-OK | /tests/content/media/test/test_mediarecorder_record_immediate_stop.html | took 10243ms
14:33:10 INFO - 318 INFO TEST-START | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html
Assignee: nobody → bechen
Found root cause, the MediaRecorder::RequestData should not call mSessions::GetEncodedData() immediately.

It is possible the task queue of mainthread is:
Task 1: DispatchStartEventRunnable event 
Task 2: pushblob event (triggered by end of stream)

When the mainthread execute Task 1, fire onstart event then call MediaRecorder::RequestData, it will
extract all the data then dispatch a new task |CreateAndDispatchBlobEventRunnable|.
So the task queue now is:
Task 1: DispatchStartEventRunnable event ==> finished
Task 2: pushblob event (triggered by end of stream)
Task 3: CreateAndDispatchBlobEventRunnable

Since the data all extracted to Task 3, the Task 2 will dispatch an empty blob => testcase fail.
Attached patch bug-1079616.v01.patch (obsolete) — Splinter Review
Attachment #8505362 - Flags: feedback?(rlin)
Attachment #8505362 - Flags: feedback?(jwwang)
Comment on attachment 8505362 [details] [diff] [review]
bug-1079616.v01.patch

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

::: content/media/MediaRecorder.cpp
@@ +819,2 @@
>  void
>  MediaRecorder::RequestData(ErrorResult& aResult)

I think we can just delegate the job to the session as we do in MediaRecorder::Resume() which calls Session::Resume(). Then MediaRecorder won't have to know about the inner class of Session::PushBlobRunnable.

::: content/media/test/test_mediarecorder_record_getdata_afterstart.html
@@ +53,5 @@
>      info('ondataavailable fired successfully');
>      if (mMediaRecorder.state == 'recording') {
>        hasondataavailable = true;
>        ok(hasonstart, "should has onstart event first");
> +      dump("data size "+ e.data.size);

Why not SimpleTest.info?
Attachment #8505362 - Flags: feedback?(jwwang)
Attached patch bug-1079616.v01.patch (obsolete) — Splinter Review
Address comment 5.
Attachment #8505362 - Attachment is obsolete: true
Attachment #8505362 - Flags: feedback?(rlin)
Comment on attachment 8505946 [details] [diff] [review]
bug-1079616.v01.patch

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

Looks good to me.
Attachment #8505946 - Flags: feedback+
r=roc
Attachment #8505946 - Attachment is obsolete: true
Attachment #8506793 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/82eb08fa0885
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Please request Aurora approval on this when you get chance. Also beta if the underlying defect is present there as well.
status-b2g-v2.1: --- → ?
Flags: needinfo?(bechen)
Comment on attachment 8506793 [details] [diff] [review]
bug-1079616.v01.patch

The patch can clean apply to mozilla-aurora.

Approval Request Comment
[Feature/regressing bug #]: 1079616
[User impact if declined]: The media recorder's blobs might be fired to JS in wrong sequence.
[Describe test coverage new/current, TBPL]: current, test_mediarecorder_record_getdata_afterstart.html
[Risks and why]: low
[String/UUID change made/needed]: none
Flags: needinfo?(bechen)
Attachment #8506793 - Flags: approval-mozilla-aurora?
Mozilla-beta patch.

Approval Request Comment
[Feature/regressing bug #]: 1079616
[User impact if declined]: The media recorder's blobs might be fired to JS in wrong sequence.
[Describe test coverage new/current, TBPL]: current, test_mediarecorder_record_getdata_afterstart.html
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8510791 - Flags: approval-mozilla-beta?
(In reply to Benjamin Chen [:bechen] from comment #13)
> Comment on attachment 8506793 [details] [diff] [review]
> bug-1079616.v01.patch
> 
> The patch can clean apply to mozilla-aurora.
> 
> Approval Request Comment
> [Feature/regressing bug #]: 1079616

Bug 1079616 is this bug. In which bug was this issue introduced? i.e. How far back does the issue go?
Flags: needinfo?(bechen)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #15)
> (In reply to Benjamin Chen [:bechen] from comment #13)
> > Comment on attachment 8506793 [details] [diff] [review]
> > bug-1079616.v01.patch
> > 
> > The patch can clean apply to mozilla-aurora.
> > 
> > Approval Request Comment
> > [Feature/regressing bug #]: 1079616
> 
> Bug 1079616 is this bug. In which bug was this issue introduced? i.e. How
> far back does the issue go?

This bug exists from the beginning of the MediaRecorder module. 
As I known, there is no other bugs were caused by this.
Flags: needinfo?(bechen)
Comment on attachment 8510791 [details] [diff] [review]
bug-1079616.beta.patch

Beta+
Attachment #8510791 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8506793 [details] [diff] [review]
bug-1079616.v01.patch

Aurora+
Attachment #8506793 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.