911450 - sndio webrtc audio backend

Status: RESOLVED FIXED

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

Description

[Landry Breuil (:gaston)]

As it was done for cubeb in #851149, we need an sndio backend for webrtc's audio_device module to get rid of the behemoth^Wpulseaudio dependency. Have a placeholder bug.

Comment 1

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

Note: BSD issue

Comment 2

[Landry Breuil (:gaston)]

Attachment: sndio backend, build bits

Comment 3

[Landry Breuil (:gaston)]

Attachment: webrtc sndio backend v0.0

Comment 4

[Landry Breuil (:gaston)]

So alex has started the sndio backend for webrtc, and we're at the point where it builds. I did the gyp magic to integrate it quickly, but this is far from acceptable since it makes the assumption that sndio will only be used on openbsd while the daemon & lib exists and works on linux (it is in arch linux packages for example).

As for the sound part itself, i'm unsure if it works or not - the gum audio test says 'success' but i cant hear anything feedbacking while talking in the mic, and the second time i try to share the mic device with the webpage an assertion triggers:

assertion "false" failed: file "/home/landry/src/m-c/media/webrtc/trunk/webrtc/modules/audio_device/audio_device_buffer.cc", line 406, function "SetRecordedBuffer"

I'm uploading the patches to have them somewhere visible so more ppl can look at it but there will be a lot of iterations imo - as for the build bits, i wonder if we should have an 'include_sndio_audio' flag in configure.in with a LINUX_SNDIO #define at the same level as include_alsa_audio/LINUX_ALSA & include_pulse_audio/LINUX_PULSE, so that it can be used elsewhere.

Comment 5

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #4)
> it makes the assumption that sndio will only be used on openbsd
> while the daemon & lib exists and works on linux (it is in
> arch linux packages for example).

Similar assumption also exists under media/libcubeb but not upstream. And Linux may want late binding in order to replace alsa with either sndio or pulse at runtime.

Comment 6

[Landry Breuil (:gaston)]

Attachment: sndio backend, build bits

Here's a new version rebased on top of webrtc 40 update, also decoupling a bit sndio from openbsd so that testing can also happen on linux (where sndio is available).

Randall, i'd like to get the skeleton/basic version in soonish so that fixes/improvements can happen on top, instead of carrying "big" patches around.. what's your opinion on that ? What do you think of the build bits ?

Comment 7

[Landry Breuil (:gaston)]

Attachment: webrtc sndio backend v0.0

And here's the actual backend, nothing changing much compared to attachment 8348238 [details] [diff] [review], only removing two *Available functions which are not anymore in the API afaict. Currently only build-tested of course.

Comment 8

[Landry Breuil (:gaston)]

Attachment: sndio backend, build bits

And here's a newer version of the build bits, ensuring WEBRTC_AUDIO_SNDIO is defined earlier so that it's also here when audio_device_impl.cc is compiled, and also creating an AudioDeviceSndio() object which wasnt the case in the previous one.

With this, i'm able to get the doorhang popup for the mic on gum_test.html, and i can try a record test on https://www.webrtc-experiment.com/RecordRTC/. It fails at runtime though:

-2102497024[80f4b700]: STREAM    ; ( 1:14:22: 95 |    0)        VOICE:    1    99;2121393664; VoEBaseImpl::RecordedDataIsAv
ailable(nSamples=480, nBytesPerSample=4, nChannels=255, samplesPerSec=48000, totalDelayMS=0, clockDrift=0, micLevel=0)
-2102497024[80f4b700]: STREAM    ; ( 1:14:22: 95 |    0)        VOICE:    1    99;2121393664; TransmitMixer::PrepareDemux(n
Samples=480, nChannels=255,samplesPerSec=48000, totalDelayMS=0, clockDrift=0,currentMicLevel=0)
-2102497024[80f4b700]: STREAM    ; ( 1:14:22: 95 |    0) AUDIO CODING:    1     0;2121393664; SendCodec()
-2102497024[80f4b700]: ERROR     ; ( 1:14:22: 95 |    0)                          2121393664; (utility.cc:106): InitializeIfNeeded failed: sample_rate_hz=48000, destination_rate=16000, num_channels=255
-2102497024[80f4b700]: ERROR     ; ( 1:14:22: 95 |    0)                          2121393664; (utility.cc:114): Resample failed: src_data=0x89cd4042, in_length=122400, dst_af->data_=0x8114c048
-2102497024[80f4b700]: ERROR     ; ( 1:14:22: 95 |    0)                          2121393664; (transmit_mixer.cc:1276): ProcessStream() error: -9


But that seems like an sndio backend issue. If i stop sharing the mic too, the webrtc logs keeps filing with lines like:

-2102497024[80f4b700]: STREAM    ; ( 1:21:46:165 |    0)        VOICE:    1     0;2216663040; Channel::SendRTCPPacket(channel=0, len=8)
-2102497024[80f4b700]: STREAM    ; ( 1:21:46:603 |  438)        VOICE:    1    99;2216663040; TransmitMixer::OnPeriodicProcess()

but i dont know if that's expected or not.

Comment 9

[Landry Breuil (:gaston)]

Attachment: sndio backend, build bits

Sorry, wrong diff posted, this one has the define WEBRTC_AUDIO_SNDIO at the right spot. should.

Comment 10

[Landry Breuil (:gaston)]

Attachment: WebRTC.log

full webrtc log from a failing audio session, and from a successful video session.

Comment 11

[Landry Breuil (:gaston)]

looking at the errors in the log, it seems the # of channels is set two times:

APICALL   ; (12:36:20:543 |  490)        VOICE:    1    99;2286523904; SetRecordingDevice(index=0, recordingChannel=2)
STATEINFO ; (12:36:20:544 |  475) AUDIO DEVICE:    1    99;2286523904; output: available=1
MEMORY    ; (12:36:20:544 |    0) AUDIO DEVICE:    1    99;2286523904; AudioDeviceBuffer::SetRecordingChannels(channels=2)

and then to 255 (!?)

MEMORY    ; (12:36:20:598 |    1) AUDIO DEVICE:    1    99;2286523904; AudioDeviceBuffer::SetRecordingSampleRate(fsHz=48000)
MEMORY    ; (12:36:20:598 |    0) AUDIO DEVICE:    1    99;2286523904; AudioDeviceBuffer::SetRecordingChannels(channels=255)

not sure if that's expected..

Comment 12

[Landry Breuil (:gaston)]

there was a mistaken copy&paste in the sndio backend, where a pchan was used instead of an rchan, thus not initializing rchan to 2 and falling back to 255. Fixing this at least shows no more errors in webrtc log, but it seems i only record silence on https://www.webrtc-experiment.com/RecordRTC/

Comment 13

[Landry Breuil (:gaston)]

Okay, it seems *now* i can record some stuff properly. Another issue is that if i do 'stop' on gum_test.html or recordRTC page, the log keeps filing like crazy as if the recording wasnt properly stopped, i need to stop the sharing of the mic to actually stop the recording.

Comment 14

[Landry Breuil (:gaston)]

so local echo "works". there are still lots of other issues/slowdowns not only related to sound, we didnt manage to get a/v (or audio or video only) p2p chat working, but at least it doesnt crash horribly (well not always) - so would be good to get this in as a starter.

Comment 15

[Landry Breuil (:gaston)]

Attachment: webrtc sndio backend v0.1

And here's the version that "works" - only change is fixing the pchan/rchan inversion in the recording path. With that i have local echo working.

Comment 16

[Alexandre Ratchov]

Comment on attachment 8588292 [details] [diff] [review]
sndio backend, build bits

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

works for me, builds fine

Comment 17

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

Comment on attachment 8588336 [details] [diff] [review]
webrtc sndio backend v0.1

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

::: media/webrtc/trunk/webrtc/modules/audio_device/sndio/audio_device_sndio.cc
@@ +16,5 @@
> +#include "webrtc/modules/audio_device/sndio/audio_device_sndio.h"
> +#include "webrtc/system_wrappers/interface/thread_wrapper.h"
> +#include "webrtc/system_wrappers/interface/trace.h"
> +
> +extern "C"

please add a separate patch to add #include Latency.h, and add LOG_FIRST_CAPTURE/LOG_CAPTURE_FRAMES.  Can be cloned from audio_device_alsa_linux.cc.

@@ +55,5 @@
> +AudioDeviceSndio::~AudioDeviceSndio()
> +{
> +    WEBRTC_TRACE(kTraceMemory, kTraceAudioDevice, _id,
> +                 "%s destroyed", __FUNCTION__);
> +    

trailing spaces

@@ +74,5 @@
> +
> +void AudioDeviceSndio::AttachAudioBuffer(AudioDeviceBuffer* audioBuffer)
> +{
> +    _ptrAudioBuffer = audioBuffer;
> +    _ptrAudioBuffer->SetRecordingSampleRate(0);

copy comment from the alsa code

@@ +83,5 @@
> +
> +int32_t AudioDeviceSndio::ActiveAudioLayer(
> +    AudioDeviceModule::AudioLayer& audioLayer) const
> +{
> +    audioLayer = AudioDeviceModule::kPlatformDefaultAudio;

Shouldn't we be creating a ::kSndIOAudio and using that here, and then adding it to appropriate switch statements like in voe_hardware_impl.cc (Set/GetAudioDeviceLayer for example, and AudioDeviceModuleImpl::ActiveAudioLayer()).

Also modify the tests if possible to use kSndIOAudio: media/webrtc/trunk/webrtc/modules/audio_device/test/audio_device_test_api.cc and media/webrtc/trunk/webrtc/modules/audio_device/test/func_test_manager.cc.  This will make uplifting this fix to webrtc.org much easier, thanks!

@@ +106,5 @@
> +    {
> +        return 0;
> +    }
> +
> +    if (_playing)

Do we need to grab the critsect here?

@@ +162,5 @@
> +}
> +
> +int32_t AudioDeviceSndio::SetSpeakerVolume(uint32_t volume)
> +{
> +    // XXX: locking + call sio_onvol()

as stated, locking

@@ +169,5 @@
> +}
> +
> +int32_t AudioDeviceSndio::SpeakerVolume(uint32_t& volume) const
> +{
> +    // XXX: copy value reported by sio_onvol() call-back

ditto, and likely returns the wrong value

@@ +175,5 @@
> +    return 0;
> +}
> +
> +int32_t AudioDeviceSndio::SetWaveOutVolume(uint16_t volumeLeft,
> +                                               uint16_t volumeRight)

indent

@@ +177,5 @@
> +
> +int32_t AudioDeviceSndio::SetWaveOutVolume(uint16_t volumeLeft,
> +                                               uint16_t volumeRight)
> +{
> +    // XXX: what's that ?!

remove comment

@@ +187,5 @@
> +int32_t AudioDeviceSndio::WaveOutVolume(
> +    uint16_t& /*volumeLeft*/,
> +    uint16_t& /*volumeRight*/) const
> +{
> +    // XXX: what's that ?!

remove comment

@@ +318,5 @@
> +}
> +
> +int32_t AudioDeviceSndio::SetAGC(bool enable)
> +{
> +    // XXX: could we return -1, as we dont expose mic level knob ?

AGC as a feature is separate, but probably we should silently ignore attempts to enable it and return false from AGC(), to avoid calculating AGC values that will have no affect.

@@ +419,5 @@
> +    {
> +        return -1;
> +    }
> +
> +    strlcpy(name, SIO_DEVANY, kAdmMaxDeviceNameSize);

strlcpy is non-standard, but standard on *BSDs, so is ok here

@@ +524,5 @@
> +	_playHandle = NULL;
> +	return -1;
> +    }
> +
> +    // XXX: check params here

Need to resolve this

@@ +593,5 @@
> +	_recHandle = NULL;
> +	return -1;
> +    }
> +
> +    // XXX: check params here

ditto

@@ +642,5 @@
> +    if (_ptrThreadRec == NULL)
> +    {
> +        WEBRTC_TRACE(kTraceCritical, kTraceAudioDevice, _id,
> +                     "  failed to create the rec audio thread");
> +        return -1;

delete the buffer?

@@ +652,5 @@
> +        WEBRTC_TRACE(kTraceCritical, kTraceAudioDevice, _id,
> +                     "  couldn't start recorording");
> +        delete _ptrThreadRec;
> +        _ptrThreadRec = NULL;
> +	return -1;

delete the buffer?

@@ +654,5 @@
> +        delete _ptrThreadRec;
> +        _ptrThreadRec = NULL;
> +	return -1;
> +    }
> +    

trailing spaces (and elsewhere, delete-trailing-spaces in emacs ;-) )

@@ +662,5 @@
> +                     "  failed to start the rec audio thread");
> +	sio_stop(_recHandle);
> +        delete _ptrThreadRec;
> +        _ptrThreadRec = NULL;
> +        return -1;

delete the buffer?

@@ +691,5 @@
> + 
> +    sio_stop(_recHandle);
> +
> +    delete _ptrThreadRec;
> +    _ptrThreadRec = NULL;

delete buffer?

@@ +733,5 @@
> +    if (_ptrThreadPlay == NULL)
> +    {
> +        WEBRTC_TRACE(kTraceCritical, kTraceAudioDevice, _id,
> +                     "    failed to create the play audio thread");
> +        return -1;

delete buffer (and in other failure spots)

@@ +778,5 @@
> +
> +    sio_stop(_playHandle);
> +
> +    delete _ptrThreadPlay;
> +    _ptrThreadPlay = NULL;

delete buffer

@@ +784,5 @@
> +}
> +
> +int32_t AudioDeviceSndio::PlayoutDelay(uint16_t& delayMS) const
> +{
> +    _critSect.Enter();

CriticalSectionScoped lock(&_critSect);

@@ +792,5 @@
> +}
> +
> +int32_t AudioDeviceSndio::RecordingDelay(uint16_t& delayMS) const
> +{
> +    _critSect.Enter();

CriticalSectionScoped lock(&_critSect);

@@ +843,5 @@
> +}
> +
> +bool AudioDeviceSndio::PlayoutError() const
> +{
> +    return _playError;

implicit conversion - make explicit
Does this need locking?  Alsa does

@@ +853,5 @@
> +}
> +
> +bool AudioDeviceSndio::RecordingError() const
> +{
> +    return _recError;

implicit conversion, make explicit
Does this need locking?  (Alsa does)

@@ +890,5 @@
> +    int n;
> +    bool res;
> +
> +    res = true;
> +    _critSect.Enter();

CriticalSectionScoped lock(&_critSect);

@@ +891,5 @@
> +    bool res;
> +
> +    res = true;
> +    _critSect.Enter();
> +    // XXX: update volume here

resolve

@@ +913,5 @@
> +    int8_t *data;
> +    bool res;
> +
> +    res = true;
> +    _critSect.Enter();

CriticalSectionScoped lock(&_critSect);

@@ +931,5 @@
> +    }
> +    if (res) {
> +	//  _ptrAudioBuffer->SetVQEData(
> +	//  _playDelay * 1000 / _playParams.rate,
> +	//  _recDelay * 1000 / _recParams.rate, 0);

Resolve this...

::: media/webrtc/trunk/webrtc/modules/audio_device/sndio/audio_device_sndio.h
@@ +160,5 @@
> +    bool RecThreadProcess();
> +    bool PlayThreadProcess();
> +
> +private:
> +    struct sio_hdl *_playHandle, *_recHandle;

nit: "foo* _bar" not "foo *_bar" for consistency

@@ +164,5 @@
> +    struct sio_hdl *_playHandle, *_recHandle;
> +    struct sio_par _playParams, _recParams;
> +
> +    AudioDeviceBuffer* _ptrAudioBuffer;
> +    

remove trailing spaces

Comment 18

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e77c92656fd

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e77c92656fd

Comment 20

[Landry Breuil (:gaston)]

Hm, by my fault, since i landed the build bits, now the build will always fail on openbsd because it cant find the sndio implem, of course;

File listed in UNIFIED_SOURCES does not exist: '/home/buildslave-amd64/comm-central-amd64/build/mozilla/media/webrtc/trunk/webrtc/modules/audio_device/sndio/audio_device_sndio.cc'

randall, what do you think of landing what we have now, improving/fixing it in tree ?

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d297060e599

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d297060e599

Comment 23

[Landry Breuil (:gaston)]

If we dont land what we have now, can we at least have a skeleton ?

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d297060e599

Comment 25

[Landry Breuil (:gaston)]

randall, 39 is now in beta, would have loved to get some initial feedback on the sndio backend with it... so do you think we can land what we have now, even unfinished/buggy but so that it fixes the build failure because of the missing files when building beta/aurora/central on OpenBSD ? this is still NPOTB..

Comment 26

[Landry Breuil (:gaston)]

Attachment: Apply review comments from #17

This should address most of your review comments from comment #17.

Note that this applies on top of attachment #8588336 [details] [diff] [review] to ease diffability, but i can of course qfold the two patches if you prefer to review only one.

- trailing spaces/tabs vs spaces fixed
- moved some stuff around to ease diffability with linux/audio_device_alsa_linux.cc
- use the new construct for critical section handling/locking
- added kSndioAudio and kAudioSndio values to enums, amended tests accordingly
- renamed _recBuffer to _recordingBuffer and _playBuffer to _playoutBuffer like in linux/alsa
- fix indentations
- delete buffers in failure cases

Also note that i *didnt* remove all the XXX comments nor fixed the params checking, because i'd really like to handle those in a second time with *smaller* diffs, and get the most of this in the beginning of this cycle for 45/next ESR. Runtime doesnt work yet... but that shouldnt prevent NPOTB code from landing in the tree :)

Also ratchov (the original author) just recently got a baby, so there'll be some time before he can get back to this code... and i's already been waiting for 6+ months :)

Comment 27

[Landry Breuil (:gaston)]

Note that there was a stray _playoutFramesLeft that crept in a copypasto that was breaking the build - removed locally.

Comment 28

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

Comment on attachment 8681683 [details] [diff] [review]
Apply review comments from #17

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

General cleanup nit: ensure spaces not tabs for whitespace; don't insert blank lines where other code doesn't have them unless explicitly to improve readability.   Fix accidental double-else

::: media/webrtc/trunk/webrtc/modules/audio_device/audio_device_impl.cc
@@ +632,1 @@
>      else

doubled 'else'

::: media/webrtc/trunk/webrtc/modules/audio_device/sndio/audio_device_sndio.cc
@@ +34,5 @@
>      }
>  
>      static void recOnmove(void *arg, int delta)
>      {
> +        static_cast<webrtc::AudioDeviceSndio *>(arg)->_recDelay += delta;

Did these just change whitespace?

@@ +125,5 @@
>  }
>  
>  int32_t AudioDeviceSndio::Terminate()
>  {
> +

don't insert blank line here

@@ +340,5 @@
>  }
>  
>  int32_t AudioDeviceSndio::StereoPlayout(bool& enabled) const
>  {
> +

Don't insert blank lines

@@ +351,5 @@
>  }
>  
>  int32_t AudioDeviceSndio::SetAGC(bool enable)
>  {
> +

ditto

@@ +499,3 @@
>      else {
> +        sio_close(hdl);
> +        available = true;

All whitespace?  Not sure which way this went, but normally we prefer spaces over tabs

@@ +518,5 @@
>  }
>  
>  int32_t AudioDeviceSndio::InitPlayout()
>  {
> +

remove blank line

@@ +632,5 @@
>  
>      _recFrames = _recParams.rate / 100;
> +    _recordingBuffer = new int8_t[_recFrames *
> +                            _recParams.bps *
> +                            _recParams.rchan];

indentation

@@ +634,5 @@
> +    _recordingBuffer = new int8_t[_recFrames *
> +                            _recParams.bps *
> +                            _recParams.rchan];
> +    if (_recordingBuffer == NULL)
> +        {

indent is wrong

@@ +844,5 @@
>  
>  int32_t AudioDeviceSndio::PlayoutDelay(uint16_t& delayMS) const
>  {
> +    CriticalSectionScoped lock(&_critSect);
> +    delayMS = (uint16_t) _playDelay * 1000 / _playParams.rate;

Add parens around numerator

@@ +851,5 @@
>  
>  int32_t AudioDeviceSndio::RecordingDelay(uint16_t& delayMS) const
>  {
> +    CriticalSectionScoped lock(&_critSect);
> +    delayMS = (uint16_t)_recDelay * 1000 / _recParams.rate;

ditto

@@ +1002,5 @@
>      }
>      if (res) {
> +        //  _ptrAudioBuffer->SetVQEData(
> +        //  _playDelay * 1000 / _playParams.rate,
> +        //  _recDelay * 1000 / _recParams.rate, 0);

Perhaps mention why this is commented out?

::: media/webrtc/trunk/webrtc/modules/audio_device/sndio/audio_device_utility_sndio.cc
@@ +28,5 @@
>                   "%s destroyed", __FUNCTION__);
> +    {
> +        CriticalSectionScoped lock(&_critSect);
> +
> +        // free stuff here...

Add XXX perhaps

Comment 29

[Landry Breuil (:gaston)]

Attachment: Webrtc sndio backend, v0.2

And here's the hopefully final version of the patch, qfolding att 8588336 and 8681683, and addressing review feedback from comment #28. As for the whitespaces, this was just to ease diffability with audio_device_alsa_linux.cc.. and all indent should be fixed now, only spaces everywhere. parens added around denominators too. Flagging for review again for safety... and doing a build here.

Comment 30

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

Comment on attachment 8687704 [details] [diff] [review]
Webrtc sndio backend, v0.2

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

r+ - my comments are all nits; you can land without re-review on dealing with any of them.  Thanks!

::: media/webrtc/trunk/webrtc/modules/audio_device/sndio/audio_device_sndio.cc
@@ +133,5 @@
> +
> +    CriticalSectionScoped lock(&_critSect);
> +
> +    if (_playing)
> +        StopPlayout();

nit: prefer {}'s around body of an if

@@ +136,5 @@
> +    if (_playing)
> +        StopPlayout();
> +
> +    if (_recording)
> +        StopRecording();

ditto

@@ +252,5 @@
> +}
> +
> +int32_t AudioDeviceSndio::SetSpeakerMute(bool enable)
> +{
> +    return -1;

Prefer for all the unsupported API calls:
    WEBRTC_TRACE(kTraceError, kTraceAudioDevice, _id,
                 "%s not supported", __FUNCTION__);
    return -1;

@@ +463,5 @@
> +
> +int16_t AudioDeviceSndio::RecordingDevices()
> +{
> +
> +    return 1;

spurious blank line

@@ +492,5 @@
> +
> +    hdl = sio_open(SIO_DEVANY, SIO_PLAY, 0);
> +    if (hdl == NULL)
> +        available = false;
> +    else {

Would prefer the if case be braced (similar to the alsa code).

@@ +506,5 @@
> +
> +    hdl = sio_open(SIO_DEVANY, SIO_REC, 0);
> +    if (hdl == NULL)
> +        available = false;
> +    else {

ditto

@@ +537,5 @@
> +    _playParams.pchan = 2;
> +    _playParams.bits = 16;
> +    _playParams.sig = 1;
> +    _playParams.le = SIO_LE_NATIVE;
> +    _playParams.appbufsz = _playParams.rate * 40 / 1000;

where does 40 come from?  Perhaps it should be a #define, or use an existing #define.  At least a comment...

@@ +554,5 @@
> +        _playHandle = NULL;
> +        return -1;
> +    }
> +
> +    // XXX: check params here

is checking here needed, or is it a nice-to-have? Or perhaps expand on the comment?

@@ +606,5 @@
> +    _recParams.rchan = 2;
> +    _recParams.bits = 16;
> +    _recParams.sig = 1;
> +    _recParams.le = SIO_LE_NATIVE;
> +    _recParams.appbufsz = _recParams.rate * 40 / 1000;

Ditto about 40

@@ +997,5 @@
> +    }
> +    if (res) {
> +        //  _ptrAudioBuffer->SetVQEData(
> +        //  _playDelay * 1000 / _playParams.rate,
> +        //  _recDelay * 1000 / _recParams.rate, 0);

Perhaps comment why this is commented-out?

Comment 31

[Landry Breuil (:gaston)]

Try push with the above nits fixed - fixed braces indentation to be consistent, added webrtc_trace() macro for unsupported apis, uncommented setvqedata call since it's done one other implems - apart from the magic 40 for which i have no idea and i couldnt get ahold of ratchov@ for insight..

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05a9c967cf76

Comment 32

[Landry Breuil (:gaston)]

Did a local build on OpenBSD, and fixed a  brace error  in StopPlayout/StopRecording, the CriticalSectionScoped lock needs to be in its own scope. Will commit the fixed version when the try push is green.

Comment 33

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69872cd154fd

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69872cd154fd

Comment 35

[Landry Breuil (:gaston)]

\o/

Comment 36

[Pulsebot]

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

Comment 37

[Landry Breuil (:gaston)]

Of course after doing a broken testbuild i realized that what i pushed didnt have the CriticalSectionScoped fix i mentioned in comment 32, so i sneaked in a followup:

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

Sorry for the inconvenience!

Comment 38

[Landry Breuil (:gaston)]

For the sake of merge..

Comment 39

[Carsten Book [:Tomcat]]

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