Should MediaEngineDefaultAudioSource generate real data?

RESOLVED FIXED in mozilla26

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: slee, Assigned: slee)

Tracking

unspecified
mozilla26
Points:
---

Firefox Tracking Flags

(blocking-b2g:-)

Details

(Whiteboard: [FT: Media Recording, Sprint 2][qa-])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
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

4 years ago
Add keyword in white board for tracking in FFOS 1.2 media recording team
Whiteboard: [FT: Media Recording, Sprint 2]

Comment 2

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

Updated

4 years ago
Blocks: 903060

Updated

4 years ago
Blocks: 895730

Updated

4 years ago
Blocks: 901798
IMO, I suggest that the fake audio should generate sound-able PCM data for testing. Just like fake video.

Comment 4

4 years ago
Hi Randell, We need your comment about this bug.

Ivan Tsay
Flags: needinfo?(rjesup)

Comment 5

4 years ago
Since this is the blocker on testing get user media, flag it to koi+
blocking-b2g: --- → koi+
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.
Flags: needinfo?(rjesup)

Updated

4 years ago
No longer blocks: 901798

Updated

4 years ago
No longer blocks: 903060
(Assignee)

Comment 7

4 years ago
Created attachment 790596 [details] [diff] [review]
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
Attachment #790596 - Flags: review?(rjesup)
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
Attachment #790596 - Flags: review?(rjesup) → review-
(Assignee)

Comment 9

4 years ago
Created attachment 792066 [details] [diff] [review]
patch v2

1. Generate sine wave audio 
2. Change the timer type. Audio needs more precise callback.
Attachment #790596 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 792073 [details] [diff] [review]
patch v2.1

Fix trailing space.
Attachment #792066 - Attachment is obsolete: true
Attachment #792073 - Flags: review?(rjesup)
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
Attachment #792073 - Flags: review?(rjesup) → review-
(Assignee)

Comment 12

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

Comment 14

4 years ago
Created attachment 797711 [details] [diff] [review]
patch v3
Attachment #792073 - Attachment is obsolete: true
Attachment #797711 - Flags: review?(rjesup)
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.
Attachment #797711 - Flags: review?(tterribe)
Attachment #797711 - Flags: review?(rjesup)
Attachment #797711 - Flags: review-
(Assignee)

Comment 16

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

Comment 17

4 years ago
Created attachment 798462 [details] [diff] [review]
patch v4

Hi jesup,

Here is the updated patch. I am not sure if I understand you correctly.
Attachment #798462 - Flags: review?(rjesup)
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
Attachment #798462 - Flags: review?(rjesup) → review+
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)
Attachment #798462 - Flags: review?(tterribe)

Updated

4 years ago
Attachment #797711 - Attachment is obsolete: true
Attachment #797711 - Flags: review?(tterribe)

Comment 20

4 years ago
Audio recording can be tested now. this one is not a blocker for v1.2
blocking-b2g: koi+ → koi?
(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.
blocking-b2g: koi? → -
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.
Attachment #798462 - Flags: review?(tterribe) → review+
(Assignee)

Comment 23

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

Comment 24

4 years ago
Created attachment 799979 [details] [diff] [review]
patch v4.1

address comment 22
Attachment #798462 - Attachment is obsolete: true
Attachment #799979 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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.
Assignee: nobody → slee
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f2a91e0445a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(Assignee)

Comment 27

4 years ago
(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. :)

Updated

4 years ago
Whiteboard: [FT: Media Recording, Sprint 2] → [FT: Media Recording, Sprint 2][qa-]
You need to log in before you can comment on or make changes to this bug.