902856 - Should MediaEngineDefaultAudioSource generate real data?

Status: RESOLVED FIXED

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

Description

[StevenLee[:slee]]

MediaEngineDefaultAudioSource generates null data and inserts to MSG. It makes every objects using MediaEngineDefaultAudioSource needs to construct the audio data. Should we generate default fake audio data in it?

[1] http://mxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineDefault.cpp#378

Comment 1

[Ivan Tsay (:ITsay)]

Add keyword in white board for tracking in FFOS 1.2 media recording team

Comment 2

[Shelly Lin [:shelly]]

I'd vote for yes on this, strongly.

I understand the purpose of letting you construct the audio data at your own kind, but it just seems very bizarre and confusing that a fake stream has every thing but the audio data. Plus, it duplicates the effort of creating audio data if any module wants to test with this fake stream.

Comment 3

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

IMO, I suggest that the fake audio should generate sound-able PCM data for testing. Just like fake video.

Comment 4

[Ivan Tsay (:ITsay)]

Hi Randell, We need your comment about this bug.

Ivan Tsay

Comment 5

[Ivan Tsay (:ITsay)]

Since this is the blocker on testing get user media, flag it to koi+

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

I agree it should generate real data (and perhaps not zeros; we have something that generates music-y fake data (in mediaconduit-unittests.cpp) that might be appropriate, if it doesn't break tests - worth a try).

NOTE: please test all the unit tests (add export MOZ_WEBRTC_TESTS=1 to .mozconfig, and set it locally in your shell, then build mozilla and run all the *unittest* executables built in objdir/dist/bin on Mac or Linux, and also run "./mach mochitest-plain dom/media/tests/mochitest" and "./mach crashtest dom/media/crashtests".  Changing the MediaDefault behavior might break tests.

Comment 7

[StevenLee[:slee]]

Attachment: Generates fake audio in MediaEngineDefaultAudioSource

Here is the try server results.
* try: -b o -p linux64,macosx64,panda,unagi -u all -t none
** https://tbpl.mozilla.org/?tree=Try&rev=d57d9cf6edda

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 790596 [details] [diff] [review]
Generates fake audio in MediaEngineDefaultAudioSource

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

r- because this won't actually generate useful "music".  either make a stateful generator (not hard), or use a small fixed array and loop it.  I suggest the first.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +334,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
>    mState = kAllocated;
> +  

trailing space

@@ +414,5 @@
>  {
>    AudioSegment segment;
> +  nsRefPtr<SharedBuffer> buffer = SharedBuffer::Create(AUDIO_FRAME_LENGTH * sizeof(int16_t));
> +  int16_t* dest = static_cast<int16_t*>(buffer->Data());
> +  

trailing space

@@ +415,5 @@
>    AudioSegment segment;
> +  nsRefPtr<SharedBuffer> buffer = SharedBuffer::Create(AUDIO_FRAME_LENGTH * sizeof(int16_t));
> +  int16_t* dest = static_cast<int16_t*>(buffer->Data());
> +  
> +  memcpy(dest, mAudioBuffer, AUDIO_FRAME_LENGTH * sizeof(int16_t));

The problem here is that GenerateMusic assumes you'll pass it a much larger buffer to generate into (since it's some type of feedback loop with randomness) in order for it to appear like "music".  This code will produce an endlessly-looping static 10ms chunk; if that's all we wanted we should just define a small static array.

The mediaconduit unit tests this is taken from generates 10 seconds of data to use.

You would need to modify it to allow you to pass in the state from the last call, and move most of the local state vars to a state structure.  I.e. an object you ask to obj->GenerateMoreMusic(buffer, samples_to_generate) roughly

Comment 9

[StevenLee[:slee]]

Attachment: patch v2

1. Generate sine wave audio 
2. Change the timer type. Audio needs more precise callback.

Comment 10

[StevenLee[:slee]]

Attachment: patch v2.1

Fix trailing space.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 792073 [details] [diff] [review]
patch v2.1

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

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +266,5 @@
> +    mTotalLength(length_in_samples),
> +    mReadLength(0) {
> +    mAudioBuffer = new int16_t[mTotalLength];
> +    for(int i = 0; i < mTotalLength - 1; i++) {
> +      mAudioBuffer[i] = (32767.0f * sin(millisecondsPerSecond * bytesPerSample * M_PI * i / audioRate));

This is likely too loud (0db it appears) and can cause clipping or distortion elsewhere.  It should take a db or other volume level.  And why is bytesPerSample in the sin() calc?  And what's the frequency? (that should be an input, unless we want it to just derive from length_in_samples, in which case we should likely replace length-in-samples with frequency and calculate the length - my preference.)

@@ +270,5 @@
> +      mAudioBuffer[i] = (32767.0f * sin(millisecondsPerSecond * bytesPerSample * M_PI * i / audioRate));
> +    }
> +  }
> +
> +  void generateMusic(void* buffer, int16_t length_in_samples) {

Not exactly music.... generateWave or generateSin

@@ +271,5 @@
> +    }
> +  }
> +
> +  void generateMusic(void* buffer, int16_t length_in_samples) {
> +    if (mTotalLength - mReadLength > length_in_samples) {

See below for detailed comments.  This needs to be recoded as a while() or equivalent.

@@ +274,5 @@
> +  void generateMusic(void* buffer, int16_t length_in_samples) {
> +    if (mTotalLength - mReadLength > length_in_samples) {
> +      memcpy(buffer, mAudioBuffer + mReadLength, length_in_samples * bytesPerSample);
> +      mReadLength += length_in_samples;
> +      if (mReadLength > mTotalLength) {

Shouldn't that be ==?  or for the paranoid, >=
Given the if() above, > should be impossible.

@@ +282,5 @@
> +      int16_t round1 = mTotalLength - mReadLength;
> +      int16_t round2 = length_in_samples - round1;
> +
> +      memcpy(buffer, mAudioBuffer + mReadLength, round1 * bytesPerSample);
> +      memcpy(buffer, mAudioBuffer, round2 * bytesPerSample);

This is totally broken if length_in_samples > 2* mTotalLength (and for smaller values if mReadLength > 0)

@@ +378,5 @@
>    mTrackID = aID;
>  
>    // 1 Audio frame per Video frame
>    mTimer->InitWithCallback(this, MediaEngine::DEFAULT_AUDIO_TIMER_MS,
> +                           nsITimer::TYPE_REPEATING_PRECISE);

This is good; we should do this in any case

Comment 12

[StevenLee[:slee]]

(In reply to Randell Jesup [:jesup] from comment #11)
> ::: content/media/webrtc/MediaEngineDefault.cpp
> @@ +266,5 @@
> > +    mTotalLength(length_in_samples),
> > +    mReadLength(0) {
> > +    mAudioBuffer = new int16_t[mTotalLength];
> > +    for(int i = 0; i < mTotalLength - 1; i++) {
> > +      mAudioBuffer[i] = (32767.0f * sin(millisecondsPerSecond * bytesPerSample * M_PI * i / audioRate));
> 
> This is likely too loud (0db it appears) and can cause clipping or
> distortion elsewhere.  It should take a db or other volume level.  And why
How could I take a db, divide by 2?
> is bytesPerSample in the sin() calc?  And what's the frequency? (that should
I am sorry, I think it should be (2 * M_PI * i) * i / audioRate.  
> be an input, unless we want it to just derive from length_in_samples, in
> which case we should likely replace length-in-samples with frequency and
> calculate the length - my preference.)
I will change it.

> @@ +274,5 @@
> > +  void generateMusic(void* buffer, int16_t length_in_samples) {
> > +    if (mTotalLength - mReadLength > length_in_samples) {
> > +      memcpy(buffer, mAudioBuffer + mReadLength, length_in_samples * bytesPerSample);
> > +      mReadLength += length_in_samples;
> > +      if (mReadLength > mTotalLength) {
> 
> Shouldn't that be ==?  or for the paranoid, >=
> Given the if() above, > should be impossible.
I will modify in the next version.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

(In reply to StevenLee from comment #12)
> (In reply to Randell Jesup [:jesup] from comment #11)
> > ::: content/media/webrtc/MediaEngineDefault.cpp
> > @@ +266,5 @@
> > > +    mTotalLength(length_in_samples),
> > > +    mReadLength(0) {
> > > +    mAudioBuffer = new int16_t[mTotalLength];
> > > +    for(int i = 0; i < mTotalLength - 1; i++) {
> > > +      mAudioBuffer[i] = (32767.0f * sin(millisecondsPerSecond * bytesPerSample * M_PI * i / audioRate));
> > 
> > This is likely too loud (0db it appears) and can cause clipping or
> > distortion elsewhere.  It should take a db or other volume level.  And why
> How could I take a db, divide by 2?

db are logarithmic; I think the calculation is 20*log10(max_positive_value/0x8000). 32768 would be 0db. halving it to 16384 would be -6db, which is a reasonable max recording level, but lower levels are fine too - -6db is loud.

Comment 14

[StevenLee[:slee]]

Attachment: patch v3

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 797711 [details] [diff] [review]
patch v3

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

r- for the unneeded multiple cycles and the past-nyquist frequency.
r? to derf over the impact of using a sine wave as a test signal; originally I was assuming (and the first patch was intended to) use the "Music" generator from the unit tests as a source; I'm not sure why this became a sine wave, and I'm concerned how Opus will react to it.  Likely it's fine, even if Opus is in "speech" mode, but I want to check.  I'm assuming this is to allow audible verification that a signal is actually getting through (and allow tests to look for non-zero audio data).
While I'm ok with using a sine wave, I suspect the "music" generator would be better overall.  However, we're close to having this one done and I think it will fulfill the purpose.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +263,5 @@
> +public:
> +  const int bytesPerSample = 2;
> +  const int millisecondsPerSecond = 1000;
> +
> +  SineWaveGenerator(int frequency, int duration_in_MSec) :

aFrequency, aDurationInMsec

@@ +264,5 @@
> +  const int bytesPerSample = 2;
> +  const int millisecondsPerSecond = 1000;
> +
> +  SineWaveGenerator(int frequency, int duration_in_MSec) :
> +    mTotalLength(frequency * duration_in_MSec / millisecondsPerSecond),

There's very little if any reason to have more than one cycle since this is a repeating sine wave.  Every cycle is identical.  So the length should be the length of one cycle.  (If we were concerned with absolute fidelity in the sinewave for any possible input frequency, we'd extend it to the lowest common denominator that allows it to repeat without any fractional part, but a) we don't care that much about a perfect pure wave, and b) we can choose a frequency with no fractional part after one cycle.)

@@ +337,5 @@
>    }
>  
>    mState = kAllocated;
> +  // generate 1 second sine wave
> +  mSineGenerator = new SineWaveGenerator(AUDIO_RATE, 1000);

16000Hz is not representable in a 16000Hz sampling rate (Nyquist limits).

Choose something in the voice band; say 500Hz or 1KHz.

Comment 16

[StevenLee[:slee]]

(In reply to Randell Jesup [:jesup] from comment #15)
> Comment on attachment 797711 [details] [diff] [review]
> patch v3
> 
> Review of attachment 797711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the unneeded multiple cycles and the past-nyquist frequency.
> r? to derf over the impact of using a sine wave as a test signal; originally
> I was assuming (and the first patch was intended to) use the "Music"
> generator from the unit tests as a source; I'm not sure why this became a
> sine wave, and I'm concerned how Opus will react to it.  Likely it's fine,
I am not very sure what kind of music(sounds) is good and I'm not familiar with the related topics. After discussing with others, I think sine wave should be OK. If other types of sounds are better, I can change the generator. :)

Comment 17

[StevenLee[:slee]]

Attachment: patch v4

Hi jesup,

Here is the updated patch. I am not sure if I understand you correctly.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 798462 [details] [diff] [review]
patch v4

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

Check the volume in a test, and see if it's good or should be a bit higher.  If the volume needs adjusting, just adjust it before checkin, no re-review needed.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +338,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
>    mState = kAllocated;
> +  // generate 1 second sine wave

1KHz, not 1 second

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 798462 [details] [diff] [review]
patch v4

Moving r? to derf to latest patch (see earlier comment as to what the question is)

Comment 20

[Ivan Tsay (:ITsay)]

Audio recording can be tested now. this one is not a blocker for v1.2

Comment 21

[Jason Smith [:jsmith]]

(In reply to Ivan Tsay (:ITsay) from comment #20)
> Audio recording can be tested now. this one is not a blocker for v1.2

Agreed. Minusing as such.

Comment 22

[Timothy B. Terriberry (:derf)]

Comment on attachment 798462 [details] [diff] [review]
patch v4

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

Opus has been tested with sine waves, and indeed, it did cause some LPC analysis instability in SILK, but that should have been fixed over a year ago. If it blows up now, I'd want to know about it.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +267,5 @@
> +  const int frequency = 1000;
> +
> +  SineWaveGenerator(int aSampleRate) :
> +    mTotalLength(aSampleRate / frequency),
> +    mReadLength(0) {

You should assert that mTotalLength*frequency == aSampleRate. Otherwise you'll generate a tone at a slightly different frequency.

Comment 23

[StevenLee[:slee]]

(In reply to Timothy B. Terriberry (:derf) from comment #22)
> Comment on attachment 798462 [details] [diff] [review]
> patch v4
> 
> Review of attachment 798462 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Opus has been tested with sine waves, and indeed, it did cause some LPC
> analysis instability in SILK, but that should have been fixed over a year
> ago. If it blows up now, I'd want to know about it.
I tested by using 2 peerconnections and they work fine.

Here is the try server result.
https://tbpl.mozilla.org/?tree=Try&rev=94fc3d2b0bcb

Comment 24

[StevenLee[:slee]]

Attachment: patch v4.1

address comment 22

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f2a91e0445a

Friendly reminder that your commit message should be stating what the patch is actually doing, not re-stating the bug summary.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f2a91e0445a

Comment 27

[StevenLee[:slee]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2f2a91e0445a
> 
> Friendly reminder that your commit message should be stating what the patch
> is actually doing, not re-stating the bug summary.
OK, I thought it's simple patch. 
Thanks for your reminding. :)