1008797 - Test case for bug 993598.

Status: NEW

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

Description

[Benjamin Chen [:bechen]]

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...)

Comment 1

[Benjamin Chen [:bechen]]

Fake GUM sample.
http://dxr.mozilla.org/mozilla-central/source/content/media/test/test_mediarecorder_record_gum_video_timeslice.html?from=test_mediarecorder_record_gum_video_timeslice.html

httpserver sample.
http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js

Comment 2

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

Comment 3

[Benjamin Chen [: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?

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8433967 [details] [diff] [review]
Patch v1.0

Hi Jason, 
Would you please give some feedback or review this patch?

Comment 5

[Jason Smith [: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?

Comment 6

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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.

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1

1. Rebase the patch. 
2. address the review comments. 
3. You can specify sync or async method in XMLHttpRequest.open when calling XMLHttpRequest.send.

Comment 8

[Benjamin Chen [: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.

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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

Comment 10

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.