nsSoundPlayer::SoundReleaser::Run should not add to existing jank

RESOLVED FIXED in Firefox 56

Status

()

P2
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: florian, Assigned: tnguyen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1][tpi:+])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
In this profile https://perf-html.io/public/f154d24c598d4cd31a98f5f02d43b06e2d123907/calltree/?callTreeFilters=prefix-01234567yqGF2Fzd&range=422.0384_444.8391~436.9647_437.3012&thread=0 nsSoundPlayer::SoundReleaser::Run() is taking time on the main thread after some existing jank.

This is started using mThread->Dispatch(releaser, NS_DISPATCH_NORMAL); at http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/widget/windows/nsSound.cpp#87

I think it should use idle dispatch instead.

Comment 1

2 years ago
Masayuki, do I understand correctly that this code runs as part of the cleanup after every time the playEventSound() API <https://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/widget/nsISound.idl#46> is called?

That's pretty bad since they're all code that runs during UI interactions...
https://searchfox.org/mozilla-central/search?q=playeventsound&case=false&regexp=false&path=

Updated

2 years ago
Flags: needinfo?(masayuki)
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #1)
> Masayuki, do I understand correctly that this code runs as part of the
> cleanup after every time the playEventSound() API
> <https://searchfox.org/mozilla-central/rev/
> 7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/widget/nsISound.idl#46> is called?

Yeah, looks like so. (and also nsISound.playSystemSound().)

> That's pretty bad since they're all code that runs during UI interactions...
> https://searchfox.org/mozilla-central/
> search?q=playeventsound&case=false&regexp=false&path=

When I work on around nsSound, that ran in the main thread synchronously (although, ::PlaySound() is called with async flag). However, now, it runs in non-main tread but the owner thread is still the main thread.

I was thinking that nsSound should be a singleton class to reduce the creation cost and prevent memory fragmentation, and owns playing sound thread, then, nsSound just post a sound to be played to the thread. Then, we can also reduce the clean up cost in the main thread.
Flags: needinfo?(masayuki)

Comment 3

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #2)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #1)
> > Masayuki, do I understand correctly that this code runs as part of the
> > cleanup after every time the playEventSound() API
> > <https://searchfox.org/mozilla-central/rev/
> > 7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/widget/nsISound.idl#46> is called?
> 
> Yeah, looks like so. (and also nsISound.playSystemSound().)

That method is unused (and should probably just be removed!)

> > That's pretty bad since they're all code that runs during UI interactions...
> > https://searchfox.org/mozilla-central/
> > search?q=playeventsound&case=false&regexp=false&path=
> 
> When I work on around nsSound, that ran in the main thread synchronously
> (although, ::PlaySound() is called with async flag). However, now, it runs
> in non-main tread but the owner thread is still the main thread.
> 
> I was thinking that nsSound should be a singleton class to reduce the
> creation cost and prevent memory fragmentation, and owns playing sound
> thread, then, nsSound just post a sound to be played to the thread. Then, we
> can also reduce the clean up cost in the main thread.

Yeah I think doing that makes sense.

Updated

2 years ago
Priority: -- → P2
Whiteboard: [qf] → [qf][tpi+]

Updated

2 years ago
Whiteboard: [qf][tpi+] → [qf][tpi:+]
Whiteboard: [qf][tpi:+] → [qf:p1][tpi:+]

Updated

2 years ago
Blocks: 1364015
(Assignee)

Updated

2 years ago
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Comment hidden (obsolete)
(Assignee)

Comment 5

2 years ago
MozReview-Commit-ID: AIM90VXFT54
(Assignee)

Updated

2 years ago
Attachment #8872856 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Comment on attachment 8872857 [details] [diff] [review]
Keep only one thread to play sound

Hi Masayuki,
Could you please take a look at the patch, presumably we only have to shutdown the player thread when close ff (ClearOnShutdown) to avoid the jank?
Since we used SND_ASYNC to play sound, I am not sure if we could run 2 sounds simultaneously in player thread, do we have to stop the last sound? 
I removed the playSystemSound which seems obsoleted and has not been used anywhere
Attachment #8872857 - Flags: feedback?(masayuki)
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #6)
> Since we used SND_ASYNC to play sound, I am not sure if we could run 2
> sounds simultaneously in player thread, do we have to stop the last sound? 

I think that even if we use SND_ASYNC, we should use different thread to call PlaySound API since:

* Windows doesn't guarantee that it does nothing in the calling thread except playing sound.
* The API may be hooked by something other process, like sound card's utility.  In such case, if we call it from the main thread, it might be blocked by such applications.

BTW, the answer for your question here is, we shouldn't change behavior when we optimize the performance. So, I think that we should keep current behavior unless it's impossible. (Personally, I think that we shouldn't stop previous sound, but not sure.)

> I removed the playSystemSound which seems obsoleted and has not been used
> anywhere

No, a lot of addons still use it (I'm not sure if they are actually using, but it's in common utilities. Sorry, my comment 2 was just looked in our code. So, I think that we should remove it when we ship 57.
(Assignee)

Updated

2 years ago
Attachment #8872857 - Attachment is obsolete: true
Attachment #8872857 - Flags: feedback?(masayuki)
(Assignee)

Updated

2 years ago
Attachment #8872856 - Flags: feedback?(masayuki)
(Assignee)

Comment 8

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #7)
> (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #6)
> > Since we used SND_ASYNC to play sound, I am not sure if we could run 2
> > sounds simultaneously in player thread, do we have to stop the last sound? 
> 
> I think that even if we use SND_ASYNC, we should use different thread to
> call PlaySound API since:
> 
> * Windows doesn't guarantee that it does nothing in the calling thread
> except playing sound.
> * The API may be hooked by something other process, like sound card's
> utility.  In such case, if we call it from the main thread, it might be
> blocked by such applications.
> 
> BTW, the answer for your question here is, we shouldn't change behavior when
> we optimize the performance. So, I think that we should keep current
> behavior unless it's impossible. (Personally, I think that we shouldn't stop
> previous sound, but not sure.)

> > I removed the playSystemSound which seems obsoleted and has not been used
> > anywhere
> 
> No, a lot of addons still use it (I'm not sure if they are actually using,
> but it's in common utilities. Sorry, my comment 2 was just looked in our
> code. So, I think that we should remove it when we ship 57.

Ah, I see, :). Thanks you
(Assignee)

Comment 9

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #7)
> (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #6)
> > Since we used SND_ASYNC to play sound, I am not sure if we could run 2
> > sounds simultaneously in player thread, do we have to stop the last sound? 
> 
> I think that even if we use SND_ASYNC, we should use different thread to
> call PlaySound API since:
> 
> * Windows doesn't guarantee that it does nothing in the calling thread
> except playing sound.
> * The API may be hooked by something other process, like sound card's
> utility.  In such case, if we call it from the main thread, it might be
> blocked by such applications.
> 
> BTW, the answer for your question here is, we shouldn't change behavior when
> we optimize the performance. So, I think that we should keep current
> behavior unless it's impossible. (Personally, I think that we shouldn't stop
> previous sound, but not sure.)
So the idea is we will keep the player thread remaining until close FF (shutdown it from main thread will create a jank). nsSound instance just have to post a sound to be played to the thread.
Comment on attachment 8872857 [details] [diff] [review]
Keep only one thread to play sound

>diff --git a/widget/nsISound.idl b/widget/nsISound.idl
> [scriptable, uuid(C3C28D92-A17F-43DF-976D-4EEAE6F995FC)]
> interface nsISound : nsISupports
> {
>   void play(in nsIURL aURL);
>-  /**
>-   * for playing system sounds
>-   *
>-   * NS_SYSSOUND_* params are obsolete. The new events will not be supported by
>-   * this method.  You should use playEventSound method instaed.
>-   */
>-  void playSystemSound(in AString soundAlias);  

So, let's wait to do this until 57.

>diff --git a/widget/windows/nsSound.cpp b/widget/windows/nsSound.cpp
> class nsSoundPlayer: public mozilla::Runnable {
> public:
>-  nsSoundPlayer(nsSound *aSound, const wchar_t* aSoundName) :
>-    mSoundName(aSoundName), mSound(aSound)
>-  {
>-    Init();
>-  }
>+  nsSoundPlayer(const wchar_t* aSoundName) :
>+    mSoundName(aSoundName) {}

nit: put ":" to the next line, so, this should be

nsSoundPlayer(...)
  : mSoundName(aSoundName)
{
}

>-  nsSoundPlayer(nsSound *aSound, const nsAString& aSoundName) :
>-    mSoundName(aSoundName), mSound(aSound)
>-  {
>-    Init();
>-  }
>+  nsSoundPlayer(const nsAString& aSoundName) :
>+    mSoundName(aSoundName) {}

same.

>-nsSound::nsSound()
>+nsSound::nsSound() : mLastSound(nullptr)
>+                   , mPlayerThread(nullptr)

nit: please use this style:

nsSound::nsSound()
 : mLastSound(nullptr)
 , mPlayerThread(nullptr)

And you don't need to initialize mPlayerThread here because it's nsCOMPtr, it initializes its raw pointer with nullptr at constructor.

> nsSound::~nsSound()
> {
>-  NS_ASSERTION(!mPlayerThread, "player thread is not null but should be");
>   PurgeLastSound();

Is it possible to do this in the other thread? Then, PlaySound() API won't block our main thread during shutting down.

>-void nsSound::ShutdownOldPlayerThread()
>-{
>+  NS_ASSERTION(mPlayerThread, "player thread is not null");
>   nsCOMPtr<nsIThread> playerThread(mPlayerThread.forget());
>-  if (playerThread)
>+  if (playerThread) {
>     playerThread->Shutdown();
>+  }

Cannot we reuse the thread for another sound?

> NS_IMETHODIMP nsSound::Init()
> {
>+  if (!mPlayerThread) {
>+    nsresult rv = NS_NewNamedThread("PlayEventSound", getter_AddRefs(mPlayerThread));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>   // This call halts a sound if it was still playing.
>   // We have to use the sound library for something to make sure
>   // it is initialized.
>   // If we wait until the first sound is played, there will
>   // be a time lag as the library gets loaded.
>   ::PlaySound(nullptr, nullptr, SND_PURGE);

I think that this should be done in mPlayerThread.

> NS_IMETHODIMP nsSound::PlayEventSound(uint32_t aEventId)
> {
>-  ShutdownOldPlayerThread();
>   PurgeLastSound();

I think that we should stop previous sound immediately before playing new sound in the other thread.

>+  NS_ASSERTION(mPlayerThread, "player thread should not be null ");

Use MOZ_ASSERT() and do nothing if mPlayerThread is nullptr (called during shutting down?).

>   nsCOMPtr<nsIRunnable> player = new nsSoundPlayer(this, sound);
>   NS_ENSURE_TRUE(player, NS_ERROR_OUT_OF_MEMORY);

How much the cost of creating nsSoundPlayer? If it's expensive, nsSound should cache the instance(s).


So, my current ideas are:

* nsSound creates a thread once and doesn't destroy it until shutting down.
* Any calls of PlaySound() should be performed in the player thread (including pursing the last sound play).
* nsSound should have at least one nsSoundPlayer instance. And nsSoundPlayer should have |bool mStopped;| or something. When the cached instance's mStopped is true, it should be reused.
* If keeping other thread is expensive, we should clear it after any sound is played for a while.

As far as I know, the possible and realistic case to play a lot of sounds is, setting menu popup sound in system settings and move mouse cursor in menu bar or bookmark bar to open/close next popup menu or folder. You can profile actual cost with this operation.

I just guess the cost and assume the worst case of PlaySound() API. So, my ideas are not decision, let me know if you don't agree with my ideas.
Attachment #8872857 - Attachment is obsolete: false
(Assignee)

Comment 11

2 years ago
> How much the cost of creating nsSoundPlayer? If it's expensive, nsSound
> should cache the instance(s).
> 
> 
> So, my current ideas are:
> 
> * nsSound creates a thread once and doesn't destroy it until shutting down.
> * Any calls of PlaySound() should be performed in the player thread
> (including pursing the last sound play).
> * nsSound should have at least one nsSoundPlayer instance. And nsSoundPlayer
> should have |bool mStopped;| or something. When the cached instance's
> mStopped is true, it should be reused.
> * If keeping other thread is expensive, we should clear it after any sound
> is played for a while.
> 
> As far as I know, the possible and realistic case to play a lot of sounds
> is, setting menu popup sound in system settings and move mouse cursor in
> menu bar or bookmark bar to open/close next popup menu or folder. You can
> profile actual cost with this operation.
> 
> I just guess the cost and assume the worst case of PlaySound() API. So, my
> ideas are not decision, let me know if you don't agree with my ideas.

Thanks Masayuki, that's great feedback.
You are right, I should move all others Playsound to player thread too (fwiw, there're some info https://blogs.msdn.microsoft.com/larryosterman/2007/05/15/blocking-your-ui-thread-with-playsound/)
It said, even with the SND_ASYNC flag, Playsound does a fair amount of work before dispatching the call to a worker thread.  Unfortunately, some of these works can take many milliseconds (especially if this is the first call to winmm.dll).
I don't think keeping a player thread is too expensive in this case, I prefer your idea which creates player thread once and doesn't destroy it until shutting down.
I will update the patch based on that
(Assignee)

Comment 12

2 years ago
MozReview-Commit-ID: 9FOBgqL4ung
(Assignee)

Updated

2 years ago
Attachment #8872857 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
MozReview-Commit-ID: 9FOBgqL4ung
(Assignee)

Comment 14

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #10)
> 
> So, my current ideas are:
> 
> * nsSound creates a thread once and doesn't destroy it until shutting down.
> * Any calls of PlaySound() should be performed in the player thread
> (including pursing the last sound play).
> * nsSound should have at least one nsSoundPlayer instance. And nsSoundPlayer
> should have |bool mStopped;| or something. When the cached instance's
> mStopped is true, it should be reused.
I probably will skip the idea of keeping the nsSoundPlayer instance in cache. Indeed, we still have to maintain a set of player runnables, while obviously there's a nsIRunnable queue in nsIThread could handle that. (moreover, dispatch will do move and take owner of nsIRunnable). In fact, creating nsSoundPlayer is not too expensive to do that.

If we move all Playsound to a seperated thread then we could get a win for the first time init/playing sound (it may block main thread, if this is the first call to sound library).
http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp#1035
But we have to init nsSound earlier to avoid any lag when playing the next playing sound.
In the patch, I put the instance() of nsSound to the init() of nsTypeAheadFind but seems it's not good enough. We probably have to put in more common place.  Do you have any idea about how we could put the instance() of nsSound?
Flags: needinfo?(masayuki)
(Assignee)

Updated

2 years ago
Attachment #8873346 - Attachment is obsolete: true
Comment on attachment 8873365 [details] [diff] [review]
Playing sound in a seperated thread to avoid jank

> class nsSoundPlayer: public mozilla::Runnable {
> public:
>+  nsSoundPlayer(const wchar_t* aSoundName,
>+                const uint8_t* aData,
>+                DWORD aFlags)
>+    : mSoundName(aSoundName)
>+    , mData(aData)
>+    , mFlags(aFlags) {}
> 
>+  nsSoundPlayer(const nsAString& aSoundName,
>+                const uint8_t* aData,
>+                DWORD aFlags)
>+    : mSoundName(aSoundName)
>+    , mData(aData)
>+    , mFlags(aFlags) {}
> 
>   NS_DECL_NSIRUNNABLE
> 
> protected:
>   nsString mSoundName;
>+  DWORD mFlags;
>+  const uint8_t* mData;

nit: DWORD is always 4 bytes. So, it should be under mData. Then, when you add a bool or int(8|16|32)_t member at the end, the instance size won't be increased.

> NS_IMETHODIMP
> nsSoundPlayer::Run()
> {
>+  NS_PRECONDITION(!mSoundName.IsEmpty() || mData,
>+    "Sound name or sound data should be specified");

If this shouldn't have actually, please use MOZ_ASSERT and detect the bug with automated tests.

> nsSound::~nsSound()
> {
>+  NS_ASSERTION(mPlayerThread, "player thread is not null");

Same here, use MOZ_ASSERT() if it shouldn't occur.

>+void nsSound::PurgeLastSound()
> {
>   if (mLastSound) {
>     // Halt any currently playing sound.
>+    if (mPlayerThread) {
>+      mPlayerThread->Dispatch(NS_NewRunnableFunction([]() {
>+        ::PlaySound(nullptr, nullptr, SND_PURGE);
>+      }), NS_DISPATCH_NORMAL);
>+    }

Hmm, if we could remove or stop the last player actually, it's the best, but I'm not so familiar with multi thread. So, I have no idea and it should be okay.

>+    nsCOMPtr<nsIRunnable> player = new nsSoundPlayer(EmptyString(), data, flags);
>+    NS_ENSURE_TRUE(player, NS_ERROR_OUT_OF_MEMORY);

Could you use NS_WARN_IF? I.e.,

if (NS_WARN_IF(!player)) {
  return NS_ERROR_OUT_OF_MEMORY;
}

>+    nsresult rv = mPlayerThread->Dispatch(player, NS_DISPATCH_NORMAL);
>+    NS_ENSURE_SUCCESS(rv, rv);

And same here,

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

> NS_IMETHODIMP nsSound::Init()
> {
>+  if (!mPlayerThread) {
>+    nsresult rv = NS_NewNamedThread("PlayEventSound", getter_AddRefs(mPlayerThread));

nit: too long line, perhaps,

    nsresult rv =
      NS_NewNamedThread("PlayEventSound", getter_AddRefs(mPlayerThread));

>+    NS_ENSURE_SUCCESS(rv, rv);

And use NS_WARN_IF here too.

> NS_IMETHODIMP nsSound::PlaySystemSound(const nsAString &aSoundAlias)
> {
>+  MOZ_ASSERT(mPlayerThread, "player thread should not be null ");
>   PurgeLastSound();
> 
>   if (!NS_IsMozAliasSound(aSoundAlias)) {
>     if (aSoundAlias.IsEmpty())
>       return NS_OK;
>-    nsCOMPtr<nsIRunnable> player = new nsSoundPlayer(this, aSoundAlias);
>+    nsCOMPtr<nsIRunnable> player =
>+      new nsSoundPlayer(aSoundAlias, nullptr, SND_NODEFAULT | SND_ALIAS | SND_ASYNC);

Too long, perhaps,

    nsCOMPtr<nsIRunnable> player =
      new nsSoundPlayer(aSoundAlias, nullptr,
                        SND_NODEFAULT | SND_ALIAS | SND_ASYNC);


Otherwise, looks fine to me. (But if you'd like me to review this, I hope you use MozReview. It's easier to check around each chunk.)
Flags: needinfo?(masayuki)
Attachment #8873365 - Flags: feedback+
(Assignee)

Comment 16

2 years ago
> Otherwise, looks fine to me. (But if you'd like me to review this, I hope
> you use MozReview. It's easier to check around each chunk.)

Oops, sure, thanks for your feedback.
Attachment #8872856 - Flags: feedback?(masayuki)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8876072 [details]
Bug 1363163 - Playing sound in a seperated thread to avoid jank

https://reviewboard.mozilla.org/r/147486/#review151764

https://perfht.ml/2rcGmCJ
https://perfht.ml/2rcNvm9
(Assignee)

Comment 19

2 years ago
Hi Masayuki,
Could you please take a look at the patch?
I did some profiling tests where we could play sound (in nsMenuFrame, nsMenuPopupFrame and fast find) and it looks pretty good.
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8876072 [details]
Bug 1363163 - Playing sound in a seperated thread to avoid jank

https://reviewboard.mozilla.org/r/147486/#review152008

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:130
(Diff revision 2)
> +  if (!mIsSoundInitialized && !mNotFoundSoundURL.IsEmpty()) {
> +    // This makes sure system sound library is loaded so that
> +    // there's no lag before the first sound is played
> +    // by waiting for the first keystroke, we still get the startup time benefits.
> +    mIsSoundInitialized = true;
> +    mSoundInterface = do_CreateInstance("@mozilla.org/sound;1");

Oh, this looks odd *only* on Windows because it works as "service" now. However, on the other platforms, nsSound may be not singleton. So, this is fine...

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:131
(Diff revision 2)
> +    if (mSoundInterface && !mNotFoundSoundURL.EqualsLiteral("beep")) {
> +      mSoundInterface->Init();

Even if mNotFoundSoundURL is "beep" here, but when actually it's played, the value may be changed. However, we don't need to care such edge case.

::: widget/windows/nsSound.cpp:83
(Diff revision 2)
> -  NS_IF_RELEASE(mSound);
> -  return NS_OK;
> +    RefPtr<nsSound> sound = new nsSound();
> +    nsresult rv = sound->CreatePlayerThread();
> +    if(NS_WARN_IF(NS_FAILED(rv))) {
> +      return nullptr;
> +    }
> +    sInstance = sound;

nit: sound.forget()

::: widget/windows/nsSound.cpp:87
(Diff revision 2)
> +    }
> +    sInstance = sound;
> +    ClearOnShutdown(&sInstance, ShutdownPhase::ShutdownThreads);
> -}
> +  }
>  
> +  RefPtr<nsISound> service = sInstance.get();

nit: Do we need |.get()| here actually?

::: widget/windows/nsSound.cpp:121
(Diff revision 2)
>  {
> -  if (mLastSound) {
> -    // Halt any currently playing sound.
> +  // Halt any currently playing sound.
> +  if (mPlayerThread) {
> +    mPlayerThread->Dispatch(NS_NewRunnableFunction([]() {
> -    ::PlaySound(nullptr, nullptr, SND_PURGE);
> +      ::PlaySound(nullptr, nullptr, SND_PURGE);

Don't we need to specify SND_ASYNC too? (Although I don't know if it's actually useful.)

::: widget/windows/nsSound.cpp:175
(Diff revision 2)
>      mLastSound = (uint8_t *) malloc(dataLen);
>      if (mLastSound) {
>        memcpy(mLastSound, data, dataLen);
>        data = mLastSound;
>        flags |= SND_ASYNC;
>      }
> -    ::PlaySoundW(reinterpret_cast<LPCWSTR>(data), 0, flags);
> +    nsCOMPtr<nsIRunnable> player = new nsSoundPlayer(EmptyString(), data, flags);

Is this really safe?

data is mLastSound. mLastSound is managed by nsSound in the main thread (freed in nsSound::PurgeLastSound()) and nsSoundPlayer doesn't copy this.

I think that mLastSound should be wrapped with refcountable threadsafe class. Then, both nsSound and nsSoundPlayer should grab it.

::: widget/windows/nsSound.cpp:181
(Diff revision 2)
>      if (mLastSound) {
>        memcpy(mLastSound, data, dataLen);
>        data = mLastSound;
>        flags |= SND_ASYNC;
>      }
> -    ::PlaySoundW(reinterpret_cast<LPCWSTR>(data), 0, flags);
> +    nsCOMPtr<nsIRunnable> player = new nsSoundPlayer(EmptyString(), data, flags);

Too long. Break after "=" and start next line under "C" of "nsCOMPtr".

::: widget/windows/nsSound.cpp:182
(Diff revision 2)
> +    if (NS_WARN_IF(!player)) {
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    }

MOZ_ASSERT is enough because this allocation shouldn't have failed if it's not crashed yet. I mean that if |new nsSoundPlayer| is failed due to OOM, the allocator should make it crash.

(bsmedberg said, "Please *do* remove error checking around `new Foo` unless it is explicitly fallible `new (mozilla::fallible) Foo()`")

::: widget/windows/nsSound.cpp:186
(Diff revision 2)
> -    ::PlaySoundW(reinterpret_cast<LPCWSTR>(data), 0, flags);
> +    nsCOMPtr<nsIRunnable> player = new nsSoundPlayer(EmptyString(), data, flags);
> +    if (NS_WARN_IF(!player)) {
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +    nsresult rv = mPlayerThread->Dispatch(player, NS_DISPATCH_NORMAL);
> +    NS_ENSURE_SUCCESS(rv, rv);

Please use:

if (NS_WARN_IF(FAILED(rv))) {
  return rv;
}

::: widget/windows/nsSound.cpp:219
(Diff revision 2)
> +nsSound::CreatePlayerThread()
> +{
> +  if (mPlayerThread) {
> +    return NS_OK;
> +  }
> +  if (NS_FAILED(NS_NewNamedThread("PlayEventSound", getter_AddRefs(mPlayerThread)))) {

Too long. Please break after "," and start next line under first '"'.

::: widget/windows/nsSound.cpp:220
(Diff revision 2)
> +{
> +  if (mPlayerThread) {
> +    return NS_OK;
> +  }
> +  if (NS_FAILED(NS_NewNamedThread("PlayEventSound", getter_AddRefs(mPlayerThread)))) {
> +    NS_WARNING("NS_NewNamedThread failed!");

If you need to use NS_WARNING() in an if (or else if) block, please use NS_WARN_IF() in the condition.

::: widget/windows/nsSound.cpp:237
(Diff revision 2)
>    // If we wait until the first sound is played, there will
>    // be a time lag as the library gets loaded.
> +  // This should be done in player thread otherwise it will block main thread
> +  // at the first time loading sound library.
> +  mPlayerThread->Dispatch(NS_NewRunnableFunction([]() {
> -  ::PlaySound(nullptr, nullptr, SND_PURGE);
> +    ::PlaySound(nullptr, nullptr, SND_PURGE);

Don't we need to append SND_ASYNC?

::: widget/windows/nsSound.cpp:254
(Diff revision 2)
> -      NS_NewNamedThread("PlaySystemSound", getter_AddRefs(mPlayerThread),
> -                        player);
> -    NS_ENSURE_SUCCESS(rv, rv);
> +    if (NS_WARN_IF(!player)) {
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    }

Same as above, this OOM check isn't necessary at least in release build.

::: widget/windows/nsSound.cpp:317
(Diff revision 2)
> -  nsresult rv =
> -    NS_NewNamedThread("PlayEventSound", getter_AddRefs(mPlayerThread), player);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  if (NS_WARN_IF(!player)) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }

Ditto. This OOM check isn't necessary at least in release build.
Attachment #8876072 - Flags: review?(masayuki) → review-
(Assignee)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8876072 [details]
Bug 1363163 - Playing sound in a seperated thread to avoid jank

https://reviewboard.mozilla.org/r/147486/#review152780

Thanks for your review Masayuki
(Assignee)

Comment 24

2 years ago
mozreview-review-reply
Comment on attachment 8876072 [details]
Bug 1363163 - Playing sound in a seperated thread to avoid jank

https://reviewboard.mozilla.org/r/147486/#review152008

> Even if mNotFoundSoundURL is "beep" here, but when actually it's played, the value may be changed. However, we don't need to care such edge case.

You are right, we should init again when mNotFoundSoundURL is changed to non-beep (preference changes). But it seems we missed to listen accessibility.typeaheadfind.soundURL, we only listen accessibility.browsewithcaret prefrence changes

> Don't we need to specify SND_ASYNC too? (Although I don't know if it's actually useful.)

I am not sure too, but I think we dont need SND_ASYNC, but any playing sound will be stopped with SND_PURGE/0 (I tried manually with SND_PURGE only).
Even, we don't have to PlaySound(nullptr, nullptr, SND_PURGE) in player thread

> Is this really safe?
> 
> data is mLastSound. mLastSound is managed by nsSound in the main thread (freed in nsSound::PurgeLastSound()) and nsSoundPlayer doesn't copy this.
> 
> I think that mLastSound should be wrapped with refcountable threadsafe class. Then, both nsSound and nsSoundPlayer should grab it.

Hmm, I am not sure if that helps. The thread we should really care about is the thread created by PlaySoundW(reinterpret_cast<LPCWSTR>(data), 0, SND_ASYNC) (A thread) that we could never touch. I guess mLastSound should be available while we playing sound (unless PlaySoundW does another copy in lower layer. If it does copy, we will have no problem).
The problem is we don't know exacly when the playsound finishes, then we have no idea when we should delete mLastSound.
What I did is PlaySound(nullptr, nullptr, SND_PURGE) (I guess it will stop A thread) when we play another sound, then deleting mLastSound. Am I missing anything?

Comment 25

2 years ago
mozreview-review-reply
Comment on attachment 8876072 [details]
Bug 1363163 - Playing sound in a seperated thread to avoid jank

https://reviewboard.mozilla.org/r/147486/#review152008

> Hmm, I am not sure if that helps. The thread we should really care about is the thread created by PlaySoundW(reinterpret_cast<LPCWSTR>(data), 0, SND_ASYNC) (A thread) that we could never touch. I guess mLastSound should be available while we playing sound (unless PlaySoundW does another copy in lower layer. If it does copy, we will have no problem).
> The problem is we don't know exacly when the playsound finishes, then we have no idea when we should delete mLastSound.
> What I did is PlaySound(nullptr, nullptr, SND_PURGE) (I guess it will stop A thread) when we play another sound, then deleting mLastSound. Am I missing anything?

I don't worry about the buffer with ::PlaySoundW(). All Win32 API are thread safe. So, PlaySound() probably copies the given data.

The problem is, nsSoundPlayer() takes data (it has same address as mLastSound) but it doesn't copy. So, nsSoundPlayer::Run() refers mLastSound *later*. However, mLastSound may be changed if nsSound::OnStreamComplete() is called again or nsSound::PurgeLastSound() is called. So, there is a race of referring mLastSound.

And if mData is too big, copying memory may be expensive. If it's usually small, it's okay nsSoundPlayer to just copy the data. Otherwise, it should be wrapped with refcountable class.
Comment hidden (mozreview-request)
(Assignee)

Comment 27

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #25)
> Comment on attachment 8876072 [details]
> The problem is, nsSoundPlayer() takes data (it has same address as
> mLastSound) but it doesn't copy. So, nsSoundPlayer::Run() refers mLastSound
> *later*. However, mLastSound may be changed if nsSound::OnStreamComplete()
> is called again or nsSound::PurgeLastSound() is called. So, there is a race
> of referring mLastSound.
> 
> And if mData is too big, copying memory may be expensive. If it's usually
> small, it's okay nsSoundPlayer to just copy the data. Otherwise, it should
> be wrapped with refcountable class.

Ah, I see, thanks Masayuki-san. I guess that we don't need mLastSound anymore. We could create a refcountable data class then directly copy the data to that class (as in the updated patch). But, I see there's an edge case that we can not copy data http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/widget/windows/nsSound.cpp#183
we may have to play sound with sync flag in main thread
> But, I see there's an edge case that we can not copy data
> http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/widget/windows/nsSound.cpp#183
> we may have to play sound with sync flag in main thread

Ah, yeah. I mislead it. |data| is an argument (should be aData). So, anyway, this copies the memory block. Okay, I worried about this case. So, I think that copying the data in nsSoundPlayer is perhaps fine. I don't think that we should call PlaySoundW() in the main thread. Anyway, it may copy the data if we specify SND_ASYNC. So, the copy cost in main thread is necessary anyway. Then, it's safer to use black box in non-main thread since we don't know what PlaySoundW() actually do in the called thread.
Comment hidden (mozreview-request)
(Assignee)

Comment 30

2 years ago
Thanks Masayuki-san.
I updated the patch (plz ignore diversion 3 for refcountable things) and I am a bit surprised that nsSound is only allowed in main process.
http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/widget/gtk/nsWidgetFactory.cpp#276
I played with fastfind and sometimes it calls sound api in child and nothing happened (I have no idea, mostly it was the first time opening fastfind after launching Firefox).
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #30)
> Thanks Masayuki-san.
> I updated the patch (plz ignore diversion 3 for refcountable things) and I
> am a bit surprised that nsSound is only allowed in main process.
> http://searchfox.org/mozilla-central/rev/
> 20d16dadd336e0c6b97e3e19dc4ff907744b5734/widget/gtk/nsWidgetFactory.cpp#276
> I played with fastfind and sometimes it calls sound api in child and nothing
> happened (I have no idea, mostly it was the first time opening fastfind
> after launching Firefox).

Because child processes are sandboxed. They cannot access most platform APIs.

Comment 32

2 years ago
mozreview-review
Comment on attachment 8876072 [details]
Bug 1363163 - Playing sound in a seperated thread to avoid jank

https://reviewboard.mozilla.org/r/147486/#review154444

I'd like to check next patch (perhaps, it should be final), therefore, keeping r-.

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:160
(Diff revision 4)
>  
>    mNotFoundSoundURL = soundStr;
>  
> +  if (!mNotFoundSoundURL.IsEmpty() && !mNotFoundSoundURL.EqualsLiteral("beep")) {
> +    mIsSoundInitialized = true;
> +    mSoundInterface = do_CreateInstance("@mozilla.org/sound;1");

Please check if mSoundInterface is already non-nullptr.

::: widget/windows/nsSound.h:33
(Diff revision 4)
>  private:
> -  uint8_t* mLastSound;
> +  nsresult CreatePlayerThread();
>    nsCOMPtr<nsIThread> mPlayerThread;
> +  static mozilla::StaticRefPtr<nsISound> sInstance;
> +  bool mInited;

nit: please insert empty line between method declarations and member declarations. And could you move static member to top or bottom of the member variables?

::: widget/windows/nsSound.cpp:66
(Diff revision 4)
> -    NS_IF_ADDREF(mSound);
> +  if (mSoundData) {
> +    ::PlaySoundW(reinterpret_cast<LPCWSTR>(mSoundData), 0, mFlags);
> +  } else {
> +    ::PlaySoundW(mSoundName.get(), nullptr, mFlags);
> +  }

I'm bit afraid about here.

When mSoundData isn't nullptr, mFlags is always SND_MEMORY | SND_NODEFAULT | SND_ASYNC, and when mSoundData is nullptr, mFlags is always SND_NODEFAULT | SND_ALIAS | SND_ASYNC.

So, I think that nsSoundPlayer shouldn't take flags with argument and you should set them here directly. Then, any regressions won't occur with s mismatch between sound resource and flags.

::: widget/windows/nsSound.cpp:67
(Diff revision 4)
> +nsSoundPlayer::Run()
> -  {
> +{
> -    NS_GetCurrentThread(getter_AddRefs(mThread));
> -    NS_ASSERTION(mThread, "failed to get current thread");
> -    NS_IF_ADDREF(mSound);
> +  MOZ_ASSERT(!mSoundName.IsEmpty() || mSoundData,
> +    "Sound name or sound data should be specified");
> +  if (mSoundData) {
> +    ::PlaySoundW(reinterpret_cast<LPCWSTR>(mSoundData), 0, mFlags);

nit: s/0/nullptr

::: widget/windows/nsSound.cpp:81
(Diff revision 4)
> -    {
> +{
> +  MOZ_ASSERT(!mSoundData, "Already initialized");
> +  MOZ_ASSERT(aSize > 0, "Size should not be zero");
> +  MOZ_ASSERT(aData, "Data shoud not be null");
> +
> +  mSoundData = (uint8_t *) malloc(aSize);

Could you use |new uint8_t[aSize]| instead of C style malloc?

::: widget/windows/nsSound.cpp:93
(Diff revision 4)
> -  NS_SetCurrentThreadName("Play Sound");
> -
> -  NS_PRECONDITION(!mSoundName.IsEmpty(), "Sound name should not be empty");
> -  ::PlaySoundW(mSoundName.get(), nullptr,
> +  if (mSoundData) {
> +    free(mSoundData);
> +    mSoundData = nullptr;
> +  }

Then, you can use |delete [] mSoundData;| without null check.

::: widget/windows/nsSound.cpp:338
(Diff revision 4)
> -  nsCOMPtr<nsIRunnable> player = new nsSoundPlayer(this, sound);
> -  NS_ENSURE_TRUE(player, NS_ERROR_OUT_OF_MEMORY);
> +  nsCOMPtr<nsIRunnable> player =
> +    new nsSoundPlayer(sound, SND_NODEFAULT | SND_ALIAS | SND_ASYNC);

|sound| is |const wchar_t*|. I guess that this is the reason why you create nsSoundPlayer::InitData() instead of another constructor.

I think that you can use |nsDependentString(sound)| here. Then, you can remove the constructor using |const wchar_t*|. Then, you can create another constructor, which takes |const uint8_t* aSoundData|. Finally, you can remove InitData().
Attachment #8876072 - Flags: review?(masayuki) → review-
(Assignee)

Comment 33

2 years ago
mozreview-review
Comment on attachment 8876072 [details]
Bug 1363163 - Playing sound in a seperated thread to avoid jank

https://reviewboard.mozilla.org/r/147486/#review154940

::: widget/windows/nsSound.cpp:338
(Diff revision 4)
> -  nsCOMPtr<nsIRunnable> player = new nsSoundPlayer(this, sound);
> -  NS_ENSURE_TRUE(player, NS_ERROR_OUT_OF_MEMORY);
> +  nsCOMPtr<nsIRunnable> player =
> +    new nsSoundPlayer(sound, SND_NODEFAULT | SND_ALIAS | SND_ASYNC);

Thanks Masayuki-san,
:) I would like to avoid exceptions. Classic method Init could return error (NS_ERROR_OUT_OF_MEMORY) and we just don't play sound in this case.
(Assignee)

Updated

2 years ago
Attachment #8873365 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 36

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #34)
> We don't need to worry about OOM case unless you use fallible allocation
> explicitly because when OOM occurs, current Gecko crashes.
> https://searchfox.org/mozilla-central/rev/
> 7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/memory/mozalloc/mozalloc_oom.cpp#24,
> 54
> https://searchfox.org/mozilla-central/rev/
> 7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/memory/mozalloc/mozalloc_abort.
> cpp#22,33

Ah, I see, thanks for pointing out :)

Comment 37

2 years ago
mozreview-review
Comment on attachment 8876072 [details]
Bug 1363163 - Playing sound in a seperated thread to avoid jank

https://reviewboard.mozilla.org/r/147486/#review155078

Nice! Thank you for your great work!

::: widget/windows/nsSound.cpp:39
(Diff revisions 4 - 5)
>  class nsSoundPlayer: public mozilla::Runnable {
>  public:
> -  nsSoundPlayer(const wchar_t* aSoundName,
> +  nsSoundPlayer(const nsAString& aSoundName)
> -                DWORD aFlags)
>      : mSoundName(aSoundName)
> -    , mSoundData(nullptr)
> +    , mSoundData(nullptr) {}

nit: Could you put {} to the next line? Then, when somebody adds new member below mSoundData, they don't need to change this line. So, it's easier to check the diff.
Attachment #8876072 - Flags: review?(masayuki) → review+
(Assignee)

Comment 38

2 years ago
Thanks for your review Masayuki-san :)
(Assignee)

Comment 40

2 years ago
It looks there're 2 failures on try. nsSound::~nsSound do some stuffs with nsIThread and expected to be earlier than xpcom-shutdown-threads. However, nsSound reference is kept somewhere which may have a longer lifetime. I may have to change a bit in the patch that shutdown and release thread before xpcom-shutdown-threads.
Comment hidden (mozreview-request)

Comment 43

2 years ago
mozreview-review
Comment on attachment 8876072 [details]
Bug 1363163 - Playing sound in a seperated thread to avoid jank

https://reviewboard.mozilla.org/r/147486/#review156572

::: widget/windows/nsSound.h:18
(Diff revisions 5 - 6)
>  class nsSound : public nsISound,
> -                public nsIStreamLoaderObserver
> +                public nsIStreamLoaderObserver,
> +                public nsIObserver

Move "," to below ":".

Otherwise, looks good to me.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 45

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 24cc8e14b55b -d ad8486907154: rebasing 403419:24cc8e14b55b "Bug 1363163 - Playing sound in a seperated thread to avoid jank r=masayuki" (tip)
merging widget/windows/nsSound.cpp
merging widget/windows/nsWidgetFactory.cpp
warning: conflicts while merging widget/windows/nsWidgetFactory.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Keywords: checkin-needed

Comment 46

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 24cc8e14b55b -d 023b96f48df8: rebasing 403423:24cc8e14b55b "Bug 1363163 - Playing sound in a seperated thread to avoid jank r=masayuki" (tip)
merging widget/windows/nsSound.cpp
merging widget/windows/nsWidgetFactory.cpp
warning: conflicts while merging widget/windows/nsWidgetFactory.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 48

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/676386703e2d
Playing sound in a seperated thread to avoid jank r=masayuki
Keywords: checkin-needed

Comment 49

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/676386703e2d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

4 months ago
Depends on: 1482659
You need to log in before you can comment on or make changes to this bug.