Open Bug 1008797 Opened 7 years ago Updated 3 years ago

Test case for bug 993598.

Categories

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

defect

Tracking

()

People

(Reporter: bechen, Assigned: vchang)

References

Details

Attachments

(1 file, 1 obsolete file)

We may need test case for bug 993598.
In c++ unit test level, we can use gtest test the nsTemporaryFileInputStream.

In mochitest, we can start a http server(@mozilla.org/server/jshttp) and do some network operations to blob generated by MediaRecorder. (uploard, redirect etc...)
Assignee: nobody → vchang
Attached patch Patch v1.0 (obsolete) — Splinter Review
Attachment #8433967 - Flags: feedback?(bechen)
Comment on attachment 8433967 [details] [diff] [review]
Patch v1.0

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

::: content/media/test/mochitest.ini
@@ +498,5 @@
>  [test_fragment_noplay.html]
>  run-if = wave
>  [test_fragment_play.html]
>  run-if = wave
> +[test_bug1008797.html]

Skip the testcase on b2g and android platform due to the webm format.
For b2g, it should be mp4 format and opus for android.

::: content/media/test/test_bug1008797.html
@@ +99,5 @@
> +
> +function uploadVideo(data) {
> +  var req = new XMLHttpRequest();
> +  req.open('POST', 'validateMediaRecorderBlob.sjs', false);
> +  req.send(data);

Is the send method synchronous or asynchronous?
Attachment #8433967 - Flags: feedback?(bechen)
Comment on attachment 8433967 [details] [diff] [review]
Patch v1.0

Hi Jason, 
Would you please give some feedback or review this patch?
Attachment #8433967 - Flags: review?(jsmith)
Comment on attachment 8433967 [details] [diff] [review]
Patch v1.0

I'm really slammed with a lot of 2.0 work lately.

Randy - Can you do the review here?
Attachment #8433967 - Flags: review?(jsmith) → review?(rlin)
Comment on attachment 8433967 [details] [diff] [review]
Patch v1.0

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

::: content/media/test/mochitest.ini
@@ +498,5 @@
>  [test_fragment_noplay.html]
>  run-if = wave
>  [test_fragment_play.html]
>  run-if = wave
> +[test_bug1008797.html]

Rename file name start with test_mediarecorder_XXXXX.html.
How about test_mediarecorder_vaild_httppostdata.html?

::: content/media/test/test_bug1008797.html
@@ +40,5 @@
> +      ok(false, 'onerror unexpectedly fired');
> +    };
> +
> +    mediaRecorder.onstop = function() {
> +      ok(false, 'Unexpected onstop callback fired');

You have another onstop eventhandler @ line 64, please remove this one.

@@ +63,5 @@
> +      if (dataAvailableCount === 1) {
> +        mediaRecorder.onstop = function (evt) {
> +          info('onstop fired');
> +
> +          if (!onDataAvailableFirst) {

Check the dataAvailableCount directly, it looks this flag can be set when received at least two ondataavailable event.

@@ +67,5 @@
> +          if (!onDataAvailableFirst) {
> +            ok(false, 'onstop unexpectedly fired before ondataavailable');
> +          }
> +
> +          ok(true, 'onstop fired successfully');

Remove this.

@@ +70,5 @@
> +
> +          ok(true, 'onstop fired successfully');
> +          is(mediaRecorder.state, 'inactive',
> +             'check recording status is inactive');
> +          SimpleTest.finish();

Finish this test when the blob content has been verified.

@@ +78,5 @@
> +        is(mediaRecorder.state, 'inactive',
> +           'Media recorder is inactive after being stopped');
> +
> +        ok(evt.data.size >= MAX_BLOB_THRESHOLD,
> +           'Blob data size received is greater than blob threshold' + evt.data.size);

How do you choose the MAX_BLOB_THRESHOLD?
Does the size of encoded data always larger than this?

@@ +80,5 @@
> +
> +        ok(evt.data.size >= MAX_BLOB_THRESHOLD,
> +           'Blob data size received is greater than blob threshold' + evt.data.size);
> +
> +        uploadVideo(evt.data);

Could we check this when finish the media recording task?
The blob can be checked in detail at validateMediaRecorderBlob.sjs.
Attached patch Patch v1.1Splinter Review
1. Rebase the patch. 
2. address the review comments. 
3. You can specify sync or async method in XMLHttpRequest.open when calling XMLHttpRequest.send.
Attachment #8433967 - Attachment is obsolete: true
Attachment #8538396 - Flags: review?(rlin)
Attachment #8538396 - Flags: review?(bechen)
Comment on attachment 8538396 [details] [diff] [review]
Patch v1.1

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

Since the blob has the mimetype attribute, we can detect the minetype to see if it is a webm or mp4 blob, then pass it to httpserver to verify the blob. So that we turn on the testcase on b2g platform too.
Attachment #8538396 - Flags: review?(bechen)
Comment on attachment 8538396 [details] [diff] [review]
Patch v1.1

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

Thanks, LGTM.

::: dom/media/test/mochitest.ini
@@ +540,5 @@
>  run-if = wave
>  [test_fragment_play.html]
>  run-if = wave
> +[test_mediarecorder_vaild_httppostdata.html]
> +skip-if = buildapp == 'b2g' || toolkit == 'android'

Please have a follow bug if you want to skip on b2g.

::: dom/media/test/test_mediarecorder_vaild_httppostdata.html
@@ +59,5 @@
> +      if (dataAvailableCount === 1) {
> +        mediaRecorder.onstop = function (evt) {
> +          info('onstop fired');
> +
> +          // Ensure we've received at least two ondataavailable events 

space
Attachment #8538396 - Flags: review?(rlin) → review+
Rank: 45
Priority: -- → P4
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
You need to log in before you can comment on or make changes to this bug.