903023 - Media Encoder - Recording muted audio generates silence on first try, but not on the 2nd try

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

STR

1. Go to http://mozilla.github.io/qa-testcase-data/webapi/mediarecorder/index.html
2. Select Microphone, setup, and accept permissions
3. In console, run mediaRecorderList[0].stream.getAudioTracks()[0].enabled = false;
4. In console, run mediaRecorderList[0].start();
5. Wait five seconds and generate sound #1
6. In console, run mediaRecorderList[0].stream.getAudioTracks()[0].enabled = true;
7. Wait five seconds and generate sound #2
8. In console, run mediaRecorderList[0].stream.getAudioTracks()[0].enabled = false;
9. Wait five seconds and generate sound #3
10. In console, run mediaRecorderList[0].stream.getAudioTracks()[0].enabled = true;
11. Wait five seconds and generate sound #4
12. In console, run mediaRecorderList[0].stop();
13. Playback the downloaded blob generated

Expected

Silence should be presence during the timeframe of sound #1 and sound #3. Sound #2 and sound #4 should be present during their respective timeframe. Meaning:

- 0 - 5 seconds - silence
- 5 - 10 seconds - sound #2
- 10 - 15 seconds - silence
- 15 - 20 seconds - sound #4

Actual

The timeframe during sound #3 doesn't generate sound #3, but generates a strange static/garbled noise that's not seen during the timeframe of sound #1. As a result, the silence seen during sound #1 timeframe is not seen during the timeframe of sound #3.

I'm thinking this is a bug in the media encoder potentially.

Comment 1

[Ivan Tsay (:ITsay)]

Since this one block the audio recording, flag it as koi+. Also add keyword in white board.

Comment 2

[Shelly Lin [:shelly]]

It sounds like an encoding issue so far, I'm working on this bug.

Comment 3

[Shelly Lin [:shelly]]

Attachment: Use memset instead of AppendElement

When appending zero value to the pcm buffer (type AudioDataValue), memset the pcm buffer with 0 instead of AppendElement(0) to the pcm buffer.

Comment 4

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

Comment on attachment 788851 [details] [diff] [review]
Use memset instead of AppendElement

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

::: content/media/encoder/OpusTrackEncoder.cpp
@@ +273,5 @@
>        // Append the interleaved data to the end of pcm buffer.
>        InterleaveTrackData(chunk, frameToCopy, mChannels,
>                            pcm.Elements() + frameCopied);
>      } else {
> +      memset(pcm.Elements(), 0, frameToCopy*mChannels*sizeof(AudioDataValue));

This change doesn't seem correct. You could be overwriting data you previously put into the array.

@@ +299,5 @@
>    // Append null data to pcm buffer if the leftover data is not enough for
>    // opus encoder.
>    if (frameCopied < GetPacketDuration() && mEndOfStream) {
> +    memset(pcm.Elements(), 0, (GetPacketDuration()-frameCopied)*mChannels*
> +           sizeof(AudioDataValue));

Same here.

Comment 5

[Shelly Lin [:shelly]]

Attachment: V2: Use memset instead of AppendElement

AppendElement() appends data after the length of array. Since we have set the length of pcm to a fixed size prior to the process of interleaving raw data, using AppendElement(0) will append zero data at the end of pcm's length and will expand the size of pcm in the same time.

This patch should correct the calculation of pcm offset when appending zero data. Thanks for pointing that out :)

Comment 6

[Shelly Lin [:shelly]]

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=8b6f972eb359

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68e65f7a0f25

Comment 10

[Jason Smith [:jsmith]]

Verified on trunk on 8/15 - I'm getting silence on the 2nd try now.