579877 - sound output via libesd broken if no esound daemon running

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

The fix in bug 479929 broke sound notifications using nsISound.play if no esound-daemon is running on the machine. 
Apparently in recent Linux distributions the esound daemon is not started by default and auto-spawning doesn't work (anmymore?)
Therefore the changed rv checks there causes audio output to fail completely even if no esd would be required to play wav files.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 458315 [details] [diff] [review]
patch

>+        /* we don't need to do esd_open_sound if we are only going to play
>+         * files but we will if we want to do things like streams, etc */
>         elib = PR_LoadLibrary("libesd.so.0");
>         if (elib) {
>             EsdOpenSoundType EsdOpenSound =
>                 (EsdOpenSoundType) PR_FindFunctionSymbol(elib, "esd_open_sound");
>             if (!EsdOpenSound) {
>                 PR_UnloadLibrary(elib);
>                 elib = nsnull;
>             } else {
>                 esdref = (*EsdOpenSound)("localhost");
>-                if (esdref < 0) {
>+                if (!esdref) {
>                     PR_UnloadLibrary(elib);
>                     elib = nsnull;
>                 }

The docs for esd_open_sound says, in the header,
  "returns EsounD socket for communication, result < 0 = error"
and in the source
  "Return Value: -1 on error, else a socket number connected and authorized to ESD."

Checking for 0 therefore is not meaningful.
Perhaps the unload and elib = nsnull block should just be removed, but I find the comment confusing because the only thing we use elib for is esd_play_stream.

Comment 3

[Karl Tomlinson (:karlt)]

esd_play_stream seems to depend on esd_open_sound succeeding:

int esd_play_stream( esd_format_t format, int rate, 
                     const char *host, const char *name )
{
    int sock;
    int proto = ESD_PROTO_STREAM_PLAY;
    char name_buf[ ESD_NAME_MAX ];
    void (*phandler)(int);
      
    /* connect to the EsounD server */
    sock = esd_open_sound( host );
    if ( sock < 0 ) 
        return sock;

Could it be that esd_open_sound is timing out (only) the first time?
If esd_open_sound may behave differently on subsequent calls, then we should probably just remove the esd_open_sound code.

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #2)
> The docs for esd_open_sound says, in the header,
>   "returns EsounD socket for communication, result < 0 = error"
> and in the source
>   "Return Value: -1 on error, else a socket number connected and authorized to
> ESD."

Yes, so we can keep the rv checks but not unload/stop if esd_open_sound() fails.
 
> Perhaps the unload and elib = nsnull block should just be removed, but I find
> the comment confusing because the only thing we use elib for is
> esd_play_stream.

I find everything confusing here. I just found via testing that esd is apparently not running while sound playing still works.
I'll investigate a bit more but no guarantees. I somehow think that reverting a broken change to fix a regression makes still sense until more details can be figured out. (I ignore the fact that the regression sat there for over a year ;-))

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #3)
> Could it be that esd_open_sound is timing out (only) the first time?
> If esd_open_sound may behave differently on subsequent calls, then we should
> probably just remove the esd_open_sound code.

I'll check that.

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

Here you go: 
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsSound.cpp#383

There is fallback code if esd_play_stream() fails so my guess is (still doing verification) we are using exactly that but obviously even this fails if we unload the lib.
So the "right" fix might be to just kill the esd_open_sound() check in init entirely?

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to comment #6)
> There is fallback code if esd_play_stream() fails so my guess is (still doing
> verification) we are using exactly that but obviously even this fails if we
> unload the lib.

Ah, thanks.  I missed that.

> So the "right" fix might be to just kill the esd_open_sound() check in init
> entirely?

That's probably the simplest thing to do.

I wonder whether we should remember whether esd_open_sound() has failed and, if so, always skip esd_play_stream (but not unload elib), but that might be more complicated.

Comment 8

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #2

I don't think that remembering the esd status is a good thing. The user might start or stop the daemon while mozilla is running and so we can pick up the change during runtime.

Comment 9

[Karl Tomlinson (:karlt)]

Can you remove the other esdref references and EsdOpenSoundType/EsdCloseType too, please?

Comment 10

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #3

Remove unused code

Comment 11

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #4

Hmm, I'm not awake yet? This should have all cleanup bits.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 458573 [details] [diff] [review]
patch #4

Thanks!

Comment 13

[Wolfgang Rosenauer [:wolfiR]]

http://hg.mozilla.org/mozilla-central/rev/3711415f9164

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 458573 [details] [diff] [review]
patch #4

The regression was introduced in 1.9.2 and hits Thunderbird and Seamonkey most as people are using new mail notifications there.
Almost no risk I'd say.

Comment 15

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 458573 [details] [diff] [review]
patch #4

I missed the approval2.0 before pushing it to m-c. Sorry for that.
Could I get it post-mortem please?

Comment 18

[Mark Banner (:standard8)]

FTR I've just landed this on GECKO1927_20100730_RELBRANCH for inclusion in Thunderbird 3.1.2:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8aad8db264ba

Once we get gecko approval, we can land it on default in mozilla-1.9.2 for future versions of Thunderbird 3.1.x.

Comment 19

[Mike Cloaked]

Please can you tell me what the current status of this bug fix is?

I have just installed Fedora F13 on a system and running either thunderbird-3.1.2-1.fc13.i686 (stock version on this system), or the latest 3.1.3 nightly for today (22nd August 2010) I cannot get sound to play by going to Edit->Preferences->General and selecting for a specific xxx.wav file to play for incoming email.  The system beeps on incoming email, even though playing a wav file is selected. Clicking the "play" button in Thunderbird gives no response and no sound, but from a terminal "aplay xxx.wav" plays the file perfectly!

Is there a fix on the way for the nightly versions for 3.1.3?

Thanks

Comment 20

[Karl Tomlinson (:karlt)]

Mike, this bug should be fixed in 3.1.2, so best file a new bug.

Comment 21

[Mike Cloaked]

https://bugzilla.mozilla.org/show_bug.cgi?id=589732

Comment 22

[Mark Banner (:standard8)]

FTR I've just landed this on the release branch for Thunderbird 3.1.3:
 
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/974355da9f0d

I'll be pushing to get this into the nightly builds during the next security & stability cycle.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 458573 [details] [diff] [review]
patch #4

Approved for 1.9.2.11, a=dveditz for release-drivers

Comment 24

[Mark Banner (:standard8)]

Checked into 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6facfad0d58a

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Do we have a specific sound or repro scenario for this for Firefox? I'm not even sure what sounds we are missing. :-)

Comment 26

[Wolfgang Rosenauer [:wolfiR]]

I think from quickly running through the UI and code that in Firefox the only thing left is the typeahead not-found sound.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

On what operating system?

Comment 28

[Wolfgang Rosenauer [:wolfiR]]

Ah, the whole bug is Linux only.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

I know that but I'm under the impression that it isn't all Linuxes. Is this bug present in Ubuntu 10.4?

Comment 30

[Wolfgang Rosenauer [:wolfiR]]

I'm pretty sure it is. No modern Linux system runs the esd daemon anymore AFAIK. The esd library is designed to fall back to direct accessing the sound device but this regressed by checking the return code and unloading the lib because no daemon was running so the fallback sound output code wasn't available anymore.

Comment 31

[Vincent (caméléon)]

It is still impossible to play a wav sound in the Thunderbird preferences menu (new mail notification or Lightning alert are impacted).
Should I reopen this bug or create a new one?

I am using Thunderbird 5.0 and Ubuntu 11.04.

Comment 32

[Karl Tomlinson (:karlt)]

(In reply to comment #31)
Please create a new bug.