960243 - Bad audio quality with MediaRecorder

Status: VERIFIED FIXED

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

Description

[David Flanagan [:djf]]

Attachment: Low quality audio clip produced by http://djf.net/audiocrop/ using MediaRecorder

For FirefoxOS, I'm trying to write code that will allow the user to "record" a clip from a music file and save it as an Opus file for use as a ringtone.

I've written a proof-of-concept at http://djf.net/audiocrop/index.html.  It uses an <audio> element and createMediaElementSource for the source sound, and uses createMediaStreamDestination as the destination with MediaRecorder.

But the Opus clips it produces sound quite bad. When derf listened to the attached clip, he said (on IRC): "Sounds low-passed, even though it is encoded as fullband."  He also wrote: "Okay, sounds reasonable. I'm not sure what the problem is, but I think it's being introduced before you get to the codec."

Possibly I'm using the API wrong, and if so, I'd like help figuring that out.

I've tested on MacOS (using Aurora and Nightly) and FirefoxOS 1.4, and get the bad audio quality on both

Comment 1

[David Flanagan [:djf]]

Randy: do you have time to look at this (or at least look at my code to see if I'm using the API incorrectly)?  We need to be able to extract high-quality clips from songs to use as ringtones. It is a high-priority feature for FirefoxOS 1.4.

Comment 2

[David Flanagan [:djf]]

This bug sounds like bug 911468, which has been resolved. The code there uses mozCaptureStreamUntilEnded() and the recorded audio sounds good.  I suppose I could try doing that as well. But it would be nice if the more standard APIs worked.

Comment 3

[David Flanagan [:djf]]

For what its worth, the recorded audio quality seems even worse when I try the same code with an mp3 file as the original instead of an ogg file.

Comment 4

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

OK,

Comment 5

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

OK, I can reproduce this on the nightly build.
Let me debug this one.

Comment 6

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

Attachment: 960243.html

It's so wired that when the src = opus or wav file(not ogg...) with this test page. it works well.

Comment 7

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

s/wired/weird

Comment 8

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

Attachment: 441-10s.wav

record but get bad sound quality.

Comment 9

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

Attachment: 48k-10s.wav

record with good sound quality

Comment 10

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

Attach two wave files, 
It look like an issue to record the 44100hz audio files. (The testing ogg file is also 44100hz)

Comment 11

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

Hi JW, 
Please help for this bug, thanks a lot.

Comment 12

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

The root cause is described below:

1. For each encoding cycle, 960 (which is GetPacketDuration()) frames are fetched from mSourceSegment (in OpusTrackEncoder::GetEncodedTrack()).
2. They are resampled into 1045 frames (44.1k -> 48k).
3. Only 960 out of 1045 frames are passed to opus_encode_float().
4. 1045 - 960 = 85 frames are discarded and therefore a discontinuous tone is heard.

Fix:
Try to fetch m frames such that there will be 960 frames after resampling which will fit into an Opus packet duration perfectly.

Comment 13

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

Change the summary and component since this bug is not about WebAudio.

Comment 14

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

Attachment: Fix frame loss which can't fit into an Opus packet.

Comment 15

[Shelly Lin [:shelly]]

Comment on attachment 8363568 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet.

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

Hi JW, I would suggest using a nsTArray<AudioDataValue> to store and manage the resampled pcm data, for the purpose of avoiding extra memcpy and an easier management.

Comment 16

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

Comment on attachment 8363568 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet.

change the reviewer.

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8363568 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet.

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

Thanks for tracking this down. r=me with the cast issue addressed.

::: content/media/encoder/OpusTrackEncoder.cpp
@@ +337,5 @@
>                                                            out, &outframes);
>  #endif
>  
> +    memcpy(pcm.Elements(), mResampledLeftover.Elements(),
> +           mResampledLeftover.Length() * sizeof(AudioDataValue));

Using PodCopy() so you don't have to reproduce sizeof(AudioDataValue) would be safer here.

@@ +339,5 @@
>  
> +    memcpy(pcm.Elements(), mResampledLeftover.Elements(),
> +           mResampledLeftover.Length() * sizeof(AudioDataValue));
> +    int outframeToCopy = std::min(static_cast<int>(outframes),
> +                                  GetPacketDuration() - frameLeft);

I think you need to MOZ_RELEASE_ASSERT(outframes <= INT_MAX) here to avoid overflow with the cast. Or move the others to uint32_t.

'frame' should be plural here: outframesToCopy. Feel free to correct frameToFetch as well.

@@ +374,4 @@
>    // Append null data to pcm buffer if the leftover data is not enough for
>    // opus encoder.
> +  if (framesInPCM < GetPacketDuration() && mEndOfStream) {
> +    memset(pcm.Elements() + framesInPCM * mChannels, 0,

Likewise PodZero.

::: content/media/encoder/OpusTrackEncoder.h
@@ +78,5 @@
> +  /**
> +   * Store the resampled frames that don't fit into an Opus packet duration.
> +   * They will be prepended to the resampled frames next encoding cycle.
> +   */
> +  nsAutoTArray<AudioDataValue, 10> mResampledLeftover;

Isn't the likely max length much larger than 10? I'd either leave off the default declaration or find the optimum size.

Comment 18

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

Attachment: Fix frame loss which can't fit into an Opus packet.

Minor fixes as suggested by Ralph.

Comment 19

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

It looks like the change doesn't work for sample rate=96K. Also the code is kinda complicated in computing input/output frames of resampling. I will try to do a little refactoring to simplify the code.

Comment 20

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

Attachment: Fix frame loss which can't fit into an Opus packet. - v4

Fix error when the sample rate of the input file is 96K.

Comment 21

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

https://tbpl.mozilla.org/?tree=Try&rev=581d4ecb93c2
try results.

Comment 22

[Ralph Giles (:rillian)]

Comment on attachment 8365758 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet. - v4

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

Thanks for making the patch smaller and using Pod calls. Please fix the thread race though.

::: content/media/encoder/OpusTrackEncoder.cpp
@@ +244,5 @@
>  nsresult
>  OpusTrackEncoder::GetEncodedTrack(EncodedFrameContainer& aData)
>  {
> +  // resampled frames left last time which didn't fit into an Opus packet duration.
> +  const int frameLeft = mResampledLeftover.Length() / mChannels;

const uint32_t framesleft;

@@ +254,5 @@
> +  // Try to fetch m frames such that there will be n frames
> +  // where (n + frameLeft) = GetPacketDuration() after resampling.
> +  const int framesToFetch = !mResampler ? GetPacketDuration()
> +      : (GetPacketDuration() - frameLeft) * mSamplingRate / kOpusSamplingRate
> +        + frameRoundUp;

Calculating this here races with the !mInitialized clause of the monitor wait just below. If we have to wait because mInitialized is false, then when it becomes true we will have used a stale result from GetPacketDuration() from before the Init() call to calculate framesToFetch.

I think you can just move this last statement and its comment to below the monitor block.

@@ +335,5 @@
>      float* out = reinterpret_cast<float*>(resamplingDest.Elements());
>      speex_resampler_process_interleaved_float(mResampler, in, &inframes,
>                                                            out, &outframes);
>  #endif
>  

Please MOZ_ASSERT that there's enough room im pcm for the PodCopy()s.

@@ +339,5 @@
>  
> +    PodCopy(pcm.Elements(), mResampledLeftover.Elements(),
> +        mResampledLeftover.Length());
> +    uint32_t outframesToCopy = std::min(outframes,
> +        static_cast<uint32_t>(GetPacketDuration() - frameLeft));

Again, please assert frameLeft < GetPacketDuration() before subtracting here (or use a CheckedInt. Otherwise the cast can underflow.

Comment 23

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

Comment on attachment 8365758 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet. - v4

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

::: content/media/encoder/OpusTrackEncoder.cpp
@@ +254,5 @@
> +  // Try to fetch m frames such that there will be n frames
> +  // where (n + frameLeft) = GetPacketDuration() after resampling.
> +  const int framesToFetch = !mResampler ? GetPacketDuration()
> +      : (GetPacketDuration() - frameLeft) * mSamplingRate / kOpusSamplingRate
> +        + frameRoundUp;

mChannels also depends on initialization. Therefore framesleft, frameRoundUp, and framesToFetch can't be calculated until initialization is done.

Comment 24

[Ralph Giles (:rillian)]

Right you are. I was thinking mResampledLeftover.Length() should return zero before Init() is called, but it isn't safe to assume that, and if that's true mChannels is likely also true, so we'd be dividing by zero.

Comment 25

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

Attachment: Fix frame loss which can't fit into an Opus packet. - v6

Fix race condition. Computing framesToFetch only after being initialized.
try: https://tbpl.mozilla.org/?tree=Try&rev=5db505a30d92

Comment 26

[Ralph Giles (:rillian)]

Comment on attachment 8370514 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet. - v6

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

r=me with the whitespace nit addressed.

The buffering logic still isn't as clean as I'd like, but should be safe as long as the chunk length doesn't change.

::: content/media/encoder/OpusTrackEncoder.cpp
@@ +245,5 @@
>  OpusTrackEncoder::GetEncodedTrack(EncodedFrameContainer& aData)
>  {
>    {
> +    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +    // Wait until initialized or cancelled. 

Please remove trailing whitespace here.

@@ +253,5 @@
> +    if (mCanceled || mEncodingComplete) {
> +      return NS_ERROR_FAILURE;
> +    }
> +  }
> +  

And here.

Comment 27

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

Attachment: Fix frame loss which can't fit into an Opus packet. - v7

Fix white spaces.

Comment 28

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

Hi Ryan,
Please check in "Fix frame loss which can't fit into an Opus packet. - v7". Thanks.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a4ce5ecaa95d

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4ce5ecaa95d

Comment 31

[Jason Smith [:jsmith]]

lgtm - test case in comment 0 works fine for me on trunk

Comment 32

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking this as verified for Firefox 30 based on Jason's comment 31.