Bug 331997

Root cause is not from dataengine, somehow system tray is adding media controller twice. Re assigning to system tray.

Is it adding it every second? This bug is primarily about the constant useless dbus traffic. If that is caused by the systray constantly adding the controller, then ok, otherwise please reassign back and file a new bug.

No, the mediaplayer widget polls every 800ms when a media player is active. I don't know if the mpris2 engine supports running without interval set (so basically signal triggering), but that would be the right fix.

Sure it can, MPRIS players always send out signals with every single MPRIS action, which the dataengine can listen to and simply react on it. We have this exact code in KTp kded module and it works perfectly. 

I'll see it fixed then.

Git commit 8d866d047e043b74f3601c074b7df6ef3021433d by Martin Klapetek.
Committed on 12/03/2014 at 23:10.
Pushed by mklapetek into branch 'master'.

Remove polling the mpris dataengine

The dataengine is updating the sources properly so no need to poll it
all the time

M  +1    -2    plasma/generic/applets/mediacontroller/contents/ui/main.qml

http://commits.kde.org/kde-workspace/8d866d047e043b74f3601c074b7df6ef3021433d

Git commit e1b6fa9a6b524ab1d79026d7256d48e1ee73f8e7 by Martin Klapetek.
Committed on 12/03/2014 at 23:14.
Pushed by mklapetek into branch 'master'.

Add a timer that updates the seek slider from our side

The slider was previously moved by the constant polling. Now we move the
slider ourselves using simple timer on our side and watching for the
track's position to change (that's signalled by the player)

The second part of solving the bug 331997

M  +23   -1    plasma/generic/applets/mediacontroller/contents/ui/ExpandedRepresentation.qml

http://commits.kde.org/kde-workspace/e1b6fa9a6b524ab1d79026d7256d48e1ee73f8e7

On Wednesday, March 12, 2014 23:19:31 Martin Klapetek wrote:
> https://bugs.kde.org/show_bug.cgi?id=331997


> --- Comment #6 from Martin Klapetek <mklapetek@kde.org> ---
> Git commit e1b6fa9a6b524ab1d79026d7256d48e1ee73f8e7 by Martin Klapetek.
> Committed on 12/03/2014 at 23:14.
> Pushed by mklapetek into branch 'master'.
> 
> Add a timer that updates the seek slider from our side
> 
> The slider was previously moved by the constant polling. Now we move the
> slider ourselves using simple timer on our side and watching for the
> track's position to change (that's signalled by the player)
> 
> The second part of solving the bug 331997
> 
> M  +23   -1
> plasma/generic/applets/mediacontroller/contents/ui/ExpandedRepresentation.qm
> l

running: root.state == "playing"

should  be 

running: plasmoid.expanded && root.state == "playing"

There's no need to update the slider if the popup isn't open, which is quite a 
likely case, namely default. :)

> 
> http://commits.kde.org/kde-workspace/e1b6fa9a6b524ab1d79026d7256d48e1ee73f8e
> 7

Cheers,

> There's no need to update the slider if the popup isn't open

I totally agree with this. However the slider is updated almost exclusively by the timer. So if the timer is stopped, then when you reopen the plasmoid, the timer will start and the slider will continue moving from the old position.

Alternatively we can query the player for its current position, but that means DBus roundtrip + possibly a visible jumpiness of the slider.

Given how many timers there are around in Plasma, I think one more won't do any harm...

This one does matter, as it wakes up unnecessarily. With your argumentation, we can add timers everywhere. That's obviously not a good idea, and we need to reduce the use of timers, especially those repeating. Really, repeating timers are very evil, and we need to get rid of them. it's not acceptable to have a bit of UI that isn't even on screen wake up the CPU.

I don't see how the progress stops at a certain position and is then wrong, doesn't it trigger, then read the actual position from the engine and set that to the slider? Anything other than that seems highly unreliable to me (i.e. assuming the timer runs all the time). 

The value of the progress slider is set to the value from a dataengine, not updating it when the plasmoid is closed should not have any effect on its position when shown. It will get updated once the popup opens, which seems soon enough. (Well, it could be that the slider needs one "tick" to update, but that's hardly a problem. 
Have you tried and actually saw it breaking with the conditional running?

> it's not acceptable to have a bit of UI that isn't even on screen wake up the CPU.

While generally I agree, keep in mind you have a music playing...from a music player...which causes already so much CPU wakes (the player also have a moving slider plus all sorts of other fanciness). But sure, no need to add more from Plasma...


> I don't see how the progress stops at a certain position and is then wrong, doesn't it trigger, then read the actual position from the engine and set that to the slider?

So what would happen is this:
 1) the timer runs -> the slider is updated every second with new position
 2) the applet gets closed
 3) the timer to update the slider is stopped
   --at this point note that the mpris/dataengine does not provide constant updates of the track position
 4) the applet is reopened
 5) now we need to rerun the timer to get the slider moving again and at the same time we need to ask the player (the mpris dbus interface) what is the actual position of the track so we can put the slider at the proper position
 
This^ will then cause visual jumping of the slider from the old position (when the applet was closed) to the new position (the old position + the time of the track that has passed since closing the popup) because of the time it will take for the dbus roundtrip (applet->datanegine->dbus->reply->dataengine->reply). More below.

> The value of the progress slider is set to the value from a dataengine

That happens only on track change or track seeking, it's not updated constantly. The mpris interface does not spam dbus every second with track position update info. Likewise, the dataengine does not keep the current track position (that would require yet another timer with yet another CPU wakes).

Summary
========
The player + the mpris dataengine already cause really lots of CPU wakes (and they are offscreen), I don't believe one tiny simple timer makes this any worse. But sure, I can change it to stop the timer on popup close, it will just look worse imho. Your call.

Created attachment 85736 [details]
Bug fid

Here's a patch to stop the timer and get updated position on plasmoid popup.

Please test and report back.

Patch looks good. Thanks for taking the time to solve this in a power-friendly way. (Feel free to commit and close this bug.)

For the record, I took the time to measure the "power-friendliness" of the patch.

I played music, run powertop and took 12 measurements (gave it a while to stabilize after executing) of the reported cpu wakeups. There was only plasma-shell and clementine running, no other apps and I was offline in KTp and I didn't touch any input device (the computer was fully idling).

Without the patch, the average wakeups per second is 427.7
With the patch, the average wakeups per second is 430.

For some reason it's even worse with the patch. But in any case, the difference is sooo small that we can hardly talk about this being more power friendly.

For the record2, plasma-shell does not even show in powertop's most wakeups-causing processes (list of ~20).

=====

Anyways, I'd like someone to actually test the patch first and report the behavior of playing a music, opening the applet around beginning of the song, looking at the slider position, closing the applet, waiting for about 3/4 of the song, opening the applet again and observing the slider behavior.

Tested, as far as I can see, it works as advertised.

Git commit af251cc79cc4ce624367867e9747fcd99d78c508 by Martin Klapetek.
Committed on 27/03/2014 at 10:43.
Pushed by mklapetek into branch 'master'.

Stop the slider's timer when the applet is hidden

When the applet gets shown again, we query the player again for the
current position and set the slider to that

M  +11   -1    plasma-workspace/applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
M  +1    -0    plasma-workspace/dataengines/mpris2/mpris2.operations
M  +2    -0    plasma-workspace/dataengines/mpris2/playeractionjob.cpp
M  +1    -1    plasma-workspace/dataengines/mpris2/playercontainer.h
M  +1    -0    plasma-workspace/dataengines/mpris2/playercontrol.cpp
M  +2    -0    plasma-workspace/dataengines/mpris2/playercontrol.h

http://commits.kde.org/kde-workspace/af251cc79cc4ce624367867e9747fcd99d78c508