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

VERIFIED FIXED in Firefox 26

Status

()

Core
Web Audio
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: rlin, Assigned: rlin)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla26
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 fixed)

Details

(Whiteboard: [FT: Media Recording, Sprint 1])

Attachments

(4 attachments, 9 obsolete attachments)

2.02 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
2.86 KB, patch
Details | Diff | Splinter Review
3.18 KB, patch
Details | Diff | Splinter Review
1.44 KB, text/html
Details
I found can't get the principal object from mediaStream.
Need to fix it.
Summary: Media Recording - Can't record the mediaStream create by AudioContext. → Media Recording - Can't record the mediaStream created by AudioContext.
Assignee: nobody → rlin
Created attachment 779096 [details]
record-tone-gen.html

test case.
can't get  principal object from mediaStream
bool MediaRecorder::CheckPrincipal()
{
  nsCOMPtr<nsIPrincipal> principal = mStream->GetPrincipal();
Created attachment 779566 [details] [diff] [review]
addPrincipal.patch

Add principal according to :roc's suggestion.
Attachment #779566 - Flags: review?(roc)
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.

Updated

5 years ago
Component: Video/Audio → Web Audio
Let jwwang to take this bug. :)
Assignee: rlin → jwwang
Component: Web Audio → Video/Audio
Component: Video/Audio → Web Audio
Created attachment 779577 [details] [diff] [review]
addPrincipal-v2.patch

Add null-check the result of GetExtantDoc.
Attachment #779566 - Attachment is obsolete: true
Attachment #779566 - Flags: review?(roc)
Attachment #779577 - Flags: review?(roc)
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.
Attachment #779577 - Flags: review?(roc) → review+
Created attachment 779585 [details] [diff] [review]
addPrincipal-v3.patch
Attachment #779577 - Attachment is obsolete: true
Attachment #779585 - Flags: review+
Created attachment 779663 [details] [diff] [review]
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)
Attachment #779663 - Flags: review?(roc)
You probably need to call MediaInputPort::Destroy on the MediaInputPort you create.
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.
Attachment #779663 - Flags: review-
Thanks Jason's help review. 
I will modify my test case later.

Comment 13

5 years ago
(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?
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();
   }
 }
(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 :-)
Created attachment 780299 [details] [diff] [review]
destory StreamPort patch v1

Avoid memory leak after mediaRecorder destroy.
Attachment #780299 - Flags: review?(roc)
Created attachment 780764 [details] [diff] [review]
check-in patch

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

Hi Jason, 
Could you cover this item also? :)
Whiteboard: checkin-needed
(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 on attachment 779663 [details] [diff] [review]
test case

:jsmith will help this test also.
Attachment #779663 - Flags: review?(roc)
Attachment #780299 - Attachment is obsolete: true
Attachment #779663 - Attachment is obsolete: true
Attachment #779585 - Flags: checkin+
Attachment #780299 - Flags: checkin+
Attachment #780299 - Flags: checkin+
Attachment #780764 - Flags: checkin+
Attachment #779585 - Flags: checkin+
Attachment #780764 - Flags: checkin+
Created attachment 781086 [details] [diff] [review]
test for record audiocontext

Hi Jason,
The mediaStream source can come from the webAudio, so I add this kind of testcase.
Attachment #781086 - Flags: review?(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?
Attachment #781086 - Flags: review?(jsmith) → review-
Created attachment 781433 [details] [diff] [review]
03.patch

test case v3, add more check for media Recorder's status, onstop event.
Attachment #781086 - Attachment is obsolete: true
Attachment #781433 - Flags: review?(jsmith)

Updated

5 years ago
Attachment #781433 - Flags: review?(jsmith) → review+
Created attachment 781656 [details] [diff] [review]
check-in patch

carry reviewer, Fix try server mochitest-test mediaRecorder crash issue.
Attachment #780764 - Attachment is obsolete: true
Created attachment 781659 [details] [diff] [review]
check-in.patch

Should this one.
try server result.  
https://tbpl.mozilla.org/?tree=Try&rev=7bb0e9092669
Attachment #781656 - Attachment is obsolete: true
Created attachment 781662 [details] [diff] [review]
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.
Attachment #781433 - Attachment is obsolete: true
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>
>
>
Attachment #779096 - Attachment is obsolete: true
follow bug
Bug 898396 - Media Recording - MediaRecorder's mimeType attribute maybe null after call the start()
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.
Whiteboard: checkin-needed

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: checkin-needed
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]
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.
Assignee: jwwang → rlin
Created attachment 782436 [details]
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.
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?
Flags: needinfo?(roc)

Updated

5 years ago
Duplicate of this bug: 899883

Updated

5 years ago
Blocks: 899878
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.
Flags: needinfo?(roc)
Thanks a lot. If there is a patch I can try, please also notify me. :)
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.

Updated

5 years ago
blocking-b2g: --- → koi+

Updated

5 years ago
Whiteboard: [FT: Media Recording, Sprint 1]
Randy - With the assertions fixed in the dependent bug, this is ready to land again, right?
Flags: needinfo?(rlin)
I will have  a try again and check-in it. :)
Flags: needinfo?(rlin)
Keywords: checkin-needed
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.

Updated

5 years ago
Keywords: verifyme
QA Contact: jsmith

Updated

5 years ago
Duplicate of this bug: 899883
Verified via successful landing of mochitest.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Duplicate of this bug: 879672
status-firefox26: --- → fixed

Updated

5 years ago
Depends on: 924724

Updated

5 years ago
Depends on: 932880
This bug has no relation to bug 932880.
No longer depends on: 932880
Depends on: 975784

Updated

5 years ago
No longer depends on: 975784
Depends on: 975784
You need to log in before you can comment on or make changes to this bug.