sndio webrtc audio backend

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: gaston, Assigned: gaston, NeedInfo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
x86_64
OpenBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [getusermedia])

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Depends on: 844818, 910875
Note: BSD issue
Whiteboard: [getusermedia]
(Assignee)

Comment 2

5 years ago
Created attachment 8348236 [details] [diff] [review]
sndio backend, build bits
Assignee: nobody → landry
(Assignee)

Comment 3

5 years ago
Created attachment 8348238 [details] [diff] [review]
webrtc sndio backend v0.0
(Assignee)

Comment 4

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

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

Comment 6

4 years ago
Created attachment 8587994 [details] [diff] [review]
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 ?
Attachment #8348236 - Attachment is obsolete: true
Attachment #8587994 - Flags: feedback?(rjesup)
Attachment #8587994 - Flags: feedback?(alex)
(Assignee)

Comment 7

4 years ago
Created attachment 8587997 [details] [diff] [review]
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.
Attachment #8348238 - Attachment is obsolete: true
Attachment #8587997 - Flags: review?(rjesup)
Attachment #8587997 - Flags: feedback?(rjesup)
Attachment #8587997 - Flags: feedback?(alex)
(Assignee)

Comment 8

4 years ago
Created attachment 8588220 [details] [diff] [review]
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.
Attachment #8587994 - Attachment is obsolete: true
Attachment #8587994 - Flags: feedback?(rjesup)
Attachment #8587994 - Flags: feedback?(alex)
Attachment #8588220 - Flags: review?(rjesup)
Attachment #8588220 - Flags: feedback?(alex)
(Assignee)

Comment 9

4 years ago
Created attachment 8588292 [details] [diff] [review]
sndio backend, build bits

Sorry, wrong diff posted, this one has the define WEBRTC_AUDIO_SNDIO at the right spot. should.
Attachment #8588220 - Attachment is obsolete: true
Attachment #8588220 - Flags: review?(rjesup)
Attachment #8588220 - Flags: feedback?(alex)
Attachment #8588292 - Flags: review?(rjesup)
Attachment #8588292 - Flags: feedback?(alex)
(Assignee)

Comment 10

4 years ago
Created attachment 8588293 [details]
WebRTC.log

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

Comment 11

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

Comment 12

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

Comment 13

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

Comment 14

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

Comment 15

4 years ago
Created attachment 8588336 [details] [diff] [review]
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.
Attachment #8587997 - Attachment is obsolete: true
Attachment #8587997 - Flags: review?(rjesup)
Attachment #8587997 - Flags: feedback?(rjesup)
Attachment #8587997 - Flags: feedback?(alex)
Attachment #8588336 - Flags: review?(rjesup)
Comment on attachment 8588292 [details] [diff] [review]
sndio backend, build bits

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

works for me, builds fine
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
Attachment #8588336 - Flags: review?(rjesup) → review-
Attachment #8588292 - Flags: review?(rjesup) → review+
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Comment 20

4 years ago
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 ?
Flags: needinfo?(rjesup)
(Assignee)

Comment 23

4 years ago
If we dont land what we have now, can we at least have a skeleton ?
(Assignee)

Comment 25

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

Comment 26

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

Comment 27

3 years ago
Note that there was a stray _playoutFramesLeft that crept in a copypasto that was breaking the build - removed locally.
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
Attachment #8681683 - Flags: review?(rjesup) → review+
(Assignee)

Comment 29

3 years ago
Created attachment 8687704 [details] [diff] [review]
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.
Attachment #8588336 - Attachment is obsolete: true
Attachment #8681683 - Attachment is obsolete: true
Attachment #8687704 - Flags: review?(rjesup)
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?
Attachment #8687704 - Flags: review?(rjesup) → review+
(Assignee)

Comment 31

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

Comment 32

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

Updated

3 years ago
Keywords: leave-open

Comment 34

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69872cd154fd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Comment 35

3 years ago
\o/
(Assignee)

Comment 37

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

Comment 38

3 years ago
For the sake of merge..
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 39

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2a05b69065e6
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Updated

2 years ago
Blocks: 1351378
You need to log in before you can comment on or make changes to this bug.