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

Set 32 for buffersize min value in gui #4336

Merged
merged 3 commits into from Jan 13, 2019

Conversation

bth
Copy link
Contributor

@bth bth commented Apr 29, 2018

Fix #4305

@Wallacoloo
Copy link
Member

Wallacoloo commented May 6, 2018

Could you add something like constexpr int BUFFERSIZE_RESOLUTION = 32; to the top of the file (or a #define if you prefer) and use that instead of rewriting 32 in a bunch of places? Hardcoding the same constant in many places (like what we currently do with 64 here) is pretty error-prone.

@Sawuare Sawuare self-requested a review May 8, 2018 16:33
@JohannesLorenz
Copy link
Contributor

@bth We just discussed in discord that the maximum of this slider should better be set to 2048 (e.g. because zynaddsubfx errors with 8192). Could you please try to code this?

Sawuare
Sawuare previously requested changes May 8, 2018
Copy link
Member

@Sawuare Sawuare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes do not allow setting the buffer size frames to 64 or 160 using a mouse, but they should.

@Wallacoloo
Copy link
Member

What @Sawuare pointed out about 64 and 160 is true. I think there just aren't enough pixels for that fine of resolution.

Right now each slider tick represents 32 samples, but we only expose that resolution for buffer sizes < 256 samples. After that, we round the tick to be a multiple of 8 (i.e. a multiple of 8*32 = 256 samples). This has the effect also of breaking keyboard navigation when above 256 samples (because if you try to advance one tick, from e.g. 256 samples to 288, we forcefully round that back down to 256).

One solution is to make each tick represent each possible setting, rather than each multiple of 32 samples. i.e. the ticks would be {32, 64, 96, ..., 224, 256, 512, 768, ..., 16384} but still be equally spaced pixel-wise. This would simultaneously give more mouse space to the lower values and make the whole range navigable by keyboard. Those sound like good things to me.

Alternatively, reducing the maximum buffer size from 16384 to something smaller does solve the issue of 64 and 160 not being selectable (I verified by limiting it to 8192). However, I'm having trouble seeing how the 2048 value @JohannesLorenz quotes was arrived at. I wouldn't be too concerned if not for this poll, which indicates that 2048 is the most frequently used value, but that people do use larger buffer sizes. There's also the claim that Because ZynAddSubFx only goes up to 4096, we shouldn't go beyond this. But I can't reproduce this on my machine - ZASFX works all the way through 16384 for me.

I really didn't want to derail this PR, but I do want to point out that before choosing to decrease the range, there should be more discussion on that. If people are actually using 4096, 8192, or even 16384 - as that poll suggests - then someone needs to find out why before removing those options. Because if some poor devil gets underruns at every buffer size < 5000, and we limit them to 2048, the tool might become unusable for them.

@JohannesLorenz
Copy link
Contributor

We have opened a poll about buffersizes. I'll close it after the weekend and we can make decisions based on the results.

@JohannesLorenz
Copy link
Contributor

The results are online. Summarizing:

  • 10% have more than 2048
  • 6% have more than 4096
  • The reasons for why more than 4096 were used, and what would happen if they would decrease the number, don't look clear or representative.

I'd say "4K ought to be enough for anyone". Note that users can still enter higher values into their config files, so this is just a restriction in the UI.

Opinions?

@Wallacoloo
Copy link
Member

@JohannesLorenz For the questions where answers were "Other (please specify)", how do we view the answers those people typed?

Also, is there any way to correlate the answers across questions? For example, could I take the 1 person who answered "4097-8192" for buffer size and see what that specific person put for "Why do you use such a high buffersize"? I notice there are three people who used a buffer size > 4096, and also exactly three people who used a sample rate > 44100 - if those are the same people then I think we can conclude that 4096 is an okay limit for people using 44.1k sample rate. Does the survey software you used expose any way to see if these are the same people?

@zonkmachine
Copy link
Member

Tested this with 4k/8k buffer sizes and the slider works well (small netbook screen). Especially with 4k. Listening to a rendered demo track, 32 bit buffer size and 192kHz sample rate. Sounds good.

Touching this PR up by reducing maximum buffer size is OK with me. I'd go with 4k.

One solution is to make each tick represent each possible setting, rather than each multiple of 32 samples. i.e. the ticks would be {32, 64, 96, ..., 224, 256, 512, 768, ..., 16384} but still be equally spaced pixel-wise.

This, but I'd go one step further and reduce the number of possible settings by using a list of values like this {32, 64, 96, 128, 256, 512, 1024, 2048, 4096, 8192, 16384}.

@PhysSong
Copy link
Member

This, but I'd go one step further and reduce the number of possible settings by using a list of values like this {32, 64, 96, 128, 256, 512, 1024, 2048, 4096, 8192, 16384}.

I'd vote for {32, 64, 96, 128, 192, 256, 384, 512, 1024, 1536, 2048, 3072, 4096, 6144, 8192, 12288, 16384}. Your suggestion looks too coarse to me.

@zonkmachine
Copy link
Member

This is now becoming a rather different solution. Do we suggest changes along these lines here or could this be merged with just the suggested change of reducing max buffer to 4k/8k. I'm leaning to the second.

@zonkmachine zonkmachine reopened this Dec 31, 2018
@zonkmachine
Copy link
Member

I suggest merging this with the 4k max fix. It's good for now I think.

@bth
Copy link
Contributor Author

bth commented Jan 4, 2019

I'm a little confused: should I change something on this PR or "4k max fix" will be done in another PR?

@zonkmachine
Copy link
Member

First. Mad sorry about the slow review pace...

should I change something on this PR or "4k max fix" will be done in another PR?

We're looking at solutions for the issue of buffer sizes 64 and 160 not being accessible on dragging the slider. The first solution is to reduce the maximum buffer size to 4096 (or 8192).

m_bufSizeSlider->setRange( 1, 512 );

I suggest that you already now change the range from 512 to 128, commit and push.

There may be a better solution however as mentioned in the last posts above. Instead of an algorithm that computes the values of the steps, we provide an array of values. That could be an interesting solution. It would reduce the number of values to a more usable selection.

Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, works well!

The issue with all values not being accessible when dragging the slider has been fixed. This now works with a maximal buffer size of 4096. If this new limit is an issue with people we can easily rework it later.

@zonkmachine
Copy link
Member

Merge?

@zonkmachine zonkmachine dismissed Sawuare’s stale review January 9, 2019 21:57

Requested changes has been addressed

@zonkmachine
Copy link
Member

If there are no objections I will merge this tomorrow.

@zonkmachine zonkmachine merged commit 68cefc1 into LMMS:master Jan 13, 2019
@bth bth deleted the gui_buffersize_min_32 branch July 18, 2019 16:14
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
This is a bit too low resolution as some values cannot be reached by dragging the slider so we also reduce the maximum buffer size to 4096.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants