Closed Bug 903781 Opened 8 years ago Closed 8 years ago

Mochitest - Record media with timeslice for Media Recording API

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jsmith, Assigned: jsmith)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 4 obsolete files)

Create a mochitest that records media with a timeslice provided for the media recording API.
Blocks: 889772
Depends on: 903758
Attached patch Patch v1 (obsolete) — Splinter Review
Comment on attachment 788553 [details] [diff] [review]
Patch v1

Note - this needs to wait for bug 903758 to land before this can ran without failures. Marking review though cause this runs cleanly other than that known failure.
Attachment #788553 - Flags: review?(roc)
This also will cover the mochitest request in bug 903758. I realized after the fact that I already had a patch in my queue covering the testing request in there that I forgot to upload, so I'm double checking with Randy in bug 903758 if we can use this patch to cover the mochitest request and land bug 903758 with manual testing confirmation that it works.
Comment on attachment 788553 [details] [diff] [review]
Patch v1

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

::: content/media/test/test_mediarecorder_record_timeslice.html
@@ +80,5 @@
> +    // On the last blob, we ensure ondataavailable fires before onstop
> +    } else if (dataAvailableCount === 3) {
> +      onDataAvailableFirst = true;
> +
> +    // We should never receive more than three ondataavailable events

Fix indent
Attachment #788553 - Flags: review?(roc) → review+
Assignee: nobody → jsmith
Status: NEW → ASSIGNED
Attachment #788553 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Comment on attachment 788737 [details] [diff] [review]
Patch v2

Carrying forward r+ with indentation fix.
Attachment #788737 - Flags: review+
Try run: https://tbpl.mozilla.org/?tree=Try&rev=c71fa1c45a24

Two things seen here:

- We're getting the known failure with the recording status due to the dependency not being landed yet.

- We're getting a couple of timeouts right now in a couple of cases. Looks like ondataavailable is failing to fire more than once in some cases. Randy - Any ideas why?
Flags: needinfo?(rlin)
I just add the bug 903758 patch (not landed yet) in it and 
result seems fine
https://tbpl.mozilla.org/?tree=Try&rev=3f4b3eca03ae.
Flags: needinfo?(rlin)
(In reply to Randy Lin [:rlin] from comment #8)
> I just add the bug 903758 patch (not landed yet) in it and 
> result seems fine
> https://tbpl.mozilla.org/?tree=Try&rev=3f4b3eca03ae.

The test run you just kicked off here actually shows this timeout occurring 4 times and one case where the blob data received was zero. Can you check again?
Flags: needinfo?(rlin)
Specifically, here's the test failures I'm seeing in the try run:

Test Failures

Ubuntu 12 Opt

148876 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_timeslice.html | Blob data received should be greater than zero

OS X 10.8 Debug

147545 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_timeslice.html | Test timed out.

Win 7 Debug

151921 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_timeslice.html | Test timed out. 

Win 8 Debug

151892 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_timeslice.html | Test timed out. 

Android 4.0 Opt

26662 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_timeslice.html | Test timed out.
Sorry, I should wait the full result and post it. Let me dig it out.
Given the problems on the other mochitest, I'm going to also dig into this and see if this is a test problem. Feel free to help with investigation though.
Flags: needinfo?(rlin)
I think this test case just wait two dataavailable event and fire stop method, 
In our design, we don't guarantee every timeslice has the dataavailable event.
If recorder start a little late, we may not have enough data to encode and no more dataavailable event for UA, so that cause timeout. Other zero blob event I think just hit the end of mediastream case, so no encoded data can provide. I think you can use longer opus file to test and can avoid those problems.
Attachment #788737 - Attachment is obsolete: true
Attached patch Patch v3 (obsolete) — Splinter Review
Comment on attachment 795807 [details] [diff] [review]
Patch v3

This addresses the intermittent failures I saw before.

1. Moved to mozCaptureStream is order to allow the stream to not end even if the media element ends (i.e. ondataavailable events can fire still, even with size 0 blobs).

2. Switched the blob size to check to be greater than or equal to zero. This is because the element could end before canplaythrough fires, so we could start after and fire ondataavailable events potentially with zero-sized blobs.
Attachment #795807 - Flags: review?(roc)
Try run in progress here - https://tbpl.mozilla.org/?tree=Try&rev=4fdc88d3a055. Looking good so far though.
Keywords: checkin-needed
Attachment #795807 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #797320 - Attachment is obsolete: true
Attached patch Patch v4Splinter Review
Comment on attachment 797321 [details] [diff] [review]
Patch v4

Looks like it's possible to have three ondataavailable events fire here per the backout - probably due to timing of when events fire. On that regard, we probably can't assume how many ondataavailable events we'll get in a mochitest. We can only ensure that:

* The state checks that should be always true with an event in ondataavailable are always correct (e.g. blob event, mime type correct)

* At least two ondataavailable events have fired before onstop fires. Our test will stop after one event is found, which means at a minimum, a 2nd ondataavailable event should fire.
Attachment #797321 - Flags: review?(roc)
Keywords: checkin-needed
Let me do a try run first before putting checkin-needed.
Keywords: checkin-needed
Try run is green. All this patch does is remove the check that caused the backout before, so this should go in cleanly now.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0bf1e520b9bf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Whiteboard: [qa-]
Component: Video/Audio → Video/Audio: Recording
You need to log in before you can comment on or make changes to this bug.