969289 - Handle image null pointer in VP8TrackEncoder.

Status: RESOLVED FIXED

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

Description

[Benjamin Chen [:bechen]]

The image pointer may be null in video chunk. (At the beginning of MSG?)
We should treat it as a mute frame instead return an error.

http://dxr.mozilla.org/mozilla-central/source/content/media/encoder/VP8TrackEncoder.cpp#247

Comment 1

[Benjamin Chen [:bechen]]

Attachment: bug-969289.patch

Hi, Randy:
please help to try this patch on your local test case.

Comment 2

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

This patch pass my test.

Comment 3

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

Please also add my test script in the test case. 
note: should take care of the b2g video mimeType.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: record_timeslice.patch

This test case failed as expected on the mimetype checking.

1568 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_video_timeslice.html | check the record mimetype return audio/ogg - got audio/ogg, expected video/webm
02-12 08:38:54.863 713 713 I GeckoDump: 1568 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_video_timeslice.html | check the record mimetype return audio/ogg - got audio/ogg, expected video/webm

Comment 5

[Benjamin Chen [:bechen]]

Attachment: record_timeslice.patch

1. Run the test case only on destop build since the mimetype is webm
(skip-if = os == "android" || appname == "b2g")

try server:
https://tbpl.mozilla.org/?tree=Try&rev=cb221d9c1da3

Hi Jason:
Would you please help tp review this test case?

Comment 6

[Benjamin Chen [:bechen]]

Comment on attachment 8373121 [details] [diff] [review]
bug-969289.patch

Hi Ralph:
Please help to review this patch, thanks.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Benjamin Chen [:bechen] from comment #5)
> Created attachment 8378757 [details] [diff] [review]
> record_timeslice.patch
> 
> 1. Run the test case only on destop build since the mimetype is webm
> (skip-if = os == "android" || appname == "b2g")

WebM support should be cross-platform, so this test should theoretically work on all platforms. Can you try testing this on all platforms?

Comment 8

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

No, on b2g platform, The media recorder would output mp4 video file format.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #8)
> No, on b2g platform, The media recorder would output mp4 video file format.

Umm...why? What makes the WebM encoder desktop-specific?

Comment 10

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

The major problem is we don't have the supported vorbis audio encoder on b2g platform. So we can't output WebM as well.

Comment 11

[Ralph Giles (:rillian)]

(In reply to Randy Lin [:rlin] from comment #10)
> The major problem is we don't have the supported vorbis audio encoder on b2g
> platform. So we can't output WebM as well.

I agree with jsmith that we should fix this. Writing VP8+Opus as WebM would be a good start. That's tracked under bug 904363.

I also opened bug 974998 to implement my suggestion of using floating-point libvorbisenc even on mobile. It will be slow, but I'd like to see a demonstration that it's unusably slow before we abandon that option.

Comment 12

[Jason Smith [:jsmith]]

Comment on attachment 8378757 [details] [diff] [review]
record_timeslice.patch

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

review- - there's some additional checks needed here & I'm concerned this could intermittently fail.

Also - make sure you include a commit message here.

::: content/media/test/mochitest.ini
@@ +259,5 @@
>  [test_mediarecorder_record_session.html]
>  [test_mediarecorder_record_startstopstart.html]
>  [test_mediarecorder_getencodeddata.html]
>  [test_mediarecorder_unsupported_src.html]
> +[test_mediarecorder_record_video_timeslice.html] skip-if = os == "android" || appname == "b2g"

Nit - I'd prefer that we would put the mochitest preffed off in the b2g.json & android.json files in testing/mochitest instead of doing this, as that's the main set of files our mochitest porting team member watches

::: content/media/test/test_mediarecorder_record_video_timeslice.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test MediaRecorder Record Stopped Stream</title>

Nit - I think we should call the filename "test_mediarecorder_record_gum_video_timeslice.html" to indicate that this is using getUserMedia. We should have the title reflect this as well with Test MediaRecorder Record gUM video with Timeslice.

@@ +10,5 @@
> +<div id="content" style="display: none">
> +</div>
> +<script class="testbody" type="text/javascript">
> +
> +function startTest() {

If possible, it might be worthwhile to allow this test to run with video only & video & audio together (i.e. repeat the test with each of these sets of gUM constraints).

@@ +11,5 @@
> +</div>
> +<script class="testbody" type="text/javascript">
> +
> +function startTest() {
> +  navigator.mozGetUserMedia({audio: true, video:true, fake: true}, function(stream) {

Nit - insert a space between video: & true

@@ +23,5 @@
> +    mediaRecorder.onerror = function() {
> +      ok(false, 'onerror unexpectedly fired');
> +    };
> +
> +    mediaRecorder.onstop = function() {

We need a check in here to make that ondataavailable fires before onstop.

@@ +25,5 @@
> +    };
> +
> +    mediaRecorder.onstop = function() {
> +      ok(true, 'onstop fired successfully');
> +      is(mediaRecorder.state, 'inactive', 'check recording status is inactive');

See below - we should check this immediately after calling stop

@@ +28,5 @@
> +      ok(true, 'onstop fired successfully');
> +      is(mediaRecorder.state, 'inactive', 'check recording status is inactive');
> +      SimpleTest.finish();
> +    };
> +    mediaRecorder.ondataavailable = function (e) {

There's a bunch of additional checks needed here - see http://hg.mozilla.org/mozilla-central/file/cf9666cc3f36/content/media/test/test_mediarecorder_record_timeslice.html for an example of stuff we need. That includes:

* Checking other attributes of the event not covered below
* Checking the media recording state post calling stop

@@ +29,5 @@
> +      is(mediaRecorder.state, 'inactive', 'check recording status is inactive');
> +      SimpleTest.finish();
> +    };
> +    mediaRecorder.ondataavailable = function (e) {
> +      if (e.target.state == 'recording') {

I don't think we need this - we should be recording at this point.

@@ +36,5 @@
> +        mediaRecorder.stop();
> +      }
> +    };
> +    //delay here...for ( i = 0; i < 10000000; i ++) {}
> +    mediaRecorder.start(3000);

To avoid risking timing out in try, let's cut back this time to 250.

Comment 13

[Benjamin Chen [:bechen]]

Attachment: record_timeslice.v01.patch

1. rename the file name to "test_mediarecorder_record_gum_video_timeslice.html"
2. modify the "mediaRecorder.ondataavailable" and "mediaRecorder.onstop"
3. modify android.json and b2g.json

Comment 14

[Jason Smith [:jsmith]]

Comment on attachment 8381850 [details] [diff] [review]
record_timeslice.v01.patch

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

The mochitest looks good, but I think you forgot to turn the test off in the following json files:

* androidx86.json
* b2g-debug.json
* b2g-desktop.json

Two other notes though:

1. Make sure you include a detailed commit message.
2. Make sure you kick off a try run with this patch.

Comment 15

[Benjamin Chen [:bechen]]

Attachment: record_timeslice.v02.patch

1. modify androidx86.json b2g-debug.json b2g-desktop.json
2. try server: https://tbpl.mozilla.org/?tree=Try&rev=d83f11eb682a

Comment 16

[Jason Smith [:jsmith]]

Comment on attachment 8382725 [details] [diff] [review]
record_timeslice.v02.patch

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

::: content/media/test/test_mediarecorder_record_gum_video_timeslice.html
@@ +14,5 @@
> +function startTest() {
> +  navigator.mozGetUserMedia({audio: true, video: true, fake: true}, function(stream) {
> +    var dataAvailableCount = 0;
> +    var onDataAvailableFirst = false;
> +  

Nit - trailing whitespace in multiple spots in this file

Comment 17

[Benjamin Chen [:bechen]]

Attachment: bug-969289.patch

r=rillian

Comment 18

[Benjamin Chen [:bechen]]

Attachment: record_timeslice.v02.patch

r=jsmith
Remove trailing whitespace.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74da898d7568
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae8c9202a6ff

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74da898d7568
https://hg.mozilla.org/mozilla-central/rev/ae8c9202a6ff

Comment 21

[u459114]

(In reply to Benjamin Chen [:bechen] from comment #0)
> The image pointer may be null in video chunk. (At the beginning of MSG?)
Is this a reasonable behavior? Carry null image in chunk/ 
If not, we should look back to the beginning of this flow
If yes, we should filter this out or change null image to mute frame in base class, VideoTrackEncoder, instead of in VP8TrackEncoder. I think all VideoTrackEncoders need to face this problem.
> We should treat it as a mute frame instead return an error.
> 
> http://dxr.mozilla.org/mozilla-central/source/content/media/encoder/
> VP8TrackEncoder.cpp#247