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

GIG Player Plugin #1234

Merged
merged 33 commits into from Nov 25, 2014

Conversation

Projects
None yet
3 participants
@floft
Copy link
Contributor

commented Oct 23, 2014

For a while I've wished to use the Maestro Concert Grand on linuxsampler.org in LMMS (#786). This piano is in the GIG file format, so I created a GIG player plugin (heavily based on the SF2 player plugin). The graphics aren't all that great and it doesn't support all of features of the format (there are quite a few; this plugin supports the features I could test), but it's a start. This plugin depends on libgig to get the data from the GIG file. Also, I haven't tested this on anything but Linux.

Previously the speed issues I was having were because I was loading the entire sample into memory at the beginning of each note. Now it loads the portions of the note as it renders each set of frames, which allows you to play many notes without glitching even at low latency (in my experience, at least).

Thoughts?

@tobydox

This comment has been minimized.

Copy link
Member

commented Oct 23, 2014

Thank you very much for your contribution! It looks like a very useful addition. I'll do a detailed review later. At a first glance I see you're using STL containers (std::list etc.) - try to avoid that and replace them with Qt's container classes (QList etc.). Also try to avoid locking mutexes in the playNote() functions as this is bad for realtime safety.

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2014

Just changed from std::list to QList.

I will try having playNote() just add the midi note and the velocity to some list which then when processing the frames actually finds the sample to play thus avoiding accessing libgig anywhere except in play() (and loading files etc.). So there will still be locking around the QList in playNote(), but not around nearly as much.

The slight annoyance with libgig is the positions of where you are in the list of regions (to find a sample) and then the position in a sample (to play a sample) are all stored in the objects themselves making it not thread-safe.

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2014

I restructured it somewhat allowing less locking to be done in playNotes() and deleteNotePluginData().

I also discovered that when exporting samples that are at 44.1 kHz to 96 kHz they sound slightly different. I think this has to do with the limited number of frames I'm providing libsamplerate, in this case (44.1k to 96k) only 117 which it resamples into 256. I'm not entirely sure how to fix that, but the SF2 plugin doesn't appear to have this issue.

@tobydox

This comment has been minimized.

Copy link
Member

commented Oct 26, 2014

Thanks again! One more thing before we merge: could you please fix the coding style? I know that Sf2Player still follows our old coding style but we're starting class names etc. upper case and try to avoid underscores at all so it has to be GigInstrument etc. Please also rename the plugin directory from "gig_player" to "GigPlayer".

Concerning the resampling: couldn't you try to render the data at the current processing sample rate and try to avoid resampling at all? Connect to Mixer's sampleRateChanged() signals if neccessary.

@diizy

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2014

On 10/26/2014 11:56 PM, Tobias Doerffel wrote:

Concerning the resampling: couldn't you try to render the data at the
current processing sample rate and try to avoid resampling at all?
Connect to Mixer's sampleRateChanged() signals if neccessary.

I second this - better to avoid resampling.

Resampling audio in realtime is problematic, because every good
resampling algorithm requires some lookahead to work properly. We get
away with resampling samples in the AFP because that doesn't have to be
done in realtime (we can always look up enough frames from the audio
file), but if you don't have any lookahead, then it's going to
inevitably cause some clicks and disturbances in every period.

The only way to avoid these problems is either:

1- use a very simple, 1st-order resampling algorithm, such as linear
interpolation - as it doesn't require almost any lookahead. This is
however problematic because it produces poor sound quality.

2- add enough latency to the resampled stream so we can look ahead
enough to use proper interpolation. This is also problematic because we
don't have any kind of latency compensation mechanism, and is also
somewhat complex to implement (requires ringbuffers and such).

So even the solutions to the problem contain problems. So it's best to
avoid resampling.

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2014

Yes, I can correct the coding style and rename those files. But, I'm a bit confused about the resampling.

For example with this piano, the samples in the GIG file are at 44.1 kHz. If I run Jack at 48k or try exporting it as 48k, then engine::mixer()->processingSampleRate() is different than the sample's sample rate so I was using using src_process (libsamplerate) to convert the rendered buffer (44.1k) to the mixer's sample rate (48k), which seems to be how the SF2 player is doing it (unless I'm reading it wrong).

I'm not quite sure how to get around this resampling since the note in the file is one sample rate and we need the output to be another yet still sound the same.

@diizy

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2014

On 10/27/2014 12:23 AM, Garrett Wilson wrote:

Yes, I can correct the coding style and rename those files. But, I'm a
bit confused about the resampling.

For example with this piano, the samples in the GIG file are at 44.1
kHz. If I run Jack at 48k or try exporting it as 48k, then
engine::mixer()->processingSampleRate() is different than the sample's
sample rate so I was using using src_process (libsamplerate) to
convert the rendered buffer (44.1k) to the mixer's sample rate (48k),
which seems to be how the SF2 player is doing it (unless I'm reading
it wrong).

IIRC the sf2player only resamples if Fluidsynth is unable to output the
requested samplerate.

I'm not quite sure how to get around this resampling since the note in
the file is one sample rate and we need the output to be another yet
still sound the same.

Can't you get libgig to do the resampling for you? Or does it only do
the file decoding, like libsndfile?

Well, with samples, you can sort of cheat. The trick is to take a bit
larger chunk from the sample than you need for the period, so you'll
have that bit of an extra margin when resampling.

You'll probably want to look at AudioFilePlayer and SampleBuffer to see
how it's done.

@floft floft force-pushed the floft:master branch 3 times, most recently from e180030 to ae9b1de Nov 3, 2014

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2014

I think the style issues are fixed.

@diizy libgig will only read the samples, so I'm stuck resampling them. I tried loading more than the number of samples for a period, but the output is still detuned when going from 44.1 to 192 kHz. Also, SRC_ZERO_ORDER_HOLD sounds terrible in all cases I've tried so far (appears to be what is used in live playback). I'm not sure if that's because of my code or just the algorithm. I'll continue playing around with this.

@diizy

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2014

If libgig only reads the samples, why not just integrate support for the
.gig format into Samplebuffer.cpp so that .gig files could be loaded
directly into AFP and sampletracks? Or are there some special features
in .gig files that AFP could not take advantage of?

To elaborate, Samplebuffer.cpp is the container class for samples, and
it contains the code for loading samples via libsndfile, currently it
supports .wav .ogg .flac and .ds files. Seems like the simplest solution
would be to just make Samplebuffer able to load .gig files, then all the
code for playing them would already be there...

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2014

It's a collection of samples for different notes and velocities. The format supports all sorts of additional functionality like round robin or random sample selection, certain keys change which samples are played, other samples played on key release, etc. I thought the soundfont plugin was the closest in functionality, so I based this upon that code.

@diizy

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2014

On 11/07/2014 01:20 AM, Garrett Wilson wrote:

It's a collection of samples for different notes and velocities, so
very similar to soundfonts. Though, the format supports all sorts of
additional functionality like round robin or random sample selection,
certain keys change which samples are played, other samples played on
key release, etc. I thought the soundfont plugin was the closest in
functionality, so I based this upon that code.

Oh, right. Sure, a separate plugin then.

The pitch problems very probably have something to do with rounding
errors... make sure you're calculating the resampling rate correctly.

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2014

Well, that's embarrassing. Rounding instead of integer truncation makes it sound a lot better. Thanks.

@diizy

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2014

On 11/07/2014 08:37 AM, Garrett Wilson wrote:

Well, that's embarrassing. Rounding instead of integer truncation
makes it sound a lot better. Thanks.

Why are you rounding it at all? If you use src for resampling, you can
use float values for the resampling ratio.

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2014

Rounding the number of samples to input to src_process, not the ratio. The ratio is (double)processRate/sample'sRate. But, on second thought, I'm not sure why this appeared to also fix the issue in my code that provides more samples than needed. I was providing an extra margin and saving that margin to use at the beginning of the next period, so I always ended up rendering this same number of new samples (except for the first period). It's as if the libsamplerate ratio also depends on the number of new samples I'm providing.

This may be related to why input_frames_used is always the number of frames provided. In the link below (at the end) Erik Lopo says input_frames_used < input_frames for the case I'm describing where you provide more samples than needed, but then Jean-Yves Avenard says input_frames_used == input_frames for this case, as I'm seeing (except in certain circumstances, e.g. when oversampling by 8x, and then input_frames_used == 0 since the margin is too small). I would expect input_frames_used ~= input_frames - margin, but that is not the case even for enormous margins.

http://comments.gmane.org/gmane.comp.audio.src.general/127

@diizy

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2014

On 11/07/2014 06:21 PM, Garrett Wilson wrote:

Rounding the number of samples to input to src_process, not the ratio.
The ratio is (double)processRate/sample'sRate. But, on second thought,
I'm not sure why this appeared to also fix the issue in my code that
provides more samples than needed. I was providing an extra margin and
saving that margin to use at the beginning of the next period, so I
always ended up rendering this same number of new samples (except for
the first period). It's as if the libsamplerate ratio also depends on
the number of new samples I'm providing.

input_frames and output_frames can be whatever, if you just set
src_ratio properly. Look at Samplebuffer::play() for example.

Remember though that you do have to calculate src_ratio BEFORE you add
the margin to input_frames.

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2014

src_ratio = (double) processRate/sample'sRate (not pushed yet), so it's not dependent upon anything other than the two sample rates, which is why it is odd that rounding the number of new samples (total - margin from last period) provided each period changed the note's pitch.

// How many frames we'll be grabbing from the sample
int samples = frames;

for( QList<GigNote>::iterator note = m_notes.begin(); note != m_notes.end(); ++note )

This comment has been minimized.

Copy link
@diizy

diizy Nov 9, 2014

Contributor

"note" is a class name in LMMS, best to change this variable to something else for now...
"it" is traditionally used for iterators

samples = round( (double) frames * oldRate / newRate );

// Buffer for the resampled data
convertBuf = new sampleFrame[samples];

This comment has been minimized.

Copy link
@diizy

diizy Nov 9, 2014

Contributor

using new in a function that is called every period (such as play() ) is a no-no.

Better to use a stack array:
sampleFrame convertBuf[samples];

This creates the array in the stack, this way we don't do dynamic allocation (as "new" does) which is bad for RT. The syntax for accessing the array is the same: convertBuf[x][y] = z;

This comment has been minimized.

Copy link
@floft

floft Nov 9, 2014

Author Contributor

Should I assume we have extensions for that (gcc, clang, and whatever else has those extensions) or put it in something like #ifdef _GNUC _ as is in the Sf2 Player?

This comment has been minimized.

Copy link
@diizy

diizy Nov 9, 2014

Contributor

On 11/09/2014 07:45 PM, Garrett Wilson wrote:

Should I assume we have extensions for that (gcc, clang, and whatever
else has those extensions)

Go ahead and assume. GCC and Clang are the only compilers used for LMMS
compilation at this moment... if someone wants to use a crappy legacy
compiler, well, that's their problem ;)

This comment has been minimized.

Copy link
@floft

floft Nov 9, 2014

Author Contributor

Sounds good.

if( m_sampleDataSize != samples )
{
delete[] m_sampleData;
m_sampleData = new sampleFrame[samples];

This comment has been minimized.

Copy link
@diizy

diizy Nov 9, 2014

Contributor

same thing here... why not create a buffer at the start which is the maximum size, then use a variable to store the "actual" or used size, this makes it so we don't have to call delete/new more than once, delete/new is bad for RT.

This comment has been minimized.

Copy link
@floft

floft Nov 9, 2014

Author Contributor

By start are you referring to in the constructor or the start of this function? If in the constructor, I can only see how that would work if we assume that we'll only run into samples down to 44.1 kHz samples (or 22.05 kHz or something), the max size should be something like frame_size_max_samplerate_max_oversampling/min_samplerate = 256_192_8/44.1 = 8917. However, I'm not sure there is a minimum supported sample rate. If in this function, I can create it after I determine the sample's sample rate.

This comment has been minimized.

Copy link
@diizy

diizy Nov 9, 2014

Contributor

Well alternatively, you could just use another stack buffer here, since those can be created with a dynamic size... there's a bit of a concern of stack overflow but I don't think it's much of an issue here and it's mainly a problem on Windows...

Btw, minimum samplerate is 44100.

// Save the new position in the sample
sample->pos = sample->sample->GetPos();

// Convert from 16 or 24 bit into 32-bit float

This comment has been minimized.

Copy link
@diizy

diizy Nov 9, 2014

Contributor

these conversion functions would be good to move into lmms_math.h as inline functions, in case we need them elsewhere...

src_data.data_out = &newBuf[0];
src_data.input_frames = oldSize;
src_data.output_frames = newSize;
src_data.src_ratio = (double) newSize / oldSize;

This comment has been minimized.

Copy link
@diizy

diizy Nov 9, 2014

Contributor

ok see here, you have src_ratio as newSize / oldSize... I'm assuming at this point you have already added the margin to oldSize?

input_frames should be oldSize (including margin)
output_frames should be newSize (no margin)
src_ratio should be newSize / ( oldSize without margin )

Otherwise, the margin will mess up the ratio calculation.
If you don't have a margin, that will cause artifacts at every period boundary, because src interpolation needs some lookahead to process...

This comment has been minimized.

Copy link
@diizy

diizy Nov 9, 2014

Contributor

Why not calculate ratio from newRate / oldRate?

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2014

I fixed the "note" variable name issues, now use stack arrays as buffers, and fixed the resampling issues (again). I was making the resampling too complex. I now provide a margin and then increment the sample positions by how many frames it actually used (apparently either 256 or 0 since margin is a multiple of 2), which I believe is the correct way of doing it. Still when converting 44.1k to 192k at 8x oversampling and best interpolation, I get no output frames for one buffer per export (with margin[0] = 128).

Was the conversion code you were referring to adding to lmms_math.h the 24 and 16 bit to float conversions? Something like float int32toFloat( int32_t ) and similarly for int16? I'm guessing libgig stores the data in a particular endianness, which I should also take into account.

@diizy

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2014

On 11/09/2014 10:55 PM, Garrett Wilson wrote:

I fixed the "note" variable name issues, now use stack arrays as
buffers, and fixed the resampling issues (again). I was making the
resampling too complex. I now provide a margin and then increment the
sample positions by how many frames it actually used (apparently
either 256 or 0 since margin is a multiple of 2), which I believe is
the correct way of doing it. Still when converting 44.1k to 192k at 8x
oversampling and best interpolation, I get no output frames for one
buffer per export (with margin[0] = 128).

Took a quick look but can't really find any fault in the code so far...

Have you tried other sample rates? Could be that 192x8 is just too big
for libsrc to handle... IIRC it had some kind of limit for resampling
ratios, might want to look into that. In any case 192x8 is a pretty
absurd sample rate to render in any case, I think we don't really need
to mind such a corner case as long as it works up to, say, 192k.

Was the conversion code you were referring to adding to lmms_math.h
the 24 and 16 bit to float conversions? Something like float
int32toFloat( int32_t ) and similarly for int16?

Yep, however we don't have to worry about that yet - let's first get
things in shape here, that's something that can be done later on.

I'm guessing libgig stores the data in a particular endianness, which
I should also take into account.

Yeah, could be, and also there's signed/unsigned sample formats, which
is a huge difference - using the wrong one gives hell of a distortion...
might want to look it up in the spec.

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2014

It works at all rates and all oversamplings except 192x8 at best interpolation. I think this has to do with the margin size. It would be nice if there was some way to calculate the needed margin size. Also, it sounds dreadful inside LMMS when resampling 44.1k to 48k (as in lots of high-pitch noise) with the default zero order hold resampling. Should I do anything about that? I figured the HQ audio output in the settings would change the interpolation used inside of LMMS, but it doesn't look like it does.

I think I properly deal with endianness now, and I believe the data is signed since libgig loads the 16-bit data into an int16_t array.

@diizy

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2014

On 11/10/2014 08:54 AM, Garrett Wilson wrote:

It works at all rates and all oversamplings except 192x8 at best
interpolation. I think this has to do with the margin size. It would
be nice if there was some way to calculate the needed margin size.
Also, it sounds dreadful inside LMMS when resampling 44.1k to 48k (as
in lots of high-pitch noise) with the default zero order hold
resampling. Should I do anything about that? I figured the HQ audio
output in the settings would change the interpolation used inside of
LMMS, but it doesn't look like it does.

Don't use zero order hold. It's useless for anything except purposefully
getting lo-fi chiptune sounds. Just ignore the interpolation settings
and use linear always. Linear interpolation should also be nice in that
it needs very little margin. But if you really want to get fancy with
it, you could add a switch to the GUI to switch between
linear/sinc/hold, kind of like how AFP does it.

I think I properly deal with endianness now, and I believe the data is
signed since libgig loads the 16-bit data into an int16_t array.

Cool.

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2014

Like using linear interpolation inside LMMS and then use the export sinc interpolations, or should I just ignore the export interpolation setting entirely? Was this export interpolation setting not designed to be used as the resampling interpolation in plugins like I am using it? I was thinking of something like if not sinc, then linear, but always using linear is even simpler.

@diizy

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2014

On 11/10/2014 06:48 PM, Garrett Wilson wrote:

Like using linear interpolation inside LMMS and then use the export
sinc interpolations, or should I just ignore the export interpolation
setting entirely? Was this export interpolation setting not designed
to be used as the resampling interpolation in plugins like I am using
it? I was thinking of something like if not sinc, then linear, but
always using linear is even simpler.

The export setting was probably meant only for resampling after
oversampling, which it is mostly used for, although I can't be sure
because I wasn't around back when they were implemented.

Just go with always linear for now. We can add other interpolation modes
as options later on.

floft added some commits Oct 22, 2014

Stream instead of loading all into memory
Now, when you press a note, it won't have to load the entire sample into
memory before playing the note. This means that now you can play many
more notes without it glitching. Frequently, the entire note sample
isn't played, so before there was a lot of wasted processing time
converting the sample into float and doing sample rate conversions if
needed.

Also, perform sample rate conversion on the final rendered-out version
of all the combined notes for a period. This drastically decreases
processing time.

Note: currently having more than one instance causes glitching
No more reference counts for GIG file instances
Since libgig can't really be used in a multithreaded way unless it was
somewhat rewritten, just use a separate instance of the file for each
new GIG file regardless of if we already have one open in the current
file. Since it's fast now, you can easily have quite a few very large
GIG files open and still have low latency.

Also removed C++11 requirement since I no longer need a move
constructor.
Better ADSR support, fixed some segfaults
Now it supports a simple envelope using attack, decay1, sustain, and
release from the GIG file. I couldn't figure out what amplitude it
should go to after decay2 (if set), so currently that is unused.

It would segfault if you had notes being played and then switched the
instrument since the samples no longer exited. Now it'll delete all
notes when you switch GIG files.
Removed unused images, replaced logo and background
They aren't all that great, but at least now it doesn't say that it's a
soundfont player.
Restructured into GigNotes which contain GigSamples
Now notes are added/removed by locking only a note mutex when pressing
or releasing a note. Then, while processing we actually find and play
the samples using libgig.
Fixed detuned resampling
Apparently the most noticeable detuning issues were caused by rounding
error by integer division.
Fixed resampling issues
When providing extra frames, libsamplerate stores the extras internally
and outputs them in the next period. But, when it has enough internally
to output a whole period, it just outputs the internal buffer while not
using any more input frames. Now I provide some extra frames, check to
see how many frames we used actually used, and update the sample
positions and ADSR accordingly.
Convert 24-bit data if on big endian system
This is needed since libgig returns 24-bit data in a little endian.
Note: untested as I don't have a big endian system.
Exponential decay for release instead of linear
It now sounds much more like the release in Linux Sampler.
Fixed resampling glitches when deleting notes
Moving the code to detect the sample rates of the currently used samples
after the code deleting notes seemed to fix these glitches. Also, fixed a
few ADSR issues that could have resulted in clipping in the attack or
glitching after the release.
Change pitch of notes if PitchTrack is set
Now if a Gig file provides a few samples per octave, it'll change the
pitch of the sample specified for a note instead of just assuming it is
the right pitch.

Also, fixed issue where if attack length was zero the note would never
sound.
Added loop support and fixed fine tunings
Now it'll honor the loop regions specified in the file and it'll
properly use the fine tuning for the samples if specified. Also,
modified the exponential decay code again since it was glitching at the
end of some notes for some reason.
Release only one note on keyup
Previously if you release a C4 then all C4 notes would be released. Now
it stores the pointer to the plugin data which is unique for each key
press and determines which to release based on the matching pointers.

@floft floft force-pushed the floft:master branch from ebaf8fa to 76e182e Nov 18, 2014

@floft

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2014

I rebased on master adding in the few lines to use MemoryManager, now resample the notes individually so that they can be pitch shifted when PitchTrack is set in the Gig file, and added loop support.

@diizy

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2014

On 11/18/2014 07:19 PM, Garrett Wilson wrote:

I rebased on master adding in the few lines to use MemoryManager, now
resample the notes individually so that they can be pitch shifted when
PitchTrack is set in the Gig file, and added loop support.

Nice. I'll take a look at it sometime soon...

floft added some commits Nov 23, 2014

Fixed release samples never being deleted
I removed code in a previous commit that deleted ended samples since
that sometimes caused issues when the samples had loop points. However,
removing the code caused issues with the release samples. Thus, now it
removes ended samples only if they are release samples. Otherwise, the
keyup event and ADSR handle ending the note.

diizy added a commit that referenced this pull request Nov 25, 2014

@diizy diizy merged commit 334a567 into LMMS:master Nov 25, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@floft floft deleted the floft:master branch Nov 25, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.