895730 - MediaRecorder - Can't get any encoded data from encoder when recording the mediaStream with mute audio

Status: VERIFIED FIXED

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

Description

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

STR: 
1. use mozGetUserMedia({audio:true, fake = true}) and get mediastream
2. recording this mediastream
NG: mediaRecorder can't get any encoded data from encoder. 
The mediaStream already notifyAudioTrackChange to encoder, but I found the encoder isn't output encoded data to recorder. That would affect our automatic test.

Comment 1

[Shelly Lin [:shelly]]

The channels number of a fake audio chunk when using the fake tag in |mozGetUserMedia({audio:true, fake:true})| is zero, this is why the MediaEncoder fails to create an AudioTrackEncoder.

The patch from attachment fixes the problem, but I'm not sure if this is the right fix.

Comment 2

[Shelly Lin [:shelly]]

Attachment: Handle null audio chunk in AudioTrackEncoder

This fix is base on the assumption where the muted audio is some chunks with valid duration but empty channel data.

Regardless of this bug, it does bring up another issue of the lack of error handling if fail to initialize the audio encoder. Currently, the encoder thread will be blocked by a monitor where it is waiting for a valid encoder.

Comment 3

[Shelly Lin [:shelly]]

Create bug 896980.

Comment 4

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

Comment on attachment 779698 [details] [diff] [review]
Handle null audio chunk in AudioTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +40,5 @@
> +      // A muted audio chunk might have a valid duration but empty channel data,
> +      // in this case, we should still initialize the audio track encoder.
> +      nsresult rv = Init((chunk.IsNull() && chunk.mDuration > 0) ?
> +                          DEFAULT_CHANNELS : chunk.mChannelData.Length(),
> +                          aTrackRate);

We should determine the channel count by searching for the first chunk that is not null.

If all the chunks are null, we should defer the Init call.

However we need to make sure that once we finally see a non-null chunk and call Init, we feed the right amount of null (zero) data into the encoder.

Comment 5

[Shelly Lin [:shelly]]

Hmmm, I got it, thanks Rob :).

Is there a maximum value for the defer? What if we never see a non-null chunk? I can't think of a case like this in real life, but a test case of using fake gUM would hang the encoder and return nothing.

Comment 6

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

(In reply to Shelly Lin [:shelly] from comment #5)
> Is there a maximum value for the defer? What if we never see a non-null
> chunk? I can't think of a case like this in real life, but a test case of
> using fake gUM would hang the encoder and return nothing.

That means the audio stream is complete silence, so it doesn't matter how many channels you use. I guess when the recording finally ends, then it's OK to default to 1 channel and emit a burst of that much silence. It should compress very well :-).

Comment 7

[Shelly Lin [:shelly]]

Attachment: v2: Handle null audio chunks in AudioTrackEncoder

This version of fix is closer to our previous discussion.

Comment 8

[Shelly Lin [:shelly]]

Comment on attachment 780816 [details] [diff] [review]
v2: Handle null audio chunks in AudioTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +79,5 @@
> +  if (!mInitialized && mSilentDuration > 0) {
> +    Init(DEFAULT_CHANNELS, DEFAULT_SAMPLING_RATE);
> +    mRawSegment->AppendNullData(mSilentDuration);
> +    mSilentDuration = 0;
> +  }

I thought it makes more sense to put the above implementation when receiving a track event of TRACK_EVENT_ENDED (ln:65), but ending the fake gUM doesn't give me a TRACK_EVENT_ENDED event, not sure if this is a bug of fake gUM.

@@ +94,5 @@
>  
> +  // Drops the aSegment receives from MediaStreamGraph if our queued segment has
> +  // overflowed, but not in the case where there is a complete silence before
> +  // the audio track encoder has initialized.
> +  if (mRawSegment->GetDuration() < MAX_FRAMES_TO_DROP || mSilentDuration > 0) {

This is for preventing the drop of any source segments if we have a very long silence before initializing the audio track encoder.

Comment 9

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

(In reply to Shelly Lin [:shelly] from comment #8)
> ::: content/media/encoder/TrackEncoder.cpp
> @@ +79,5 @@
> > +  if (!mInitialized && mSilentDuration > 0) {
> > +    Init(DEFAULT_CHANNELS, DEFAULT_SAMPLING_RATE);
> > +    mRawSegment->AppendNullData(mSilentDuration);
> > +    mSilentDuration = 0;
> > +  }
> 
> I thought it makes more sense to put the above implementation when receiving
> a track event of TRACK_EVENT_ENDED (ln:65), but ending the fake gUM doesn't
> give me a TRACK_EVENT_ENDED event, not sure if this is a bug of fake gUM.

I think it is. Can you track it down?

> @@ +94,5 @@
> >  
> > +  // Drops the aSegment receives from MediaStreamGraph if our queued segment has
> > +  // overflowed, but not in the case where there is a complete silence before
> > +  // the audio track encoder has initialized.
> > +  if (mRawSegment->GetDuration() < MAX_FRAMES_TO_DROP || mSilentDuration > 0) {
> 
> This is for preventing the drop of any source segments if we have a very
> long silence before initializing the audio track encoder.

I don't understand why we need to do this. Null AudioChunks don't take up much space. (sequential null chunks should all be combined into a single null chunk automatically.)

Comment 10

[Shelly Lin [:shelly]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> (In reply to Shelly Lin [:shelly] from comment #8)
> > ::: content/media/encoder/TrackEncoder.cpp
> > @@ +79,5 @@
> > > +  if (!mInitialized && mSilentDuration > 0) {
> > > +    Init(DEFAULT_CHANNELS, DEFAULT_SAMPLING_RATE);
> > > +    mRawSegment->AppendNullData(mSilentDuration);
> > > +    mSilentDuration = 0;
> > > +  }
> > 
> > I thought it makes more sense to put the above implementation when receiving
> > a track event of TRACK_EVENT_ENDED (ln:65), but ending the fake gUM doesn't
> > give me a TRACK_EVENT_ENDED event, not sure if this is a bug of fake gUM.
> 
> I think it is. Can you track it down?
Sure.
> 
> > @@ +94,5 @@
> > >  
> > > +  // Drops the aSegment receives from MediaStreamGraph if our queued segment has
> > > +  // overflowed, but not in the case where there is a complete silence before
> > > +  // the audio track encoder has initialized.
> > > +  if (mRawSegment->GetDuration() < MAX_FRAMES_TO_DROP || mSilentDuration > 0) {
> > 
> > This is for preventing the drop of any source segments if we have a very
> > long silence before initializing the audio track encoder.
> 
> I don't understand why we need to do this. Null AudioChunks don't take up
> much space. (sequential null chunks should all be combined into a single
> null chunk automatically.)
Say we have received MAX_FRAMES_TO_DROP null chunks and then receive the first non-null chunk with duration N (and this is when the encoder is initialized), the duration of mRawSegment is MAX_FRAMES_TO_DROP because it has already appended this many TrackTicks after the call to Init(), thus the original logic will make it not append this non-null chunk.

An alternative way is to append the null data here, something like: 

if (mRawSegment->GetDuration() < MAX_FRAMES_TO_DROP) {
  if (mSilentDuration > 0) {
     mRawSegment->AppendNullData(mSilentDuration);
     mSilentDuration = 0;
  }
  ......

Comment 11

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

(In reply to Shelly Lin [:shelly] from comment #10)
> if (mRawSegment->GetDuration() < MAX_FRAMES_TO_DROP) {
>   if (mSilentDuration > 0) {
>      mRawSegment->AppendNullData(mSilentDuration);
>      mSilentDuration = 0;
>   }

That sounds like it would work.

However, actually I don't understand what MAX_FRAMES_TO_DROP is for. Can you explain it to me?

Comment 12

[Shelly Lin [:shelly]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> (In reply to Shelly Lin [:shelly] from comment #10)
> > if (mRawSegment->GetDuration() < MAX_FRAMES_TO_DROP) {
> >   if (mSilentDuration > 0) {
> >      mRawSegment->AppendNullData(mSilentDuration);
> >      mSilentDuration = 0;
> >   }
> 
> That sounds like it would work.
> 
> However, actually I don't understand what MAX_FRAMES_TO_DROP is for. Can you
> explain it to me?

Sure. It was designed for the situation when the pull from recorder is much slower than the receive from MediaStramGraph, we set a limitation from storing raw data to the segment queue in audio track encoder, to avoid buffer overflow or memory burst.

Although I don't think it is necessary since the overhead of audio data is not that big, as oppose to video frames. Me personally would vote for removing this implementation since when it comes to recording video, it'll prefer dropping video frames over dropping audio chunks. How do you think of that?

Comment 13

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

Yeah, I don't think we should have that. Let's take it out.

We may or may not need it for video. Let's worry about that once we can test for it.

Comment 14

[Shelly Lin [:shelly]]

Attachment: v3: Handle null audio chunks in AudioTrackEncoder

This patch has removed the implementation of "dropping in-coming frames if the speed of encoding is much slower than receiving in-coming raw data.".

Comment 15

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

Comment on attachment 783095 [details] [diff] [review]
v3: Handle null audio chunks in AudioTrackEncoder

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

Great!

::: content/media/encoder/TrackEncoder.cpp
@@ +95,3 @@
>    }
> +
> +  while(!iter.IsEnded()) {

space before (

Comment 16

[Shelly Lin [:shelly]]

Attachment: [check-in] Handle null audio chunks in AudioTrackEncoder

W/ nit fixed and carry r+ from Rob.

Comment 17

[Shelly Lin [:shelly]]

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=3a54081a6169

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/9b6901ab2652

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b6901ab2652

Comment 20

[Jason Smith [:jsmith]]

So if I understand this bug correctly - we can't record audio from a fake gUM stream? And this wouldn't work as well if we muted the audio media stream track?

I'll do some exploratory testing around this, but some clarity on the above questions would be helpful.

Comment 21

[Jason Smith [:jsmith]]

Specifically in reference to muting - I'm meaning trying to record a media stream in which the audio media stream track is muted by setting enabled to false.

Comment 22

[Shelly Lin [:shelly]]

My bad, I should have state the change for you :(.

You can, if you record a complete silence, or record with fake gUM, it won't return the blob data periodically during recording. Instead, it'll emit the silent audio data w/ such duration once you stop the recorder.

Comment 23

[Jason Smith [:jsmith]]

Verified - confirmed we're getting silence on muted audio recorded.