Ubuntu
unity-settings-daemon package

Merge lp://staging/~yuningdodo/ubuntu/trusty/unity-settings-daemon/unity-settings-daemon.lp1396464-set-volume-to-zero-before-mute into lp://staging/ubuntu/trusty-updates/unity-settings-daemon

  1. Trusty (14.04)
  2. unity-settings-daemon.lp1396464-set-volume-to-zero-before-mute
  3. Merge into trusty-updates
Proposed by Yu Ning
Status: Rejected
Rejected by: Martin Pitt
Proposed branch: lp://staging/~yuningdodo/ubuntu/trusty/unity-settings-daemon/unity-settings-daemon.lp1396464-set-volume-to-zero-before-mute
Merge into: lp://staging/ubuntu/trusty-updates/unity-settings-daemon
Diff against target: 27 lines (+5/-5)
1 file modified
plugins/media-keys/gsd-media-keys-manager.c (+5/-5)
To merge this branch: bzr merge lp://staging/~yuningdodo/ubuntu/trusty/unity-settings-daemon/unity-settings-daemon.lp1396464-set-volume-to-zero-before-mute
Reviewer Review Type Date Requested Status
Yu Ning (community) Disapprove
David Henningsson Pending
Ubuntu Development Team Pending
Review via email: mp+242869@code.staging.launchpad.net

Description of the change

Set volume to zero before mute, otherwise volume will stay in a non-zero value while system is marked as mute. This will cause some issue, such as bug #1396464.

To post a comment you must log in.
Revision history for this message
Sebastien Bacher (seb128) wrote :

Thanks for the work. Do you know if that change applies to gnome-settings-daemon as well? Did you submit if for lp:unity-settings-daemon (the trunk version), it should be fixed in the current version before being SRUed

Revision history for this message
Sebastien Bacher (seb128) wrote :

David, you know a bit that stack, does the issue/fix make sense to you?

Revision history for this message
Yu Ning (yuningdodo) wrote :

Sebastien, thanks very much for the review, I have only tested it on my own trusty box, however I checked the trunk source code just now and I think the patch could be safely applied to trunk.

Revision history for this message
David Henningsson (diwic) wrote :

 > David, you know a bit that stack, does the issue/fix make sense to you?

I need to understand more of the issue to give a proper review, I think. I was able to reproduce the bug here though.

Yu, can you explain your change a bit, and why it solves the problem? What loop are we breaking by just changing the code order?

Revision history for this message
Yu Ning (yuningdodo) wrote :

David, when the bug #1396464 is reproduced I dump the volume value and is_muted by adding some printf() and found is_muted is true while volume is not zero, however to be honest I don't know the root cause, the underlying logic in unity-settings-daemon and unity-control-center is complicated and I didn't find out how this inconsistency finally leads to the loop. My patch is only a toplevel workaround, it didn't fix the internal logic trap, if there is any.

Revision history for this message
David Henningsson (diwic) wrote :

Thanks for the explanation. My fear is just changing the order of things without fully understanding why might solve one race and cause another, i e, maybe there is some other loop that can happen instead.

Therefore I don't want to approve this patch without understanding fully why this happens. Would you like to look into it further, or do you want me to do that?

Revision history for this message
Yu Ning (yuningdodo) wrote :

Yes, I would like to have a try, I'll update my progress on the bug, however since I have little knowledge on audio / u-s-d / u-c-c, no promise on the result. Good luck to me :)

Revision history for this message
Yu Ning (yuningdodo) wrote :

After some further digs I proposed a patch for unity-control-center, please find the details here:
https://code.launchpad.net/~yuningdodo/unity-control-center/unity-control-center.lp1396464-prevent-update-sink-input-in-parallel/+merge/245927

With that patch we don't need to swap the order in u-s-d, I'll close this proposal now.

review: Disapprove

Unmerged revisions

16. By Yu Ning

set volume to zero before mute. (LP: #1396464)

15. By PS Jenkins bot

[ Alberto Milone ]
* gsd-xrandr-manager.c:
  - Add support for mapping the main touchscreen onto the laptop
    display (LP: #1287341).
    This makes sure that the input device knows exactly the area
    that represents the display when the screen configuration
    changes. Note: this doesn't cover the tablet use case.
  - Add support for matching displays with touch input devices
    according to the reported size. This is particularly
    useful on systems that don't use embedded display connectors
    i.e. all-in-one systems such as the Dell Optiplex 9030 AIO.
  - This work is a partial backport of the upstream work on
    touchscreens. When we finally sync with the upstream code
    we can drop this.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: