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

Jack driver new api #128

Merged
merged 7 commits into from Sep 23, 2019

Conversation

@swesterfeld
Copy link
Collaborator

swesterfeld commented Sep 20, 2019

Ok, here is a port of the Jack driver (#31) to the new driver API. I tested playback and also stereo-through. This doesn't yet have midi support.

swesterfeld added 6 commits Sep 19, 2019
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
For instance for synchronizing the play position pointer ui with the audio
buffering, you want to know how much write latency you have, but don't care
about the amount of read latency.

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Copy link
Owner

tim-janik left a comment

Great work Stefan, thanks for the updates.
I've pointed out a few places that still need work.

Block::copy (n_values, ovalues, ivalues);
}

#if 0 // <- avoid unused warning

This comment has been minimized.

Copy link
@tim-janik

tim-janik Sep 20, 2019

Owner

Use BSE_USED or BSE_UNUSED to silence the compiler here.

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld Sep 21, 2019

Author Collaborator

Done.

else
{
// should we try to generate and show an error message if connecting jack failed?
}

This comment has been minimized.

Copy link
@tim-janik

tim-janik Sep 20, 2019

Owner

No, if there's no jack, there are no devices to list. Simple. Stay silent.

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld Sep 21, 2019

Author Collaborator

Ok, removed the else branch.

Driver::Entry entry;
entry.devid = device_name;
entry.priority = Driver::JACK;
entries.push_back (entry);

This comment has been minimized.

Copy link
@tim-janik

tim-janik Sep 20, 2019

Owner

Two Driver::Entry structures must never have the same priority. That's why we have predefined priority constants for drivers, cards, devices, subdevices. Also, we need the other Entry fields to be filled to allow GUI selection. For now, it doesn't matter too much how to match name/blurb/status fields to extra info, each is just another line in my current UI prototype, but we need something to display devices to the user that make them humanly identifyable.

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld Sep 21, 2019

Author Collaborator

There are never two Driver::Entry structures here. There is a loop, so you might expect it, but basically the code picks the only device which it considers to be the "default" device from the devices map. It used generate a list of devices, but I think thats more harmful than it helps, but I left the devices query and generate list code there, in case we want to do it differently.

Jack is not very descriptive. Typically we only get the client name "system" and the port names "playback_1", "playback_2",... and we know that it is a hardware device. So ok, I'm trying to give all that information to the user in my new version, but still, we for instance do not get a soundcard name or anything useful. There is a Jack Metadata API which could be used to attach descriptions to clients, but at least on my system, this returns nothing. So really what we have is "system", "is hardware device", "2in 2out", nothing really useful for the user.


/* macro for jack dropout tests - see below */
#define TEST_DROPOUT() if (unlink ("/tmp/drop") == 0) usleep (1.5 * 1000000. * buffer_frames_ / mix_freq_); /* sleep 1.5 * buffer size */

This comment has been minimized.

Copy link
@tim-janik

tim-janik Sep 20, 2019

Owner

I know this is disabled, atm, but please still make this /tmp/bse-dropout (or similar) so we're not stamping over namespaces if this is activated on purpose or by accident.

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld Sep 21, 2019

Author Collaborator

Ok, done.

vector<vector<T> > channel_buffer_;
std::atomic<int> atomic_read_frame_pos_;
std::atomic<int> atomic_write_frame_pos_;
uint channel_buffer_size_; // = n_frames + 1; the extra frame allows us to

This comment has been minimized.

Copy link
@tim-janik

tim-janik Sep 20, 2019

Owner

This is C++17, always and unconditionally initialize all struct members that don't have a ctor. If in doubt, just add =0, =false, =EnumType(0).

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld Sep 21, 2019

Author Collaborator

Of course. Fixed. Seems the jack driver predates C++11.

static void
static_shutdown_callback (void *jack_handle)
{
static_cast<JackPcmDriver *> (jack_handle)->shutdown_callback();

This comment has been minimized.

Copy link
@tim-janik

tim-janik Sep 20, 2019

Owner

Use a lambda instead of static_*(), see below.

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld Sep 21, 2019

Author Collaborator

Done.

{
jack_set_process_callback (jack_client_, static_process_callback, this);
jack_set_latency_callback (jack_client_, static_latency_callback, this);
jack_on_shutdown (jack_client_, static_shutdown_callback, this);

This comment has been minimized.

Copy link
@tim-janik

tim-janik Sep 20, 2019

Owner

Note that lambdas that don't take context arguments boil down to normal functions that can be used as function pointer. For example, here is how it looks when you replace the static_* wrappers with lambda:

jack_on_shutdown (jack_client_, [] (void *p) {
  static_cast<JackPcmDriver*> (p)->shutdown_callback();
}, this); 

This could also be a one liner if you have a wider terminal.

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld Sep 21, 2019

Author Collaborator

Fixed. Interesting, didn't know that this would work.

}
~JackPcmDriver()
{
}

This comment has been minimized.

Copy link
@tim-janik

tim-janik Sep 20, 2019

Owner

The dtor should release all resources, e.g. call close(), either unconditionally, or with if (jack_client_). It may also be executed without open() being called previously, or after a failing open() without close().

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld Sep 21, 2019

Author Collaborator

Right. Fixed.

disconnect_jack (jack_client_);
jack_client_ = nullptr;
}
JDEBUG ("JACK: opening PCM \"%s\" readupble=%d writable=%d: %s", devid_.c_str(), readable(), writable(), bse_error_blurb (error));

This comment has been minimized.

Copy link
@tim-janik

tim-janik Sep 20, 2019

Owner

Please make this "JACK: %s: ...", devid_, ...
And fix "readable" ;-)

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld Sep 21, 2019

Author Collaborator

Fixed.

bse/driver-jack.cc Outdated Show resolved Hide resolved
@swesterfeld

This comment has been minimized.

Copy link
Collaborator Author

swesterfeld commented Sep 21, 2019

Ok, last commit should have all the fixes you requested.

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@swesterfeld swesterfeld force-pushed the swesterfeld:jack-driver-new-api branch from f0c4879 to a960497 Sep 21, 2019
@tim-janik tim-janik merged commit a960497 into tim-janik:master Sep 23, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.