1021761 - opensound (OSS) sound driver doesn't work

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P4)

Description

[Damian]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36

Steps to reproduce:

get any linux with OSS and play audio in FF.

It's REALLY old bug but as most of browsers available for linux are actively developed I thought that maybe it's temporary especially that old versions of firefox (below 15 if I remember well) worked well with OSS. Chrome also suffered from this issues but olny versions 24 - 28 (first releases with Web Audio API), so I thought It's maybe the same in case of FF

logs in ff:
https://dl.dropboxusercontent.com/u/44131220/undeletable/log

for comparison log in chromium:
https://dl.dropboxusercontent.com/u/44131220/undeletable/logchrome

I know that OSS isn't supported by FF, but it isn't supported also by Chrome, neither by most of ALSA apps which work,http://www.opensuse.org/en/ so FF must be just doing some fancy magic with sound as all other ALSA apps I have work correctly including even strictly audio-related apps. firefox seems to misnotice that even something is wrong as log doesn't seem to contain any info about issues with sound


Actual results:

No sound. Neither HTML5 nor flash


Expected results:

sound should work as in case of all other browsers available for linux

Comment 1

[nucrap]

Indeed I confirm this bug.

Comment 2

[Evgeniy]

Attachment: add OSS support to firefox

This patch adds an --enable-oss configure option that allows building firefox with OSS sound output instead of ALSA.

This patch doesn't work for 31.0 and should be applied to the latest version from mercurial.

Comment 3

[Jan Beich]

Comment on attachment 8483623 [details] [diff] [review]
add OSS support to firefox

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

Fails to build with --enable-warnings-as-errors.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=150e97c90b03

media/libcubeb/src/cubeb_oss.c: In function 'run_data_callback':
media/libcubeb/src/cubeb_oss.c:85:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
     long got = 0;
     ^
media/libcubeb/src/cubeb_oss.c: In function 'writer':
media/libcubeb/src/cubeb_oss.c:104:9: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
         int got;
         ^
media/libcubeb/src/cubeb_oss.c:108:13: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
             int i;
             ^
media/libcubeb/src/cubeb_oss.c:109:23: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
             for(i=0; i<got*stream->params.channels; i++){
                       ^
media/libcubeb/src/cubeb_oss.c:117:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
   size_t i = 0;
   ^
media/libcubeb/src/cubeb_oss.c: In function 'oss_stream_init':
media/libcubeb/src/cubeb_oss.c:143:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
     int i, j;
     ^
cc1: all warnings being treated as errors

Also many style bugs: missing license header, trailing whitespace, broken indentation, very long lines.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: configure.in
@@ +5425,5 @@
> +   MOZ_OSS=1,
> +   MOZ_OSS=)
> +
> +if test -n "$MOZ_OSS"; then
> +   MOZ_ALSA=

Better enable both on Linux by default but put after alsa_init in cubeb.c:cubeb_init() to help prevent bitrot. See an example in my Try push above.

::: media/libcubeb/src/cubeb_oss.c
@@ +152,5 @@
> +    pthread_mutex_init(&stream->state_mutex, NULL);
> +    
> +    stream->floating = 0;
> +    SET(SNDCTL_DSP_CHANNELS, stream_params.channels);
> +    SET(SNDCTL_DSP_SPEED, stream_params.rate);

.channels and .rate are unsigned but you're writing a signed value.

@@ +215,5 @@
> +    return CUBEB_OK;
> +}
> +
> +int oss_stream_set_volume(cubeb_stream * stream, float volume){
> +    stream->volume=volume;

Don't you need locking here like in cubeb_alsa.c and cubeb_sndio.c?

Comment 4

[Jan Beich]

There's another effort upstream. Matthew, what happened to reviewing that? It's green on Try but a bit behind in API support (missing set_volume/set_panning).

https://github.com/kinetiknz/cubeb/pull/41
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2464b9841ea3

Comment 5

[Evgeniy]

Attachment: add OSS support to firefox take 2

I think I've followed mozilla coding style pretty close this time and it should compile without warnings.

Comment 6

[Evgeniy]

(In reply to Jan Beich from comment #3)
> ::: configure.in
> @@ +5425,5 @@
> > +   MOZ_OSS=1,
> > +   MOZ_OSS=)
> > +
> > +if test -n "$MOZ_OSS"; then
> > +   MOZ_ALSA=
> 
> Better enable both on Linux by default but put after alsa_init in
> cubeb.c:cubeb_init() to help prevent bitrot. See an example in my Try push
> above.

Maybe I am missing something, but it looks like libcubeb does not support picking backends at runtime.
So I don't actually know how to do this properly. 
 
> @@ +215,5 @@
> > +    return CUBEB_OK;
> > +}
> > +
> > +int oss_stream_set_volume(cubeb_stream * stream, float volume){
> > +    stream->volume=volume;
> 
> Don't you need locking here like in cubeb_alsa.c and cubeb_sndio.c?

Well... stream->volume is only modified here. I don't think there can be any problems without the lock.

Comment 7

[Evgeniy]

(In reply to Jan Beich from comment #4)
> There's another effort upstream. Matthew, what happened to reviewing that?
> It's green on Try but a bit behind in API support (missing
> set_volume/set_panning).
> 
> https://github.com/kinetiknz/cubeb/pull/41
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2464b9841ea3

I wish I'd seen this before I started writing my own implementation. That code seems overcomplicated to me though.

Comment 8

[Jan Beich]

Note, another place lacking OSS backend is WebRTC support under

  media/webrtc/trunk/webrtc/modules/audio_device/

(In reply to Evgeniy from comment #6)
> (In reply to Jan Beich from comment #3)
> > ::: configure.in
> > @@ +5425,5 @@
> > > +   MOZ_OSS=1,
> > > +   MOZ_OSS=)
> > > +
> > > +if test -n "$MOZ_OSS"; then
> > > +   MOZ_ALSA=
> > 
> > Better enable both on Linux by default but put after alsa_init in
> > cubeb.c:cubeb_init() to help prevent bitrot. See an example in my Try push
> > above.
> 
> Maybe I am missing something, but it looks like libcubeb does not support
> picking backends at runtime.
> So I don't actually know how to do this properly. 

If one backend fails cubeb_init tries another (bug 837564). Unfortunately, alsa_init never fails unlike pulse_unit, opensl_init or audiotrack_init.

Comment 9

[Evgeniy]

(In reply to Jan Beich from comment #8)
> If one backend fails cubeb_init tries another (bug 837564). Unfortunately,
> alsa_init never fails unlike pulse_unit, opensl_init or audiotrack_init.

Yes, cubeb_pulse.c uses dlopen/dlsym to load pulseaudio while cubeb_alsa.c links to alsa directly.

Comment 10

[Evgeniy]

Attachment: Make ALSA optional on linux

I've made a patch for libcubeb to load libasound.so with dlopen/dlsym in the same manner the pulseaudio backend does this.
It doesn't test if libasound is actually capable to output sound though.

Two problems with this:

1) I havn't tested if I broke sound output with ALSA (I don't have ALSA support in my kernel).

2)I'm not very good with autotools so I don't know how to prevent libcubeb from linking with libasound.so. I've added the MOZ_ALSA_LIBS="" to configure.in for testing purposes.

Comment 11

[Jan Beich]

(In reply to Evgeniy from comment #10)
> 1) I havn't tested if I broke sound output with ALSA (I don't have ALSA
> support in my kernel).

It works fine here and on Try.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8c70a6c8fac5

Comment 12

[Matthew Gregan [:kinetik]]

Comment on attachment 8485552 [details] [diff] [review]
add OSS support to firefox take 2

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

r+ with comments addressed.

This looks good, thanks for your contribution!  I've made a few comments.  Unfortunately there's also another OSS backend awaiting review upstream, but I've been too busy to deal with it.  This is nice and simple, and assuming it works for you, I'm happy to take this now.  Would you mind looking at the other OSS backend and seeing if there's any additional functionality worth cherry picking?

::: media/libcubeb/src/cubeb.c
@@ +90,5 @@
>  cubeb_init(cubeb ** context, char const * context_name)
>  {
>    int (* init[])(cubeb **, char const *) = {
> +#if defined(USE_OSS)
> +    oss_init,

I'd prefer to keep the default order as: pulse, alsa, oss when all are built.  It may be that we need to add a method/Gecko pref for the user to specify their own backend preference.

::: media/libcubeb/src/cubeb_oss.c
@@ +62,5 @@
> +}
> +
> +static char const * oss_get_backend_id(cubeb * context)
> +{
> +  static char oss_name[] = "OSS";

The existing backends use lower case backend ID names, so let's keep that pattern.

@@ +75,5 @@
> +
> +static int oss_get_min_latency(cubeb * context, cubeb_stream_params params,
> +                               uint32_t * latency_ms)
> +{
> +  *latency_ms = 10;

Please document where this value comes from/what guarantees it.  If it's chosen arbitrarily, please note that also.

@@ +81,5 @@
> +}
> +
> +static int oss_get_preferred_sample_rate(cubeb *context, uint32_t * rate)
> +{
> +  *rate = 48000;

As above.

@@ +114,5 @@
> +  while (stream->running) {
> +    if (stream->stopped) {
> +      run_state_callback(stream, CUBEB_STATE_STOPPED);
> +      while (stream->stopped) {
> +        usleep(10000); /* This better be done with conditionals */

It should be fairly easy to convert this to use a pthread_cond_t?

@@ +211,5 @@
> +  stream->stopped = 1;
> +  stream->position = 0;
> +
> +  pthread_create(&stream->th, NULL, writer, (void*)stream);
> +  pthread_detach(stream->th);

See comment I make about stream_destroy below.

@@ +226,5 @@
> +  pthread_mutex_lock(&stream->state_mutex);
> +  stream->running = 0;
> +  stream->stopped = 0;
> +  pthread_mutex_unlock(&stream->state_mutex);
> +  pthread_join(stream->th, NULL);

You can't join a detached thread according to the manual.

Comment 13

[Matthew Gregan [:kinetik]]

Comment on attachment 8486767 [details] [diff] [review]
Make ALSA optional on linux

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

This looks good!

::: media/libcubeb/src/cubeb_alsa.c
@@ +1032,4 @@
>    params.format = CUBEB_SAMPLE_FLOAT32NE;
>    params.channels = 2;
>  
> +  /* snd_pcm_hw_params_alloca is actually a macro */

It's clearer (to me, at least) if this comment is where we have the commented out definition/LOAD call.  It seems slightly out of context here.

@@ +1063,4 @@
>    snd_pcm_t * pcm;
>    snd_pcm_hw_params_t * hw_params;
>  
> +  /* snd_pcm_hw_params_alloca is actually a macro */

Ditto the above.

Comment 14

[Evgeniy]

Attachment: add OSS support to firefox take 3

I think I've made the necessary fixes. Also OSS support is enabled by default on linux.

Comment 15

[Evgeniy]

Attachment: Make ALSA optional on linux take 2

Moved the comments to suggested places.

I still don't know how to prevent the -lasound option from being passed to the compiler. Is adding MOZ_ALSA_LIBS="" after  PKG_CHECK_MODULES(MOZ_ALSA, alsa, .. ) a good idea?

Comment 16

[Jan Beich]

Attachment: add OSS take3 + improved build glue

Let's put OSS last at cubeb_init() as it can be a fallback for sndio as well. Also, add more platforms where OSS is enabled by default. Mark feedback- if you don't like.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=92b8eeea96dd

Comment 17

[Jan Beich]

Attachment: Make ALSA optional take 2 + build glue

Only configure.in change on top of attached 8488195.

(In reply to Evgeniy from comment #15)
> I still don't know how to prevent the -lasound option from being passed to
> the compiler. Is adding MOZ_ALSA_LIBS="" after  PKG_CHECK_MODULES(MOZ_ALSA,
> alsa, .. ) a good idea?

I think so. May as well expose DISABLE_LIBASOUND_DLOPEN as a configure option.

Comment 18

[Evgeniy]

Comment on attachment 8488664 [details] [diff] [review]
Make ALSA optional take 2 + build glue

Looks good to me.

Comment 19

[Evgeniy]

Comment on attachment 8488651 [details] [diff] [review]
add OSS take3 + improved build glue

sndio option before OSS on openBSD looks like the right choice.

Comment 20

[Jan Beich]

buffer_size change regresses A/V sync for me. In OSS patch take 3 video playback has lower FPS than in take 2. Worked around by hardcoding latency to 20 instead of whatever cubeb passes.

Comment 21

[Evgeniy]

I'll take a look at it.

Comment 22

[Evgeniy]

(In reply to Jan Beich from comment #20)
> buffer_size change regresses A/V sync for me. In OSS patch take 3 video
> playback has lower FPS than in take 2. Worked around by hardcoding latency
> to 20 instead of whatever cubeb passes.

I confirm this. Looks like the fps is limited to 1000/latency.

Comment 23

[Evgeniy]

Attachment: add OSS support to firefox take 4

Volume controls work for both float and int16 sample types.
Fixed regression of sample rate.
Implemented setting the output latency.

Comment 24

[Jan Beich]

Comment on attachment 8488992 [details] [diff] [review]
add OSS support to firefox take 4

Another regression.

$ firefox http://www.quirksmode.org/html5/videos/big_buck_bunny.webm
...
[New Thread 852baf400 (LWP 101320 Media Audio)]
[New Thread 853fc3400 (LWP 101329)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 853fc3400 (LWP 101329)]
0x00000008086b4c3f in writer (stm=0x2192818ad) at media/libcubeb/src/cubeb_oss.c:178
178         stream->position+=got;
(gdb) bt
#0  0x00000008086b4c3f in writer (stm=0x2192818ad) at media/libcubeb/src/cubeb_oss.c:178
#1  0x00000002005cdb96 in ?? ()
#2  0x0000000000000000 in ?? ()
(gdb) i loc
stream = 0x2192818ad
buffer = {0 <repeats 1024 times>}
f_buffer = {0 <repeats 238 times>, -0 <repeats 256 times>, 0 <repeats 269 times>, -0 <repeats 256 times>, -3.83804056e-12, -2.51251398e-11, -2.28114028e-10, 7.1810155e-11, -1.56889932e-10}
got = 1024
i = 1024

$ ./media/libcubeb/tests/test_audio

[New Thread 815002400 (LWP 101279)]
--------------------------
Testing 1 channel(s), 16000 Hz, short (oss)
[New Thread 815002c00 (LWP 101635)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 815002c00 (LWP 101635)]
0x000000000040aea2 in apply_volume (buffer=0x7fffffbfd7a0, n=2048, volume=1, panning=0) at media/libcubeb/src/cubeb_oss.c:135
135         buffer[i] = ((int)buffer[i])*pan[i%2]/128;
(gdb) bt f
#0  0x000000000040aea2 in apply_volume (buffer=0x7fffffbfd7a0, n=2048, volume=1, panning=0) at media/libcubeb/src/cubeb_oss.c:135
        left = 1
        right = 1
        i = 1072
        pan = {128, 128}
#1  0x000000000040abd3 in writer (stm=0x815064160) at media/libcubeb/src/cubeb_oss.c:169
        stream = 0x815064160
        buffer = {0, 565, 1126, 1679...}
        f_buffer = {0 <repeats 1024 times>}
        got = 1024
        i = 0
#2  0x0000000805ab2555 in thread_start (curthread=0x815002c00) at /usr/src/lib/libthr/thread/thr_create.c:284
        set = {
          __bits = {0, 0, 0, 0}
        }
#3  0x0000000000000000 in ?? ()
No symbol table info available.
Backtrace stopped: Cannot access memory at address 0x7fffffbfe000

Comment 25

[Evgeniy]

Attachment: add OSS support to firefox take 5

This should fix the segfaults, fix low fps on some videos and improve audio video sync.

./mach test test_audio segfaults for me though. No idea why. (also not sure if this is a correct way to test)

Comment 26

[Evgeniy]

There are two problems with how html5 videos work in firefox

1)libcubeb has a nice function cubeb_stream_get_latency. Unfortunately, it is not used during video playback. Currently firefox requests cubeb for a fixed latency of 100ms. The problem is that the driver may not be able to achieve this exact latency. OTOH, cubeb_stream_get_latency could return the more or less exact value of latency.

2)it looks like firefox relies on the data_callback to time video frame rendering. As the result, the fps is limited to the amount of times the data_callback is called by the audio backend per second. This would not be a problem if this call could be made every 1/fps seconds. Setting aside the problems that the operating system's audio interface may not be able to provide this, libcubeb lacks the API to set the required fps.

Comment 27

[Evgeniy]

Comment on attachment 8489130 [details] [diff] [review]
add OSS support to firefox take 5

I've tested it quit a lot and it seems to work just fine for me.

Comment 28

[Matthew Gregan [:kinetik]]

(In reply to Evgeniy from comment #26)
> 2)it looks like firefox relies on the data_callback to time video frame
> rendering. As the result, the fps is limited to the amount of times the
> data_callback is called by the audio backend per second. This would not be a
> problem if this call could be made every 1/fps seconds. Setting aside the
> problems that the operating system's audio interface may not be able to
> provide this, libcubeb lacks the API to set the required fps.

It shouldn't be tied to the rate of data_callback calls, but it is dependent on the resolution of get_position, which in the OSS backend is only updated once per data_callback invocation, tying the two together.  Is there a way to get intermediate stream position updates from the OSS driver?

Comment 29

[Matthew Gregan [:kinetik]]

Comment on attachment 8489130 [details] [diff] [review]
add OSS support to firefox take 5

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

::: media/libcubeb/src/cubeb_oss.c
@@ +1,2 @@
> +/*
> + * Copyright © 2011 Mozilla Foundation

This should be 2014.

And please add yourself to AUTHORS.

Comment 30

[Evgeniy]

Attachment: add OSS support to firefox take 6

Implemented oss_stream_get_position in a proper way. This improved A/V syncing and frame rate.

+added myself as author and fixed the copyright notice

Comment 31

[Andriy Voskoboinyk]

1. Playback counter (which is obtained via SNDCTL_DSP_CURRENT_OPTR / SNDCTL_DSP_GETOPTR calls) will continue running (under 4Front Technologies/OpenIndiana implementations) if underrun occurs (and it will occur after calling oss_stream_stop).
2. There is no checking of the return value of write(2) + SNDCTL_DSP_CHANNELS / SNDCTL_DSP_SPEED / SNDCTL_DSP_SETFMT argument values are not checked.
3. Calling SNDCTL_DSP_SETFRAGMENT after SNDCTL_DSP_GETOSPACE will work only under FreeBSD implementation - otherwise, it will have no effect (see http://manuals.opensound.com/developer/callorder.html).

Comment 32

[Jan Beich]

Comment on attachment 8489461 [details] [diff] [review]
add OSS support to firefox take 6

Works fine here. OSS emulation on Linux maybe not.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=06cf66bc9bb5

TEST-START | test_audio
Testing 1 channel(s), 16000 Hz, short (oss)
Error initializing cubeb stream: -1
test_audio: /builds/slave/try-lx-00000000000000000000000/build/media/libcubeb/tests/test_audio.cpp:191: int main(int, char**): Assertion `run_test(channel_values[j], freq_values[i], 0) == CUBEB_OK' failed.
TEST-UNEXPECTED-FAIL | test_audio | test failed with return code -6

TEST-START | test_sanity
START test_init_destroy_context
END test_init_destroy_context
START test_init_destroy_multiple_contexts
END test_init_destroy_multiple_contexts
START test_init_destroy_stream
test_sanity: /builds/slave/try-lx-00000000000000000000000/build/media/libcubeb/tests/test_sanity.cpp:110: void test_init_destroy_stream(): Assertion `r == 0 && stream' failed.
TEST-UNEXPECTED-FAIL | test_sanity | test failed with return code -6

TEST-START | test_tone
Error initializing cubeb stream
TEST-UNEXPECTED-FAIL | test_tone | test failed with return code 255

Comment 33

[Evgeniy]

Attachment: add OSS support to firefox take 7

> 1. Playback counter (which is obtained via SNDCTL_DSP_CURRENT_OPTR /
> SNDCTL_DSP_GETOPTR calls) will continue running (under 4Front 
> Technologies/OpenIndiana implementations) if underrun occurs (and it will
> occur after calling oss_stream_stop).
I've fixed this for the oss_stream_stop. There might still be some problems if underun occurs for some reason. However, relying on the number of fragments written with write as the current position results in bad fps and/or lags in video.

>2. There is no checking of the return value of write(2) + SNDCTL_DSP_CHANNELS / SNDCTL_DSP_SPEED /
> SNDCTL_DSP_SETFMT argument values are not checked.
Fixed it.

>3. Calling SNDCTL_DSP_SETFRAGMENT after SNDCTL_DSP_GETOSPACE will work only under FreeBSD 
>implementation - otherwise, it will have no effect 
>(see http://manuals.opensound.com/developer/callorder.html).
You are right about this. 
I actually don't like this idea of setting latency with SNDCTL_DSP_SETFRAGMENT.
I've removed the SNDCTL_DSP_GETOSPACE ioctl call and moved setting latency before setting speed and channels.


Right now test_audio fails for me becouse the test ignores get_max_channel_count and my soundcard only supports stereo.

test_sanity will fail for everyone since oss virtual mixer support only 8 streams max and test_sanity tries to create 16.

Comment 34

[Matthew Gregan [:kinetik]]

The discussion is slightly split between here and https://github.com/kinetiknz/cubeb/pull/41 now; I asked a brief question there about non-blocking OSS with poll(2).

(In reply to Evgeniy from comment #33)
> Right now test_audio fails for me becouse the test ignores
> get_max_channel_count and my soundcard only supports stereo.

We should fix the test to deal with that.

> test_sanity will fail for everyone since oss virtual mixer support only 8
> streams max and test_sanity tries to create 16.

test_sanity was changed to create only 8 recently, due to a lowered limit in an OS X backend, so it should work now.  Probably there needs to be an API call to expose the context and stream limits.

Comment 35

[Evgeniy]

(In reply to Matthew Gregan [:kinetik] from comment #34)
> The discussion is slightly split between here and
> https://github.com/kinetiknz/cubeb/pull/41 now; I asked a brief question
> there about non-blocking OSS with poll(2).

What is the reason to keep thread count to a minimum? From the oss backend point of view this looks like a bad decision. I can make oss work in non-blocking mode and use one thread if this is necessary though.

Comment 36

[Matthew Gregan [:kinetik]]

Ultimately, it's up to you.  The reason for the minimal threads requirement is the intention to be able to run a large(ish) number of streams without consuming unnecessary resources.  Traditionally, the media playback in Firefox has used a lot of threads, which when multiplied over multiple active playing media elements, became problematic.  This has been partially addressed now (the decoding uses pools, state management has been reduced to a single thread for all elements, and the MediaStreamGraph used by WebRTC and WebAudio was recently switch to a pull based approach using cubeb's callback), but we still have an audio thread per media element within Firefox (which is now legacy, but existed because we used to use a blocking push based audio library).  There's a plan to address that, but nobody has had time to complete the necessary work.  We also try to limit the cost of threads by shrinking the thread stack size to 256kB.

In the long term, it looks like we'll move towards using less active streams at once, and perform mixing/etc. within either cubeb or Firefox, since various OSes have revealed low level audio stream limitations that we can't work around (crashes or mysterious hangs in some OS X and Win32 backends, etc.).

Comment 37

[Mike Hommey [:glandium]]

Comment on attachment 8488664 [details] [diff] [review]
Make ALSA optional take 2 + build glue

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

::: configure.in
@@ +5465,5 @@
> +if test -n "$DISABLE_LIBASOUND_DLOPEN"; then
> +    AC_DEFINE(DISABLE_LIBASOUND_DLOPEN)
> +else
> +    MOZ_ALSA_LIBS=
> +fi

That seems wrong to me. You're essentially making dlopen of alsa the default (so, effectively changing how things work), but allow to disable that? why?

::: media/libcubeb/src/cubeb_alsa.c
@@ +308,5 @@
>    draining = 0;
>  
>    pthread_mutex_lock(&stm->mutex);
>  
> +  r = WRAP(snd_pcm_poll_descriptors_revents)(stm->pcm, stm->fds, stm->nfds, &revents);

The WRAP thing is error prone. You can be sure someone upstream will add an alsa function call without a WRAP in the future. #define snd_* cubeb_snd_* would work better.

Comment 38

[Evgeniy]

(In reply to Mike Hommey [:glandium] from comment #37)

> The WRAP thing is error prone. You can be sure someone upstream will add an
> alsa function call without a WRAP in the future. #define snd_* cubeb_snd_*
> would work better.

This is how things work with pulseaudio now. I wanted to be consistent. Should I change the ALSA plugin to use #define snd_* cubeb_snd_* ?

Comment 39

[Mike Hommey [:glandium]]

(In reply to Evgeniy from comment #38)
> (In reply to Mike Hommey [:glandium] from comment #37)
> 
> > The WRAP thing is error prone. You can be sure someone upstream will add an
> > alsa function call without a WRAP in the future. #define snd_* cubeb_snd_*
> > would work better.
> 
> This is how things work with pulseaudio now. I wanted to be consistent.
> Should I change the ALSA plugin to use #define snd_* cubeb_snd_* ?

If it's this way upstream, then I don't care. The other comment still applies, though.

Comment 40

[Jan Beich]

Comment on attachment 8491860 [details] [diff] [review]
add OSS support to firefox take 7

Current build glue still needs review.

Comment 41

[Mike Hommey [:glandium]]

Comment on attachment 8491860 [details] [diff] [review]
add OSS support to firefox take 7

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

::: configure.in
@@ +5460,5 @@
> +    AC_MSG_CHECKING([MOZ_OSS_CFLAGS])
> +    if test -z "$MOZ_OSS_CFLAGS"; then
> +        for oss_conf in /etc/oss.conf /usr/local/etc/oss.conf; do
> +            if test -e "$oss_conf"; then
> +                . "$oss_conf"

I'd rather have a --with-oss=/path thing like e.g. --with-system-bz2.

@@ +5470,5 @@
> +    fi
> +    AC_MSG_RESULT([$MOZ_OSS_CFLAGS])
> +
> +    CFLAGS="$CFLAGS $MOZ_OSS_CFLAGS"
> +    MOZ_CHECK_HEADERS(sys/soundcard.h linux/soundcard.h soundcard.h)

Is there a normally installed linux system that has linux/soundcard.h without sys/soundcard.h? At least on my system, sys/soundcard.h exists, and includes linux/soundcard.h. It seems to me linux/soundcard.h should be skipped.
Then, aiui, soundcard.h is only really going to be found if using MOZ_OSS_CFLAGS="-I$OSSLIBDIR/include", so you may want to skip that when not. But in fact, if there is a sys/soundcard.h, it's not going to be used even when there is MOZ_OSS_CFLAGS="-I$OSSLIBDIR/include"... You may want to change the logic to something like:
- If path given to --with-oss, search for soundcard.h there. If found use that.
- If not, search for sys/soundcard.h.
- If not found, barf.

@@ +5479,5 @@
> +        AC_MSG_ERROR([Need OSS for Ogg, Wave or WebM decoding on $OS_TARGET.  Disable with --disable-ogg --disable-wave --disable-webm.])
> +    fi
> +
> +    dnl Assume NetBSD implementation over SunAudio
> +    AC_CHECK_LIB(ossaudio, _oss_ioctl,

How does your code end up actually using that symbol from ossaudio?

Comment 42

[Andriy Voskoboinyk]

(In reply to Mike Hommey [:glandium] from comment #41)
> Comment on attachment 8491860 [details] [diff] [review]
> add OSS support to firefox take 7
> 
> Review of attachment 8491860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +5470,5 @@
> > +    fi
> > +    AC_MSG_RESULT([$MOZ_OSS_CFLAGS])
> > +
> > +    CFLAGS="$CFLAGS $MOZ_OSS_CFLAGS"
> > +    MOZ_CHECK_HEADERS(sys/soundcard.h linux/soundcard.h soundcard.h)
> 
> Is there a normally installed linux system that has linux/soundcard.h
> without sys/soundcard.h? At least on my system, sys/soundcard.h exists, and
> includes linux/soundcard.h. It seems to me linux/soundcard.h should be
> skipped.

AFAIK, most of implementations have that header in the 'sys' directory (the only exception is OpenBSD - however, sndio backend works better than OSS via compatibility layer)

> @@ +5479,5 @@
> > +        AC_MSG_ERROR([Need OSS for Ogg, Wave or WebM decoding on $OS_TARGET.  Disable with --disable-ogg --disable-wave --disable-webm.])
> > +    fi
> > +
> > +    dnl Assume NetBSD implementation over SunAudio
> > +    AC_CHECK_LIB(ossaudio, _oss_ioctl,
> 
> How does your code end up actually using that symbol from ossaudio?

It's defined in the sys/soundcard.h (#define ioctl _oss_ioctl).

Comment 43

[Andriy Voskoboinyk]

Comment on attachment 8491860 [details] [diff] [review]
add OSS support to firefox take 7

stream->volume / stream->panning should be under mutex lock

Comment 44

[Jan Beich]

The reply was ready days ago but I couldn't find time to write/test new version of the build glue. ;\

(In reply to Jan Beich from comment #32)
> TEST-START | test_audio
> Testing 1 channel(s), 16000 Hz, short (oss)
> Error initializing cubeb stream: -1
> test_audio:
> /builds/slave/try-lx-00000000000000000000000/build/media/libcubeb/tests/
> test_audio.cpp:191: int main(int, char**): Assertion
> `run_test(channel_values[j], freq_values[i], 0) == CUBEB_OK' failed.
> TEST-UNEXPECTED-FAIL | test_audio | test failed with return code -6

Ubuntu, Fedora, etc don't provide in-kernel OSS emulation anymore.

  linux$ cat /dev/dsp
  cat: /dev/dsp: No such file or directory

  linux$ modprobe snd_pcm_oss
  FATAL: Module snd_pcm_oss not found.

Manually tested with aoss(1) on Ubuntu 12.04 to confirm it works fine using

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-06cf66bc9bb5/try-linux/firefox-35.0a1.en-US.linux-i686.tar.bz2

(In reply to Mike Hommey [:glandium] from comment #41)
> Then, aiui, soundcard.h is only really going to be found if using
> MOZ_OSS_CFLAGS="-I$OSSLIBDIR/include", so you may want to skip that when
> not. But in fact, if there is a sys/soundcard.h, it's not going to be used
> even when there is MOZ_OSS_CFLAGS="-I$OSSLIBDIR/include"... You may want to
> change the logic to something like:
> - If path given to --with-oss, search for soundcard.h there. If found use
> that.
> - If not, search for sys/soundcard.h.
> - If not found, barf.

soundcard.h from libossaudio is at least partially emulated in
userland hence doesn't live under sys/.

MOZ_OSS_CFLAGS/LIBS are exposed in case of weird configuration e.g.,
PFX/lib/oss/include -> PFX/include or PFX/lib/libossaudio.so

sys/soundcard.h may exists in more than one place. In this case
MOZ_OSS_CFLAGS can manipulate precedence with -isystem/-idirafter.

Note, reading /etc/oss.conf is the recommended way to get OSSLIBDIR.

> @@ +5479,5 @@
> > +        AC_MSG_ERROR([Need OSS for Ogg, Wave or WebM decoding on $OS_TARGET.  Disable with --disable-ogg --disable-wave --disable-webm.])
> > +    fi
> > +
> > +    dnl Assume NetBSD implementation over SunAudio
> > +    AC_CHECK_LIB(ossaudio, _oss_ioctl,
> 
> How does your code end up actually using that symbol from ossaudio?

libossaudio overrides ioctl() with its own symbol.

http://bxr.su/NetBSD/lib/libossaudio/soundcard.h#385

Comment 45

[Mikhail T.]

Attachment: patch-cubeb_alsa.c

This patch for just the media/libcubeb/src/cubeb_alsa.c incorporates the preceding ones, but also rearranges the API calls to avoid the assert-failures: when a stream is opened asynchronously, it is perfectly possible for a call to return EAGAIN or consume only part of the submitted buffer.

The caller is supposed to loop and feed the rest of the buffer later. With these changes I can now watch online videos, whereas before the entire browser (!) was crashing with:
Assertion failed: (wrote >= 0 && wrote == got), function alsa_refill_stream

The idea is from here:
http://lists.freebsd.org/pipermail/freebsd-gecko/2013-November/003793.html

I may have put too many printfs in there -- feel free to remove them...

Comment 46

[Jan Beich]

Comment on attachment 8537596 [details] [diff] [review]
patch-cubeb_alsa.c

Moved to bug 1130155 and split from other changes. The issue isn't related to using cubeb_oss.

Comment 47

[Jan Beich]

Attachment: Make ALSA optional take 2 + build glue, v2

Carrying over r=kinetik from attachment 8488664 [details] [diff] [review].

(In reply to Mike Hommey [:glandium] from comment #37)
> ::: configure.in
> @@ +5465,5 @@
> > +if test -n "$DISABLE_LIBASOUND_DLOPEN"; then
> > +    AC_DEFINE(DISABLE_LIBASOUND_DLOPEN)
> > +else
> > +    MOZ_ALSA_LIBS=
> > +fi
> 
> That seems wrong to me. You're essentially making dlopen of alsa the default
> (so, effectively changing how things work), but allow to disable that? why?

Do you suggest to stop passinng MOZ_ALSA_LIBS in moz.build files or always override the value set by PKG_CHECK_MODULES ? This version is for the former.

Comment 48

[Jan Beich]

Attachment: Add OSS support to firefox take 7 + build glue, v2

Carrying over r=kinetik from 8491860 and comment 44 for :glandium review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=693d50137f4e
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-693d50137f4e

IIRC, I've stalled after being confused by --without-foo behaving differently when defined by MOZ_ARG_WITH_STRING or MOZ_ARG_WITH_BOOL.

Comment 49

[Mike Hommey [:glandium]]

Comment on attachment 8560776 [details] [diff] [review]
Make ALSA optional take 2 + build glue, v2

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

r+ for the build glue.

::: media/libcubeb/tests/moz.build
@@ +57,1 @@
>      OS_LIBS += CONFIG['MOZ_PULSEAUDIO_LIBS']

Note that if we dlopen libpulse, this is not necessary and there's a separate bug to file for that.

Comment 50

[Mike Hommey [:glandium]]

Comment on attachment 8560777 [details] [diff] [review]
Add OSS support to firefox take 7 + build glue, v2

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

r+ for the build glue

::: configure.in
@@ +5635,5 @@
> +    MOZ_CHECK_HEADERS(sys/soundcard.h soundcard.h)
> +
> +    if test "$ac_cv_header_sys_soundcard_h" != "yes" -a \
> +            "$ac_cv_header_soundcard_h" != "yes"; then
> +        AC_MSG_ERROR([Need OSS for Ogg, Wave or WebM decoding on $OS_TARGET.  Disable with --disable-ogg --disable-wave --disable-webm.])

Make that "Disable with --without-oss".

Comment 51

[Jan Beich]

(In reply to Mike Hommey [:glandium] from comment #49)
> ::: media/libcubeb/tests/moz.build
> @@ +57,1 @@
> >      OS_LIBS += CONFIG['MOZ_PULSEAUDIO_LIBS']
> 
> Note that if we dlopen libpulse, this is not necessary and there's a
> separate bug to file for that.

Do you mean bug 889652 isn't necessary anymore?

Comment 52

[Jan Beich]

Attachment: Add OSS support to firefox take 7 + build glue, v2

Fixed up AC_MSG_ERROR message as suggested in comment 50.

Carrying over r=kinetik from attachment 8491860 [details] [diff] [review].
Carrying over r=glandium from attachment 8560777 [details] [diff] [review].

Comment 53

[Mike Hommey [:glandium]]

(In reply to Jan Beich from comment #51)
> (In reply to Mike Hommey [:glandium] from comment #49)
> > ::: media/libcubeb/tests/moz.build
> > @@ +57,1 @@
> > >      OS_LIBS += CONFIG['MOZ_PULSEAUDIO_LIBS']
> > 
> > Note that if we dlopen libpulse, this is not necessary and there's a
> > separate bug to file for that.
> 
> Do you mean bug 889652 isn't necessary anymore?

I don't know, thus the "if" in my statement.

Comment 54

[Jan Beich]

I guess dlopen fails on B2G Desktop:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86096d5edd55

Comment 55

[Gethyn ThomasQuail]

Has there been any more work on this? I think it would be great of Firefox could support OSS.

Comment 56

[Maire Reavy [:mreavy]]

@evgeniy - do you plan to come back to this?

Comment 57

[Evgeniy]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #56)
> @evgeniy - do you plan to come back to this?

I will, if someone tells me what else should be done here. I'm not very good with autotools, though

Comment 58

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

Kinetik - can you give evgeniy some guidance on how to proceed here?

Comment 59

[Matthew Gregan [:kinetik]]

I assume this just needs the same changes to the build system and media/libcubeb/update.sh that JACK did in bug 783733?  Evgeniy, is there a more specific way I can assist?

Comment 60

[Jan Beich]

comment 54 neeeds a refresh. libcubeb tests failed on B2G which meant either finding out why (does libasound.so exist? is it an ldscript? etc) or passing DISABLE_LIBASOUND_DLOPEN thus partially reverting to comment 37 by restoring MOZ_ALSA_LIBS.

And the last version from Evgeniy has minor audio crackle, easy to notice with some music videos. My guess is because music videos tend to normalize volume to the max. Here's an example: https://www.youtube.com/watch?v=rmYU2ikxjpA&t=41

Note, according to downstream attachment 8560776 [details] [diff] [review] needs a rebase:
https://svnweb.freebsd.org/ports/head/www/firefox/files/patch-bug1021761?r1=380090&r2=393690

Comment 61

[Evgeniy]

Attachment: Make ALSA optional (updated for newer firefox version)

This is basically the old fix to make alsa build optional but edited to work with new firefox version.

Comment 62

[Evgeniy]

Attachment: Implements OSS support for libcubeb

Just minor tweaks to work with newer firefox. 
I've fixed the cracking on most music videos. I don't know why, but firefox sends floating PCM outside the [-1.0, 1.0] range. Is this a bug or I am misunderstanding something?

Comment 63

[Matthew Gregan [:kinetik]]

(In reply to Evgeniy from comment #62)
> Created attachment 8779845 [details] [diff] [review]
> Implements OSS support for libcubeb
> 
> Just minor tweaks to work with newer firefox. 
> I've fixed the cracking on most music videos. I don't know why, but firefox
> sends floating PCM outside the [-1.0, 1.0] range. Is this a bug or I am
> misunderstanding something?

That seems wrong, are you seeing that with everything or with specific files?

Comment 64

[Evgeniy]

(In reply to Matthew Gregan [:kinetik] from comment #63)
> (In reply to Evgeniy from comment #62) 
> > I've fixed the cracking on most music videos. I don't know why, but firefox
> > sends floating PCM outside the [-1.0, 1.0] range. Is this a bug or I am
> > misunderstanding something?
> 
> That seems wrong, are you seeing that with everything or with specific files?

This happens with almost any youtube music video. Particularly with the one Jan Beich has posted.

Comment 65

[Evgeniy]

I've just tested ALSA backend to see if maybe somehow this is specific to OSS. No, it appears that ALSA also receives values outside the [-1.0, 1.0] range. Weird.

Clipping occurs when I convert from floating point to integer PCM. One way would be to try and use floating point directly with OSS. However, I think OSS manual advices against that.

Comment 66

[Matthew Gregan [:kinetik]]

(In reply to Evgeniy from comment #65)
> I've just tested ALSA backend to see if maybe somehow this is specific to
> OSS. No, it appears that ALSA also receives values outside the [-1.0, 1.0]
> range. Weird.

I grabbed the Opus and Vorbis encodes with youtube-dl and the out of range values are coming straight out of the decoders.  Looking at the decoded file in Audacity shows that most of the file is heavily clipped.

Maybe we should be clamping the values as they're produced by the decoders, I don't know.

> Clipping occurs when I convert from floating point to integer PCM. One way
> would be to try and use floating point directly with OSS. However, I think
> OSS manual advices against that.

Looking at cubeb_oss.c, this conversion needs to clamp or it'll do the wrong thing:

+          buffer[i] = f_buffer[i]*32767.0;

e.g. if f_buffer[i] == -1.0078491 then after underflow buffer[i] == 32512.

Comment 67

[Evgeniy]

Attachment: new_oss.diff

I've done two things to address the overflow:
1)I do more or less proper clipping without overflowing int16_t.
2)Volume control for float pcm is done before the float -> int16_t conversion. This should improve quality and allow a user to avoid clipping at all by adjusting the volume.

Plus I think I've added a correct line to update.sh.

Is this the way to go?

Comment 68

[Matthew Gregan [:kinetik]]

It looks like that patch is just the build changes, no cubeb_oss.c.

BTW it'd be good to get the cubeb_oss.c bits into the upstream repo.

Comment 69

[Evgeniy]

Attachment: Add OSS support

This is the correct patch now.

Comment 70

[Matthew Gregan [:kinetik]]

(In reply to Evgeniy from comment #69)
> Created attachment 8781404 [details] [diff] [review]
> Add OSS support
> 
> This is the correct patch now.

Looks good, does that solve the popping you and Jan were hearing earlier?

Comment 71

[Evgeniy]

(In reply to Matthew Gregan [:kinetik] from comment #70)
> Looks good, does that solve the popping you and Jan were hearing earlier?
It does. With a good ear you can hear distortion on high volumes though. 
I'm not sure if I should ask the codec people about the [-1.0, 1.0] range or as an alternative reduce the output sound level a bit, that is 
do something like this:
    buffer[i] = f_buffer[i]*25000.0
instead of this:
    buffer[i] = f_buffer[i]*32767.0

Or do nothing at all.

Comment 72

[Evgeniy]

So... what's next?

Comment 73

[Matthew Gregan [:kinetik]]

(In reply to Evgeniy from comment #72)
> So... what's next?

Let's get this into the tree.  What needs to happen is:

1. submit a pull request to https://github.com/kinetiknz/cubeb for the ALSA dlopen changes
2. submit a pull request to https://github.com/kinetiknz/cubeb to add the OSS backend

I'll merge both of them ASAP.

3. obsolete all the patches attached here that we don't need to land
4. split out the Gecko-side changes (which should just be build system changes AFAIK), make sure they still apply or update them and get them re-reviewed by glandium

5. once the PRs are merged, run media/libcubeb/update.sh against a checkout of upstream libcubeb
6. land the libcubeb update and the build system changes

Make sense?

Comment 74

[Jan Beich]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3639125b054c

Comment 75

[Jan Beich]

Comment on attachment 8781404 [details] [diff] [review]
Add OSS support

Can you assert in oss_stream_init() that "Device selection not yet implemented" like other backends did?

https://github.com/kinetiknz/cubeb/commit/ff08f6601f7bfefafdd364cf11a621f6e063dc9b

Comment 76

[Jan Beich]

Does anyone else notice OSS latency is worse than PulseAudio? Try pausing/seeking fast or play music live. Here're steps to reproduce on FreeBSD:

  # pkg install firefox
  # pkg delete -f alsa-lib pulseaudio
  # sysctl hw.snd.verbose=2
  # sysctl hw.snd.latency=3
  # sysctl hw.snd.latency_profile=0

  $ firefox -no-remote -profile $(mktemp -d) http://lowlag.alienbill.com/playground.cgi
  <play a tune with "pluck" buttons or drum machine>
  $ fgrep -A3 firefox /dev/sndstat
        pcm3:play:dsp3.p0[pcm3:virtual:dsp3.vp1]: spd 44100/48000, fmt 0x00200010, flags 0x10001100, 0x00000029, pid 5760 (firefox)
        interrupts 0, underruns 0, feed 0, ready 0 [b:0/0/0|bs:131072/1024/128]
        channel flags=0x10001100<BUSY,HAS_SIZE,VIRTUAL>
        {userland} -> feeder_root(0x00200010) -> feeder_volume(0x00200010) -> feeder_rate(0x00200010 q:4 44100 -> 48000) -> {hardware}

131072 is very large buffer size, compare with

  $ mpv --no-config http://lowlag.alienbill.com/plucks/pluck.mp3
  $ fgrep -A3 mpv /dev/sndstat
        pcm3:play:dsp3.p0[pcm3:virtual:dsp3.vp1]: spd 44100/48000, fmt 0x00100010/0x00200010, flags 0x10000104, 0x00000069, pid 15182 (mpv)
        interrupts 0, underruns 0, feed 194, ready 0 [b:0/0/0|bs:4096/256/16]
        channel flags=0x10000104<RUNNING,BUSY,VIRTUAL>
        {userland} -> feeder_root(0x00100010) -> feeder_rate(0x00100010 q:4 44100 -> 48000) -> feeder_matrix(1.0 -> 2.0) -> feeder_volume(0x00200010) -> {hardware}

  $ mpg123 http://lowlag.alienbill.com/plucks/pluck.mp3
  $ fgrep -A3 mpg123 /dev/sndstat
        pcm3:play:dsp3.p0[pcm3:virtual:dsp3.vp1]: spd 44100/48000, fmt 0x00100010/0x00200010, flags 0x1000012c, 0x00000069, pid 14962 (mpg123)
        interrupts 0, underruns 0, feed 464, ready 4096 [b:0/0/0|bs:4096/256/16]
        channel flags=0x1000012c<RUNNING,TRIGGERED,SLEEPING,BUSY,VIRTUAL>
        {userland} -> feeder_root(0x00100010) -> feeder_rate(0x00100010 q:4 44100 -> 48000) -> feeder_matrix(1.0 -> 2.0) -> feeder_volume(0x00200010) -> {hardware}

Comment 77

[coypu]

ping, can we please get this merged? having the code as an out of tree patch really duplicates work
I personally spent several hours to independently fix the overflow issue not knowing about this bug report

Comment 78

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.

Comment 79

[coypu]

I've updated the patch to latest cubeb and run the testsuite there.
Merged some changes from the alternate OSS patch to hard-code things less.
Now it tries to open /dev/dsp and /dev/sound, I need the latter.

https://github.com/kinetiknz/cubeb/pull/440

Comment 80

[Matthew Gregan [:kinetik]]

https://github.com/mozilla/cubeb/pull/600 merged a new OSS backend that is reported to work well on FreeBSD's OSS. I'll mark this as resolved. Any remaining OSS issues should be filed against https://github.com/mozilla/cubeb for the backend maintainers to investigate.