Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added per-sound channel individual volume core options (#166) #167

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

eadmaster
Copy link
Contributor

improvement suggestions are welcomed

@lgtm-com
Copy link

lgtm-com bot commented Nov 25, 2020

This pull request introduces 1 alert when merging ddd4664 into 93e27de - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@jdgleaver
Copy link
Contributor

@eadmaster This is a good idea, but this implementation won't work properly. You're replaced soChVol_ with soVol_, but the rest of the code still expects soVol_ to be used. Search for the function setSoVolume() - this is used to modify soVol_ dynamically at runtime, and when loading a save state. You need to dig a little deeper, and ensure that all instances of volume modification are properly accounted for :)

@eadmaster
Copy link
Contributor Author

eadmaster commented Nov 26, 2020

Actually i've tested my changes with a few games and the volume controls options are working fine as intended.

setSoVolume() is invoked only in 2 places: when loading a savestate, and inside Memory::nontrivial_ff_write.

Since i want to override the savestate volume in any case the 1st call can be ignored. (this also affects rewinding, which should not reset the custom set ch-volumes)

About the 2nd one i am not sure about its purpose, maybe some games use that for volume fading effects?

If this is the case then PSG::setSoChanVolume() should adjust soChVol according to the current value of soVol_, or these effects are lost (i may need some help with the bitwise math here).

@jdgleaver
Copy link
Contributor

About the 2nd one i am not sure about its purpose, maybe some games use that for volume fading effects?

Yes, among other things - NR50 controls the left/right master volume, which can be set to zero in software, or ramped somewhat (you can see the later in e.g. Zelda Oracle of Ages). And because it can be set in software, you absolutely do have to restore the value when loading a save state :)

So basically, PSG::setSoChanVolume() should only record volume 'modifiers', which are then multiplied by soVol_ inside PSG::accumulateChannels().

One way you could do this is as follows:

  • Each element of soChVol_ takes a number from [0,10]

  • You call each channel update function in PSG::accumulateChannels() like this:

ch1_.update(buf, (soChVol_[0] * soVol_ * 0x1999999A) >> 32, cycles);

This would be the path of least resistance, since it means you wouldn't have to worry about changing the existing setSoVolume() calls.

Alternatively, you'd need to have 2 soChVol_ arrays - one to store the modifiers, and another to store the 'real' values. The modifiers would get set in check_variables(), and the real values would get set inside PSG::setSoVolume() (derived from the soVol_ obtained there). I'll leave it up to you to benchmark which method is faster :)

@eadmaster
Copy link
Contributor Author

Thank you for your help, i'll definitively try the 1st solution since it looks cleaner.

@jdgleaver
Copy link
Contributor

Yes, that's much better :)

You might just want to update the void setSoChanVolume(unsigned nr50, unsigned ch); function prototype in sound.h, though (nr50 -> percent)

@eadmaster
Copy link
Contributor Author

eadmaster commented Nov 27, 2020

all done :-)

I've tested the new core options with savestates and rewind and they are working fine.

EDIT: do you think it is safe to store soChVol_ as an unsigned short?

@jdgleaver
Copy link
Contributor

EDIT: do you think it is safe to store soChVol_ as an unsigned short?

Ah, no - it should be 'safe' on most platforms, but you want to keep it as a 64 bit data type to avoid implicit casting

(i.e. you want everything in the (soChVol_[0] * soVol_ * 0x1999999A) >> 32 calculation to have the same data type, or you might see a tiny performance penalty)

So if you change that back, we should be all good :)

Also, could you squash your commits? There a quite a few now!

@eadmaster
Copy link
Contributor Author

uhm, i am not sure if i've squashed them properly, sorry but i am new to git...

@jdgleaver
Copy link
Contributor

uhm, i am not sure if i've squashed them properly, sorry but i am new to git...

No worries - everyone has to start somewhere :)

For this particular PR, I believe you just need to run:

git rebase -i fc79bb48a38093ace1d9667aa60ed644540b4137

...and replace 'pick' on the second and subsequent commits with 'squash' (that string is the hash of your first commit)

Once that's done, just force push to your branch:

git push --force origin master

For future reference, it's good practice to work in a feature branch, as described here: https://blog.scottlowe.org/2015/01/27/using-fork-branch-git-workflow/

The benefits of this are:

  • It is trivial to rebase against current master - i.e.:
git checkout master
git pull upstream master
git checkout my_branch
git rebase master
  • It is trivial to squash your commits into one - i.e. if you made 5 commits in your feature branch, you'd just do:
git rebase -i HEAD~5

...then follow the instructions here: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

If you work in master, then squashing commits can be bothersome (since you can end up with upstream commits in-between yours - that's not the case with this particular PR, but it happens a lot!)

fixes for max and 0 volumes

reworked accumulateChannels to preserve volume effects

minor fixes

reverted soChVol_ to long
@eadmaster
Copy link
Contributor Author

i think i finally got it right:
eadmaster@a0a0f84

@jdgleaver
Copy link
Contributor

Yes, that's all good now - ready for merging :)

@cmitu
Copy link

cmitu commented Dec 21, 2020

Looks like this PR broke sound on some system (#170).

I think 32 bit systems are getting a 0 sound level because of the bit operations in https://github.com/libretro/gambatte-libretro/pull/167/files#diff-6446e2d4750bfd3239e97aa0b1bfbd7274b03105941eabb36fe0ac3ef637970eR106-R109.

@jdgleaver
Copy link
Contributor

I can confirm that there is no sound on 32 bit systems.

Clearly this code requires further testing. I'm afraid we can't have a bug of this magnitude in the core so close to the scheduled Christmas release - @twinaphex I think this will have to be reverted until after the new infrastructure work is completed.

@eadmaster
Copy link
Contributor Author

Sorry about the regression, prolly it is due to an overflow on 32-bit systems.

I'll see if i can fix this, in the meanwhile i suggest you to revert to an older core build if you can (just download and install manually from the buildbot server).

@jdgleaver
Copy link
Contributor

@eadmaster Yes, I didn't originally notice the 32 bit issue either - what an annoying bug!

We need to have a working core by Christmas Eve - if you can get a fix PR'd in time for that, then it's all good :)

If not, we'll revert temporarily, and revisit this after the holidays (either way is fine, to be honest)

@jdgleaver
Copy link
Contributor

@eadmaster I believe this should fix the issue:

diff --git a/libgambatte/src/sound.cpp b/libgambatte/src/sound.cpp
index 1b0f3a0..2346723 100644
--- a/libgambatte/src/sound.cpp
+++ b/libgambatte/src/sound.cpp
@@ -103,10 +103,10 @@ namespace gambatte
       uint_least32_t *const buf = buffer_ + bufferPos_;
 
       std::memset(buf, 0, cycles * sizeof(uint_least32_t));
-      ch1_.update(buf, (soChVol_[0] * soVol_ * 0x1999999A) >> 32, cycles);
-      ch2_.update(buf, (soChVol_[1] * soVol_ * 0x1999999A) >> 32, cycles);
-      ch3_.update(buf, (soChVol_[2] * soVol_ * 0x1999999A) >> 32, cycles);
-      ch4_.update(buf, (soChVol_[3] * soVol_ * 0x1999999A) >> 32, cycles);
+      ch1_.update(buf, ((uint_fast64_t)0x1999999A * soChVol_[0] * soVol_) >> 32, cycles);
+      ch2_.update(buf, ((uint_fast64_t)0x1999999A * soChVol_[1] * soVol_) >> 32, cycles);
+      ch3_.update(buf, ((uint_fast64_t)0x1999999A * soChVol_[2] * soVol_) >> 32, cycles);
+      ch4_.update(buf, ((uint_fast64_t)0x1999999A * soChVol_[3] * soVol_) >> 32, cycles);
    }
 
    void PSG::generateSamples(unsigned long const cycleCounter, bool const doubleSpeed)

I am very busy with the new build infrastructure at the moment, however, and have not had time for proper testing. I will try to do this later...

@eadmaster
Copy link
Contributor Author

ok, i'm adding an extra check to revert to the old formula if the volume is 100% to be sure...

@bslenul
Copy link
Contributor

bslenul commented Dec 22, 2020

I now have sounds back with that diff on Windows 10 64bit, however it seems to only work properly with "volume %" at 100 or 50 (or well... 0 :p), any other value gives a terrible distorted sounds.

2020-12-22.12-05-41.mp4

edit: Same results with #172

@eadmaster
Copy link
Contributor Author

eadmaster commented Dec 22, 2020

confimed! I'm writing another fix ... :-)

@eadmaster
Copy link
Contributor Author

UPDATE: try my latest commit, should be fine on all systems now:
eadmaster@987955b

@bslenul
Copy link
Contributor

bslenul commented Dec 23, 2020

No more sound distortions so it's much better 👍 But it sounds like depending on the "volume %" the sounds only come out from the left speaker, like 100 is fine but if I set the options to 80 I can hear most of the sounds only in my left ear and then if I set to 70 it's fine again.

2020-12-23.13-28-45.mp4

Check the video, it's especially obvious (even more if you use headphones) when I go from 80 to 70.

@inactive123
Copy link
Contributor

Hi there, as per @jdgleaver's recommendation, I am reverting this PR for now until the Holidays are over, and then we can properly go over this and implement it in a way so everyone is happy and all platforms work. My bad.

@eadmaster
Copy link
Contributor Author

sure, no problem, i am reopening the original issue: #166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants