Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rbp] Make gui limit default to 720 when memory is limited #5094

Merged
merged 1 commit into from Aug 5, 2014

Conversation

@popcornmix
Copy link
Member

popcornmix commented Jul 23, 2014

On a 256M Pi, it's safest to default to 720p gui as memory is quite limited.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Jul 24, 2014
<constraints>
<minimum>540</minimum>
<minimum>0</minimum>

This comment has been minimized.

Copy link
@jmarshallnz

jmarshallnz Jul 24, 2014

Member

Hmm, does this mean the user can set it to 16 px high? Perhaps the range needs changing a bit to make it less cumbersome to the user (a toggle (auto, 720) may suffice?)

This comment has been minimized.

Copy link
@popcornmix

popcornmix Jul 25, 2014

Author Member

I have seen people reporting using other values (as some compromise between memory usage/performance/quality).
But you might be right that a short fixed list may be better.
I think forcing 720p (even on 512M Pi) and forcing unlimited/1080p (on a 256M Pi where add-ons and network streaming are not used) are valid options in addition to auto setting.

This comment has been minimized.

Copy link
@da-anda

da-anda Jul 26, 2014

Member

can we make this a spinner with just a hand full of options?

  • 480
  • 540
  • 720
  • 900
  • match display resolution

This comment has been minimized.

Copy link
@huceke

huceke Jul 28, 2014

Contributor

Like the idea with the spinner.

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jul 28, 2014

Updated PR to use a spinner for resolutions limits.

@da-anda

This comment has been minimized.

Copy link
Member

da-anda commented Jul 28, 2014

I'm not sure about "unlimited" - it sounds odd - especially because there is still a limit. Does the PI allow resolutions beyond 1080p in case display would support it? I'd probably name it what it is "1080p" and make the "auto" mode more intelligent and only use the actual display resolution in case of enough GPU mem but display is running a smaller res (or is using composite). If it's already limited to display res, all is fine, but i'd still label it 1080

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jul 28, 2014

All the values are upper limits, rather than actual resolution used.
So if 1080 is explicitly chosen, but HDMI mode is 720p then GUI will be at 720p.

I could change the number used by unlimited, from 1080 to any arbitrarily large number (e.g. 2160) and it will behave the same.

1080p is the highest resolution I've ever seen anyone using. I guess with custom modes you could push it higher (I have seen reports of the Pi driving a 4K display, but I'm guessing performance wouldn't be great).

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Aug 1, 2014

jenkins build this please

@MartijnKaijser MartijnKaijser modified the milestones: Pending for inclusion, Helix 14.0-alpha3 Aug 2, 2014
@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Aug 2, 2014

Rebased. jenkins build this please

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Aug 4, 2014

Okay to merge?

@@ -75,6 +76,9 @@ bool CRBP::Initialize()
if (m_gpu_mem < 128)
setenv("V3D_DOUBLE_BUFFER", "1", 1);

if (!CSettings::Get().GetInt("videoscreen.limitgui"))
CSettings::Get().SetInt("videoscreen.limitgui", m_gpu_mem < 128 ? 720:1080);

This comment has been minimized.

Copy link
@jmarshallnz

jmarshallnz Aug 4, 2014

Member

Does this mean that the user can't change it at all, even if they want to, as it'll be overridden next load?

On a 256M Pi, it's safest to default to 720p gui as memory is quite limited.
@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Aug 4, 2014

@jmarshallnz
I've just tested. Initially videoscreen.limitgui is 0 and will get set to 720 or 1080 on first run.
The user can still set it to a value of their choice, and that will be respected.
Setting it to auto will mean videoscreen.limitgui will be 0 on next boot, and will be set to 720 or 1080 as appropriate.

So, nothing too bad happens, but it won't stay permanently on auto. This means switching an sdcard from a Pi with less memory to one with more memory won't magically alter the resolution.

So, I've updated to avoid using the setting for the automatically chosen resolution. This means that the setting stays on auto, and the resolution is chosen on each boot. I agree that is better behaviour.

jenkins build this please

@jmarshallnz

This comment has been minimized.

Copy link
Member

jmarshallnz commented Aug 4, 2014

Sounds good - feel free to merge once built.

popcornmix added a commit that referenced this pull request Aug 5, 2014
[rbp] Make gui limit default to 720 when memory is limited
@popcornmix popcornmix merged commit f88f66b into xbmc:master Aug 5, 2014
1 check passed
1 check passed
default Merged build #1153 succeeded in 1 hr 45 min
Details
@popcornmix popcornmix deleted the popcornmix:limitres branch Aug 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.