Bad audio quality with MediaRecorder

VERIFIED FIXED in Firefox 30, Firefox OS v1.4

Status

()

VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: djf, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 verified, b2g-v1.4 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 8360615 [details]
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
(Reporter)

Comment 1

5 years ago
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.
Flags: needinfo?(rlin)
(Reporter)

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
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.
OK,
Blocks: 879688
blocking-b2g: --- → 1.4?
Flags: needinfo?(rlin)
OK, I can reproduce this on the nightly build.
Let me debug this one.
Created attachment 8362111 [details]
960243.html

It's so wired that when the src = opus or wav file(not ogg...) with this test page. it works well.
Created attachment 8362117 [details]
441-10s.wav

record but get bad sound quality.
Created attachment 8362118 [details]
48k-10s.wav

record with good sound quality
Attach two wave files, 
It look like an issue to record the 44100hz audio files. (The testing ogg file is also 44100hz)
Hi JW, 
Please help for this bug, thanks a lot.
Assignee: nobody → jwwang
(Assignee)

Comment 12

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Bad audio quality with MediaRecorder(createMediaStreamDestination) → Bad audio quality with MediaRecorder
(Assignee)

Updated

5 years ago
Component: Web Audio → Video/Audio
(Assignee)

Comment 13

5 years ago
Change the summary and component since this bug is not about WebAudio.
(Assignee)

Comment 14

5 years ago
Created attachment 8363568 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet.
Attachment #8363568 - Flags: review?(cpearce)
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.
(Assignee)

Comment 16

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

change the reviewer.
Attachment #8363568 - Flags: review?(cpearce) → review?(giles)
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.
Attachment #8363568 - Flags: review?(giles) → review+
(Assignee)

Comment 18

5 years ago
Created attachment 8364835 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet.

Minor fixes as suggested by Ralph.
Attachment #8364835 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #8363568 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Comment 20

5 years ago
Created attachment 8365758 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet. - v4

Fix error when the sample rate of the input file is 96K.
Attachment #8364835 - Attachment is obsolete: true
Attachment #8365758 - Flags: review?(giles)
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.
Attachment #8365758 - Flags: review?(giles) → review-
(Assignee)

Comment 23

5 years ago
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.
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.

Updated

5 years ago
blocking-b2g: 1.4? → 1.4+
(Assignee)

Comment 25

5 years ago
Created attachment 8370514 [details] [diff] [review]
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
Attachment #8365758 - Attachment is obsolete: true
Attachment #8370514 - Flags: review?(giles)
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.
Attachment #8370514 - Flags: review?(giles) → review+
(Assignee)

Comment 27

5 years ago
Created attachment 8372028 [details] [diff] [review]
Fix frame loss which can't fit into an Opus packet. - v7

Fix white spaces.
Attachment #8370514 - Attachment is obsolete: true
Attachment #8372028 - Flags: review+
(Assignee)

Comment 28

5 years ago
Hi Ryan,
Please check in "Fix frame loss which can't fit into an Opus packet. - v7". Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a4ce5ecaa95d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Updated

5 years ago
Keywords: verifyme
QA Contact: jsmith

Updated

5 years ago
Depends on: 969583
lgtm - test case in comment 0 works fine for me on trunk
Status: RESOLVED → VERIFIED
Keywords: verifyme
Component: Video/Audio → Video/Audio: Recording
status-b2g-v1.4: --- → fixed
status-firefox28: --- → wontfix
status-firefox29: --- → wontfix
status-firefox30: --- → fixed
Marking this as verified for Firefox 30 based on Jason's comment 31.
status-firefox30: fixed → verified
You need to log in before you can comment on or make changes to this bug.