| Summary: | SampleManager prohibits a sample to be used by consumer more than once at a time | ||
|---|---|---|---|
| Product: | LinuxSampler | Reporter: | Jacek Roszkowski <j.roszk> |
| Component: | other | Assignee: | Christian Schoenebeck <cuse> |
| Status: | CLOSED FIXED | ||
| Severity: | enhancement | ||
| Priority: | P5 | ||
| Version: | SVN Trunk | ||
| Hardware: | PC | ||
| OS: | Windows 10 | ||
| Attachments: |
Patch
sfz file Sample file |
||
Created attachment 92 [details]
sfz file
Created attachment 93 [details]
Sample file
That SampleManager class was written by Grigor, so I can't say for sure why he implemented it in this way. On gig engine side this class is not used at all, the gig engine only uses ResourceManager which behaves like you suggested. And yes, I agree with you, from my perspective something should either a) be a consumer *and* having the respective resource "in use", or b) if the entity is not using the resource, then that entity should not be registered as a consumer of that resource. Now I haven't really reviewed the sfz engine's code for knowing it for sure, but I think it has to do with a specific feature in the sampler: delayed releasing an instrument/file/samples; when you press and hold a chord on the keyboard, and then switch to another instrument while still holding the chord, you will notice that the sampler will not kill those voices, they still play. And only once you released the last note on they keyboard the sampler will finally free the instrument. This delayed release is done by the disk thread, which calls HandBackRegion() for releasing the respective samples no longer in use. In general you can ask questions here or on the mailing list. Do whatever you find appropriate. But note that much more people will read and listen to what you write on the mailing list then on this bug tracker. Looking again at your suggested changes, I wonder have you tested whether your changes might introduce a potential memory leak? For now I'm running linuxsampler in developer mode with --enable-dev-mode and --enable-debug-level=5 and my conclusion based on htop program and debug messages is that memory is freed correctly, but I haven't tested such advanced features as delayed instrument release. I just applied your patch for now (SVN r3492). I only added some more comments to potentially consider to revise the current layout of the SampleManager class in case new problems appear in future (e.g. memory leaks). Thanks for your patch! |
Created attachment 91 [details] Patch Current implementation of SampleManager makes use of std::set of consumers to determine whether a sample is "InUse". This container type doesn't hold multiple instances of the same value, so one sample can be used by multiple regions, but only once per region at a time. When one sample is being used by a region more than once at a time, it's being closed too early Steps to reproduce: 1. SET VOICES 1 # (easier to reproduce) 2. LOAD INSTRUMENT crash.sfz 3. Play sample (key 49) 4. During playback, play it again (key 49) Result: Sample is being played back twice Console: SampleFile::SetPos() crash1_OH_FF_1.flac not opened Expected result: Play it only once My solution is to use std::multiset instead of std::set to keep track of multiple uses of the sample by the same consumer and close the sample after last playback is finished. By the way: why does SampleManager keep track of sample consumers in separate std::map<S*, std::set<C*> > sampleMap? In other words: why does a separate concept of samples being "in use" (which means adding consumers) exist if there's already a map of sample consumers anyway? Is it ok to ask it here, or should I post it to the mailing list?