Chatterino / chatterino2

Chat client for https://twitch.tv
MIT License
2.05k stars 449 forks source link

🚫🎵 No Sound from Pings🎵🚫 #1228

Closed Felanbird closed 1 year ago

Felanbird commented 5 years ago

Describe the bug

Ping sounds don't generate a noise

To Reproduce

This issue was referenced to me by another person so I recreated it by doing this:

I downloaded the newest nightly and added a portable folder (as to not add any settings), added my account, added (and subsequently disabled) an extra highlight of my username for testing, and pinged my username via another chatterino instance + account, and no sound was generated.

Expected behavior

Ping sound to make a noise.

Screenshots

https://streamable.com/ex5qg Commit on 2nd instance covered by box is https://github.com/Chatterino/chatterino2/commit/cbd93f95374d2377b21f2386fb47c8f6c517f45b

Chatterino version

Chatterino Nightly 2.1.0 (21.08.2019 3079765)

Additional context

I use multiple instances of chatterino normally, a forked version of https://github.com/Chatterino/chatterino2/commit/feef6c6aaa9ed7c250a818272c6da7eef612e550 & https://github.com/Chatterino/chatterino2/commit/cbd93f95374d2377b21f2386fb47c8f6c517f45b both of these instances have no ping problems at all.

I also tested on 381aaaa and I thought it generated the same issue as 3079765 but after closing OBS I was unable to recreate the issue with this instance, even with reopening OBS and recording with it.

I think this issue involves the fact that 3079765 doesn't show up on volume mixer , as the person who referenced this issue to me mentioned the same problem. I do personally use VoiceMeeter Banana to keep discord out of OBS but the person who referenced me this does not use any sound alteration programs (that he knows of).

Myself and the person who referenced this are both on Windows Version 1903 (OS Build 18362.295) , and I was forcibly updated to this version a few days ago actually. 😤

I closed the normal gaming issue suspects, Battle.net/Steam/Discord , included a PC restart. I think this is probably a windows side issue but I tried my best. /shrug

gempir commented 4 years ago

Happend on stable 2.1.7 recently sound worked for a few mins and after some setting changes suddendly sound was not working despite the Setting being on

Maybe a combination of adding new highlights and enable sounds on them, disabling on other ones?

Someonexddd commented 4 years ago

Still experiencing this on 2.2.2

XanthosD commented 3 years ago

Anyone still having this issue? I just started experiencing it.

Someonexddd commented 3 years ago

Anyone still having this issue? I just started experiencing it.

Yes still experiencing it

Felanbird commented 3 years ago

While I don't want to randomly bump this ancient issue, I want to make a note of it.

This issue seems to always been specific to Chatterino not appearing on the Volume Mixer (and the technical aspects of this fact), as most people who have the issue are able to use this visual example while restarting to "fix" the issue.

Not sure if there's anyway we can re-force Chatterino in this manor, but I just wanted to note my findings over the past year.

Greenlandicsmiley commented 1 year ago

I may have found the issue.

https://github.com/Chatterino/chatterino2/blob/8d4ee724781c8d00be5cbfcce14028d222ef4f29/src/messages/SharedMessageBuilder.cpp#L231-L237

By commenting out the if statement and leaving player->setMedia and currentPlayerUrl variables uncommented in the above code snippet, I've found that the ping sounds work perfectly as they should.

I've only tested with my own system so far, so if someone could test this or look into a better solution then that'd be perfect.

Specs: Chatterino 7.4.0 commit 79c64e96 QT 5.15.7 Arch Linux kernel 6.0.10-zen2-1-zen Pipewire 1:0.3.61-1

Greenlandicsmiley commented 1 year ago

I realized that there are only two functions with the variable currentPlayerUrl

https://github.com/Chatterino/chatterino2/blob/8d4ee724781c8d00be5cbfcce14028d222ef4f29/src/messages/SharedMessageBuilder.cpp#L210

https://github.com/Chatterino/chatterino2/blob/8d4ee724781c8d00be5cbfcce14028d222ef4f29/src/messages/SharedMessageBuilder.cpp#L231-L237

https://github.com/Chatterino/chatterino2/blob/8d4ee724781c8d00be5cbfcce14028d222ef4f29/src/controllers/notifications/NotificationController.cpp#L99

https://github.com/Chatterino/chatterino2/blob/8d4ee724781c8d00be5cbfcce14028d222ef4f29/src/controllers/notifications/NotificationController.cpp#L107-L112

The currentPlayerUrl variable is never used anywhere else to my knowledge, so is there any reason to keep them?

If there is no reason, then maybe the following code snippet can be turned from this

if (currentPlayerUrl != this->highlightSoundUrl_)
    {
        player->setMedia(this->highlightSoundUrl_);

        currentPlayerUrl = this->highlightSoundUrl_;
    }
player->play();

into this?

player->setMedia(this->highlightSoundUrl_);

player->play();

As well as removing static QUrl currentPlayerUrl;

I'm new to this, so I'm not sure if I'm doing this correctly :)

Nerixyz commented 1 year ago

The currentPlayerUrl variable is never used anywhere else to my knowledge, so is there any reason to keep them?

One reason might be, that setMedia doesn't have this check internally. Thus, on some platforms, the player will reopen the file, even though it was already loaded (e.g. on Windows). Not sure if this is a blocker.

Assuming this bug is because media doesn't get loaded, it might be possible to listen to the signals of the player - specifically error and mediaChanged and update/reset the currentPlayerUrl.

Greenlandicsmiley commented 1 year ago

What does everyone think of the following code snippets?

https://github.com/Greenlandicsmiley/chatterino2/blob/85423b0885f0e5228207877de5633b90b088a5fc/src/controllers/notifications/NotificationController.cpp#L96-L113

https://github.com/Greenlandicsmiley/chatterino2/blob/a615d5a78386b6a4273b036cd62434afd5709251/src/messages/SharedMessageBuilder.cpp#L211-L246

The new lines check if mediaStatus is loaded or fully buffered and will load media if they are not.

Nerixyz commented 1 year ago

What does everyone think of the following code snippets?

Looks like the correct approach, but your comments don't match your implementation. Your implementation should probably be:

if (!(player->mediaStatus() == QMediaPlayer::LoadedMedia ||
      player->mediaStatus() == QMediaPlayer::BufferedMedia)) {}
// or the equivalent status != loaded && status != buffered

I think you could include the LoadingMedia, BufferingMedia, EndOfMedia statuses, and potentially StalledMedia.

Greenlandicsmiley commented 1 year ago

Thank you for guiding me, the lines have been updated

https://github.com/Greenlandicsmiley/chatterino2/blob/1f7d231328a89f5bd89cc12323c27ef81fa8d5e1/src/controllers/notifications/NotificationController.cpp#L106-L114

https://github.com/Greenlandicsmiley/chatterino2/blob/26223ccf6746ef64b5dca9efb6ee7e9de1d27e07/src/messages/SharedMessageBuilder.cpp#L232-L240

I tried including the EndOfMedia status and found that the ping sound goes away after a few pings, so that cannot be included. However the other statuses you suggested are fine.