Bug 308 - SampleManager prohibits a sample to be used by consumer more than once at a time
Summary: SampleManager prohibits a sample to be used by consumer more than once at a time
Status: RESOLVED FIXED
Alias: None
Product: LinuxSampler
Classification: Unclassified
Component: other (show other bugs)
Version: SVN Trunk
Hardware: PC Windows 10
: P5 enhancement
Assignee: Christian Schoenebeck
URL:
Depends on:
Blocks:
 
Reported: 2019-02-28 15:00 CET by Jacek Roszkowski
Modified: 2019-03-07 13:41 CET (History)
0 users

See Also:


Attachments
Patch (1.66 KB, application/octet-stream)
2019-02-28 15:00 CET, Jacek Roszkowski
Details
sfz file (68 bytes, application/octet-stream)
2019-02-28 15:01 CET, Jacek Roszkowski
Details
Sample file (675.96 KB, audio/flac)
2019-02-28 15:01 CET, Jacek Roszkowski
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Roszkowski 2019-02-28 15:00:42 CET
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?
Comment 1 Jacek Roszkowski 2019-02-28 15:01:37 CET
Created attachment 92 [details]
sfz file
Comment 2 Jacek Roszkowski 2019-02-28 15:01:56 CET
Created attachment 93 [details]
Sample file
Comment 3 Christian Schoenebeck 2019-02-28 16:32:36 CET
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.
Comment 4 Christian Schoenebeck 2019-03-03 19:50:17 CET
Looking again at your suggested changes, I wonder have you tested whether your changes might introduce a potential memory leak?
Comment 5 Jacek Roszkowski 2019-03-04 11:19:30 CET
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.
Comment 6 Christian Schoenebeck 2019-03-07 13:41:16 CET
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!