Re: [tim-janik/beast] Jack driver (#31)



configure.ac: work the "Output summary message" into a different patch set, that's to be discussed seperately form a jackd driver. And for what's worth, if we do that, I'd want a format similar to what Rapicorn has, and also add a lot more information about other packages there. I.e. that's why it should become a seperate discussion. And, IMHO, software should never command a user the way it's done at the end of the summary, that's can easily feel insulting.

I've suggested a patch for a configure summary message here: #33

Once this is merged, we can rebase this branch.

In the comment on "class FrameRingBuffer", add a brief summary in the first line, our doxygen config extracts one-liner briefs from the first line. Same for the other function comments, Add a brief one-liner, before starting with @returns ;-)

Done. Although the class is in an anonymous namespace, so doxygen will probably ignore it.

Sigh, emacs suckage, please insert a space between angel brackets when writing "vector". C++11 can handle it fine but my emacs highlighter still trips up on << >>.

Done.

guint, gfloat, etc. We've stopped using the useless glib type aliases years ago. The only case this actually helps (arguably) is: gchar string_to_free_via_glib = g_strdup ("..."); So you know to use g_free on the gchar later on. In any case we definitely don't use these in newly written code, so please adjust by using uint and fix/squash your commit into the first one introducing the types.

We've got quite a bit of history here, so it wasn't easy, but I've eliminated guint and friends.

Remove TEST_DROPOUT (unused)

I've documented what it does instead, it is used to create dropouts in the driver, from which it should recover properly, I've used this during development.

Move to top of file: define PDEBUG - and, since this is jack, it should be JDEBUG ;-)

Done.

Using '%' is notoriously slow (it's actually a DIV for the CPU), is there a way the ring buffer is likely to be useful with power-of-2 values only? We could use a bit mask instead of '%' then.

Well, its not a huge problem, as % it is only called once per block size, so I believe if you were to profile the jack driver, practically no time would be spend here, but hopefully somewhere in the synthesis.

We do have non-power-of-two values, because bse itself has non-power-of-two engine blocks. In fact for jack we could do better on that side, i.e. if the jack callback is 64 values, then our engine block size should probably be 64 values as well. At least I believe that is what everybody else is doing.

However, the ring buffer requires one extra sample space to distinguish between a full and empty buffer. So what we could do is allocate the next power of two as ringbuffer size. As we're only speaking of a speculative optimization that I believe would not make any difference at all here, I'm not particularily keen on changing things, though. It works as it is, is tested, and I don't think it really needs to be changed, or would make a measurable difference anyway.

Add the actual values to a warning like this: Bse::warning ("JACK driver: ring buffer size not correct: (jack->buffer_frames != buffer_frames)\n"); I.e. add ": (%u != %u)", so if we get a user report, we can see what went wrong, not just where.

Done.

ctors: DeviceDetails() : ports (0), input_ports (0), output_ports (0), etc - Please use direct field initializers in newly written code, i.e. uint ports = 0;

Done.

While you're at it, also always rebase to latest master and do a non-linear push.

Done.

Finally, you have a bunch of trailing whitespaces, please remove. Emacs has a command here, there's probably something in vim as well that can display/kill trailing whitespaces.

Fixed.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]