896353 - Media Recording - Can't record the mediaStream created by AudioContext.

Status: VERIFIED FIXED

(Core :: Web Audio, defect)

Description

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

I found can't get the principal object from mediaStream.
Need to fix it.

Comment 1

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

Attachment: record-tone-gen.html

test case.

Comment 2

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

can't get  principal object from mediaStream
bool MediaRecorder::CheckPrincipal()
{
  nsCOMPtr<nsIPrincipal> principal = mStream->GetPrincipal();

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: addPrincipal.patch

Add principal according to :roc's suggestion.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 779566 [details] [diff] [review]
addPrincipal.patch

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

::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp
@@ +74,5 @@
>    MediaStreamDestinationEngine* engine = new MediaStreamDestinationEngine(this, tus);
>    mStream = aContext->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM);
>    mPort = tus->AllocateInputPort(mStream, 0);
> +
> +  mDOMStream->CombineWithPrincipal(aContext->GetParentObject()->GetExtantDoc()->NodePrincipal());

You need to null-check the result of GetExtantDoc. If it's null I guess you just skip this call.

Comment 5

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

Let jwwang to take this bug. :)

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Attachment: addPrincipal-v2.patch

Add null-check the result of GetExtantDoc.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 779577 [details] [diff] [review]
addPrincipal-v2.patch

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

::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp
@@ +74,5 @@
>    MediaStreamDestinationEngine* engine = new MediaStreamDestinationEngine(this, tus);
>    mStream = aContext->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM);
>    mPort = tus->AllocateInputPort(mStream, 0);
> +
> +  nsCOMPtr<nsIDocument> doc = aContext->GetParentObject()->GetExtantDoc();

You can just use nsIDocument* here.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Attachment: addPrincipal-v3.patch

Comment 9

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

Attachment: test case

oh, got possible memory leak
 1:05.46                                               Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
 1:05.46    0 TOTAL                                          22      200  1012738       11 ( 8667.89 +/-  7578.15)  1265373        1 ( 2747.69 +/-  5558.76)
 1:05.46   52 CondVar                                        32       32       33        1 (   14.48 +/-     7.43)        0        0 (    0.00 +/-     0.00)
 1:05.46  168 MediaInputPort                                 40       80        6        2 (    3.50 +/-     1.58)        6        1 (    3.27 +/-     1.68)
 1:05.46  175 MediaStreamGraph                               16       16        1        1 (    1.00 +/-     0.00)        0        0 (    0.00 +/-     0.00)
 1:05.46  203 Mutex                                          24       24      497        1 (  151.97 +/-    72.15)        0        0 (    0.00 +/-     0.00)
 1:05.46  853 nsTArray_base                                   8       48   505570        6 (15645.28 +/-  3011.14)        0        0 (    0.00 +/-     0.00)

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You probably need to call MediaInputPort::Destroy on the MediaInputPort you create.

Comment 11

[Jason Smith [:jsmith]]

Comment on attachment 779663 [details] [diff] [review]
test case

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

Drive-by review. review- mainly because of some cleanup needed here.

Also - I'm noticing some common logic were building here. We might want to refactor common setup logic into a separate JS file. However, the refactoring I do not think is a blocker to landing this patch. We could get that in a followup if needed.

::: content/media/test/test_mediarecorder_record_audiocontext.html
@@ +25,5 @@
> +  source.connect(dest);
> +  var elem = document.createElement('audio');
> +  elem.token = token;
> +  manager.started(token);
> +  elem.mozSrcObject = dest.stream;

The one thing I'm confused about in this test is how you are making us of the providing test files (in this case, the opus file). Is it ever used here? If not, we should reconsider the test framework/approach being used here.

@@ +29,5 @@
> +  elem.mozSrcObject = dest.stream;
> +  mMediaStream = dest.stream;
> +  source.start(0);
> +  elem.play();
> +  mMediaRecorder = new MediaRecorder(dest.stream);

I'd also add a check here to ensure that onwarning, onerror, and onstop does not fire here. You can do that by doing the following:

mMediaRecorder.onwarning = function() {
  ok(false, 'onwarning unexpectedly fired');
};

mMediaRecorder.onerror = function() {
  ok(false, 'onerror unexpectedly fired');
};

mMediaRecorder.onstop = function() {
  ok(false, 'onstop unexpectedly fired');
};

@@ +32,5 @@
> +  elem.play();
> +  mMediaRecorder = new MediaRecorder(dest.stream);
> +  mMediaRecorder.ondataavailable = function (e) {
> +    if (mMediaRecorder.state == 'recording') {
> +      mMediaRecorder.stop();

Since you are already planning to call stop, why not also add a test for onstop here?

So do the following in the if statement:

ok(e.data.size > 0, 'check blob has data');

mMediaRecorder.onstop = function() {
  info('onstop fired successfully');
  // add checks for the recording state & mimetype here
  manager.finished(token); 
};

mMediaRecorder.stop();

@@ +37,5 @@
> +      is(e.data.size >0, true, 'check blob has data');
> +      manager.finished(token);
> +    }
> +  };
> +  try {  

Nit - trailing whitespace.

@@ +41,5 @@
> +  try {  
> +    mMediaRecorder.start(1000);
> +    is('recording', mMediaRecorder.state, "check record state recording");
> +  } catch (e) {
> +    is('false', '', "can't record audio context");

Note - you probably instead want this instead:

ok(false, 'Can't record audio context');

I would include a call to manager.finished(token) as well.

Comment 12

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

Thanks Jason's help review. 
I will modify my test case later.

Comment 13

[(no longer active)]

(In reply to comment #10)
> You probably need to call MediaInputPort::Destroy on the MediaInputPort you
> create.

We do that in MediaStreamAudioDestinationNode::DestroyMediaStream.  Am I missing something?

Comment 14

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

Hi Ehsan, 
I try to call destroy inputport before destory TUM. 
It can solve the memory leak problem. 
Do I miss something?

 MediaRecorder::~MediaRecorder()
 {
   if (mTrackUnionStream) {
     mInputPort->Destroy();  <----
     mTrackUnionStream->Destroy();
   }
 }

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randy Lin [:rlin] from comment #14)
> Hi Ehsan, 
> I try to call destroy inputport before destory TUM. 
> It can solve the memory leak problem. 
> Do I miss something?
> 
>  MediaRecorder::~MediaRecorder()
>  {
>    if (mTrackUnionStream) {
>      mInputPort->Destroy();  <----
>      mTrackUnionStream->Destroy();
>    }
>  }

That's what I meant --- the port created by MediaRecorder. Please submit this as a proper patch for review :-)

Comment 16

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

Attachment: destory StreamPort patch v1

Avoid memory leak after mediaRecorder destroy.

Comment 17

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

Attachment: check-in patch

check-in patch, carry reviewer

Comment 18

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

Hi Ryan, 
Could you help to check-in the  "addPrincipal-v3.patch" and  "check-in patch"?

Hi Jason, 
Could you cover this item also? :)

Comment 19

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #18)
> Hi Ryan, 
> Could you help to check-in the  "addPrincipal-v3.patch" and  "check-in
> patch"?
> 
> Hi Jason, 
> Could you cover this item also? :)

Yup, sure.

Comment 20

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

Comment on attachment 779663 [details] [diff] [review]
test case

:jsmith will help this test also.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/7111c04c1158
https://hg.mozilla.org/projects/birch/rev/73188e11f7b1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-1 crashes.
https://hg.mozilla.org/projects/birch/rev/b64e00a36321

https://tbpl.mozilla.org/php/getParsedLog.php?id=25716334&tree=Birch

Comment 23

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

Attachment: test for record audiocontext

Hi Jason,
The mediaStream source can come from the webAudio, so I add this kind of testcase.

Comment 24

[Jason Smith [:jsmith]]

Comment on attachment 781086 [details] [diff] [review]
test for record audiocontext

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

Closer. Still needs some cleanup.

::: content/media/test/test_mediarecorder_record_audiocontext.html
@@ +10,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +var manager = new MediaTestManager;
> +
> +function startTest(test, token) {

Because you are providing your own media source generated from the web audio API, you don't need to run this test as part of the media test manager. I'd modify this mochitest just to do exactly what you are doing below outside of running within media test manager.

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

What you really should do here is set this up such that onstop shouldn't fire upon calling start. When you are about to call stop, setup the actual onstop handler.

@@ +44,5 @@
> +    info('onstop fired successfully');
> +  };
> +  mMediaRecorder.ondataavailable = function (e) {
> +    if (mMediaRecorder.state == 'recording') {
> +      mMediaRecorder.stop();

For anything below this stop call, I'd move it into the onstop handler.

@@ +52,5 @@
> +    }
> +  };
> +  try {
> +    mMediaRecorder.start(1000);
> +    is('recording', mMediaRecorder.state, "check record state recording");

You need to check the mime type here. What mime type should we be expecting here?

Comment 25

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

Attachment: 03.patch

test case v3, add more check for media Recorder's status, onstop event.

Comment 26

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

Attachment: check-in patch

carry reviewer, Fix try server mochitest-test mediaRecorder crash issue.

Comment 27

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

Attachment: check-in.patch

Should this one.
try server result.  
https://tbpl.mozilla.org/?tree=Try&rev=7bb0e9092669

Comment 28

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

Attachment: test case

I will add another bug for the mimeType may retuen null after call the mediaRecorder Start(). It's timing issue. Let the mimeType check at blob event first.

Comment 29

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

Comment on attachment 779096 [details]
record-tone-gen.html

><!DOCTYPE HTML>
><html>
><head>
>  <title>Media test: mediaRecorder</title>
>  <meta charset='utf-8'>
>  <script type="text/javascript" src="manifest.js"></script>
></head>
><body>
><pre id="test">
><audio id="audioelem"></audio>
><script class="testbody" type="text/javascript">
>
>  var context = new AudioContext();
>  var buffer = context.createBuffer(1, 204800, context.sampleRate);
>  for (var i = 0; i < 204800; ++i) {
>    buffer.getChannelData(0)[i] = Math.sin(440 * 2 * Math.PI * i / context.sampleRate);
>  }
>
>  var source = context.createBufferSource();
>  source.buffer = buffer;
>
>  var dest = context.createMediaStreamDestination();
>  source.connect(dest);
>
>  var elem = document.getElementById('audioelem');
>  elem.mozSrcObject = dest.stream;
>  elem.onloadedmetadata = function() {
>    setTimeout(function() {
>    document.mr = new MediaRecorder(dest.stream);
>    document.mr.ondataavailable = function(e) {dump(e);};
>    document.mr.start(1000);
>
>    }, 1000);
>  };
>
>  source.start(0);
>  elem.play();
>
></script>
></pre>
></body>
></html>
>
>

Comment 30

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

follow bug
Bug 898396 - Media Recording - MediaRecorder's mimeType attribute maybe null after call the start()

Comment 31

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

Hi Ryan, 
Sorry causing the test case fail. Could you help this again? Thanks a lot. Need to check-in  addPrincipal-v3.patch,  check-in.patch,  test case by sequence.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/f8f855f0b7dc
https://hg.mozilla.org/projects/birch/rev/4fe81f58dbef
https://hg.mozilla.org/projects/birch/rev/f2862e93b710

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Backed out for asserting like crazy in the new tests. Note that your Try run was opt-only, so you never would have caught it there.
https://hg.mozilla.org/projects/birch/rev/5aa02ee02f4b

https://tbpl.mozilla.org/php/getParsedLog.php?id=25784353&tree=Birch

10:49:26     INFO -  [Parent 2296] ###!!! ASSERTION: Slice out of range: 'aStart >= 0 && aEnd <= aSource.mDuration', file ../../../content/media/MediaSegment.h, line 240
10:49:26     INFO -  mozilla::MediaSegmentBase<mozilla::AudioSegment, mozilla::AudioChunk>::AppendSliceInternal(mozilla::MediaSegmentBase<mozilla::AudioSegment, mozilla::AudioChunk> const&, long long, long long) [content/media/MediaSegment.h:239]
10:49:26     INFO -  mozilla::TrackUnionStream::CopyTrackData(mozilla::StreamBuffer::Track*, unsigned int, long long, long long, bool*) [content/media/TrackUnionStream.h:248]
10:49:26     INFO -  mozilla::TrackUnionStream::ProduceOutput(long long, long long) [content/media/TrackUnionStream.h:71]
10:49:26     INFO -  mozilla::MediaStreamGraphImpl::ProduceDataForStreamsBlockByBlock(unsigned int, int, long long, long long) [obj-firefox/dist/include/nsAutoPtr.h:880]
10:49:26     INFO -  mozilla::MediaStreamGraphImpl::RunThread() [content/media/MediaStreamGraph.cpp:1081]
10:49:26     INFO -  mozilla::::MediaStreamGraphThreadRunnable::Run [content/media/MediaStreamGraph.cpp:1230]
10:49:26     INFO -  nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:622]
10:49:26     INFO -  NS_ProcessNextEvent(nsIThread*, bool) [obj-firefox/xpcom/build/nsThreadUtils.cpp:238]

Comment 34

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

Let me take it because there are still bugs need to solve.
This issue can't reproduce on my Ubuntu 13.04 debug build and still find a way to reproduce & solve this issue. sorry for back out twice.

Comment 35

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

Attachment: test_crazy_assert.html

Sometimes the mDuration is less than aEnd in AppendSliceInternal function..
(ex aEnd = 113665, mDuration = 113664), so this assertion hits.
    NS_ASSERTION(aStart >= 0 && aEnd <= aSource.mDuration,
                 "Slice out of range");

Repo step: use Firefox, load this test html and reload by F5 can find this problem.

Comment 36

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

Hi Roc, 
I add this work-around in TrackUnionStream.h can avoid this problem. 
         // We'll take the latest samples we can.
         TrackTicks inputEndTicks = TimeToTicksRoundUp(rate, inputEnd);
+        if (inputEndTicks > aInputTrack->GetSegment()->GetDuration())
+          inputEndTicks = aInputTrack->GetSegment()->GetDuration();

But I'm not sure what reason cause the inputEndTicks is equal to the GetDuration()+1.
Is timing issue?

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I encountered a similar issue in bug 856361. There's something fundamentally broken here, and it's tricky, but I think I've fixed it by rewriting the code. I'll submit a patch in bug 856361.

Comment 39

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

Thanks a lot. If there is a patch I can try, please also notify me. :)

Comment 40

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

Test with this patch https://bugzilla.mozilla.org/attachment.cgi?id=783698 (Part 7: Fix copying of track data from input streams to output streams in TrackUnionStream )

and it can solve this NS_ASSERTION(aStart >=....) problem.

Comment 41

[Jason Smith [:jsmith]]

Randy - With the assertions fixed in the dependent bug, this is ready to land again, right?

Comment 42

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

I will have  a try again and check-in it. :)

Comment 43

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

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

Comment 44

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

Please help to check-in the 
addPrincipal-v3.patch (2.02 KB, patch)
check-in.patch (2.86 KB, patch)
test case (3.18 KB, patch)

Thanks a lot.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9a466a6b1f46
https://hg.mozilla.org/integration/b2g-inbound/rev/980905e0308e
https://hg.mozilla.org/integration/b2g-inbound/rev/6ca6b7563842

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a466a6b1f46
https://hg.mozilla.org/mozilla-central/rev/980905e0308e
https://hg.mozilla.org/mozilla-central/rev/6ca6b7563842

Comment 48

[Jason Smith [:jsmith]]

Verified via successful landing of mochitest.

Comment 50

[Jason Smith [:jsmith]]

This bug has no relation to bug 932880.