image-source: Make maximum memory usage a source property#6264
Conversation
RytoEX
left a comment
There was a problem hiding this comment.
Per our Contributing guildelines, please make these changes:
- Use the correct commit prefix, in this case "image-source: ".
f555874 to
c2affb7
Compare
👍🏻 done, ty for reviewing :) |
| SlideShow.NextSlide="Next Slide" | ||
| SlideShow.PreviousSlide="Previous Slide" | ||
| SlideShow.HideWhenDone="Hide when slideshow is done" | ||
| SlideShow.MaxMemoryUsage="Maximum Memory Usage (megabytes)" |
There was a problem hiding this comment.
| SlideShow.MaxMemoryUsage="Maximum Memory Usage (megabytes)" | |
| SlideShow.MaxMemoryUsage="Maximum VRAM Usage" |
There was a problem hiding this comment.
Maybe even better: "Maximum Graphics Memory Usage"?
I think "VRAM" is an abbreviation that might already be confusing to non-programmers/non-gamers.
There was a problem hiding this comment.
"Maximum GPU Memory Usage" sounds clearer to me. "Graphics" is pretty abstract, while this directly refers to the hardware in question, just like you would say "CPU Usage" and not "Compute usage".
There was a problem hiding this comment.
👍 updated to "Maximum GPU Memory Usage"
| file_filter, path.array); | ||
| dstr_free(&path); | ||
|
|
||
| obs_properties_add_int(ppts, S_MAX_MEMORY_USAGE, T_MAX_MEMORY_USAGE, 0, |
There was a problem hiding this comment.
Rather than (megabytes) in the displayed string, use obs_property_int_set_suffix() to display it in the input (ideally as " MB").
There was a problem hiding this comment.
Oh, cool, added the implementation :)
d579b20 to
b76aa28
Compare
Moves the maximum memory usage setting of the slide show source to a per-source configurable property. The current default of 400MB is maintained.
b76aa28 to
ad0aee3
Compare
|
Hi, if someone wouldn't mind taking a look at this again, it would be much appreciated :) I have made the requested changes and think this patch overall might alleviate some slideshow source-related spam. |
|
PRs are reviewed as time allows. We have weekly meetings to go over new and pending PRs in the order they are submitted, and in the priority and urgency we assign to them. No need to remind us, we know this is here. :) |
|
Oh, gotcha, thank you 👍🏻 👍🏻 |
|
Thank you for doing this an0ndev, I'm hoping this temporary change can be implemented so I can stop using Wallpaper Engine to display fan-art slideshows because I hit the hardcoded memory limit after just a dozen or so images. |
|
any chance somebody could review this? it causes big issues in my workflow (church services with slides), a quick look would be greatly appreciated. thank you :) |
|
This is no longer necessary because the slideshow has been rewritten to allow as many files as you'd like: |
Description
Moves the maximum memory usage setting to a per-source configurable
property. The current default of 400MB is maintained:
Motivation and Context
Partial fix for a pinned issue, #3366.
A redesign is complete but needs review. This is a much more minor change that should make the current version
of the slideshow source more user-friendly until the redesign can be reviewed.
How Has This Been Tested?
Tested on Ubuntu 21.10 from upstream master.
Changing the limit through the UI changes the number of images that successfully load.
Types of changes
Checklist: