tim-janik requested changes on this pull request.
Great work Stefan, thanks for the updates.
I've pointed out a few places that still need work.
> +fast_copy (uint n_values,
+ Data *ovalues,
+ const Data *ivalues)
+{
+ copy (ivalues, ivalues + n_values, ovalues);
+}
+
+template<> void
+fast_copy (uint n_values,
+ float *ovalues,
+ const float *ivalues)
+{
+ Block::copy (n_values, ovalues, ivalues);
+}
+
+#if 0 // <- avoid unused warning
Use BSE_USED or BSE_UNUSED to silence the compiler here.
> +}
+
+static void
+list_jack_drivers (Driver::EntryVec &entries)
+{
+ map<std::string, DeviceDetails> devices;
+ jack_client_t *jack_client = connect_jack();
+ if (jack_client)
+ {
+ devices = query_jack_devices (jack_client);
+ disconnect_jack (jack_client);
+ }
+ else
+ {
+ // should we try to generate and show an error message if connecting
jack failed?
+ }
No, if there's no jack, there are no devices to list. Simple. Stay silent.
> + }
+
+ for (std::map<std::string, DeviceDetails>::iterator di = devices.begin(); di
!= devices.end(); di++)
+ {
+ const std::string &device_name = di->first;
+ DeviceDetails &details = di->second;
+
+ /* the default device is usually the hardware device, so things should
work as expected
+ * we could show try to show non-default devices as well, but this could
be confusing
+ */
+ if (details.default_device)
+ {
+ Driver::Entry entry;
+ entry.devid = device_name;
+ entry.priority = Driver::JACK;
+ entries.push_back (entry);
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.
> + /* the default device is usually the hardware device, so things should
> work as expected
+ * we could show try to show non-default devices as well, but this could
be confusing
+ */
+ if (details.default_device)
+ {
+ Driver::Entry entry;
+ entry.devid = device_name;
+ entry.priority = Driver::JACK;
+ entries.push_back (entry);
+ }
+ }
+}
+
+/* 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 */
+
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.
> + *
+ * Implementation: the synchronization between the two threads is only
+ * implemented by two index variables (read_frame_pos and write_frame_pos)
+ * for which atomic integer reads and writes are required. Since the
+ * producer thread only modifies the write_frame_pos and the consumer thread
+ * only modifies the read_frame_pos, no compare-and-swap or similar
+ * operations are needed to avoid concurrent writes.
+ */
+template<class T>
+class FrameRingBuffer {
+ //BIRNET_PRIVATE_COPY (FrameRingBuffer);
+private:
+ 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 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).
> + * implemented by two index variables (read_frame_pos and write_frame_pos)
+ * for which atomic integer reads and writes are required. Since the
+ * producer thread only modifies the write_frame_pos and the consumer thread
+ * only modifies the read_frame_pos, no compare-and-swap or similar
+ * operations are needed to avoid concurrent writes.
+ */
+template<class T>
+class FrameRingBuffer {
+ //BIRNET_PRIVATE_COPY (FrameRingBuffer);
+private:
+ 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
+ // see the difference
between an empty/full ringbuffer
+ uint n_channels_;
n_channels_ = 0;
> + std::atomic<int> atomic_active_ {0};
+ std::atomic<int> atomic_xruns_ {0};
+ int printed_xruns_ = 0;
+
+ bool is_down_ = false;
+ bool printed_is_down_ = false;
+
+ uint64 device_read_counter_ = 0;
+ uint64 device_write_counter_ = 0;
+ int device_open_counter_ = 0;
+
+ static int
+ static_process_callback (jack_nframes_t n_frames,
+ void *jack_handle)
+ {
+ return static_cast <JackPcmDriver *> (jack_handle)->process_callback
(n_frames);
Use a lambda instead of static_*().
> +
+ if (!p) // first port
+ range = port_range;
+ else
+ {
+ range.min = std::min (range.min, port_range.min);
+ range.max = std::max (range.max, port_range.max);
+ }
+ }
+ return range;
+ }
+ static void
+ static_latency_callback (jack_latency_callback_mode_t mode,
+ void *jack_handle)
+ {
+ static_cast <JackPcmDriver *> (jack_handle)->latency_callback (mode);
Use a lambda instead of static_*().
> + jack_port_set_latency_range (port, mode, &range);
+ }
+ else
+ {
+ jack_latency_range_t range = get_latency_for_ports (output_ports_,
mode);
+ range.min += buffer_frames_;
+ range.max += buffer_frames_;
+
+ for (auto port : input_ports_)
+ jack_port_set_latency_range (port, mode, &range);
+ }
+ }
+ static void
+ static_shutdown_callback (void *jack_handle)
+ {
+ static_cast<JackPcmDriver *> (jack_handle)->shutdown_callback();
Use a lambda instead of static_*(), see below.
> + vector<const float *> silence_buffers
> (output_ringbuffer_.get_n_channels());
+
+ fill (silence_buffers.begin(), silence_buffers.end(), &silence[0]);
+
+ uint frames_written = output_ringbuffer_.write (buffer_frames,
&silence_buffers[0]);
+ if (frames_written != buffer_frames)
+ Bse::warning ("JACK driver: output silence init failed:
(frames_written != jack->buffer_frames)\n");
+
+ }
+
+ /* activate */
+ if (error == 0)
+ {
+ 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);
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.
> + void
+ shutdown_callback()
+ {
+ is_down_ = true;
+ }
+public:
+ explicit JackPcmDriver (const String &devid) : PcmDriver (devid) {}
+ static PcmDriverP
+ create (const String &devid)
+ {
+ auto pdriverp = std::make_shared<JackPcmDriver> (devid);
+ return pdriverp;
+ }
+ ~JackPcmDriver()
+ {
+ }
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().
> + }
+
+ /* setup PCM handle or shutdown */
+ if (error == 0)
+ {
+ flags_ |= Flags::OPENED;
+
+ uint dummy;
+ pcm_latency (&dummy, &dummy); // debugging only: print latency values
+ }
+ else
+ {
+ 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));
Please make this "JACK: %s: ...", devid_, ...
And fix "readable" ;-)
> +
+ /* enable processing in callback (if not already active) */
+ atomic_active_ = 1;
+
+ /* report jack driver xruns */
+ if (atomic_xruns_ != printed_xruns_)
+ {
+ printed_xruns_ = atomic_xruns_;
+ Bse::printerr ("JACK: %d beast driver xruns\n", printed_xruns_);
+ }
+ /* report jack shutdown */
+ if (is_down_ && !printed_is_down_)
+ {
+ printed_is_down_ = true;
+ Bse::printerr ("JACK: connection to jack server lost\n");
+ Bse::printerr ("JACK: -> to continue, manually stop playback and
restart\n");
Please add the devid_ to all printerr calls, I've also fixed this in the ALSA
driver.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/tim-janik/beast/pull/128#pullrequestreview-291347602_______________________________________________
beast mailing list
[email protected]
https://mail.gnome.org/mailman/listinfo/beast
