get-volume values differ from those displayed by pavucontrol by +0/-1

[ahmogit]

Observed with "ponymix 2-2" and "pavucontrol 2.0-2" on a recently synched Arch system, running on a Thinkpad T-510.

Probably just a round() vs. floor() inconsistency between the two tools. Not implying that ponymix is "wrong", just bringing the inconsistency to your attention in case you want to look into it. If you do look into it and think that ponymix is behaving sensibly and pavucontrol isn't, just let me know and I'll file a ticket against pavucontrol (referencing this ticket so they can get with you if there's any finger pointing).

On my system (soundcard = "HDA Intel MID") it's readily reproducible: Slide the pavucontrol output device volume to a random place, then interrogate the same device via ponymix. The ponymix value is +0/-1 relative to the pavucontrol displayed slider value.

Not a big deal, but would be nice if they matched exactly for W&Fness.

Thanks. Nice tool.

[falconindy]

Feel free to play with where I suspect the rounding difference is:

https://github.com/falconindy/ponymix/blob/master/pulse.cc#L74

I'm sorry, but I don't really have much time/interest in looking into this myself.

[ahmogit]

No problem, thanks for the pointer. Looks like the 0.5 is being summed in the wrong place, it should probably be (value * PA_VOLUME_NORM / 100.0) + 0.5.

I'll fix/test this and submit a patch.

Thanks.

[ahmogit]

The following seems to fix the problem. Patch is against the Arch 2-2 sources, not the git sources here. If that's useless to you, let me know how to better supply what you need.

*** pulse.cc    2014-08-18 20:30:21.579688164 -0600
--- ORIGpulse.cc        2014-08-18 20:06:32.036572259 -0600
***************
*** 78,84 ****
  }

  static int volume_as_percent(const pa_cvolume* cvol) {
!   return (pa_cvolume_avg(cvol) * 100.0 / PA_VOLUME_NORM + 0.5);
  }

  static int xstrtol(const char *str, long *out) {
--- 78,84 ----
  }

  static int volume_as_percent(const pa_cvolume* cvol) {
!   return pa_cvolume_avg(cvol) * 100.0 / PA_VOLUME_NORM;
  }

  static int xstrtol(const char *str, long *out) {

[falconindy]

Ah, sure. I guess this mirrors what's done in value_to_cvol. Thanks! I've committed this as 067f37d641ab.

In the future, you might want to look at the toys github has around -- you could have written this patch and pull request against git without ever leaving the website!

[ahmogit]

Dave Reisner notifications@github.com [2014-08-19 05:45:00 -0700]:

Ah, sure. I guess this mirrors what's done in value_to_cvol.

Yes. The value_to_cvol() function was actually unrelated to the reported problem. But your pointer got me into the general area and it was then easy to spot the problem, so thanks for that hint.

In the future, you might want to look at the toys github has around -- you could have written this patch and pull request against git without ever leaving the website!

Yeah, I just wasn't sure which would be more useful to you, diffing against git or Arch repo. Since I had to test it against the 2-2 Arch sources anyway, it was easy to just post that diff. In retrospect, since the patch was tiny, it would have made more sense to do as you suggested above and just manually re-make the change in the git sources and send you a pull request against it using the site tools. Oh well, next time...

Anyway, thanks for your help! Big improvement over our previous acrimonious interaction. Let's stay with this mode, much more constructive. :)

Regards,

• ahmogit/grepfor

[falconindy]

FYI, this introduces a functional bug:

$ ponymix set-volume 88
88
$ ponymix decrease 1
88

[ahmogit]

Hm, ok, let me look into this.

Do you recall offhand whether "set-volume N" sets the volume to one end or the other of the underlying value range that gets quantized to N%? Or does it set it to the center of that range? Something like this is possibly implicated, I'm pretty sure last night's patch was correct in the opposite direction (i.e. value -> percent).

Anyway, just guessing. Let me have a look at it tonight and will let you know what I find. If I can patch it readily I will send you a pull request.

[ahmogit]

FYI: The comment that you supplied with the patch commit last night ("use ceil() instead of floor() for volume percentages") is not correct. The patch adds 0.5 to the float computation, and then, using the implicit truncation upon conversion to integer in the return value, effectively rounds (not ceil()s) the return value to the nearest integer percent. That rounding operation is correct as written in the patch. The pre-patched function is not correct because it truncates the float value to integer. That is why the value displayed by ponymix differed from the value displayed by pavucontrol: Pavucontrol also correctly rounds the underlying value to percentage. If you want to call that a "cosmetic change" that's up to you. Most people would call it a bug.

What seems to have happened is that the patched correction of the bug in volume_as_percent() has exposed a new bug, a symptom of which you showed above in the "set-volume 88" example.

I'll have a look at that, and suggest a patch if I can, but only if you'll agree to cooperate in a genteel manner rather than making snide comments in the commit texts ("introduced a functional bug at the cost of attempting to fix a cosmetic one"). C'mon Dave grow up. Acting that way is just childish and accomplishes nothing. Let's attempt to actually get a good technical solution here, one that fixes the original issue and the newly exposed one, ok? I'll take it as a given that you hate my guts for some reason and/or just enjoy being nasty, but let's at least try to cooperate in a civil manner for a few days and get a good technical solution here. OK?

[falconindy]

Hrmm. I don't know who you are, and I don't know what prior acrimony you're referring to. I reopened this and re-raised the issue because I was interested in finding a correct solution. Your goal here seems to be to chide me over inaccurate and allegedly inflammatory commit messages, while harping on some aging prejudice.

Thanks for raising the bug.

[ahmogit]

Excellent work thestinger, thanks. Your explicit use of round() in volume_as_percent() is a nice improvement over the implicit rounding that I'd done in my patch by simply adding 0.5 (though functionally identical).

The source of the secondary problem that Dave pointed out is that fixing the rounding bug placed the quantized percentage in the middle of the underlying value range where it should have been all along, whereas the original function had it at the bottom end, due to the integer truncation. Thus, the 0.5 that was being added to "value" in volume_to_cvol() was actually a compensation for the bug in volume_as_percent() that was fixed by my (and your) patch. This in turn was causing the value computed by volume_to_cvol() to be occasionally too large by 1%, if it happened to be right on the quantization boundary to begin with. Thus "set-volume 88" wound up returning a value corresponding to 89%, and then decrementing by 1 wound up at 88 again.

Again, nice work, good solution.

[ahmogit]

Dave, our earlier interaction was in an Arch forum a year or so ago. I had put it behind me and was glad to have the opportunity here to interact with you in a constructive and less polemical way.

Given my obvious attempt to be friendly earlier in the ticket, I'm completely baffled as to how you could come to the conclusion that my "goal was to chide you" etc. etc. My goal was to come to a technical solution, and perhaps in the process as a bonus, make peace with you by interacting in a positive, friendly way. Instead, you took what was an offered patch -- and a technically correct one, afaict -- and reverted it with an abrasive comment that accomplished nothing towards the technical solution and was somewhat insulting to boot. And what was my reaction in return (as you can read above)? To once again attempt to be constructive and positive with you, to work together in a civil way towards a good technical solution. If you see that interaction in any other way, I'm baffled as to how.

With thestinger's help, the technical goal of fixing the dual interacting bugs seems to have been achieved. As for the personal issue, I'll once again -- third time I think -- express the desire to work with you in the future in a constructive, positive way if we cross paths again. You're an intelligent guy and a talented developer, there's no reason for us to be at odds. Look at your own language and you'll see the source of the friction.