After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 710424 - Volume up key sets volume back to 100%, if volume previously surpassed 100%
Volume up key sets volume back to 100%, if volume previously surpassed 100%
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: media-keys
3.24.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
Depends on: 790280 790281
Blocks: 790988
 
 
Reported: 2013-10-17 20:36 UTC by Titus
Modified: 2019-02-11 02:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH 1/4] Send correct volume level even if amplified (3.47 KB, patch)
2017-11-13 08:31 UTC, Didier Roche
none Details | Review
[PATCH 2/4] Add optional override level parameter to the Shell (7.96 KB, patch)
2017-11-13 08:31 UTC, Didier Roche
none Details | Review
[PATCH 3/4] Use similar logic than the Shell to show amplified icon (2.90 KB, patch)
2017-11-13 08:32 UTC, Didier Roche
none Details | Review
[PATCH 4/4] Prompt for volume override if key up is pressed multiple times (11.87 KB, patch)
2017-11-13 08:33 UTC, Didier Roche
rejected Details | Review
[PATCH 1/5] media-keys: rename normalized volume var to level (2.75 KB, patch)
2017-11-14 14:33 UTC, Didier Roche
none Details | Review
[PATCH 2/5] media-keys: Allow volume above 100% (4.93 KB, patch)
2017-11-14 14:34 UTC, Didier Roche
none Details | Review
[PATCH 3/5] media-keys: Add optional max_level parameter to the Shell (8.46 KB, patch)
2017-11-14 14:34 UTC, Didier Roche
none Details | Review
[PATCH 4/5] media-keys: Use similar logic than the Shell to show amplified icon (1.62 KB, patch)
2017-11-14 14:35 UTC, Didier Roche
none Details | Review
[PATCH 1/5] media-keys: rename normalized volume var to level (2.91 KB, patch)
2017-11-14 17:59 UTC, Didier Roche
none Details | Review
[PATCH 2/5] media-keys: Allow volume above 100% (4.94 KB, patch)
2017-11-14 18:00 UTC, Didier Roche
none Details | Review
[PATCH 3/5] media-keys: Add optional max_level parameter to the Shell (8.47 KB, patch)
2017-11-14 18:00 UTC, Didier Roche
none Details | Review
Sorry, it seems I didn't format-patch before posting back and attaching yesterday. (2.90 KB, patch)
2017-11-15 14:46 UTC, Didier Roche
none Details | Review
media-keys: Allow volume above 100% (6.05 KB, patch)
2017-11-15 14:47 UTC, Didier Roche
none Details | Review
media-keys: Add optional max_level parameter to the Shell (4.96 KB, patch)
2017-11-15 14:47 UTC, Didier Roche
none Details | Review
media-keys: Use similar logic to the Shell to show volume above 100% icon (1.84 KB, patch)
2017-11-15 14:47 UTC, Didier Roche
none Details | Review
media-keys: Rename normalized volume var to level (2.99 KB, patch)
2017-11-15 15:54 UTC, Didier Roche
none Details | Review
media-keys: Allow volume above 100% (4.79 KB, patch)
2017-11-15 15:55 UTC, Didier Roche
none Details | Review
media-keys: Add optional max_level parameter to the Shell (5.29 KB, patch)
2017-11-15 15:55 UTC, Didier Roche
none Details | Review
media-keys: Use similar logic to the Shell to show volume above 100% icon (1.84 KB, patch)
2017-11-15 15:55 UTC, Didier Roche
none Details | Review
media-keys: Allow volume above 100% (4.70 KB, patch)
2017-11-16 07:45 UTC, Didier Roche
none Details | Review
media-keys: Add optional max_level parameter to the Shell (5.73 KB, patch)
2017-11-16 07:45 UTC, Didier Roche
none Details | Review
media-keys: Use similar logic to the Shell to show volume above 100% icon (1.73 KB, patch)
2017-11-16 07:45 UTC, Didier Roche
none Details | Review
media-keys: Allow volume above 100% (4.70 KB, patch)
2017-11-16 14:57 UTC, Didier Roche
none Details | Review
media-keys: Add optional max_level parameter to the Shell (5.79 KB, patch)
2017-11-16 14:57 UTC, Didier Roche
none Details | Review
media-keys: Use similar logic to the Shell to show volume above 100% icon (1.79 KB, patch)
2017-11-16 14:57 UTC, Didier Roche
none Details | Review
media-keys: Allow volume above 100% (4.97 KB, patch)
2018-02-08 09:17 UTC, Didier Roche
none Details | Review
media-keys: Add optional max_level parameter to the Shell (5.83 KB, patch)
2018-02-08 09:18 UTC, Didier Roche
none Details | Review
media-keys: Use similar logic to the Shell to show volume above 100% icon (1.84 KB, patch)
2018-02-08 09:18 UTC, Didier Roche
none Details | Review
media-keys: reset volume max to 100% for current stream when disabled (2.66 KB, patch)
2018-02-08 09:18 UTC, Didier Roche
none Details | Review

Description Titus 2013-10-17 20:36:21 UTC
When you set the volume to more than 100% (e.g. via system settings), using the volume up key will set it back to 100% and therefore decreases the volume.
This behavior should be fixed.
At least, pressing volume up should not decrease system volume.

Having browsed through the code, I suppose changing the do_sound_action() in gsd-media-keys-manager.c would make this possible (Note: I do not have any experience in this area).
One would have to change:

line 1256: new_vol = MIN (old_vol + norm_vol_step, MAX_VOLUME);

to

new_vol = MIN (old_vol + norm_vol_step, MAX(MAX_VOLUME, old_vol));
Comment 1 Palash Vijay 2014-10-06 21:24:05 UTC
Hi , I am intrested in this bug .
Comment 2 Palash Vijay 2014-10-06 21:35:44 UTC
Hi , I am intrested in this bug .
Comment 3 Rithvik Patibandla 2016-12-24 11:23:51 UTC
In addition to the above bug, there is no way of going beyond 100% using multimedia keys. How about adding a checkbox in volume settings UI to set MAX_VOLUME to 200% if needed? The similar approach is used in Unity DE and it works fine.
Comment 4 Didier Roche 2017-08-15 15:37:02 UTC
Let me detail and add a little bit more context here:

Some devices have very low volume even when pushed at maximum. One example for this is the x220 when most of videos on youtube, or listening to music in rhythmbox don't give great results at the maximum volume.
Pulseaudio can amplify some of those sound devices (detecting if amplification is available on those output sources). Doing it, at the price of sound quality, makes at least the embedded speakers usable.

There are 3 components involved:
- gnome-control-center, which has a slider to push volume for supported device upper than 100%. http://imgur.com/VKQAjnj is the upstream current design.
- gnome-settings-daemon, detecting media keys, for volume up and down
- gnome-shell, showing up a slider in the volume menu.

The issue with the current design is that if you set the volume above 100% in gnome-control-center, and then press the media key up, the volume is reset to 100%, and you can't go further. A similar behavior is observable using GNOME Shell slider which reset the sound below what you did set in gnome control center.

What we did in Unity and ported to the ubuntu GNOME Shell session (not the GNOME vanilla session), is to have a switch enabling setting volume above 100% or disabling it completely. This switch is of course only available if one of the device output supports amplified volume and is off by default.
Consequently, when the switch is off, you can't set sound above 100%, even in gnome-control-center: http://imgur.com/ANUtHDx. Media keys and GNOME Shell also are capped to 100%.
If you switch it to on, in any sources that is selected, if they support amplified volume, then the 100% limit is visible and can be bypassed in gnome-control-center: http://imgur.com/a/mnl54. Media keys or GNOME Shell slider supports the new upper bound which is controlled via pulseaudio.

If no output source supports amplified volume, the switch is disabled.

Do you think that could be something that GNOME would want? I'm using in the interim an ubuntu specific key and only showing this behavior in our session. I can share and adapt the patch which are impacting gnome-control-center, gnome-settings-daemon and gnome-shell.
Comment 5 Didier Roche 2017-08-16 08:12:20 UTC
Here is a couple of videos illustrating the issue and the proposed fix:
Current behavior: https://www.youtube.com/watch?v=jptl2L15kKI
Proposed fix: https://www.youtube.com/watch?v=jptl2L15kKI
Comment 6 Bastien Nocera 2017-08-16 14:20:41 UTC
(In reply to Didier Roche from comment #5)
> Here is a couple of videos illustrating the issue and the proposed fix:
> Current behavior: https://www.youtube.com/watch?v=jptl2L15kKI
> Proposed fix: https://www.youtube.com/watch?v=jptl2L15kKI

I would make sure, first, that the problem isn't something that's fixable in other places. ALSA, PulseAudio, or the application.

1. If the problem is with the in-application's volume level (browsers can have separate volume sliders that don't affect per-application volumes), or the application's volume level (as visible in the Applications tab of the sound panel). I'd fix this by removing the intermediate "per-application" volume and have a per-"application class" volume.
Discussed in: https://wiki.gnome.org/action/info/Design/SystemSettings/Sound

2. Whether a whole class of laptops has this problem, we might be able to re-scale the volume in PulseAudio, in a manner that's transparent to the application and the desktop environment.

3. Consider whether the "higher than 100%" can always be turned on, and visual cues used in the sliders. iOS shows a segmented level bar in the OSD which will go red above a certain level. Something similar would be harder to achieve in the shell's status menu.

The proposed patch above is IMO, crude, not taking into account that different outputs will have different needs. Also to explore is iOS' "Volume Limit" restrictions, which caps the volume to avoid damaging younger ears.

Definitely needs design work.
Comment 7 Allan Day 2017-11-02 17:47:59 UTC
From a design perspective I largely agree with Bastien's comments. Removing per-application volumes would help with this issue, and it would be nice if we could find a way not to need the ability to go over 100% volume.

That said, if we are going to allow going over 100%:

 * I'd prefer not to have a preference in the control center, but can't think of a way around it!
 * We ought to provide feedback in the shell's system menu and volume OSD [1].
 * We could also think about adding an extra audio status icon to show in the top bar [1].
 * If someone repeatedly hits the volume up media key, it would be great if we could show a shell dialog which asks whether they want to enable going over 100% [2].

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/shell/volume-overdrive-status.png
[2] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/shell/volume-overdrive-confirmation.png
Comment 8 Didier Roche 2017-11-06 08:23:19 UTC
Thanks for your answer!

The per-application volume isn't the really issue there: the global system volume is really low. Applications are all already up to 100%. I also check other parameters with alsamixer and such.

I agree that preventing having a preference as on the screenshot would be good. I like your proposals and the "repeats hits volum up media key" -> trigger a shell dialog.
The question is then how to revert that decision? And I think this is where in the Settings, we can't avoid that. I don't think we should have the user required to press volume up to release that feature at every login, isn't it?
Comment 9 Allan Day 2017-11-06 11:45:38 UTC
(In reply to Didier Roche from comment #8)
> Thanks for your answer!
> 
> The per-application volume isn't the really issue there: the global system
> volume is really low.

I understand that there can be volume limitations with some hardware. The point is that, while there are genuine reasons why someone might want to go over 100%, there is also the possibility that someone could mistakenly use the feature because they have their music app turned down low.

> The question is then how to revert that decision? And I think this is where
> in the Settings, we can't avoid that. I don't think we should have the user
> required to press volume up to release that feature at every login, isn't it?

Indeed!
Comment 10 Didier Roche 2017-11-06 13:40:42 UTC
(In reply to Allan Day from comment #9)
> (In reply to Didier Roche from comment #8)
> > Thanks for your answer!
> > 
> > The per-application volume isn't the really issue there: the global system
> > volume is really low.
> 
> I understand that there can be volume limitations with some hardware. The
> point is that, while there are genuine reasons why someone might want to go
> over 100%, there is also the possibility that someone could mistakenly use
> the feature because they have their music app turned down low.

Oh, that makes totally sense to me now. Yeah, there is indeed two potential source of the issue and the user may not know. Should we add a 3rd button to the dialog opening Settings in the application panel (this feature doesn't exist right now, we might introduce deep linking there)?

> 
> > The question is then how to revert that decision? And I think this is where
> > in the Settings, we can't avoid that. I don't think we should have the user
> > required to press volume up to release that feature at every login, isn't it?
> 
> Indeed!

Ok, I'm going to work later this week or next one on the OSD and Shell-side for now until you work out designing the Settings part!

Thanks for your suggestion :)
Comment 11 Allan Day 2017-11-07 16:30:56 UTC
(In reply to Didier Roche from comment #10)
> Should we add a 3rd button to
> the dialog opening Settings in the application panel (this feature doesn't
> exist right now, we might introduce deep linking there)?

Nice idea. I've added it to the mockups.
Comment 12 Didier Roche 2017-11-13 08:31:07 UTC
Created attachment 363473 [details] [review]
[PATCH 1/4] Send correct volume level even if amplified
Comment 13 Didier Roche 2017-11-13 08:31:47 UTC
Created attachment 363474 [details] [review]
[PATCH 2/4] Add optional override level parameter to the Shell
Comment 14 Didier Roche 2017-11-13 08:32:44 UTC
Created attachment 363475 [details] [review]
[PATCH 3/4] Use similar logic than the Shell to show amplified icon
Comment 15 Didier Roche 2017-11-13 08:33:30 UTC
Created attachment 363476 [details] [review]
[PATCH 4/4] Prompt for volume override if key up is pressed multiple  times
Comment 16 Didier Roche 2017-11-13 08:49:44 UTC
It seems I can't add depends after the fact, here is the settings-daemon part. The Shell part is on https://bugzilla.gnome.org/show_bug.cgi?id=790280 and it depends on a new schema added in https://bugzilla.gnome.org/show_bug.cgi?id=790281.

Now, the remaining part is to get design on the Settings UI, to in term of slider representation (knowing the override key is global for all output channels) and how to disable this override once it's set.
Comment 17 Bastien Nocera 2017-11-14 11:11:10 UTC
Review of attachment 363473 [details] [review]:

> Send correct volume level even if amplified
 
There's a prefix for all commits here as well. "Send correct volume level" makes it look like the level was incorrect before this patch. Please reword.

> Use the pulseaudio maximum allowed volume if the allow amplified volume key
is true.

Name the exact GSettings key, in quotes. It makes for a weird sentence otherwise.

> Pass that information to the OSD and depend on the correct schema version.

Again with "correct". It wasn't incorrect before. It's the required version for the new feature rather.

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +1353,3 @@
                 return;
 
+        settings = g_settings_new ("org.gnome.desktop.sound");

Leak. And this should really be done once, and saved.
Comment 18 Bastien Nocera 2017-11-14 11:19:39 UTC
Review of attachment 363474 [details] [review]:

We're not "sending a key", we're showing an OSD.

The name "override" is just weird. We're overriding what? I'd expect a boolean, not a double.

Please also add a "See https://bugzilla..." with the gnome-shell bug.
Comment 19 Bastien Nocera 2017-11-14 11:24:54 UTC
Review of attachment 363475 [details] [review]:

> Shows up volume amplification icon for output only in case we have a ratio

"Shows up" means something different. "Show" would probably be enough. Why a ratio?

> where volume is more than max volume.

> Implements a similar logic than the shell there, ensuring we have the exact

"Implement", similar "to" the shell. Remove "there".

> same ratios and limitations.

I don't understand why the ratios again. And did you mean limits?

Please mention the gnome-shell bug, and add the schemas prefix to the commit subject.

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +408,3 @@
                 }
+                /* amplified volume */
+                if (volume > PA_VOLUME_NORM && !is_mic) {

No braces for one-line conditionals.

@@ +1184,2 @@
         if (!muted) {
+                vol = (int) (100 * (double) abs_volume / max_volume);

It's not a volume anymore you're calculating here now. The variable will need renaming first, in a separate patch.
Comment 20 Bastien Nocera 2017-11-14 11:29:39 UTC
Review of attachment 363476 [details] [review]:

For me, that's a complete no-no. It's completely hidden, not reversible, doesn't respect lockdown on the setting, and probably not required on the majority of devices.
I also don't think it would play well with a high key repeat rate.
Comment 21 Didier Roche 2017-11-14 14:33:45 UTC
Created attachment 363588 [details] [review]
[PATCH 1/5] media-keys: rename normalized volume var to level

Here is the new patch set, I didn't repost 5/5 as it's being reject right now, we can see once we get Allan's feedback.

The rest should match our discussion on IRC + addressing the concerns raised here. The D-BUS API being changed, now sending level and max_level (which are normalized values instead of ratios). max_level is ensure to be > 100 if sent.

The only thing I didn't change compared to our discussion is duplicating the show_osd(), as this function doesn't take a gvariant builder as a parameter, it would mean duplicating all this. I have used the same logic (which was the one with -1 I was already doing), and with other optional parameters: Only send max_level if > 100, don't send antyhing otherwise.

Keep me posted!
Comment 22 Didier Roche 2017-11-14 14:34:18 UTC
Created attachment 363589 [details] [review]
[PATCH 2/5] media-keys: Allow volume above 100%
Comment 23 Didier Roche 2017-11-14 14:34:42 UTC
Created attachment 363590 [details] [review]
[PATCH 3/5] media-keys: Add optional max_level parameter to the Shell
Comment 24 Didier Roche 2017-11-14 14:35:02 UTC
Created attachment 363591 [details] [review]
[PATCH 4/5] media-keys: Use similar logic than the Shell to show amplified icon
Comment 25 Didier Roche 2017-11-14 17:59:45 UTC
Created attachment 363629 [details] [review]
[PATCH 1/5] media-keys: rename normalized volume var to level

I was missing a var replacement, rebasing on this patch 1 to 3.
Comment 26 Didier Roche 2017-11-14 18:00:08 UTC
Created attachment 363630 [details] [review]
[PATCH 2/5] media-keys: Allow volume above 100%
Comment 27 Didier Roche 2017-11-14 18:00:28 UTC
Created attachment 363631 [details] [review]
[PATCH 3/5] media-keys: Add optional max_level parameter to the Shell
Comment 28 Bastien Nocera 2017-11-15 13:46:04 UTC
Review of attachment 363629 [details] [review]:

Please upper case after the ":" in the commit subject, and explain why you're doing this in the commit log.

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +1177,2 @@
         if (!muted) {
+                level = (int) (100 * (double) vol / PA_VOLUME_NORM);

Did you test this? You changed the first vol, not the second one.
Comment 29 Bastien Nocera 2017-11-15 13:48:50 UTC
Review of attachment 363630 [details] [review]:

> Pass that information to the OSD and depend on the required schema version.

"required gsettings-desktop-schemas version."

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +1357,3 @@
                 return;
 
+        if (g_settings_get_boolean (manager->priv->sound_settings, ALLOW_VOLUME_ABOVE_100_PERCENT_KEY))

Keep and track the current max volume in GsdMediaKeysManager, so you're not hitting GSettings for each key press.
Comment 30 Bastien Nocera 2017-11-15 13:51:53 UTC
Review of attachment 363631 [details] [review]:

Can you please rename this function to shell_show_osd_with_max_level() and add one that doesn't change signature?

Ditto for the internal show_osd().
Comment 31 Bastien Nocera 2017-11-15 13:54:47 UTC
Review of attachment 363591 [details] [review]:

"similar to".

This is going to warn in git-bz:
> See Shell implementation side on
> https://bugzilla.gnome.org/show_bug.cgi?id=790280.

Use "See https://..."
Comment 32 Didier Roche 2017-11-15 14:46:47 UTC
Created attachment 363692 [details] [review]
Sorry, it seems I didn't format-patch before posting back and attaching yesterday.

You thus convinced me to use git-bz ;)

The requested changes should now be done. I tested them with an adapting GNOME Shell patch set that I'm going to post next.

Note that I didn't repost patch 5/5 on trigger the UI in Shell part until we get more new from Allan.
Comment 33 Didier Roche 2017-11-15 14:47:09 UTC
Created attachment 363693 [details] [review]
media-keys: Allow volume above 100%

Use the pulseaudio maximum allowed volume if allow-volume-above-100-percent
gsettings key is true.
Pass that information to the OSD and depend on the required schema version.
Comment 34 Didier Roche 2017-11-15 14:47:16 UTC
Created attachment 363694 [details] [review]
media-keys: Add optional max_level parameter to the Shell

This option is sent to the Shell so that we can set the maximum level limit
when showing the OSD.
No value corresponds to 100. max_level can only be above 100.
Doesn't impact other calls or if the shell doesn't support that option.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 35 Didier Roche 2017-11-15 14:47:21 UTC
Created attachment 363695 [details] [review]
media-keys: Use similar logic to the Shell to show volume above 100% icon

Shows a volume above 100% icon for output only in case we have a current
level above 100. Use a double to ensure this icon is chosen as soon as
level is above 100.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 36 Bastien Nocera 2017-11-15 15:05:56 UTC
Review of attachment 363692 [details] [review]:

Looks good apart from the upper-case in the commit subject.
Comment 37 Bastien Nocera 2017-11-15 15:10:07 UTC
Review of attachment 363693 [details] [review]:

> Pass that information to the OSD and depend on the required schema version.

Same commit as earlier about this.

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +1150,3 @@
+                                                     GsdMediaKeysManager *manager)
+{
+        g_assert (g_str_equal(settings_key, ALLOW_VOLUME_ABOVE_100_PERCENT_KEY));

Space before parenthesis.

@@ +1151,3 @@
+{
+        g_assert (g_str_equal(settings_key, ALLOW_VOLUME_ABOVE_100_PERCENT_KEY));
+        manager->priv->allow_sound_above_100_percent = g_settings_get_boolean (settings, settings_key)

I would store the max_volume directly here.

allow_sound_above_100_percent = g_settings_get...
manager->priv->max_volume = allow_sound_above_100_percent ? PA_VOLUME_UI_MAX : PA_VOLUME_NORM;

@@ +1182,3 @@
                gboolean             muted,
                gboolean             sound_changed,
+               pa_volume_t          max_volume,

Then read manager->priv->max_volume directly, instead of passing it as a variable.

@@ +2859,3 @@
 
+        manager->priv->sound_settings = g_settings_new (SETTINGS_SOUND_DIR);
+        g_signal_connect (G_OBJECT (manager->priv->sound_settings), "changed::" + ALLOW_VOLUME_ABOVE_100_PERCENT_KEY,

Did this compile? There's no "+" in a static string concatenation in C.
Comment 38 Didier Roche 2017-11-15 15:54:55 UTC
Created attachment 363699 [details] [review]
media-keys: Rename normalized volume var to level

We'll compare it later on to a max_level, normalized volume level, for naming
consistency.
Comment 39 Didier Roche 2017-11-15 15:55:04 UTC
Created attachment 363700 [details] [review]
media-keys: Allow volume above 100%

Use the pulseaudio maximum allowed volume if allow-volume-above-100-percent
gsettings key is true.
Pass that information to the OSD and depend on the required
gsettings-desktop-schemas version.
Comment 40 Didier Roche 2017-11-15 15:55:09 UTC
Created attachment 363701 [details] [review]
media-keys: Add optional max_level parameter to the Shell

This option is sent to the Shell so that we can set the maximum level limit
when showing the OSD.
No value corresponds to 100. max_level can only be above 100.
Doesn't impact other calls or if the shell doesn't support that option.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 41 Didier Roche 2017-11-15 15:55:15 UTC
Created attachment 363702 [details] [review]
media-keys: Use similar logic to the Shell to show volume above 100% icon

Shows a volume above 100% icon for output only in case we have a current
level above 100. Use a double to ensure this icon is chosen as soon as
level is above 100.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 42 Bastien Nocera 2017-11-15 16:06:42 UTC
Review of attachment 363699 [details] [review]:

I don't think we need this anymore. We used to pass a level between 0 and 100 for the shell to display. But as we're now passing a maximum value, the "volume" variable keeps being the volume, and doesn't need to be recomputed to fit between 0 and 100 as the shell expected.
Comment 43 Bastien Nocera 2017-11-15 16:10:36 UTC
Review of attachment 363700 [details] [review]:

Looks good otherwise.

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +1146,3 @@
 
+static void
+gsettings_allow_volume_above_100_percent_changed_cb (GSettings           *settings,

Remove the "gsettings_" in front here.

@@ +1151,3 @@
+{
+        g_assert (g_str_equal (settings_key, ALLOW_VOLUME_ABOVE_100_PERCENT_KEY));
+        gboolean allow_volume_above_100_percent = g_settings_get_boolean (settings, settings_key);

We're old school. No function calls before variable declarations. So you'll also need to split off the declaration and assignment.

@@ +1192,3 @@
         if (!muted) {
                 level = (int) (100 * (double) vol / PA_VOLUME_NORM);
+                level = CLAMP (level, 0, (int) (100 * (double) manager->priv->max_volume / PA_VOLUME_NORM));

Will need changes as we're not renaming vol to level, as per previous patch review.
Comment 44 Bastien Nocera 2017-11-15 16:13:51 UTC
Review of attachment 363701 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +380,3 @@
 
+static void
+show_osd_with_max_level (GsdMediaKeysManager *manager,

You should do the same thing you did for the code in gsd-shell-helper.[ch]

@@ +1204,3 @@
         const char *icon;
         guint level;
+        guint max_level = (int) (100 * (double) manager->priv->max_volume / PA_VOLUME_NORM);

Don't declare and assign on the same line.
Comment 45 Bastien Nocera 2017-11-15 16:18:13 UTC
Review of attachment 363702 [details] [review]:

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +423,2 @@
                 /* select image */
+                m = 3.0 * level / 100 + 1;

n = ceill (3.0 * level / 100 + 1);
No need for "m".

@@ +426,2 @@
                         n = 1;
+                /* volume above 100% */

"output volume", so the "!is_mic" is clear
Comment 46 Didier Roche 2017-11-16 07:45:01 UTC
Created attachment 363789 [details] [review]
media-keys: Allow volume above 100%

Use the pulseaudio maximum allowed volume if allow-volume-above-100-percent
gsettings key is true.
Pass that information to the OSD and depend on the required
gsettings-desktop-schemas version.
Comment 47 Didier Roche 2017-11-16 07:45:13 UTC
Created attachment 363790 [details] [review]
media-keys: Add optional max_level parameter to the Shell

This option is sent to the Shell so that we can set the maximum level limit
when showing the OSD.
No value corresponds to 100. max_level can only be above 100.
Doesn't impact other calls or if the shell doesn't support that option.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 48 Didier Roche 2017-11-16 07:45:28 UTC
Created attachment 363791 [details] [review]
media-keys: Use similar logic to the Shell to show volume above 100% icon

Shows a volume above 100% icon for output only in case we have a current
level above 100. Use a double to ensure this icon is chosen as soon as
level is above 100.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 49 Didier Roche 2017-11-16 07:47:46 UTC
Here we go, I think this address latest comments. The only things missing for this side of the patches are:
- getting some feedback from Allan on the "triggering" UI side for this (I did refresh, but didn't reattach patch 4)
- getting the svg for the volume above 100% icon, visible once the gsettings key is set to true.
Comment 50 Bastien Nocera 2017-11-16 12:17:51 UTC
Review of attachment 363789 [details] [review]:

Looks good apart from that nit.

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +1152,3 @@
+        g_assert (g_str_equal (settings_key, ALLOW_VOLUME_ABOVE_100_PERCENT_KEY));
+
+        gboolean allow_volume_above_100_percent;

No, that should be above the assert. Declarations first, then code. g_assert() is code.
Comment 51 Bastien Nocera 2017-11-16 12:24:40 UTC
Review of attachment 363790 [details] [review]:

> Doesn't impact other calls or if the shell doesn't support that option.

How does it look when the shell doesn't support the option? Does it just show as 100%? Please add the answer to the commit message.

Marking as needs-work for the code review, though those are nits. We're waiting on the shell code landing anyway so there's time for another round of reviews.

::: plugins/media-keys/gsd-media-keys-manager.c
@@ +1203,3 @@
         const GvcMixerStreamPort *port;
         const char *icon;
+        guint max_volume_pct;

Why do you declare it a uint, but cast it to an int lower down in the code?

@@ +1204,3 @@
         const char *icon;
+        guint max_volume_pct;
+        max_volume_pct = (int) (100 * (double) manager->priv->max_volume / PA_VOLUME_NORM);

Linefeed before this.

(Maybe we could save the percentage directly in manager->priv, given that it's just one of 2 different values, not sure this is a big deal though.)
Comment 52 Bastien Nocera 2017-11-16 12:25:22 UTC
Review of attachment 363791 [details] [review]:

Looks good, pending the shell code being merged.
Comment 53 Didier Roche 2017-11-16 12:52:03 UTC
(In reply to Bastien Nocera from comment #51)
> Review of attachment 363790 [details] [review] [review]:
> 
> > Doesn't impact other calls or if the shell doesn't support that option.
> 
> How does it look when the shell doesn't support the option? Does it just
> show as 100%? Please add the answer to the commit message.

I'll add the answer to the commit message (and indeed, that's what you told, any volume > 100% shows up as 100 when the shell doesn't support the option).



> 
> ::: plugins/media-keys/gsd-media-keys-manager.c
> @@ +1203,3 @@
>          const GvcMixerStreamPort *port;
>          const char *icon;
> +        guint max_volume_pct;
> 
> Why do you declare it a uint, but cast it to an int lower down in the code?

Just following what was done for the "vol" variable to which we compare it to, which is declared as:

guint                vol,

but the assignement is: vol = (int) (100 * (double) vol / PA_VOLUME_NORM);

I can change it to int, but I thought respecting the surrounding code was better, wdyt?

> 
> @@ +1204,3 @@
>          const char *icon;
> +        guint max_volume_pct;
> +        max_volume_pct = (int) (100 * (double) manager->priv->max_volume /
> PA_VOLUME_NORM);
> 
> Linefeed before this.
> 
> (Maybe we could save the percentage directly in manager->priv, given that
> it's just one of 2 different values, not sure this is a big deal though.)


I'll add the linefeed. I think keeping the unormalized max_volume in manager->priv makes logically more sense if you are not opposed to it.
Comment 54 Didier Roche 2017-11-16 14:57:36 UTC
Created attachment 363837 [details] [review]
media-keys: Allow volume above 100%

Use the pulseaudio maximum allowed volume if allow-volume-above-100-percent
gsettings key is true.
Pass that information to the OSD and depend on the required
gsettings-desktop-schemas version.
Comment 55 Didier Roche 2017-11-16 14:57:44 UTC
Created attachment 363838 [details] [review]
media-keys: Add optional max_level parameter to the Shell

This option is sent to the Shell so that we can set the maximum level limit
when showing the OSD.
No value corresponds to 100. max_level can only be above 100.
Old versions of the shell will display a level 100% in the OSD if current
level is above 100 and will ignore the max_level parameter.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 56 Didier Roche 2017-11-16 14:57:53 UTC
Created attachment 363839 [details] [review]
media-keys: Use similar logic to the Shell to show volume above 100% icon

Shows a volume above 100% icon for output only in case we have a current
level above 100.
This new icon will be sent to any Shell version, including older version
which will show up 100 for level and this volume above 100% icon.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 57 Allan Day 2017-11-23 08:44:35 UTC
(In reply to Didier Roche from comment #49)
> Here we go, I think this address latest comments. The only things missing
> for this side of the patches are:
> - getting some feedback from Allan on the "triggering" UI side for this (I
> did refresh, but didn't reattach patch 4)

I guess this is about comment #20, so I'll reply to that below.

> - getting the svg for the volume above 100% icon, visible once the gsettings
> key is set to true.

I've asked the icon gods for this; see bug 790754.

(In reply to Bastien Nocera from comment #20)
> Review of attachment 363476 [details] [review] [review]:
> 
> For me, that's a complete no-no. It's completely hidden, not reversible,
> doesn't respect lockdown on the setting, and probably not required on the
> majority of devices.

The idea is that it would a) only be activated after the confirmation dialog [1] and b) be reversible from the sound settings [2].

If it's true that the majority of devices don't need this behaviour, then you're probably right that this would generate more false than real positives.

> I also don't think it would play well with a high key repeat rate.

I'm not sure that's an issue for a media key, but maybe the point is moot. :)

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/shell/volume-overdrive-confirmation.png
[2] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/sound/sound-over-amplification.png
Comment 58 Allan Day 2017-11-23 08:47:02 UTC
As mentioned in the above comment, I've just pushed a quick mockup for what the setting could look like in the sound panel:

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/sound/sound-over-amplification.png

Note that I've put it as a per output device setting, based on the idea that someone shouldn't end up deafening themselves with their headphones as a result of having crappy laptop speakers. :)
Comment 59 Didier Roche 2017-11-23 13:09:25 UTC
(In reply to Allan Day from comment #57)
> 
> (In reply to Bastien Nocera from comment #20)
> > Review of attachment 363476 [details] [review] [review] [review]:
> > 
> > For me, that's a complete no-no. It's completely hidden, not reversible,
> > doesn't respect lockdown on the setting, and probably not required on the
> > majority of devices.
> 
> The idea is that it would a) only be activated after the confirmation dialog
> [1] and b) be reversible from the sound settings [2].
> 
> If it's true that the majority of devices don't need this behaviour, then
> you're probably right that this would generate more false than real
> positives.
> 
> > I also don't think it would play well with a high key repeat rate.
> 
> I'm not sure that's an issue for a media key, but maybe the point is moot. :)

I don't understand as well about the potential impact that can't be mitigated:
- we can send the DBUS call and have a signal from the Shell telling that the dialog is closed, having g-s-d waiting on this before taking volume up account.
- we can as well have an heuristic for triggering the dialog: 5 consecutives volume up, with a minimum delay between them (avoiding the "I keep up pressed"), and a maximum delay of like 3s between each repetitive call.
Comment 60 Didier Roche 2017-11-23 13:11:30 UTC
(In reply to Allan Day from comment #58)
> As mentioned in the above comment, I've just pushed a quick mockup for what
> the setting could look like in the sound panel:
> 
> https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/
> system-settings/sound/sound-over-amplification.png
> 
> Note that I've put it as a per output device setting, based on the idea that
> someone shouldn't end up deafening themselves with their headphones as a
> result of having crappy laptop speakers. :)

That's a good point, the key is global for now, I'll look how we can have it per devices, storing in a list the device name I guess.
Comment 61 Didier Roche 2018-02-08 09:17:45 UTC
Created attachment 368136 [details] [review]
media-keys: Allow volume above 100%

Use the pulseaudio maximum allowed volume if allow-volume-above-100-percent
gsettings key is true.
Pass that information to the OSD and depend on the required
gsettings-desktop-schemas version.
Comment 62 Didier Roche 2018-02-08 09:18:03 UTC
Created attachment 368137 [details] [review]
media-keys: Add optional max_level parameter to the Shell

This option is sent to the Shell so that we can set the maximum level limit
when showing the OSD.
No value corresponds to 100. max_level can only be above 100.
Old versions of the shell will display a level 100% in the OSD if current
level is above 100 and will ignore the max_level parameter.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 63 Didier Roche 2018-02-08 09:18:11 UTC
Created attachment 368138 [details] [review]
media-keys: Use similar logic to the Shell to show volume above 100% icon

Shows a volume above 100% icon for output only in case we have a current
level above 100.
This new icon will be sent to any Shell version, including older version
which will show up 100 for level and this volume above 100% icon.

See https://bugzilla.gnome.org/show_bug.cgi?id=790280
Comment 64 Didier Roche 2018-02-08 09:18:24 UTC
Created attachment 368139 [details] [review]
media-keys: reset volume max to 100% for current stream when disabled

If the current stream volume is > 100% and the allow over 100% key is set
to false, reset the current stream volume to 100%.
Comment 65 Didier Roche 2018-02-08 09:21:35 UTC
I've resend the whole patch set, rebased on latest master.
In addition:
- it now deps on g-d-schemas 3.27.90, as it's just been released with the new key
- I added a new commit on Allan's request to reset the current output stream to 100% at max if the volume above 100% key is set to false. (Do you mind confirming that using index 0 here is correct to get the stream?)
- As I needed to move the gsettings callback there due to using get_stream_*() and update_dialog(), I rebase a previous commit to move the callback to its final place directly to minimize the base.
Comment 66 Jeremy Bicha 2019-02-11 02:29:40 UTC
I'm closing this bug since it has been merged as https://gitlab.gnome.org/GNOME/gnome-settings-daemon/merge_requests/38

But it doesn't look like "media-keys: reset volume max to 100% for current stream when disabled" was part of that merge request.