D22316: Avoid garbling the sound if the volume slider is moved fast

[kezi]

kezik updated this revision to Diff 63008.
kezik added a comment.


  I talked with 4-5 people and they all said that giving real time feedback in the slider is useless and bad, so here's that, the feedback is given on mouse release, and every problem is automatically solved.
  
  If you want more technical details about the alternative version, read the rest of this comment, otherwise just ignore, now everything is fine.
  
  **useless part of the comment:**
  
  In D22316#505584 <https://phabricator.kde.org/D22316#505584>, @anthonyfieroni wrote:
  
  > updateTimer uses slider value and it takes 200 ms to react, so you can sync the timer with keyboard one by increase the interval https://phabricator.kde.org/source/plasma-pa/browse/master/applet/contents/ui/ListItemBase.qml$198
  
  
  I'm not sure if I am understanding this, the timer seems to only provide feedback to the slider .. ?
  
  The main issue here is inside libcanberra (or pulseaudio): in some conditions (I found that the bug occurs mostly if no other sound is playing) ca_context_playing simply does not work correctly, i'm fairly sure that my patch is correct, I tested libcanberra independently and I can confirm that "bug"
  
  I tested that by setting CA_PROP_CANBERRA_CACHE_CONTROL to "never", the problem seems to go away, that's probably suboptimal but it's the only decent workaround I can come up with
  
    diff --git a/applet/contents/ui/ListItemBase.qml b/applet/contents/ui/ListItemBase.qml
    index d962eb5..6bdbcb7 100644
    --- a/applet/contents/ui/ListItemBase.qml
    +++ b/applet/contents/ui/ListItemBase.qml
    @@ -190,6 +190,10 @@ PlasmaComponents.ListItem {
                                     // whereas PA rejected the volume change and is
                                     // still at v15 (e.g.).
                                     updateTimer.restart();
    +
    +                                if (type == "sink") {
    +                                    playFeedback(Index);
    +                                }
                                 }
                             }
     
    diff --git a/src/qml/volumefeedback.cpp b/src/qml/volumefeedback.cpp
    index 69bc260..3e5abae 100644
    --- a/src/qml/volumefeedback.cpp
    +++ b/src/qml/volumefeedback.cpp
    @@ -55,9 +55,9 @@ void VolumeFeedback::play(quint32 sinkIndex)
     
         // NB Depending on how this is desired to work, we may want to simply
         // skip playing, or cancel the currently playing sound and play our
    -    // new one... for now, let's do the latter.
    +    // new one... for now, let's do the former.
         if (playing) {
    -        ca_context_cancel(context, cindex);
    +        return;
         }
     
         char dev[64];
    @@ -70,7 +70,7 @@ void VolumeFeedback::play(quint32 sinkIndex)
             cindex,
             CA_PROP_EVENT_DESCRIPTION, "Volume Control Feedback Sound",
             CA_PROP_EVENT_ID, "audio-volume-change",
    -        CA_PROP_CANBERRA_CACHE_CONTROL, "permanent",
    +        CA_PROP_CANBERRA_CACHE_CONTROL, "never",  // XXX workaround a libcamberra bug
             CA_PROP_CANBERRA_ENABLE, "1",
             nullptr
         );

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22316?vs=62863&id=63008

REVISION DETAIL
  https://phabricator.kde.org/D22316

AFFECTED FILES
  applet/contents/ui/ListItemBase.qml

To: kezik, #plasma, drosca, davidedmundson, ngraham
Cc: anthonyfieroni, filipf, ngraham, davidedmundson, plasma-devel, kezik, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190802/0bb51f48/attachment.html>