[Merged] Power Saver feature

pjft · 5 Jul 2017, 16:56

@Hex Sure.

I'm not sure I'm being clear, though. What I'm saying is that if you have the "UseOMXPlayer" option set to true, you can pretty much keep PS on rather than keeping it off because of VLC. Especially, if you're coming from the angle that this is useful for lower-power devices, they certainly won't be using VLC anyways. Meaning, if that setting is on, you can treat video view as a Detailed view.

And I'm not sure if that's what you're referring to, but I believe we do not support animated GIFs in ES.

Hex · 5 Jul 2017, 17:00

@pjft I am interested in making it default. This means lesser problems to manage in the code/feature. Since i am coding on laptop i didnt account for OMX not needing PS disabled. I would like to test it and then i dont have to make assumptions.

I would say scrolling of description is a small sacrifice for PS. No one reads it anyways.

pjft · 5 Jul 2017, 17:20

@Hex said in Testers needed :: Power Saver features :: PR #172:

I would say scrolling of description is a small sacrifice for PS. No one reads it anyways.

I see your point and the momentum behind this, but I'd rather not make assumptions on behalf of the wide number of users. We have more suggestions of it being the contrary, rather than what you claim:

If it wasn't relevant, probably theme makers wouldn't dedicate that much real estate to it, or at least we'd see more themes without a description;
If it wasn't relevant, people would probably not have noticed that it wasn't scrolling anyway in just this small sample, and wouldn't have voiced their concerns;

The fact that it may or may not matter to the select few that are discussing this right now does not mean that that is the truth or factual. :)

Let's try to keep this conversation and any such decisions data-driven, and - for the lack of data - let's at least try not to break things for users based on assumptions, or without a really good reason behind it other than just we'd want to make it a default.

We either come up with a solution for it to make it a default, or it will likely be a non-default option as it's bringing non-immediately-tangible benefits for a wide audience that has lived without it until now (I'm looking at everyone with a Pi 2 or 3, a Linux box, a Windows-based setup, or people who use this while plugged in), but it's breaking something that the audience has come to expect and rely on - no matter how small we may think it is.

I hate to be on the receiving end when something I dearly use is deemed to be deprecated because there are apparently not enough users using it. Google Reader: I'm looking at you.

Once again: I think this is a great thing to have, and I am extremely supportive of having it. But it is also my personal opinion that there need to be some - for lack of a better word - reasonable product standards for what is made a default or not, or at least that a lot more people are involved in making that decision - project owners included.

Us 5 or 6 in this thread, we are far from that representative quorum, as knowledgeable and involved in this as we may be in this project.

I like consensus-driven approaches - not unanimous decisions as those are rare, but ones where everyone's opinions can be shared and taken into account so that the best decision can be taken as a group and everyone can rally behind it, even if it wasn't their preference.

This is my personal opinion, no particular attachment to it, and happy to be proven otherwise or that several others have different thoughts on the guidance and share them as well.

Hope this helps.

Hex · 5 Jul 2017, 17:32

@pjft I get what you are saying. I would like to clarify what I meant when i said so. PS can be implemented now and this can be worked on next. I am looking into way it can work for all.

If we implement a switch for PS, majority of the users might not even be aware of the improvements as ES doesnt carry a changelog on update.

If we dont have a switch, it will break the description scrolling. What i can do for that is reduce the scrolling timeout.

What would you suggest in this case?

meleu · 5 Jul 2017, 17:41

@Hex said in Testers needed :: Power Saver features :: PR #172:

If we implement a switch for PS, majority of the users might not even be aware of the improvements as ES doesnt carry a changelog on update.

As it is the RetroPie's EmulationStation, the next RetroPie release notes can have a note about it.

Zigurana · 5 Jul 2017, 18:32

@Hex said in Testers needed :: Power Saver features :: PR #172:

@Zigurana said in Testers needed :: Power Saver features :: PR #172:

I've made this comment earlier on the Github thread:

There are several ways to cope with the scrolling text issue. For instance, you could think of linking it with the screensaver time-out. This to me seems the more natural solution, going into PS mode only once the screensaver kicks in.

So you are suggesting that scrolling occur when screensaver starts? that wont help.

No, what I meant is that if it's somehow difficult to combine PS mode with normal usage because of the scrolling text and other animations, it could always be apllied in situations where no such animations occur, i.e. when either of those two screensaver modes is activated.

That way, we also circumvent the video display issue: If you select a dim/black screensaver, you also get PS, if you select video-screensaver, you're better wired to a wall...

Not necessarily. Power savings reduces overhead processing and hence heating of cpu. Even walled devices are helped. BTW ES utilizes 60% on Pi zero while OMX hovers on 10% with video. So you see.

I'm not denying that, and we should strive to create a leaner, less power-hungry, ES whenever possible. It will benefit everyone if we are not heating up the Rpi's unnecessarily.
My remark was intended to adress the possible confusion for users when they have options for both video-screensavers and a Power Saving mode, when these two options are really mutually exclusive (leaving out the OMX argument for now).

All in all I think we should strive to limit the amount of options, especially for features that are supposed to work under-the-hood like this one. If we want to signal it explicitly, we could consider renaming the dim/black screensaver types to "DIM (POWERSAVER)" and "BLACK (POWERSAVER)".

On one hand I do not think combining options is a good way to proceed. On the other I am against increasing options in ES

You say combining options, I say fixing an implementation bug, potayto, potahto 😜.

I think a lot of this hinges on the question if we are able to come up with a PS mode that is functionally equivalent to the current ES. If that's the case, then there is no reason not to enable it by default. If not, users should have the option to switch off the feature, and you are back at the discussion of how to make it into an understandable menu item, given the caveats that that design would have. (Not with video's, sacrifice scrolling text, whatever).

In any case, I still think that an 'invisible' PS enhancement is possible, but it might take some work in the rendering department. That will likely take some more time to mature. In de meantime we should consider 'just' linking your enhancements to the screensaver, as that would be a quick win for those of us running on batteries.

Hex · 5 Jul 2017, 20:39

@pjft said in Testers needed :: Power Saver features :: PR #172:

@Hex Sure.

I'm not sure I'm being clear, though. What I'm saying is that if you have the "UseOMXPlayer" option set to true, you can pretty much keep PS on rather than keeping it off because of VLC. Especially, if you're coming from the angle that this is useful for lower-power devices, they certainly won't be using VLC anyways. Meaning, if that setting is on, you can treat video view as a Detailed view.

And I'm not sure if that's what you're referring to, but I believe we do not support animated GIFs in ES.

@ruckage This information is quite useful. Only libvlc needs PS on. I will implement this

I have also made some progress regarding description not scrolling. How about it scrolling only once?

Sano · 5 Jul 2017, 21:22

@Hex said in Testers needed :: Power Saver features :: PR #172:

I have also made some progress regarding description not scrolling. How about it scrolling only once?

That was @dankcushions suggestion, and I second that :)

Hex · 7 Jun 2017, 00:45

Game description is now scrolling once or more . I have tweaked the timings. So need to test it on all systems before finalizing.

I shall find an elegant solution for this later. At present I have delayed the time taken from the last event to when PS kicks in and reduced time description text waits to scroll.

EDIT : Please update the repo and build again

pjft · 6 Jul 2017, 14:06

Sorry for the delay in getting back here.

I am supportive of a "scroll once, then reset (i.e. go back to the original state) and stop animating" approach. In fact, if there's a way to detect if the description text will even scroll or not (i.e. if it's bigger than the container), I'm sure that in several occasions it will fit it so there's no scrolling involved, which means PS can be on from the start on those cases.

dankcushions · 6 Jul 2017, 14:11

just to clarify, it's not just the descriptions that scroll, but also the game titles, depending on your theme. for example, street fighter games on mame, with their crazy subtitles and version info :) i'm not sure if any other ES elements scroll.

just making sure the game titles don't get left out...

pjft · 6 Jul 2017, 14:15

Agreed, good call. Any text that's larger than the container will scroll, I think.

But if it scrolls once and then stops at the beginning, I don't have much of an issue for the moment.

Hex · 6 Jul 2017, 16:54

Game names were working well before. Descriptions were not. Now they are working but it is a hack as i had some work and needed it tested before that. Can everyone who can test it, GIT pull and then build.

pjft · 6 Jul 2017, 19:05

@Hex I'll look into it in the coming days, if not sooner. Thanks!

dankcushions · 6 Jul 2017, 20:52

tested (pi3). builds and looks fine to me! descriptions and game titles scroll fine. i don't use a video theme, so can't comment on that.

idle cpu went from 6-7% (dimmed) to 3% (dimmed/not)

cyperghost · 7 Jul 2017, 16:40

@Hex
There is still issue with removing favorite by pressing Y button if you are alredy in favorite - even with your newest branch and pjfts newest commit.

All your features... Dimming, Blanking and CPU savings work
Scrolling I didn't test
Gamelist shows number of game entries without pressing button.

I load binary to my github repo: This is a merged branch @pjft favorite and your newest commits (4 times cherry pick). Is there a better way to reload all cherry picks with only one command? I used the command git cherry-pick abcdef12345678790 4 times

https://github.com/crcerror/emulationstation-binary-archive

Hex · 7 Jul 2017, 19:11

@cyperghost you should use git pull --rebase to update your repository. You dont need to cherry pick every time

I have not tested my changes with the branch you are testing. If that PR gets merged first then i can test it. I will not test it on unmerged code.

pjft · 7 Jul 2017, 18:21

@Hex I agree. That's the wise approach to take.

cyperghost · 7 Jul 2017, 20:07

@pjft said in Testers needed :: Power Saver features :: PR #172:

@Hex I agree. That's the wise approach to take.

Is it rather about:

Don't trust a binary compiled by someone else
Don't do working on a fix that may never be needed because the branch isn't merged into main release?
Annother codex or good practice I don't know about it?

@Hex
I think fix is easy ... but needs some work in looking @pjfts routines.
Anyway, I hope that my posting can be treated as alpha test.

ah.. and thanks for helping in using gitcommands

pjft · 7 Jul 2017, 19:23

@cyperghost No, no, don't get this wrong :)

It's just that:
a) he won't have the source code to exactly debug it, by just using the binary -- we trust you that it crashes, but without the exact code, it's a bit of guesswork on his end as he doesn't know what code to look at exactly.
b) given that my current branch (and his branch) are both currently being developed, it may be that any time invested at this stage will be rendered obsolete - either because the bug may disappear, or because it may resurface while we keep developing.

As such, after one of them is merged, it's easier for everyone to have a stable code-base to use for actual runtime debugging. The best he could attempt to do right now would only be to try to replicate it on his current code base, seeing if deleting an entry from the list would trigger the same error. That way he would be able to narrow it down to the code to remove an entry from the list.

But if that doesn't work, he's kind of flying blind :)

Thanks for testing!