Closed Bug 969289 Opened 6 years ago Closed 6 years ago

Handle image null pointer in VP8TrackEncoder.

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bechen, Assigned: bechen)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 5 obsolete files)

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
Attached patch bug-969289.patch (obsolete) — Splinter Review
Hi, Randy:
please help to try this patch on your local test case.
Attachment #8373121 - Flags: feedback?(rlin)
This patch pass my test.
Please also add my test script in the test case. 
note: should take care of the b2g video mimeType.
Attachment #8373121 - Flags: feedback?(rlin) → feedback+
Attached patch record_timeslice.patch (obsolete) — Splinter Review
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
Attached patch record_timeslice.patch (obsolete) — Splinter Review
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?
Attachment #8374729 - Attachment is obsolete: true
Attachment #8378757 - Flags: review?(jsmith)
Comment on attachment 8373121 [details] [diff] [review]
bug-969289.patch

Hi Ralph:
Please help to review this patch, thanks.
Attachment #8373121 - Flags: review?(giles)
(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?
No, on b2g platform, The media recorder would output mp4 video file format.
(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?
The major problem is we don't have the supported vorbis audio encoder on b2g platform. So we can't output WebM as well.
Attachment #8373121 - Flags: review?(giles) → review+
(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 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.
Attachment #8378757 - Flags: review?(jsmith) → review-
Blocks: 970795
Attached patch record_timeslice.v01.patch (obsolete) — Splinter Review
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
Attachment #8378757 - Attachment is obsolete: true
Attachment #8381850 - Flags: review?(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.
Attachment #8381850 - Flags: review?(jsmith) → review-
Attached patch record_timeslice.v02.patch (obsolete) — Splinter Review
1. modify androidx86.json b2g-debug.json b2g-desktop.json
2. try server: https://tbpl.mozilla.org/?tree=Try&rev=d83f11eb682a
Attachment #8381850 - Attachment is obsolete: true
Attachment #8382725 - Flags: review?(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
Attachment #8382725 - Flags: review?(jsmith) → review+
Component: Video/Audio → Video/Audio: Recording
Attached patch bug-969289.patchSplinter Review
r=rillian
Attachment #8373121 - Attachment is obsolete: true
Attachment #8384398 - Flags: review+
r=jsmith
Remove trailing whitespace.
Attachment #8382725 - Attachment is obsolete: true
Attachment #8384401 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/74da898d7568
https://hg.mozilla.org/mozilla-central/rev/ae8c9202a6ff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(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
Flags: needinfo?(bechen)
Duplicate of this bug: 890852
Flags: needinfo?(bechen)
You need to log in before you can comment on or make changes to this bug.