Redesign nsSound structure to put threading in xpwidget

RESOLVED WONTFIX

Status

()

defect
P3
normal
RESOLVED WONTFIX
10 years ago
9 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Depends on 1 bug, Blocks 4 bugs, {hang})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [See comment 3] for the detail tpi:+)

Attachments

(1 attachment, 37 obsolete attachments)

285.62 KB, patch
Details | Diff | Splinter Review
I worked on bug 498079, I implemented the async system sound playing only on Win32. But Linux has same problem, that's reported in bug 504670. We should implement the asynchronous player on XP level.

And I think it should be a service. We are create the nsISound instance. But perhaps, the cost may be expensive on Linux. And we should implement the navigation sounds (bug 248380), but it has tp damage in the current patch. This bug might help it...

I'll post the patch soon.
Flags: wanted1.9.2?
Posted patch Patch v1.0 (obsolete) — Splinter Review
Simple fix.

The nsISoundService inherits the nsISound interface. Therefore, the frontend developers and the add-on developers can use the new service easily.

They wrote like following code:

> Components.classes["@mozilla.org/sound;1"]
>           .createInstance(Components.interfaces.nsISound)
>           .playEventSound(Components.interfaces.nsISound.EVENT_SELECT_DIALOG_OPEN);

They should rewrite this as:

> Components.classes["@mozilla.org/soundservice;1"]
>           .getService(nsISoundService)
>           .playEventSound(Components.interfaces.nsISound..EVENT_SELECT_DIALOG_OPEN);
Attachment #404482 - Flags: superreview?(roc)
Attachment #404482 - Flags: review?(roc)
Posted patch Patch v1.1 (obsolete) — Splinter Review
sorry for the spam. fix a nit.
Attachment #404482 - Attachment is obsolete: true
Attachment #404484 - Flags: superreview?(roc)
Attachment #404484 - Flags: review?(roc)
Attachment #404482 - Flags: superreview?(roc)
Attachment #404482 - Flags: review?(roc)
nsSound is not threadsafe on Windows at least. In particular it doesn't support threadsafe refcounting.

I don't think this is quite the right approach. Here's what I think we should do for application sounds:

-- create a new service nsISoundPlayer that plays Wave files asynchronously given a URI. This can be fully cross-platform code that talks to libsydney_audio and maybe reuses some code from nsWaveDecoder for Wave parsing. Perhaps it should live in content/media/wave. This is what people should use to play Wave files from chrome or C++ when they can't use nsHTMLAudioElement.

-- create a new service nsISystemSound that implements nsISound::PlayEventSound for each platform (and Beep). This would use the current create-a-thread implementation on Windows, but doesn't need to on GTK2 AFAIK. Making it a service may address performance issues.

-- make nsSound cross-platform. nsSound::Play delegates to nsISoundPlayer. nsSound::PlayEventSound delegates to nsISystemSound. nsSound::PlaySystemSound delegates to either nsISoundPlayer or nsISystemSound.

This would get rid of all esd-related code from GTK2 nsSound.
One way to create nsISoundPlayer might be to simply use <audio> elements attached to the hidden window's document. I'm not sure if that will work for embedders though.
Flags: wanted1.9.2?
Flags: blocking1.9.2?
(In reply to comment #3)
> -- create a new service nsISystemSound that implements nsISound::PlayEventSound
> for each platform (and Beep). This would use the current create-a-thread
> implementation on Windows, but doesn't need to on GTK2 AFAIK. 

Why do you think that the GTK2 doesn't need to create a thread? There are two bugs excepting bug 504670: bug 110385 and bug 250186.

> Making it a service may address performance issues.

What does this mean? Ts regression?
(In reply to comment #5)
> (In reply to comment #3)
> > -- create a new service nsISystemSound that implements nsISound::PlayEventSound
> > for each platform (and Beep). This would use the current create-a-thread
> > implementation on Windows, but doesn't need to on GTK2 AFAIK. 
> 
> Why do you think that the GTK2 doesn't need to create a thread? There are two
> bugs excepting bug 504670: bug 110385 and bug 250186.

Because, as far as I know, those bugs involve esd and we won't need esd to play Wave files anymore. Is there any evidence that libcanberra's system sounds cause significant main-thread delays?

> > Making it a service may address performance issues.
> 
> What does this mean? Ts regression?

Yes.
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > -- create a new service nsISystemSound that implements nsISound::PlayEventSound
> > > for each platform (and Beep). This would use the current create-a-thread
> > > implementation on Windows, but doesn't need to on GTK2 AFAIK. 
> > 
> > Why do you think that the GTK2 doesn't need to create a thread? There are two
> > bugs excepting bug 504670: bug 110385 and bug 250186.
> 
> Because, as far as I know, those bugs involve esd and we won't need esd to play
> Wave files anymore. Is there any evidence that libcanberra's system sounds
> cause significant main-thread delays?

Ah, O.K. I understand.
Attachment #404484 - Flags: superreview?(roc)
Attachment #404484 - Flags: review?(roc)
Summary: Implement XP level asynchronous system sound playing service → Refactor nsSound structure
Whiteboard: [See comment 3] for the detail
Summary: Refactor nsSound structure → Redesign nsSound structure
Posted patch Patch v2.0 (obsolete) — Splinter Review
Adding nsISystemSoundService.
Attachment #404484 - Attachment is obsolete: true
Posted patch Patch v2.1 (obsolete) — Splinter Review
Attachment #404578 - Attachment is obsolete: true
(In reply to comment #4)
> One way to create nsISoundPlayer might be to simply use <audio> elements
> attached to the hidden window's document. I'm not sure if that will work for
> embedders though.

If the element works without document and other related classes (e.g., docshell, presContext and presShell), it might work fine.
Posted patch Patch v2.2 (obsolete) — Splinter Review
Attachment #404583 - Attachment is obsolete: true
Posted patch Patch v2.3 (obsolete) — Splinter Review
Attachment #404585 - Attachment is obsolete: true
Posted patch Patch v3.0 (only for Win32) (obsolete) — Splinter Review
Maybe, we shouldn't kill the old nsISound::Play method completely because the new nsIAudioPlayer is only enabled |#ifdef MOZ_MEDIA|.
Attachment #404631 - Attachment is obsolete: true
I think it's OK to depend on MOZ_MEDIA. That's only undefined if you have --disable-wave and --disable-ogg (perhaps because you're building on Linux without any sound libraries?), and in that case you probably don't want any sounds to play.
Isn't it ok on tire2 (and tire3) platforms?
I don't know of any platforms that don't want to implement a libsydney_audio backend but do support nsSound. The only two ports in that state currently are BeOS and Photon, neither of which is currently maintained.

If someone does want to do that, they can override nsISoundPlayer with their own implementation and we can find a way for them to do it then.
But they should really just implement libsydney_audio instead because that's pretty easy and it gives them <video> and <audio> support as well.

Using an audio element for nsISoundPlayer is also much better than the existing nsSound implementations, because it can start playing before the download finishes, and it supports buffering.

BTW in your patch I think instead of calling Play() I think you should call SetAutoplay(PR_TRUE). That way we'll only start playing when we think we can play the audio without having to pause for more data. For files this shouldn't make any difference, it should start playing immediately.
(In reply to comment #16)
> If someone does want to do that, they can override nsISoundPlayer with their
> own implementation and we can find a way for them to do it then.

ok, I have an idea: nsAudioPlayer implements the nsIStreamLoader interface and sends the raw data to nsISystemSoundService::PlayRawData() which isn't scriptable. So, it's easy. If it's needed, we can use this way.

(In reply to comment #17)
> But they should really just implement libsydney_audio instead because that's
> pretty easy and it gives them <video> and <audio> support as well.

There are no build options which only enables the audio support...

> BTW in your patch I think instead of calling Play() I think you should call
> SetAutoplay(PR_TRUE). That way we'll only start playing when we think we can
> play the audio without having to pause for more data. For files this shouldn't
> make any difference, it should start playing immediately.

Thanks.
(In reply to comment #18)
> (In reply to comment #16)
> > If someone does want to do that, they can override nsISoundPlayer with their
> > own implementation and we can find a way for them to do it then.
> 
> ok, I have an idea: nsAudioPlayer implements the nsIStreamLoader interface and
> sends the raw data to nsISystemSoundService::PlayRawData() which isn't
> scriptable. So, it's easy. If it's needed, we can use this way.

I'm not sure what you mean. The element (actually, its decoder) manages the data loading and we don't want to interfere with that. I honestly don't think we need to worry about this now, it's entirely hypothetical.

> (In reply to comment #17)
> > But they should really just implement libsydney_audio instead because that's
> > pretty easy and it gives them <video> and <audio> support as well.
> 
> There are no build options which only enables the audio support...

What do you mean? You can --disable-ogg in which case you won't get the Ogg codec, if for some reason you don't want it. But I don't know why anyone would want to disable video and enable audio.
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > If someone does want to do that, they can override nsISoundPlayer with their
> > > own implementation and we can find a way for them to do it then.
> > 
> > ok, I have an idea: nsAudioPlayer implements the nsIStreamLoader interface and
> > sends the raw data to nsISystemSoundService::PlayRawData() which isn't
> > scriptable. So, it's easy. If it's needed, we can use this way.
> 
> I'm not sure what you mean. The element (actually, its decoder) manages the
> data loading and we don't want to interfere with that. I honestly don't think
> we need to worry about this now, it's entirely hypothetical.

I said about that a fallback path for the some tire2 platform which cannot support MOZ_MEDIA. So, it's not for the MOZ_MEDIA supported platforms.

> > (In reply to comment #17)
> > > But they should really just implement libsydney_audio instead because that's
> > > pretty easy and it gives them <video> and <audio> support as well.
> > 
> > There are no build options which only enables the audio support...
> 
> What do you mean? You can --disable-ogg in which case you won't get the Ogg
> codec, if for some reason you don't want it. But I don't know why anyone would
> want to disable video and enable audio.

When some tire2 platform developers want to enable only the audio element for the nsIAudioPlayer, don't they need to enable video element too? I guess it means they need to implement the video's platform dependent code too. If it's easy, they should already support them. But if there are some platforms which don't support the media elements, they might want to enable only audio element for the nsISound::Play().

But they are just my worry, not a part of this bug. I'm sorry to make it get confused.
(In reply to comment #20)
> When some tire2 platform developers want to enable only the audio element for
> the nsIAudioPlayer, don't they need to enable video element too? I guess it
> means they need to implement the video's platform dependent code too.

Video doesn't depend on any platform-specific code.

However, you can use --enable-wave --disable-ogg if you don't want to build any video code.
Posted patch Patch v4.0 (obsolete) — Splinter Review
moved nsAudioPlayer from content/media to content/base/src because the class existence doesn't depend on MOZ_MEDIA. (content/media isn't build if MOZ_MEDIA isn't defined.)
Attachment #404849 - Attachment is obsolete: true
Posted patch Patch v4.0.1 (obsolete) — Splinter Review
updating for the latest trunk.
Attachment #405080 - Attachment is obsolete: true
You only have one element here. That means if you start playing one sound and then start another before the first sound is finished, the first sound will be cut off, which seems bad. We support mixing simultaneous sounds so you may as well use it. So I think we need multiple elements here. You'll need to hold references to them, so I suggest an nsCOMArray. You will probably need to attach an "ended" event listener to remove the element from your array when it's done. This also has the benefit that you don't keep an element around when sound is not playing.

+  mAudioElement->Play();

Don't do this, call Load() instead. You don't want to start playing immediately, you want autoplay to do its job.

We'll need a test here, at least something that plays a Wave file to make sure we don't crash. Right now I can't think of a good way to test that anything was actually played.
(In reply to comment #25)
> You only have one element here. That means if you start playing one sound and
> then start another before the first sound is finished, the first sound will be
> cut off, which seems bad. We support mixing simultaneous sounds so you may as
> well use it. So I think we need multiple elements here. You'll need to hold
> references to them, so I suggest an nsCOMArray. You will probably need to
> attach an "ended" event listener to remove the element from your array when
> it's done. This also has the benefit that you don't keep an element around when
> sound is not playing.

I have another idea. nsIAudioPlayer should have more methods if it's needed the complex controlling methods, e.g., stop, pause and something. Then, the multiple elements way makes complex code.

I suggest that when someone need to play multiple sounds, they should create multiple instances of nsAudioPlayer. So, nsXPSound should be able to hold the multiple instances of nsIAudioPlayer.
(In reply to comment #26)
> I have another idea. nsIAudioPlayer should have more methods if it's needed the
> complex controlling methods, e.g., stop, pause and something. Then, the
> multiple elements way makes complex code.

If someone needs to do that, they should just use the API on an nsHTMLAudioElement.

In fact, you could have nsIAudioPlayer::Play return a reference to the element. Then the caller could manipulate the element if they want to.
(In reply to comment #27)
> (In reply to comment #26)
> > I have another idea. nsIAudioPlayer should have more methods if it's needed the
> > complex controlling methods, e.g., stop, pause and something. Then, the
> > multiple elements way makes complex code.
> 
> If someone needs to do that, they should just use the API on an
> nsHTMLAudioElement.
> 
> In fact, you could have nsIAudioPlayer::Play return a reference to the element.
> Then the caller could manipulate the element if they want to.

If so, why do we need nsIAudioPlayer? I think that if we use nsIAudioPlayer interface, we should hide the nsIDOMHTMLMediaElement interface from the clients of nsIAudioPlayer. Otherwise, the clients should create the nsHTMLAudioElement directly.

And looks like that the current implementation kills the playing sound before the nsSound plays a system sound on some platforms (only same instance owns sound). I need to implement it.
(In reply to comment #28)
> If so, why do we need nsIAudioPlayer? I think that if we use nsIAudioPlayer
> interface, we should hide the nsIDOMHTMLMediaElement interface from the clients
> of nsIAudioPlayer. Otherwise, the clients should create the nsHTMLAudioElement
> directly.

Maybe we don't. Can JS that doesn't have access to a document (e.g., a JS XPCOM component) create an nsHTMLAudioElement that works correctly? How complicated is that?

> And looks like that the current implementation kills the playing sound before
> the nsSound plays a system sound on some platforms (only same instance owns
> sound). I need to implement it.

What do you mean?
(In reply to comment #29)
> (In reply to comment #28)
> > If so, why do we need nsIAudioPlayer? I think that if we use nsIAudioPlayer
> > interface, we should hide the nsIDOMHTMLMediaElement interface from the clients
> > of nsIAudioPlayer. Otherwise, the clients should create the nsHTMLAudioElement
> > directly.
> 
> Maybe we don't. Can JS that doesn't have access to a document (e.g., a JS XPCOM
> component) create an nsHTMLAudioElement that works correctly? How complicated
> is that?

The comment in the some code (e.g., said that the audio element is a special element which doesn't need some node info. See CreateHTMLAudioElement in nsLayoutModule.cpp.
http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#677

In JS, the element can be created by |new Audio([url])|.At that time, the element doesn't belong to any documents. I think the special case is needed for this case.
# I'm not sure how to catch the DOM event in this case.

> > And looks like that the current implementation kills the playing sound before
> > the nsSound plays a system sound on some platforms (only same instance owns
> > sound). I need to implement it.
> 
> What do you mean?

Just an additional information. In nsISound::Play of some platforms, the current implementation stops the previous playing sound, first. So, I need to implement nsIAudioPlayer::Stop or to store the nsIDOMHTMLAudioElement in nsXPSound...

When we use my idea:
* The clients need to manage one or more nsAudioPlayer instances.
* nsAudioPlayer needs to send a notification when the play is finished (by callback function?).
* nsIAudioPlayer doesn't depend on nsIDOMHTMLMediaElement interface.
  (I.e., we also can use another implementation in future)

When we use your idea:
* nsAudioPlayer needs to manage one or more nsIDOMHTMLAudioElement instances.
* The caller of nsIAudioPlayer::Play needs to manage them *too* if it needs to stop the play.
* nsAudioPlayer needs to send a notification when the play is finished (by callback function?).
* nsIAudioPlayer depends on nsIDOMHTMLMediaElement interface.
(In reply to comment #30)
> The comment in the some code (e.g., said that the audio element is a special
> element which doesn't need some node info. See CreateHTMLAudioElement in
> nsLayoutModule.cpp.
> http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#677

Oops, sorry.

The comments in the same code said that nsHTMLAudioElement doesn't need document and something. E.g., CreateHTMLAudioElement in nsLayoutModule.cpp said that the audio element is the special element.
http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#677
(In reply to comment #30)
> In JS, the element can be created by |new Audio([url])|.At that time, the
> element doesn't belong to any documents. I think the special case is needed
> for this case.
> # I'm not sure how to catch the DOM event in this case.

Ah, right. You can use element.addEventListener to catch the event, I think. I wonder if that works from a JS XPCOM component, say. If it does, then everyone can just use new Audio.
OK, smaug says that JS that doesn't have its own associated document can't use "new Audio()", it will fail.

So I think my idea of returning an nsHTMLAudioElement from nsISoundPlayer::Play is still the best, assuming you can listen for events on it. I'm pretty sure almost all clients will not need to do anything with the element. I don't want to add APIs to nsISoundPlayer that duplicate what's already on the element --- seeking, volume control, pausing, getting the duration, etc etc.

> * nsAudioPlayer needs to send a notification when the play is finished (by
> callback function?).

No, it doesn't.

> * nsIAudioPlayer depends on nsIDOMHTMLMediaElement interface.

I don't think it's a problem to depend on nsIDOMHTMLMediaElement. This interface is in HTML and can never go away.
(In reply to comment #33)
> OK, smaug says that JS that doesn't have its own associated document can't use
> "new Audio()", it will fail.

Sounds strange. Is it only about JS case? I created the nsHTMLAudioElement from the nsAudioPlayer which isn't related to any documents. (And of course I succeeded to play the sound from Error console. I use following code on Windows:
> javascript:var sound = Components.classes["@mozilla.org/sound;1"].createInstance(Components.interfaces.nsISound); sound.play(Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService).newURI("file:///c:/windows/Media/Windows%20Logon%20Sound.wav", null, null));
)

> > * nsAudioPlayer needs to send a notification when the play is finished (by
> > callback function?).
> 
> No, it doesn't.

So, when can nsSound release the owning pointers of the nsHTMLAudioElement (that is returned from |Play()|)? For stopping to play the previous sound of |nsISound::Play()|, we need to store them. (Note that current nsSound doesn't stop the previous sound when |nsISound::Play()| is called again.

So, if we return the nsIDOMHTMLAudioElement from Play method, the both candidate names (nsISoundPlayer and nsIAudioPlayer) sound strange for me because the new interface doesn't have other methods (also in the future). The new class jobs are to create a nsIDOMHTMLAudioElement at |Play()| and to release the element when it was finished. So, is the better name nsIAudioElementProvider or something? And should it have only |CreateElement| method?
Posted patch Patch v4.3 (obsolete) — Splinter Review
This patch can be build on tire1 platforms.

# The nsIAudioPlayer isn't changed yet.
Attachment #405088 - Attachment is obsolete: true
(In reply to comment #34)
> (In reply to comment #33)
> > OK, smaug says that JS that doesn't have its own associated document can't
> > use "new Audio()", it will fail.
> 
> Sounds strange. Is it only about JS case?

yes, I think so.

> So, when can nsSound release the owning pointers of the nsHTMLAudioElement
> (that is returned from |Play()|)? For stopping to play the previous sound of
> |nsISound::Play()|, we need to store them. (Note that current nsSound doesn't
> stop the previous sound when |nsISound::Play()| is called again.

I rethought this and actually you don't need to hold an owning reference, which will make this even easier. See bug 518659: we can make sure that the element keeps itself alive while sound is playing. The patch in that bug does this, although currently I think it won't work in the case where there's no ownerDocument. We need to tweak that patch so that the element will keep itself alive while it's playing with no owner document.

> So, if we return the nsIDOMHTMLAudioElement from Play method, the both
> candidate names (nsISoundPlayer and nsIAudioPlayer) sound strange for me
> because the new interface doesn't have other methods (also in the future). The
> new class jobs are to create a nsIDOMHTMLAudioElement at |Play()| and to
> release the element when it was finished. So, is the better name
> nsIAudioElementProvider or something? And should it have only |CreateElement|
> method?

I think nsISoundPlayer is still a good name because most of the time people are just going to call Play() and forget about the element.
Depends on: 518659
Posted patch Patch v5.0 (obsolete) — Splinter Review
(In reply to comment #36)
> (In reply to comment #34)
> > So, when can nsSound release the owning pointers of the nsHTMLAudioElement
> > (that is returned from |Play()|)? For stopping to play the previous sound of
> > |nsISound::Play()|, we need to store them. (Note that current nsSound doesn't
> > stop the previous sound when |nsISound::Play()| is called again.
> 
> I rethought this and actually you don't need to hold an owning reference, which
> will make this even easier. See bug 518659: we can make sure that the element
> keeps itself alive while sound is playing. The patch in that bug does this,
> although currently I think it won't work in the case where there's no
> ownerDocument. We need to tweak that patch so that the element will keep itself
> alive while it's playing with no owner document.

I don't understand well.

This patch's nsSoundPlayer holds each nsIDOMHTMLMediaElement until the playing is finished. Is it ok?

And nsISoundPlayer::Stop can stop the all playing sounds.

For example:

javascript:var sound =
  Components.classes["@mozilla.org/sound;1"].
             createInstance(Components.interfaces.nsISound);
  sound.play(Components.
             classes["@mozilla.org/network/io-service;1"].
             getService(Components.interfaces.nsIIOService).
             newURI("file:///c:/windows/Media/Windows%20Logon%20Sound.wav", null, null));
  sound.playEventSound(0);

In this case, the first Play methods should be stopped by the PlayEventSound method (for the compatibility with current build, I think this is good behavior, the notification sounds should be played clearly, but unfortunately, this cannot kill the another nsSound's playing sound).
Attachment #405237 - Attachment is obsolete: true
(In reply to comment #37)
> javascript:var sound =
>   Components.classes["@mozilla.org/sound;1"].
>              createInstance(Components.interfaces.nsISound);
>   sound.play(Components.
>              classes["@mozilla.org/network/io-service;1"].
>              getService(Components.interfaces.nsIIOService).
>              newURI("file:///c:/windows/Media/Windows%20Logon%20Sound.wav",
> null, null));
>   sound.playEventSound(0);
> 
> In this case, the first Play methods should be stopped by the PlayEventSound
> method (for the compatibility with current build, I think this is good
> behavior, the notification sounds should be played clearly, but unfortunately,
> this cannot kill the another nsSound's playing sound).

Why is this good behavior? I think the sounds should just overlap. That seems much more natural.
(In reply to comment #38)
> (In reply to comment #37)
> > javascript:var sound =
> >   Components.classes["@mozilla.org/sound;1"].
> >              createInstance(Components.interfaces.nsISound);
> >   sound.play(Components.
> >              classes["@mozilla.org/network/io-service;1"].
> >              getService(Components.interfaces.nsIIOService).
> >              newURI("file:///c:/windows/Media/Windows%20Logon%20Sound.wav",
> > null, null));
> >   sound.playEventSound(0);
> > 
> > In this case, the first Play methods should be stopped by the PlayEventSound
> > method (for the compatibility with current build, I think this is good
> > behavior, the notification sounds should be played clearly, but unfortunately,
> > this cannot kill the another nsSound's playing sound).
> 
> Why is this good behavior? I think the sounds should just overlap. That seems
> much more natural.

I don't think so. The system sounds shouldn't overlap to other sounds. E.g., on Windows, the menu popup sound is killed by next menu popup sound. I think if they overlap, they create a heavy chord (discord?) when you move the mouse cursor. (Note that this is just a sample, we don't use nsISoundPlayer for playing the menu popup sound.)

I agree that nsISoundPlayer::Play (nsISound::Play) doesn't kill the previous sounds. But nsISound::PlaySystemSound and nsISound::PlayEvnetSound should kill the previous sounds if they are played by same instance.

If some developers don't want to be stopped by them, they can use nsISoundPlayer directly.
OK, if you really want to do it this way you can, however I think in this case we should make nsISoundPlayer::Play not return an element, because it's too easy for the receiver of such an element to do something (e.g., pause the element) which makes nsSoundPlayer leak the element for the rest of the lifetime of the application.

If some client needs an element, and can't create its own, then we could add a different method to nsISoundPlayer, e.g. CreateElement. Those sounds would not be stopped by nsISoundPlayer::Stop (which is probably a good thing since that could confuse the client using the element).
Posted patch Patch v6.0 (obsolete) — Splinter Review
nsSound isn't platform-specific completely.
Attachment #405423 - Attachment is obsolete: true
Posted patch Patch v7.0 (obsolete) — Splinter Review
Attachment #405538 - Attachment is obsolete: true
Posted patch Patch v7.1 (obsolete) — Splinter Review
Attachment #405637 - Attachment is obsolete: true
Posted patch Patch v7.2 (obsolete) — Splinter Review
Attachment #405649 - Attachment is obsolete: true
Posted patch Patch v7.3 (obsolete) — Splinter Review
O.K. Let's go. Adding two interfaces:
nsISoundPlayer.idl and nsISystemSoundService.idl. I don't change nsISound interface. Instead, I add same methods and consts to nsISystemSoundService. I designed that nsISound inherits nsISystemSoundService in the old patch. But I think it's bad design, the new methods of nsISystemSoundService also changes the nsISound. So, when we will change nsISystemSoundService, we also need to change nsSound. Therefore, I duplicate the interface.

Now, nsSound is completely XP level class. nsSoundPlayer is so too. nsSystemSoundService is platform-specific. However, it inherits nsSystemSoundServiceBase which is XP level implementation. So, even if we'll need to change nsISystemSoundService only for some platforms, we don't need to change all platform's nsSystemSoundService.

nsSoundPlayer creates multiple nsIDOMHTMLMediaElement (audio element). When the playing is finished, or stopped by nsISoundPlayer::Stop, the element(s) is released.

The automated test only tests whether the tests can run completely.
Attachment #405666 - Attachment is obsolete: true
Attachment #405735 - Flags: superreview?(roc)
Attachment #405735 - Flags: review?(roc)
And I renamed nsSound.* to nsSystemSoundService.* in each platform-specific directly.
(In reply to comment #46)
> And I renamed nsSound.* to nsSystemSoundService.* in each platform-specific
> directly.

s/directly/directory
Posted patch Patch v7.4 (obsolete) — Splinter Review
fix some nits on OS/2.
Attachment #405735 - Attachment is obsolete: true
Attachment #405786 - Flags: superreview?(roc)
Attachment #405786 - Flags: review?(roc)
Attachment #405735 - Flags: superreview?(roc)
Attachment #405735 - Flags: review?(roc)
+ *  nsISoundPlayer provides to wave file playing.

nsISoundPlayer provides simple sound playback.

+ *  Actually, this interface implementation uses HTML5 audio element.  So,
+ *  the supported formats depend on it.

The implementation uses the HTML5 <audio> element, so supports any format supported by <audio>. Gecko in its default configuration supports Wave (with 8-bit or 16-bit PCM samples and at most 2 channels), and Ogg Vorbis.

+ *  When you call play method before the previous playing sound is finished,
+ *  the previous sound isn't stopped automatically.  So, the sounds overlap.

When you call play() before the previously playing sound is finished,
the previous sound isn't stopped automatically, so the sounds overlap.

+ *  If you don't want the overlap, you should call stop method before play.
+ *  The stop method stops all playing sounds which are played by the instance.

If you don't want the overlap, you should call stop() before play().
The stop() method stops playing all in-progress sounds that are being played via this interface.

+ *  NOTE: If you get the instance as a service, the instance is used by other
+ *        clients too, so, your playing sound may be stopped unexpectedly.
+ *        E.g., nsISystemSoundService stops the playing sounds when it plays
+ *        a system sound.  Therefore, if you don't want to stop the sound, you
+ *        should create an instance.

I think we should just have a service and allow only one instance. It's much simpler and clearer.

+  /**
+    * Not strictly necessary, but avoids delay before first event sound.
+    * playEventSound method call this automatically.
+    */
+  void init(); 

I think we should take this out and just make the service implementation be responsible for whatever's needed to avoid noticeable delays when playing the first sound.

If we need to avoid library load delays in Windows and Linux, then they could use a startup observer and initialize those libraries some seconds after startup (and possibly on a separate thread). Have we actually measured those delays on Windows and Linux to be sure we need this, though?

+  /**
+   * Plays system sounds of the given alias name.  Please note that you cannot
+   * use the sound event names which begins "_moz_" (defined in nsISound.idl).
+   * If you want to play such event sounds, you can use playEventSound.
+   */
+  void playAlias(in AString aSoundAlias);

I would just say
/**
 * Play the platform's sound with the given alias name. Names and supported
 * sounds are platform specific. _moz_ alias names are not supported here.
 */

Does PlayAlternativeSound only exist to support Photon? That is unnecessary, let's get rid of it.

+  if (NS_IsMozAliasSound(aSoundAlias)) {

We don't IsMozAliasSound anymore, do we? We can just use the chain of "if" statements in nsSound::PlaySystemSound.
(In reply to comment #49)
> + *  NOTE: If you get the instance as a service, the instance is used by other
> + *        clients too, so, your playing sound may be stopped unexpectedly.
> + *        E.g., nsISystemSoundService stops the playing sounds when it plays
> + *        a system sound.  Therefore, if you don't want to stop the sound, you
> + *        should create an instance.
> 
> I think we should just have a service and allow only one instance. It's much
> simpler and clearer.

Can I suppress the createInstance()? Looks like that createInstance() always creates a new instance even if there is a service instance.

> +  /**
> +    * Not strictly necessary, but avoids delay before first event sound.
> +    * playEventSound method call this automatically.
> +    */
> +  void init(); 
> 
> I think we should take this out and just make the service implementation be
> responsible for whatever's needed to avoid noticeable delays when playing the
> first sound.
> 
> If we need to avoid library load delays in Windows and Linux, then they could
> use a startup observer and initialize those libraries some seconds after
> startup (and possibly on a separate thread). Have we actually measured those
> delays on Windows and Linux to be sure we need this, though?

I'm not sure on Linux. But I think that the delay on Windows causes bug 498079 and bug 498089. So, I think that we could just remove the Init() of Windows. The delay isn't problem on Windows now.

How about all platform system sounds are played on another thread (including the initializing)? If we just put out the initializing from main thread, we need to ignore (or delay) the play methods during initializing.
(In reply to comment #50)
> (In reply to comment #49)
> > + *  NOTE: If you get the instance as a service, the instance is used by other
> > + *        clients too, so, your playing sound may be stopped unexpectedly.
> > + *        E.g., nsISystemSoundService stops the playing sounds when it plays
> > + *        a system sound.  Therefore, if you don't want to stop the sound, you
> > + *        should create an instance.
> > 
> > I think we should just have a service and allow only one instance. It's much
> > simpler and clearer.
> 
> Can I suppress the createInstance()? Looks like that createInstance() always
> creates a new instance even if there is a service instance.

If I create a delegate class which is created by the service manager and it creates only one instance of nsSoundPlayer, we could ensure that there is only one instance. Do you like this behavior?
(In reply to comment #49)
> +  if (NS_IsMozAliasSound(aSoundAlias)) {
> 
> We don't IsMozAliasSound anymore, do we? We can just use the chain of "if"
> statements in nsSound::PlaySystemSound.

Do you like this?

> NS_IMETHODIMP
> nsSound::PlaySystemSound(const nsAString &aSoundAlias)
> {
>   if (aSoundAlias.IsEmpty())
>     return NS_OK;
> 
>   // Stop all sounds before we play a system sound.
>   nsresult rv = Stop();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   NS_ASSERTION(!NS_IsMozAliasSound(aSoundAlias),
>     "nsISound::playSystemSound is called with \"_moz_\" events, they are obsolete, use nsISystemSoundService::playEventSound instead");
>   if (aSoundAlias.Equals(NS_SYSSOUND_MAIL_BEEP)) {
>     PlayEventSound(EVENT_NEW_MAIL_RECEIVED);
>   } else if (aSoundAlias.Equals(NS_SYSSOUND_CONFIRM_DIALOG)) {
>     PlayEventSound(EVENT_CONFIRM_DIALOG_OPEN);
>   } else if (aSoundAlias.Equals(NS_SYSSOUND_ALERT_DIALOG)) {
>     PlayEventSound(EVENT_ALERT_DIALOG_OPEN);
>   } else if (aSoundAlias.Equals(NS_SYSSOUND_PROMPT_DIALOG)) {
>     PlayEventSound(EVENT_PROMPT_DIALOG_OPEN);
>   } else if (aSoundAlias.Equals(NS_SYSSOUND_SELECT_DIALOG)) {
>     PlayEventSound(EVENT_SELECT_DIALOG_OPEN);
>   } else if (aSoundAlias.Equals(NS_SYSSOUND_MENU_EXECUTE)) {
>     PlayEventSound(EVENT_MENU_EXECUTE);
>   } else if (aSoundAlias.Equals(NS_SYSSOUND_MENU_POPUP)) {
>     PlayEventSound(EVENT_MENU_POPUP);
>   } else if (NS_IsMozAliasSound(aSoundAlias)) {
>     return NS_OK;
>   }
> 
>   // First, assume that the given string is an alias sound name.
>   nsCOMPtr<nsISystemSoundService> sysSound;
>   ...

Or should the file playing code be in the else block without |else if (NS_IsMozAliasSound(aSoundAlias))|?
roc, would you check the previous 3 comments?
(In reply to comment #50)
> Can I suppress the createInstance()? Looks like that createInstance() always
> creates a new instance even if there is a service instance.

I don't know, ask someone who knows more about XPCOM than me...

> How about all platform system sounds are played on another thread (including
> the initializing)? If we just put out the initializing from main thread, we
> need to ignore (or delay) the play methods during initializing.

We don't need to do the initializing on a different thread. We just need to make sure the initializing doesn't impact Ts and try to avoid it happening during a user action. So doing it on a timer after startup should be fine.

(In reply to comment #51)
> If I create a delegate class which is created by the service manager and it
> creates only one instance of nsSoundPlayer, we could ensure that there is only
> one instance. Do you like this behavior?

I'm not really sure what you mean, you should ask someone who knows more about XPCOM.

> Or should the file playing code be in the else block without |else if
> (NS_IsMozAliasSound(aSoundAlias))|?

Yes.
If you want a service that doesn't get recreated on createInstance, hack the CreateInstance method to return a singleton.  The getService/createInstance mismatch is probably one of the more fundamental botches of XPCOM, incidentally.
Thank you, Jeff. I'm trying it now.
(In reply to comment #54)
> (In reply to comment #50)
> > How about all platform system sounds are played on another thread (including
> > the initializing)? If we just put out the initializing from main thread, we
> > need to ignore (or delay) the play methods during initializing.
> 
> We don't need to do the initializing on a different thread. We just need to
> make sure the initializing doesn't impact Ts and try to avoid it happening
> during a user action. So doing it on a timer after startup should be fine.

The service instance is created when the class is used first. So, we don't need to worry about ts regression. However, the first system sound might be delayed... We need to find a way to create the instance at start-up.
ok, looks like nsBaseAppShell::Init() is run at start-up on all platforms.
# the constructor of nsBaseWidget isn't run on some platforms :-(
Posted patch Patch v8.2 (obsolete) — Splinter Review
checking on the tryserver.
Attachment #405786 - Attachment is obsolete: true
Attachment #405786 - Flags: superreview?(roc)
Attachment #405786 - Flags: review?(roc)
Posted patch Patch v8.2.1 (obsolete) — Splinter Review
fix a nit.
Attachment #406694 - Attachment is obsolete: true
Attachment #406695 - Flags: superreview?(roc)
Attachment #406695 - Flags: review?(roc)
Shouldn't the body of CreateAudioElement be #ifdef MOZ_MEDIA? Maybe nsSoundPlayer shouldn't even be built if we don't have MOZ_MEDIA?

I think we should have a utility method nsContentUtils::PlaySystemSound for all the content/layout consumers.

+  static nsresult GetFileURL(const nsAString &aFilePath,
+                             nsIFileURL **aFileURL);

Just return already_AddRefed<nsIFileURL>.

+  static nsresult GetSystemSoundService(nsISystemSoundService **aService);

Just return already_AddRefed<nsISystemSoundService>.

+  static nsresult GetSoundPlayer(nsISoundPlayer **aPlayer);

Just return already_AddRefed<nsISoundPlayer>.

+  static nsresult PlayOnSoundPlayer(nsIURL *aURL);

I think better to call this just "Play".

+  static nsresult StopSoundPlayer();

Need not return a result, just void.
(In reply to comment #62)
> Maybe nsSoundPlayer shouldn't even be built if we don't have MOZ_MEDIA?

I don't think so. It returns NS_ERROR_NOT_IMPLEMENTED that makes the failed reason clearly.

> I think we should have a utility method nsContentUtils::PlaySystemSound for all
> the content/layout consumers.

What's the method? nsISystemSoundService has beep, playEventSound and playAlias. So, do you want me to create the all methods to the nsContentUtils?
(In reply to comment #63)
> (In reply to comment #62)
> > Maybe nsSoundPlayer shouldn't even be built if we don't have MOZ_MEDIA?
> 
> I don't think so. It returns NS_ERROR_NOT_IMPLEMENTED that makes the failed
> reason clearly.

And that's easier to maintain than not built. E.g., some nsSoundPlayer consumers can think that the instance must be returned on all platforms.
(In reply to comment #63)
> I don't think so. It returns NS_ERROR_NOT_IMPLEMENTED that makes the failed
> reason clearly.

OK.

> > I think we should have a utility method nsContentUtils::PlaySystemSound for all
> > the content/layout consumers.
> 
> What's the method? nsISystemSoundService has beep, playEventSound and
> playAlias. So, do you want me to create the all methods to the nsContentUtils?

Sorry, I meant PlayEventSound. I think that's the only one we need, because there are several calls to that one but not the others.
Posted patch Patch v9.0 (obsolete) — Splinter Review
ok, I added nsISystemSoundService::EVENT_MENU_NOT_FOUND for nsISystemSoundService::Beep() in the XUL menu code. And also I added nsSystemSoundService::IsSupportedEvent() which checks the event ID is supported on the platform. If it's not supported, we shouldn't stop the sound player by the event.
Attachment #406695 - Attachment is obsolete: true
Attachment #406837 - Flags: superreview?(roc)
Attachment #406837 - Flags: review?(roc)
Attachment #406695 - Flags: superreview?(roc)
Attachment #406695 - Flags: review?(roc)
The body of HandleEvent also needs to be #ifdef MOZ_MEDIA right, otherwise nsIDOMHTMLMediaElement will not exist?

> And also I added nsSystemSoundService::IsSupportedEvent() which checks the
> event ID is supported on the platform. If it's not supported, we shouldn't stop
> the sound player by the event.

Why not just call PlayEventSound first and if succeeds then call nsISoundPlayer::Stop?

+  nsCOMPtr <nsILocalFile> file;
+  nsCOMPtr <nsIURI> fileURI;

Why the space between nsCOMPtr and <?

+  NS_ADDREF(fileURL);
+  return fileURL.get();

Just use fileURL.forget();

+  NS_ADDREF(player);
+  return player.get();

Same here.
(In reply to comment #67)
> The body of HandleEvent also needs to be #ifdef MOZ_MEDIA right, otherwise
> nsIDOMHTMLMediaElement will not exist?

nsIDOMHTMLMediaElement is always existing, probably.
(In reply to comment #67)
> > And also I added nsSystemSoundService::IsSupportedEvent() which checks the
> > event ID is supported on the platform. If it's not supported, we shouldn't stop
> > the sound player by the event.
> 
> Why not just call PlayEventSound first and if succeeds then call
> nsISoundPlayer::Stop?

Because we should stop the sounds *before* we play a system sound...
I assume that stopping the sounds a microsecond after we start playing a system sound is not noticeably different to stopping the sounds a microsecond before we start playing the system sound.
Posted patch Patch v10.0 (obsolete) — Splinter Review
(In reply to comment #70)
> I assume that stopping the sounds a microsecond after we start playing a system
> sound is not noticeably different to stopping the sounds a microsecond before
> we start playing the system sound.

I don't like the overlap even if it's short. So, this patch calls StopSoundPlayer() from each platform implementation before they call the native sound APIs.
Attachment #406837 - Attachment is obsolete: true
Attachment #406857 - Flags: superreview?(roc)
Attachment #406857 - Flags: review?(roc)
Attachment #406837 - Flags: superreview?(roc)
Attachment #406837 - Flags: review?(roc)
Attachment #406857 - Flags: superreview?(roc)
Attachment #406857 - Flags: superreview+
Attachment #406857 - Flags: review?(roc)
Attachment #406857 - Flags: review+
Comment on attachment 406857 [details] [diff] [review]
Patch v10.0

I don't think we need to worry about the overlap, but it doesn't matter.
http://hg.mozilla.org/mozilla-central/rev/d2ac46221368

landed, thank you roc!
# I'll check the dependent bugs after checking the tinderbox status.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 74

10 years ago
  38.165 +NS_IMETHODIMP
  38.166 +nsSystemSoundService::PlayEventSound(PRUint32 aEventID)
  38.167 +{
  38.168 +    nsresult rv = nsSystemSoundServiceBase::PlayEventSound(aEventID);
  38.169 +    NS_ENSURE_SUCCESS(rv, rv);
  38.170 +
  38.171 +    if (!libcanberra)
  38.172 +        return NS_OK;
  38.173 +

Does library initialization happen anywhere? Did you test this? I really hope you didn't regress event sounds on Linux again...

Comment 75

10 years ago
If a regression has occurred, do you mind just readding the mInited variable and the Init() call? Thanks. :)
Michael:

Init() is automatically called after immediately startup process is finished. If they called before the Init() called, any sounds are not played on all platforms (the base class's same methods returns NS_ERROR_NOT_INITIALIZED).
This appears to have caused a 6=8% Ts dirty profiled regression on XP. Thoughts?
er, 6-8%.

Comment 80

10 years ago
(In reply to comment #76)
> Michael:
> 
> Init() is automatically called after immediately startup process is finished.
> If they called before the Init() called, any sounds are not played on all
> platforms (the base class's same methods returns NS_ERROR_NOT_INITIALIZED).

Oh yes, that's right. Thank you.
Posted patch Additional patch v1.0 (obsolete) — Splinter Review
(In reply to comment #77)
> This appears to have caused a 6=8% Ts dirty profiled regression on XP.
> Thoughts?

(In reply to comment #79)
> Better back this out.

Looks like the regression is mainly on Linux. And also on Win32 but it's lesser than Linux. I don't find any regression on Mac.

So, It means that the delayed nsSystemSoundService::Init() is executed during starting-up. I hoped that it's run after the start-up process is completely finished. But it's failed.

So, I have two ideas for fixing it:

1. Move the nsSystemSoundServiceBase::InitService() call to another point.
2. nsSystemSoundService::Init() should be run in another thread.

This patch is plan #2. How do you think, roc?
Attachment #406857 - Attachment is obsolete: true

Comment 82

10 years ago
 nsresult
 nsSystemSoundService::Init()
-{
+{NS_WARNING("************************************************************************************inited!!!!");

Don't forget the leftover debug prints!
Thank you, the patch is draft, it cannot be built. The new patch coming soon.
Posted patch Additional patch v2.0 (obsolete) — Splinter Review
this should fix the ts regression.

The nsSystemSoundService::Init() is run on another thread. And the nsSystemSoundServiceBase::InitService() is called from at widget creation.
# If we call nsSystemSoundServiceBase::InitService() from nsBaseAppShell::Init(), we shouldn't get the service instance directly because the recursive service registration happens.
Attachment #406895 - Attachment is obsolete: true
Attachment #406897 - Flags: superreview?(roc)
Attachment #406897 - Flags: review?(roc)
backed out, now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Initializing stuff on another thread isn't free, that thread has to compete with the main thread for resources. What makes you think this will fix the Ts regression completely?
I don't really understand why we have a regression here. If a sound plays during Ts, what extra work are doing now that we weren't doing before?
(In reply to comment #86)
> Initializing stuff on another thread isn't free, that thread has to compete
> with the main thread for resources. What makes you think this will fix the Ts
> regression completely?

The main works of the initialization is that we load the system library. Probably, you are right, another thread might make the main thread slowly. However, the main thread doesn't need to wait the initialization, it might help.

(In reply to comment #87)
> I don't really understand why we have a regression here. If a sound plays
> during Ts, what extra work are doing now that we weren't doing before?

Windows/Linux/OS/2 need to initialize the system sound library before first system sound playing. If we initialize them at the first playing, we may have the bad UI response (That is same as the ts regression). So, we should initialize them at idle time.

The landed patch tried to initialize with timer, but the timer event was fired during the start up, unfortunately.
With additional patch:

qm-pubuntu-try01 ts: 624.58
qm-pubuntu-try02 ts: 627.95
qm-pubuntu-try03 ts: 626.79
qm-pxp-try02     ts: 523.74
qm-pvista-try01  ts: 579.68

Only Patch v10.0:

qm-pubuntu-try02 ts: 605.26
qm-pubuntu-try03 ts: 625.53
qm-pubuntu-try03 ts: 624.0
qm-pubuntu-try05 ts: 628.26
qm-pxp-try04     ts: 546.74
qm-pvista-try04  ts: 606.68

Um...
Attachment #406897 - Flags: superreview?(roc)
Attachment #406897 - Flags: review?(roc)
The most important thing is to fix bug 504670 by the patch. The delay at the first system sound playing *in* another thread isn't a big problem (because it doesn't block the UI operation).

Perhaps, we should use another thread for the playEventSound and playAlias.

1. We should just remove the automatic initializing code.
2. The playEventSound and the playAlias initialize the library themselves.
3. We should initialize and play on another thread on Windows or Linux or OS/2.

Updated

10 years ago
Keywords: dev-doc-needed
Posted patch Patch v11.0 (obsolete) — Splinter Review
draft of the new approach (might have build error).
Posted patch Patch v11.2 (obsolete) — Splinter Review
Attachment #406911 - Attachment is obsolete: true

Comment 93

10 years ago
On OS/2, the two menu event sounds fail to play.  When nsPlayer::CreateAudioElement() calls do_CreateInstance(), it returns NS_ERROR_OUT_OF_MEMORY.  Sounds for the various dialogs work OK, and so does the sound associated with accessibility.typeaheadfind.soundURL.  This is with Patch 10 installed but I don't see any changes in Patch 11.2 that might eliminate the error.

BTW, Masayuki...  almost all of the code you carried over from OS/2's old nsSound is obsolete (we were about to commit a complete rewrite).  When you're back online, I'll send you a revision for the OS/2 portion of your patch.  One question:  in Patch 11.2, why is OS/2 the only platform where you call InitService() from nsWindow?
(In reply to comment #88)
> The landed patch tried to initialize with timer, but the timer event was fired
> during the start up, unfortunately.

OK, so we're not actally playing sounds during startup, just accidentally firing the timer event then?
+nsSystemSoundService::PlayEventSound(PRUint32 aEventID)
+{
+    nsresult rv = nsSystemSoundServiceBase::PlayEventSound(aEventID);
+    NS_ENSURE_SUCCESS(rv, rv);

It's misleading to call the base class method here, since we're not asking the base class to play anything, we just want it to stop the currently playing sounds. So I suggest you call a method named Stop instead.

I think we should move all the thread-related code down into each platform implementation, since it's confusing to have mThread and Post for platforms that don't need it.

I think we can optimize the Windows and GTK2 code so we don't have to keep a thread around all the time. Can we do this:
-- On Windows, every time an event sound is played, create a new thread to play the sound, exiting the thread immediately.
-- On GTK2, the first time an event sound is played, create a new thread to load libcanberra and play the sound, exiting the thread immediately. After the first time, we shouldn't need to create any more threads to play sounds. I think it's fine to release libcanberra on the main thread during shutdown. I'm not sure if there's a safety issue about releasing libcanberra while a sound is playing, but it seems to me if there is, it would already be a problem on trunk.
I haven't heard any reports of delays opening libcanberra, so perhaps there may be no need to do that on another thread or even off an event, and opening on first event sound might be fine?
I guess initializing libcanberra takes some time, since that must be the cause of the Ts regression, right?

But I agree we should just load libcanberra on the main thread for now, when the first event sound plays. At the moment we don't have any reason to believe that will be a noticeable problem, so we should go with the simplest solution until we have proof it needs to be improved.
(In reply to comment #94)
> (In reply to comment #88)
> > The landed patch tried to initialize with timer, but the timer event was fired
> > during the start up, unfortunately.
> 
> OK, so we're not actally playing sounds during startup, just accidentally
> firing the timer event then?

Yes.

(In reply to comment #95)
> +nsSystemSoundService::PlayEventSound(PRUint32 aEventID)
> +{
> +    nsresult rv = nsSystemSoundServiceBase::PlayEventSound(aEventID);
> +    NS_ENSURE_SUCCESS(rv, rv);
> 
> It's misleading to call the base class method here, since we're not asking the
> base class to play anything, we just want it to stop the currently playing
> sounds. So I suggest you call a method named Stop instead.

Ah, they can be just removed now.

> I think we should move all the thread-related code down into each platform
> implementation, since it's confusing to have mThread and Post for platforms
> that don't need it.

I'll think about this.

> -- On GTK2, the first time an event sound is played, create a new thread to
> load libcanberra and play the sound, exiting the thread immediately. After the
> first time, we shouldn't need to create any more threads to play sounds. I
> think it's fine to release libcanberra on the main thread during shutdown. I'm
> not sure if there's a safety issue about releasing libcanberra while a sound is
> playing, but it seems to me if there is, it would already be a problem on
> trunk.

If we initialize the libs in another thread, we need to check whether the thread is completely finished or not if we release them from main thread. Of course, it's rare case, but it's dangerous issue. For it, we need to hold the thread in the current design.

(In reply to comment #96)
> I haven't heard any reports of delays opening libcanberra, so perhaps there may
> be no need to do that on another thread or even off an event, and opening on
> first event sound might be fine?

(In reply to comment #97)
> I guess initializing libcanberra takes some time, since that must be the cause
> of the Ts regression, right?
>
> But I agree we should just load libcanberra on the main thread for now, when
> the first event sound plays. At the moment we don't have any reason to believe
> that will be a noticeable problem, so we should go with the simplest solution
> until we have proof it needs to be improved.

See:
> http://graphs.mozilla.org/#tests=[{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%2251%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%2252%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%2253%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%2254%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%2273%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22188%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22189%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22190%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22191%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22192%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22193%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22194%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22195%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22196%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22197%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22198%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22199%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22200%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22201%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22202%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22203%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22204%22},{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%22205%22}]&sel=1255753004,1255895647

Looks like there is 5ms or more delay. And the delay depends on the environment of course. So, I'm afraid that some users can hate the delay at first sound blocks the UI.

(In reply to comment #93)
> On OS/2, the two menu event sounds fail to play.  When
> nsPlayer::CreateAudioElement() calls do_CreateInstance(), it returns
> NS_ERROR_OUT_OF_MEMORY.  Sounds for the various dialogs work OK, and so does
> the sound associated with accessibility.typeaheadfind.soundURL.  This is with
> Patch 10 installed but I don't see any changes in Patch 11.2 that might
> eliminate the error.
>
> BTW, Masayuki...  almost all of the code you carried over from OS/2's old
> nsSound is obsolete (we were about to commit a complete rewrite).  When you're
> back online, I'll send you a revision for the OS/2 portion of your patch.

Thanks a lot.

> question:  in Patch 11.2, why is OS/2 the only platform where you call
> InitService() from nsWindow?

It's just a mistake, it should be removed.
> I think we can optimize the Windows and GTK2 code so we don't have to keep a
> thread around all the time.

I guess that creating a new thread is expensive, isn't it right? The sound for an action should be played immediately as far as possible.
(In reply to comment #98)
> Looks like there is 5ms or more delay. And the delay depends on the 
> environment of course. So, I'm afraid that some users can hate the delay at
> first sound blocks the UI.

A 5ms delay would not be perceptible. Friends who worked on real-time audio synthesis once told me that if you're a really good piano player, delays of about 8ms between pressing a key and sound being heard are perceptible, but smaller delays are not. In other situations, and for people with normal hearing, the threshold will be much higher. Let's not worry about this until we know there's a problem.
Sorry for the delay, everyone.

(In reply to comment #100)
> (In reply to comment #98)
> > Looks like there is 5ms or more delay. And the delay depends on the 
> > environment of course. So, I'm afraid that some users can hate the delay at
> > first sound blocks the UI.
> 
> A 5ms delay would not be perceptible. Friends who worked on real-time audio
> synthesis once told me that if you're a really good piano player, delays of
> about 8ms between pressing a key and sound being heard are perceptible, but
> smaller delays are not. In other situations, and for people with normal
> hearing, the threshold will be much higher. Let's not worry about this until we
> know there's a problem.

Yes, I don't think the 5ms delay isn't problem too. But the example score is on the tryserver machines and each machine has different delay scores. So, it's possible that there are some environments need more time for the initialization. Should we wait the reports from such environment users if I want to support async system sound play on Linux?

Rich:

Thank you for your patch of new nsSound of OS/2. It helps me very well!

I have a question. Do you think the async system sound play is needed on OS/2? 

The OS/2 sound code loads the some libraries on the main thread (initSounds()). But other libraries are loaded in another thread (initDlls()).

If initSounds() needs long time, the users experience the hanging up when first menu access (bug 498089 and bug 504670) or first dialog open (bug 498079).
(In reply to comment #101)
> Yes, I don't think the 5ms delay isn't problem too. But the example score is on
> the tryserver machines and each machine has different delay scores. So, it's
> possible that there are some environments need more time for the
> initialization. Should we wait the reports from such environment users if I
> want to support async system sound play on Linux?

Yes. We should load the library synchronously now and if there are common situations where the load delay creates a perceptible problem, we should solve it then.

Let's do the same for OS/2. Except that for OS/2, if there is a problem then I think an OS/2 maintainer should fix it, not you.

Comment 104

10 years ago
(In reply to comment #101)
> I have a question. Do you think the async system sound play is needed on
> OS/2? 

It has always been asynchronous.  The only difference is that now we're using nsRunnable instead of native code to create a playback thread.

> The OS/2 sound code loads the some libraries on the main thread
> (initSounds()).  But other libraries are loaded in another thread
> (initDlls()).

initSounds() has to be run so PlayEventSound() knows whether the user has assigned a sound to an event - if not, there's no reason to continue.  initDlls() is called as late as possible to avoid loading the dlls if they're not needed.  Both functions are reasonably fast & cause less than a 500ms delay on my 5+ year old machine.

(In reply to comment #102)
> for OS/2, if there is a problem then I think an OS/2 maintainer should
> fix it, not you.

Not to worry, I provided Masayuki with a complete OS/2 implementation and will make tweaks as needed.
Posted patch Patch v12.0 (obsolete) — Splinter Review
Attachment #406857 - Attachment is obsolete: true
Attachment #406897 - Attachment is obsolete: true
Attachment #406920 - Attachment is obsolete: true
(In reply to comment #104)
> (In reply to comment #101)
> > I have a question. Do you think the async system sound play is needed on
> > OS/2? 
> 
> It has always been asynchronous.  The only difference is that now we're using
> nsRunnable instead of native code to create a playback thread.

Ah, I removed nsRunnable from your patch because Post() is removed from nsSystemSoundServiceBase. But I think OS/2 also doesn't need the another thread by your latter comment.
Attachment #408374 - Flags: superreview?(roc)
Attachment #408374 - Flags: review?(roc)
Comment on attachment 408374 [details] [diff] [review]
Patch v12.0

Only win32 creates another thread at every time.
Comment on attachment 408374 [details] [diff] [review]
Patch v12.0

Thanks!!!

Needs independent super-review of the interfaces.
Attachment #408374 - Flags: superreview?(roc)
Attachment #408374 - Flags: superreview?(Olli.Pettay)
Attachment #408374 - Flags: review?(roc)
Attachment #408374 - Flags: review+

Comment 109

10 years ago
Patch 12 will break the OS/2 build because it adds a call to nsWindow.cpp that no longer exists:  nsSystemSoundServiceBase::InitService().
Could you please remove the widget/src/os2/nsWindow.cpp portion of the patch before pushing it?

(In reply to comment #106)
> I think OS/2 also doesn't need the another thread by your latter comment.

It's required for application stability in case the audio subsystem malfunctions.  I'll restore it after this patch has landed.  (Actually, if I could get menu-event sounds working, I'd abandon all of the native code and would use your nsSoundPlayer instead.)
Posted patch Patch v12.1 (obsolete) — Splinter Review
(In reply to comment #109)
> Patch 12 will break the OS/2 build because it adds a call to nsWindow.cpp that
> no longer exists:  nsSystemSoundServiceBase::InitService().
> Could you please remove the widget/src/os2/nsWindow.cpp portion of the patch
> before pushing it?

done.

> (In reply to comment #106)
> > I think OS/2 also doesn't need the another thread by your latter comment.
> 
> It's required for application stability in case the audio subsystem
> malfunctions.  I'll restore it after this patch has landed.  (Actually, if I
> could get menu-event sounds working, I'd abandon all of the native code and
> would use your nsSoundPlayer instead.)

O.K. I restored the your patch. And I copied the Post method from Win32's.
Attachment #408374 - Attachment is obsolete: true
Attachment #408557 - Flags: superreview?(Olli.Pettay)
Attachment #408374 - Flags: superreview?(Olli.Pettay)
Comment on attachment 408374 [details] [diff] [review]
Patch v12.0

>+  nsresult rv;
>+  nsCOMPtr<nsIDOMHTMLMediaElement> audioElement =
>+    do_CreateInstance("@mozilla.org/content/element/html;1?name=audio", &rv);
This looks scary. You create an element which ownerDocument is whatever happens
to be in js context stack. The document can be also some content document, I think.
And what happends if the document is deleted before the audio is played back.
This API shouldn't depend on if some random document is deleted while playing back.

(I wish we could just remove supporting createInstance on audio/option/image elements.
 It is there only for JS 'new Audio/Option/Image')


>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(audioElement,
>+               "do_CreateInterface succeeded, but the result is null");
>+  audioElement->SetAutoplay(PR_TRUE);
>+  nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(audioElement));
>+  target->AddEventListener(NS_LITERAL_STRING("ended"), this, PR_TRUE);
>+  target->AddEventListener(NS_LITERAL_STRING("error"), this, PR_TRUE);
>+  NS_ADDREF(*aElement = audioElement);
>+  return NS_OK;
>+}
I think you expect to have an audio element which doesn't have ownerDocument at all,
but because of security reasons event handling wouldn't work on DOM nodes which don't have ownerDocument.

So, I think you need to create a document (using NS_NewXMLDocument or something like that), which you
keep alive and then create elements using document->CreateElementNS('xhtml namespace', 'audio'); 


Another thing, why is nsSystemSoundServiceBase::StopSoundPlayer() thread safe, but nsSystemSoundServiceBase::PlayFile etc aren't?

Could you document somewhere that nsISoundPlayer is for main thread only.
Attachment #408374 - Flags: superreview-
Hmm, I thought I asked someone about the document issue. Oh well, thanks for catching it.

Maybe we could use the document of the hidden window?
(In reply to comment #111)
> Another thing, why is nsSystemSoundServiceBase::StopSoundPlayer() thread safe,
> but nsSystemSoundServiceBase::PlayFile etc aren't?
> 
> Could you document somewhere that nsISoundPlayer is for main thread only.

Good point. I think nsISoundPlayer might be used by XUL applications. And it's singleton. So, probably, nsSoundPlayer should be thread safe.

(In reply to comment #112)
> Maybe we could use the document of the hidden window?

I'm not sure the hidden window life time. I think that the creating a independent document is safer.
Yeah, don't use hidden window. That won't work in Electrolysis, I think.
(In reply to comment #114)
> Yeah, don't use hidden window. That won't work in Electrolysis, I think.
Or, maybe it will, but only on chrome.

But anyway, better to use a separate document, I think.
Um, I created the document and created the audio elements from it. Then, the document cannot access to the local resource (e.g., file:///...).
Posted patch Patch v12.2 (obsolete) — Splinter Review
ok, this works fine for me.
Attachment #408557 - Attachment is obsolete: true
Attachment #408602 - Flags: superreview?(Olli.Pettay)
Attachment #408557 - Flags: superreview?(Olli.Pettay)
Posted patch Patch v12.2.1 (obsolete) — Splinter Review
removed an unused variable.
Attachment #408602 - Attachment is obsolete: true
Attachment #408604 - Flags: superreview?(Olli.Pettay)
Attachment #408602 - Flags: superreview?(Olli.Pettay)
Comment on attachment 408604 [details] [diff] [review]
Patch v12.2.1

>+nsresult
>+nsSoundPlayer::CreateAudioElement(nsIDOMHTMLMediaElement **aElement)
>+{
>+  NS_ASSERTION(NS_IsMainThread(),
>+    "nsSoundPlayer::CreateAudioElement is called on non-main thread");
>+  nsresult rv;
>+  if (!mDocument) {
>+    nsCOMPtr<nsIDocument> document;
>+    rv = NS_NewXMLDocument(getter_AddRefs(document));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    NS_ASSERTION(document,
>+      "NS_NewXMLDocument succeeded, but the result is null");
>+
>+    // Give the chrome permission to the document.
>+    nsCOMPtr<nsIScriptSecurityManager> securityManager =
>+      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIPrincipal> systemPrincipal;
>+    rv = securityManager->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    document->SetPrincipal(systemPrincipal);
>+
>+    mDocument = do_QueryInterface(document);
>+    NS_ASSERTION(mDocument,
>+      "The XML document doesn't have nsIDOMDocument interface");
...doesn't implement nsIDOMDocument interface.

>+  }
>+  nsCOMPtr<nsIDOMElement> audioElement;
>+  rv = mDocument->CreateElementNS(
>+         NS_LITERAL_STRING("http://www.w3.org/1999/xhtml"),
>+         NS_LITERAL_STRING("audio"), getter_AddRefs(audioElement));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(audioElement,
>+    "CreateElementNS succeeded, but the result is null");
>+  nsCOMPtr<nsIDOMHTMLMediaElement> mediaElement =
>+    do_QueryInterface(audioElement);
>+  NS_ASSERTION(mediaElement,
>+    "The audio element doesn't have nsIDOMHTMLMediaElement interface");
>+  mediaElement->SetAutoplay(PR_TRUE);
Useless assertion, since if mediaElement is null, we'll crash right after the assertion.

>+NS_IMETHODIMP
>+nsSoundPlayer::HandleEvent(nsIDOMEvent *aEvent)
HandleEvent isn't an nsISoundPlayer method, and is always run
in main thread, so no need for thread safety handling.

>+
>+  const kDelay = 500;
I assume and hope the timeouts won't cause any problems.

>+/**
>+ * WARNING:  nsISound is now OBSOLETE.
Is there any reason why we can't remove nsISound?
(In reply to comment #119)
> >+
> >+  const kDelay = 500;
> I assume and hope the timeouts won't cause any problems.

The automated tests test:
1. they aren't stopped by any errors.
2. they aren't crashed.

So, we need to wait start of the async play (Win32's nsSystemSoundPlayer and nsSoundPlayer).

> >+/**
> >+ * WARNING:  nsISound is now OBSOLETE.
> Is there any reason why we can't remove nsISound?

nsISound might be used by some XUL apps (including Thunderbird and SeaMonkey) and the add-ons. We should support it until they become really obsolete. When 1.9.3 goes to the maintenance cycle, we can/should drop it from trunk because add-ons can use the new interfaces on all maintained Gecko at that time.
Ok. Let's drop nsISound once Thunderbird and Seamonkey are updated.
Please file followup bugs for those.

I'd hope we could drop nsISound asap. It isn't a frozen interface or anything,
so I don't see any problems dropping it already in 1.9.3.

Updated

10 years ago
Attachment #408604 - Flags: superreview?(Olli.Pettay) → superreview+
Comment on attachment 408604 [details] [diff] [review]
Patch v12.2.1

So, fix the comments I had, sr=me.
Thanks, but I have a problem. 12.2 and later has leak bug on XP level. Maybe, it's in the nsSoundPlayer.

> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 8303 bytes during test execution
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 4 instances of AtomImpl with size 16 bytes each (64 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Connection with size 88 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Service with size 24 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of Statement with size 48 bytes each (144 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of StringAdopt with size 1 bytes each (3 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsBaseURLParser with size 8 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 34 instances of nsCStringKey with size 16 bytes each (544 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsCacheEntryDescriptor with size 24 bytes each (72 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsCacheEntryHashTable with size 36 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsCacheService with size 124 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsCategoryObserver with size 56 bytes each (112 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsCookiePermission with size 32 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsCookieService with size 152 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsDNSService with size 44 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsEffectiveTLDService with size 44 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsHTMLMediaElement::MediaLoadListener with size 24 bytes each (72 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 34 instances of nsHashKey with size 4 bytes each (136 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsHashPropertyBag with size 44 bytes each (132 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsHashtable with size 44 bytes each (88 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsHttpChannel with size 456 bytes each (1368 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsHttpConnectionInfo with size 40 bytes each (120 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsHttpHandler with size 356 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsIDNService with size 60 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsIOService with size 120 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsInputStreamPump with size 80 bytes each (240 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsLocalFile with size 120 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsMediaChannelStream::Listener with size 20 bytes each (60 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsObserverService with size 44 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsPermissionManager with size 88 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsPipe with size 148 bytes each (444 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsPrefBranch with size 52 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsRecyclingAllocatorImpl with size 36 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsSocketTransportService with size 1680 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsStandardURL with size 180 bytes each (540 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsStreamConverterService with size 12 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 75 instances of nsStringBuffer with size 8 bytes each (600 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 13 instances of nsTArray_base with size 4 bytes each (52 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsThread with size 68 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsTimerImpl with size 48 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsUnicodeNormalizer with size 8 bytes
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsVariant with size 44 bytes each (132 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 19 instances of nsVoidArray with size 4 bytes each (76 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 5 instances of nsWeakReference with size 12 bytes each (60 bytes total)
Posted patch Patch v12.3 (obsolete) — Splinter Review
latest patch in my tree.
Attachment #408604 - Attachment is obsolete: true
I might need to call nsIDocument::Destroy()??
Posted patch Patch v12.4 (obsolete) — Splinter Review
Ugh, this still has the leak...
Attachment #408829 - Attachment is obsolete: true
Posted patch Patch v12.5 (obsolete) — Splinter Review
Testing...
Attachment #408871 - Attachment is obsolete: true
I give up the current approach. Creating a document isn't easy I think. The current patch cannot play via network. If the given URI is http://..., the play method fails to load it. I tried to initialize and finalize the document, but I cannot find the correct way. So, I think we should chose another approach...
Posted patch Patch v13.0 (obsolete) — Splinter Review
I hope that the hidden window's document approach has no problem.
# It works fine on my environment, I'll post this to try server.
Attachment #408879 - Attachment is obsolete: true
I doubt hidden window approach works properly when someone embeds gecko.
Oh. Um... I have no idea how to fix the leak and network connection issues...
Another possible approach would be to add a way to create audio elements with a null ownerDocument. In that case they would not dispatch events; instead we'd have a C++-only interface that lets a C++ user be notified when the element is done. Obviously this would require some changes to nsHTMLMediaElement, but I think that would be OK. Olli, what do you think of that?

Another possible approach would be to move the nsISoundPlayer implementation into content/media and instead of creating an element, create an nsMediaDecoder directly (like the element would). This would be like the previous idea except that instead of hacking some alternate code paths into nsHTMLMediaElement, we duplicate some of its code instead.

I'm not sure which alternative would be best. My gut feeling is that allowing nsHTMLMediaElement to have a null ownerdocument would be less work and less maintenance burden as we evolve media code, but it may well be unpalatable from the content point of view.
(In reply to comment #132)
> Another possible approach would be to add a way to create audio elements with a
> null ownerDocument.
Well, it is possible already now, if you create artificial nodeinfomanager.
But that would be very very hacky. And we're aiming to change DOM so that
GetOwnerDoc() never returns null (except perhaps during document tear down).

I wonder why loading doesn't work with the document approach.
Is the document missing a valid loadgroup or something?

> Another possible approach would be to move the nsISoundPlayer implementation
> into content/media and instead of creating an element, create an nsMediaDecoder
> directly (like the element would). This would be like the previous idea except
> that instead of hacking some alternate code paths into nsHTMLMediaElement, we
> duplicate some of its code instead.
This sounds better.
Posted patch Patch v12.6 (obsolete) — Splinter Review
This is my final patch of the document creation.

With this patch, I can see following assertions during sound.xul.

> WARNING: cannot post event if not initialized: file c:/mozilla-a/src/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp, line 167
> WARNING: cannot post event if not initialized: file c:/mozilla-a/src/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp, line 167
> WARNING: NS_ENSURE_TRUE(docEvent && target) failed: file c:/mozilla-a/src/content/html/content/src/nsHTMLMediaElement.cpp, line 1752
> WARNING: cannot post event if not initialized: file c:/mozilla-a/src/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp, line 167
> WARNING: NS_ENSURE_TRUE(docEvent && target) failed: file c:/mozilla-a/src/content/html/content/src/nsHTMLMediaElement.cpp, line 1752
> WARNING: cannot post event if not initialized: file c:/mozilla-a/src/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp, line 167
> WARNING: NS_ENSURE_TRUE(mDB) failed: file c:/mozilla-a/src/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp, line 1192
> WARNING: nsExceptionService ignoring thread destruction after shutdown: file c:/mozilla-a/src/xpcom/base/nsExceptionService.cpp, line 194
> nsXPConnect::CommenceShutdown()
> WARNING: NS_ENSURE_TRUE(factory) failed: file c:/mozilla-a/src/docshell/base/nsDocShell.cpp, line 9879
Attachment #409369 - Attachment is obsolete: true
Posted patch Patch v14.0 (obsolete) — Splinter Review
Attachment #409489 - Attachment is obsolete: true
Posted patch Patch v14.1.1 (obsolete) — Splinter Review
Attachment #410465 - Attachment is obsolete: true
Posted patch Patch v15.0 (obsolete) — Splinter Review
Attachment #410507 - Attachment is obsolete: true
Comment on attachment 410744 [details] [diff] [review]
Patch v15.0

O.K. This works fine.

nsISound will be removed by the smaug's suggestion. So, I changed nsSound.* are changed to nsBaseSystemSoundService.*. And the nsSystemSoundServiceBase class is now renamed to nsBaseSystemSoundService because other base classes in xpwidgets are named as nsBase*.

nsSoundPlayer creates nsSoundPlayerItem instead of <audio> element. nsSoundPlayerItem and nsHTMLMediaElement has common interface nsIMediaPlayerInternal. nsMediaDecoder, nsMediaStream and nsMediaStreamListener access them via the interface.

Some nsHTMLMediaElement's code is separated to nsMediaUtils and nsMediaStreamListener.
Attachment #410744 - Flags: review?(roc)
Ugh... The media element related code was changed again...
Posted patch Patch v15.0.1Splinter Review
updated for the latest trunk.
Attachment #410744 - Attachment is obsolete: true
Attachment #410795 - Flags: review?(roc)
Attachment #410744 - Flags: review?(roc)
nsIMediaPlayerInternal is a bit ugly since most of its methods are only implemented on nsHTMLMediaElement. I think it might be simpler and less intrusive if the decoder had a reference to an nsHTMLMediaElement* and an nsIMediaPlayerInternal* and just did null-checking on its media element pointer.

Does nsIMediaPlayerInternal really need to support nsISupports?

I wonder if the functionality of InitializeDecoderForChannel etc would be best implemented in its own object that we store as a field of nsHTMLMediaElement ans nsSoundPlayer. nsMediaDecoderHost or something like that.

I need to look at this patch more but I don't have time right now. Also it's going to conflict with ongoing Ogg work so we probably should pause work in this bug until that's done.
> I wonder if the functionality of InitializeDecoderForChannel etc would be best
> implemented in its own object that we store as a field of nsHTMLMediaElement
> ans nsSoundPlayer. nsMediaDecoderHost or something like that.

Umm... I cannot imagine the class. Does it own mChannel and mDecoder? If so, I don't understand what is the merit.

Maybe, we should create nsMediaPlayer class because any other ideas are not strict way. They are not smart approach (including my patch!).

The nsMediaPlayer's code is copied from most part of nsHTMLMediaPlayer (This guarantees low risk). nsMediaDecoder and nsMediaStream should access only this class. And it notifies some events to nsHTMLMediaElement and nsSoundPlayer( or nsSoundPlayerItem). This might give us the best code...

> I need to look at this patch more but I don't have time right now. Also it's
> going to conflict with ongoing Ogg work so we probably should pause work in
> this bug until that's done.

O.K., I want to know the bug#.
> > I wonder if the functionality of InitializeDecoderForChannel etc would be best
> > implemented in its own object that we store as a field of nsHTMLMediaElement
> > ans nsSoundPlayer. nsMediaDecoderHost or something like that.
> 
> Umm... I cannot imagine the class. Does it own mChannel and mDecoder?

Yes, that's what I was thinking. This might be useful for implementing 'loop'.

> > I need to look at this patch more but I don't have time right now. Also it's
> > going to conflict with ongoing Ogg work so we probably should pause work in
> > this bug until that's done.
> 
> O.K., I want to know the bug#.

I'm not sure if there's a bug for it yet.
Comment on attachment 410795 [details] [diff] [review]
Patch v15.0.1

I'm not sure how we're going to do this. To handle multiple decoders (e.g. with seamless looping) we may need to introduce some kind of "decoder host" object that the decoder talks to instead of talking directly to the element; that would make this work easier.
Attachment #410795 - Flags: review?(roc)
Summary: Redesign nsSound structure → Redesign nsSound structure to put threading in xpwidget
Depends on: 683184

Updated

7 years ago
Blocks: 676258

Updated

2 years ago
Priority: -- → P3
Whiteboard: [See comment 3] for the detail → [See comment 3] for the detail tpi:+
xpwidgets/nsSound has gone and each nsSound is simply created with nsISound interface now.  Additionally, nsSound for Windows has already played sound in player thread, and this bug is too old.

So, I close this bug. If somebody still thinks that we should make playing sound in XP level, I think you should work on new bug isntead of reusing this bug since there are a lot of outdated comments here.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago9 months ago
Keywords: dev-doc-needed
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.