Bug 443329 - JuK volume setting gets into infinite loop and uses incorrect rounding
Summary: JuK volume setting gets into infinite loop and uses incorrect rounding
Status: REOPENED
Alias: None
Product: juk
Classification: Applications
Component: general (other bugs)
Version First Reported In: 21.12.3
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Scott Wheeler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-05 03:27 UTC by David
Modified: 2022-04-12 23:28 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David 2021-10-05 03:27:25 UTC
SUMMARY
Since version 21.08.1, JuK always starts muted. I've dug a bit into the source code (not faimiliar with it, nor with the language, programming in general, or frameworks), and I think the problem comes from getting into a loop as follows:

- File 'playermanager.cpp', method 'PlayerManager::setupAudio()', sets a listener 'PlayerManager::setVolume' for 'AudioOutput::volumeChanged'.
- File 'playermanager.cpp', method 'PlayerManager::setVolume', has a line 'emit volumeChanged(volume)' right after setting the volume, which causes it to call itself again.
- After a while from getting stuck in this loop, somehow the volume gets set to NAN or -NAN (passing that as a parameter to 'PlayerManager::setVolume'), at which point it sets the volume to zero and stops changing it thereafter as it compares to the same volume that was set.

If I remove that line, the issue of starting muted seems to be fixed, but every time it starts, it decreases the volume by 1, which I guess happens because of two issues:
- It convers fractional volumes 0-1 between 'double' and 'float', which loses precision. For example, it turns a number like 0.5 into 0.499something.
- It then uses 'int(100 * float)' and 'int(int * 0.01)' for rounding volumes to integers/fractions, when it should be using 'std::round' instead, which combined with the issue above makes it pick a smaller volume in an integer 0-100 scale.

From some experiments of my own changing those seems to fix the issues, but I don't know if it introduces any new issues along. In any case, here is a callback trace of the moment in which 'PlayerManager::setVolume' is called with a NAN value:
PlayerManager::setVolume(PlayerManager * const this, qreal volume) (/home/david/del/juk/juk-21.08.0/playermanager.cpp:437)
QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<double>, void, void (PlayerManager::*)(double)>::call(void (PlayerManager::*)(PlayerManager * const, double) f, PlayerManager * o, void ** arg) (/usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:152)
QtPrivate::FunctionPointer<void (PlayerManager::*)(double)>::call<QtPrivate::List<double>, void>(QtPrivate::FunctionPointer<void (PlayerManager::*)(double)>::Function f, PlayerManager * o, void ** arg) (/usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:185)
QtPrivate::QSlotObject<void (PlayerManager::*)(double), QtPrivate::List<double>, void>::impl(int which, QtPrivate::QSlotObjectBase * this_, QObject * r, void ** a, bool * ret) (/usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:418)
libQt5Core.so.5![Unknown/Just-In-Time compiled code] (Unknown Source:0)
libphonon4qt5.so.4!Phonon::AudioOutput::volumeChanged(double) (Unknown Source:0)
libQt5Core.so.5![Unknown/Just-In-Time compiled code] (Unknown Source:0)
libQt5Core.so.5!QObject::event(QEvent*) (Unknown Source:0)
libQt5Widgets.so.5!QApplicationPrivate::notify_helper(QObject*, QEvent*) (Unknown Source:0)
libQt5Core.so.5!QCoreApplication::notifyInternal2(QObject*, QEvent*) (Unknown Source:0)
libQt5Core.so.5!QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (Unknown Source:0)
libQt5Core.so.5![Unknown/Just-In-Time compiled code] (Unknown Source:0)
libglib-2.0.so.0!g_main_context_dispatch (Unknown Source:0)
libglib-2.0.so.0![Unknown/Just-In-Time compiled code] (Unknown Source:0)
libglib-2.0.so.0!g_main_context_iteration (Unknown Source:0)
libQt5Core.so.5!QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (Unknown Source:0)
libQt5Core.so.5!QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (Unknown Source:0)
libQt5Core.so.5!QCoreApplication::exec() (Unknown Source:0)
main(int argc, char ** argv) (/home/david/del/juk/juk-21.08.0/main.cpp:122)

(source code was taken from debian sid source package)

STEPS TO REPRODUCE
1. Start JuK.

OBSERVED RESULT
Starts muted, and after that issue is fixed, stars with a lower volume every time.

EXPECTED RESULT
Should start with the same volume that's stored in the configuration file.

SOFTWARE/OS VERSIONS
Windows: 
macOS: 
Linux/KDE Plasma: 
(available in About System)
KDE Plasma Version: 5.21.5
KDE Frameworks Version: 5.86.0
Qt Version: 5.15.2

ADDITIONAL INFORMATION
Comment 1 David 2021-10-05 04:15:33 UTC
A small correction about the change of volume: seems to come from calling method 'Phonon::AudioOutput::volume()' from 'PlayerManager::volume()', which returns something slightly different than what is set through 'PlayerManger::setVolume'.
Comment 2 Michael 2022-04-05 04:08:06 UTC
That is great that you have found the issue and have an idea for a fix. Whatever you can do is better than what we have now. I've gone through the bugs for juk that are related to "volume" and there are 4 others that seem to be pointing at the same issue that you are addressing, like https://bugs.kde.org/show_bug.cgi?id=452191
Comment 3 David 2022-04-05 07:15:39 UTC
Mostly fixed in the latest version.
Comment 4 Michael 2022-04-05 20:51:19 UTC
Great! Are you referring to the upcoming 22.04 version of Juk?
Comment 5 David 2022-04-06 06:19:29 UTC
(In reply to Michael from comment #4)
> Great! Are you referring to the upcoming 22.04 version of Juk?

No, this is 21.12.3.
Comment 6 Michael 2022-04-08 04:51:19 UTC
I'm using 21.12.3. Are you saying that you haven't submitted your fix to the code base for next version? Because in this version, I'm not seeing the issue is fixed.
Comment 7 David 2022-04-08 06:24:40 UTC
(In reply to Michael from comment #6)
> I'm using 21.12.3. Are you saying that you haven't submitted your fix to the
> code base for next version? Because in this version, I'm not seeing the
> issue is fixed.

I have not submitted any fix. At least for me, it doesn't start muted now, but the volume bar does show it as being at -2^31 volume.
Comment 8 Michael 2022-04-12 02:34:53 UTC
I see. Well, would it be too much to ask for you to submit the code changes to the developer and leave it at that? The reason is that there are people like myself and one other person that I know personally for whom Juk starts out muted at every song change. It's unusable now.
Comment 9 David 2022-04-12 08:24:35 UTC
(In reply to Michael from comment #8)
> I see. Well, would it be too much to ask for you to submit the code changes
> to the developer and leave it at that? The reason is that there are people
> like myself and one other person that I know personally for whom Juk starts
> out muted at every song change. It's unusable now.

I don’t think the code changes I tried would solve the issue and I don't even know how to submit a pull request, but if it’s of any use, I’ve since switched to Tauon Music box: https://tauonmusicbox.rocks
Perhaps you should give it a try. I’ve found it to be a very suitable replacement if you tweak its config to mimic JuK, although you’ll need a few tricks to make it work correctly for KDE (e.g. set a kwin application rule so that it wouldn’t block compositing).

If you know how to compile JuK (I did so by downloading the debian source package), you could also try hard-coding the volume to what you use in every volume variable referenced in the source code. Or perhaps updating QT would solve some of the issues you have.
Comment 10 Michael 2022-04-12 23:27:07 UTC
Hello,

I haven't used Tauon Music box, but even if I did, it wouldn't help others to know about it and switch. 

It's not necessary to submit a pull request for simple code changes. In the past, I've just submitted an issue ticket with instructions for manual changes and that was enough.
Comment 11 Michael 2022-04-12 23:28:13 UTC
Also, I'd like to change the status of this bug to REOPENED as it's not truly fixed and your report is the most detailed filed so far.