sound output via libesd broken if no esound daemon running

RESOLVED FIXED in mozilla2.0b2

Status

()

Core
Widget: Gtk
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: wolfiR, Assigned: wolfiR)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla2.0b2
All
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking1.9.2 .11+, status1.9.2 .11-fixed)

Details

(Whiteboard: [duptome][tb31needed][fixed tb3.1.2][fixed tb3.1.3] [qa-needs-str])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

7 years ago
Created attachment 458315 [details] [diff] [review]
patch
(Assignee)

Updated

7 years ago
Attachment #458315 - Flags: review?(roc)
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.
Attachment #458315 - Flags: review?(roc) → review-
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.
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

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

Comment 8

7 years ago
Created attachment 458569 [details] [diff] [review]
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.
Attachment #458315 - Attachment is obsolete: true
Attachment #458569 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #458569 - Flags: review? → review?(karlt)
Can you remove the other esdref references and EsdOpenSoundType/EsdCloseType too, please?
(Assignee)

Comment 10

7 years ago
Created attachment 458571 [details] [diff] [review]
patch #3

Remove unused code
Attachment #458569 - Attachment is obsolete: true
Attachment #458571 - Flags: review?(karlt)
Attachment #458569 - Flags: review?(karlt)
(Assignee)

Comment 11

7 years ago
Created attachment 458573 [details] [diff] [review]
patch #4

Hmm, I'm not awake yet? This should have all cleanup bits.
Attachment #458571 - Attachment is obsolete: true
Attachment #458573 - Flags: review?(karlt)
Attachment #458571 - Flags: review?(karlt)
Comment on attachment 458573 [details] [diff] [review]
patch #4

Thanks!
Attachment #458573 - Flags: review?(karlt) → review+
Whiteboard: duptome
(Assignee)

Comment 13

7 years ago
http://hg.mozilla.org/mozilla-central/rev/3711415f9164
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b2
(Assignee)

Comment 14

7 years ago
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.
Attachment #458573 - Flags: approval1.9.2.8?
(Assignee)

Comment 15

7 years ago
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?
Attachment #458573 - Flags: approval2.0?
Attachment #458573 - Flags: approval2.0? → approval2.0+
Duplicate of this bug: 534033
Duplicate of this bug: 551460
Whiteboard: duptome → [duptome][tb31needs]
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.
Whiteboard: [duptome][tb31needs] → [duptome][tb31needs][fixed tb3.1.2]
Blocks: 479929

Comment 19

7 years ago
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
Mike, this bug should be fixed in 3.1.2, so best file a new bug.

Comment 21

7 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=589732
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.
Whiteboard: [duptome][tb31needs][fixed tb3.1.2] → [duptome][tb31needs][fixed tb3.1.2][fixed tb3.1.3]

Updated

7 years ago
blocking1.9.2: --- → ?
blocking1.9.2: ? → .11+
status1.9.2: --- → wanted
Comment on attachment 458573 [details] [diff] [review]
patch #4

Approved for 1.9.2.11, a=dveditz for release-drivers
Attachment #458573 - Flags: approval1.9.2.9? → approval1.9.1.14+

Updated

7 years ago
Attachment #458573 - Flags: approval1.9.1.14+ → approval1.9.2.11+
Checked into 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6facfad0d58a
status1.9.2: wanted → .11-fixed
Whiteboard: [duptome][tb31needs][fixed tb3.1.2][fixed tb3.1.3] → [duptome][tb31needed][fixed tb3.1.2][fixed tb3.1.3]
Do we have a specific sound or repro scenario for this for Firefox? I'm not even sure what sounds we are missing. :-)
Whiteboard: [duptome][tb31needed][fixed tb3.1.2][fixed tb3.1.3] → [duptome][tb31needed][fixed tb3.1.2][fixed tb3.1.3] [qa-needs-str]
(Assignee)

Comment 26

7 years ago
I think from quickly running through the UI and code that in Firefox the only thing left is the typeahead not-found sound.
On what operating system?
(Assignee)

Comment 28

7 years ago
Ah, the whole bug is Linux only.
I know that but I'm under the impression that it isn't all Linuxes. Is this bug present in Ubuntu 10.4?
(Assignee)

Comment 30

7 years ago
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.
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.
(In reply to comment #31)
Please create a new bug.
You need to log in before you can comment on or make changes to this bug.