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

Indeed I confirm this bug.

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 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?

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 think I've followed mozilla coding style pretty close this time and it should compile without warnings.

(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.

(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.

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.

(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.

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.

(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 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 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.

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

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?

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

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 on attachment 8488664 [details] [diff] [review] Make ALSA optional take 2 + build glue Looks good to me.

Comment on attachment 8488651 [details] [diff] [review] add OSS take3 + improved build glue sndio option before OSS on openBSD looks like the right choice.

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'll take a look at it.

(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.

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

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

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)

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 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.

(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 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.

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

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 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

> 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.

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.

(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.

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 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.

(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_* ?

(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 on attachment 8491860 [details] [diff] [review] add OSS support to firefox take 7 Current build glue still needs review.

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?

(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 on attachment 8491860 [details] [diff] [review] add OSS support to firefox take 7 stream->volume / stream->panning should be under mutex lock

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/jbeich@vfemail.net-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

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 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.

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.

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/jbeich@vfemail.net-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 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 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".

(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?

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].

(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.

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

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

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

(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

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

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 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

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

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?

(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?

(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.

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.

(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.

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?

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.

This is the correct patch now.

(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?

(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.

So... what's next?

(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 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

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}

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

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

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