Bug 251125 - audio/jack: update to jack2 or add new port audio/jack2
Summary: audio/jack: update to jack2 or add new port audio/jack2
Status: New
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-multimedia (Nobody)
URL: https://github.com/jackaudio/jackaudi...
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-14 04:20 UTC by VVD
Modified: 2021-01-09 15:18 UTC (History)
6 users (show)

See Also:
bugzilla: maintainer-feedback? (multimedia)


Attachments
jack.diff (37.95 KB, patch)
2020-11-24 15:04 UTC, Goran Mekić
no flags Details | Diff
jackdbus.log (2.78 KB, text/plain)
2020-11-24 15:55 UTC, Goran Mekić
no flags Details
jackdbus-dump-values.log (1.83 KB, text/plain)
2020-11-25 12:40 UTC, Goran Mekić
no flags Details
/etc/rc.conf.d/virtual_oss (152 bytes, text/plain)
2020-11-25 12:42 UTC, Goran Mekić
no flags Details
jack-port-no-opus-and-readline.diff (37.61 KB, patch)
2020-11-27 16:55 UTC, Goran Mekić
no flags Details | Diff
jack-no-opus-and-readline.diff (37.61 KB, patch)
2020-11-29 13:32 UTC, Goran Mekić
no flags Details | Diff
Fix 24bit sample handling in FreeBSD (3.00 KB, patch)
2020-11-29 23:59 UTC, Florian Walpen
no flags Details | Diff
jackdbus log (wrong buffer calculation but jackdbus works) (5.32 KB, text/plain)
2020-11-30 11:33 UTC, Goran Mekić
no flags Details
jack-no-opus-and-readline.diff (37.46 KB, patch)
2020-12-02 23:38 UTC, Goran Mekić
no flags Details | Diff
Handle non-power-of-2 sized buffers on the OSS side (5.31 KB, patch)
2020-12-04 01:55 UTC, Florian Walpen
no flags Details | Diff
jack-no-opus-and-readline.diff (37.46 KB, patch)
2020-12-05 11:46 UTC, Goran Mekić
no flags Details | Diff
jackdbus log with standard values and ardour session (37.43 KB, text/plain)
2020-12-05 11:48 UTC, Goran Mekić
no flags Details
jack-transport-problem.diff (37.80 KB, patch)
2020-12-05 19:36 UTC, Goran Mekić
no flags Details | Diff
Fix port always build classic jackd (1.42 KB, patch)
2020-12-06 15:41 UTC, Florian Walpen
no flags Details | Diff
Fix port, always build jackd with rc service (2.74 KB, patch)
2020-12-06 18:13 UTC, Florian Walpen
no flags Details | Diff
jack.diff (36.74 KB, patch)
2020-12-07 18:10 UTC, Goran Mekić
no flags Details | Diff
jack.diff (37.24 KB, patch)
2020-12-09 18:52 UTC, Goran Mekić
no flags Details | Diff
jack.diff (37.08 KB, patch)
2020-12-09 18:55 UTC, Goran Mekić
no flags Details | Diff
jack.diff (37.05 KB, patch)
2020-12-12 21:47 UTC, Goran Mekić
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description VVD 2020-11-14 04:20:41 UTC
1.9.16 released this 28 days ago:
https://jackaudio.org/downloads/
https://github.com/jackaudio/jack2/releases

Upstream recommends to use jack2 instead jack1:
> Which one should I choose?
> Use JACK2, as that is one being actively worked on and maintained.

P.S. Need patch.
Comment 1 Jan Beich freebsd_committer 2020-11-14 07:03:30 UTC
Try https://github.com/DankBSD/ports/commit/ee428f545c1b
Comment 2 Hans Petter Selasky freebsd_committer 2020-11-14 13:26:54 UTC
Are our OSSv4 patches in JACK2 ?

--HPS
Comment 3 Goran Mekić 2020-11-24 15:04:31 UTC
Created attachment 219927 [details]
jack.diff

I just took https://github.com/DankBSD/ports/tree/lite/audio/jack and made a git patch out of it. It works incredibly more stable than jack1 on my laptop and my studio machine.

It would be nice to have the patches upstreamed, but at this state where jack1 just fails to work with ardour6, I have no choice but to use this patch for myself. Can we properly upstream this patch? I mean, legaly, respecting author and other possible non-technical issues?
Comment 4 Hans Petter Selasky freebsd_committer 2020-11-24 15:09:46 UTC
Hi Goran,

Can you quickly test all bit-types with JACK2, 16-, 24-, and 32-bit ?

That's all I have in mind. If that works we are good as I know.

--HPS
Comment 5 Jan Beich freebsd_committer 2020-11-24 15:11:18 UTC
Greg, do you plan to update your jack2 port to 1.9.16? See also questions in comment 2 and comment 3.
Comment 6 Hans Petter Selasky freebsd_committer 2020-11-24 15:13:02 UTC
If it doesn't have support for OSS it will break my audio setup at least, because I use a custom OSS device for the 4 channels of JACK interfaces. I think ALSA routes everything through /dev/dsp :-(
Comment 7 Goran Mekić 2020-11-24 15:55:09 UTC
Created attachment 219928 [details]
jackdbus.log

That was premature. It's a promising patch, but needs more work. I tried to run it with 18 channels, and it failed. I'm attaching a log of first, running jack with /dev/dsp and then /dev/vdsp. My virtual_oss conf is:

virtual_oss_enable="YES"
virtual_oss_dsp="-T /dev/sndstat -S -i 8 -C 18 -c 18 -r 48000 -b 32 -s 512 -f /dev/dsp0 -c 2 -d dsp -c 18 -d vdsp -t vdsp.ctl"
Comment 8 Greg V 2020-11-25 01:39:40 UTC
> Are our OSSv4 patches in JACK2 ?

It's a complete rewrite in C++, no exact patches, but it might have the code you want? I have no idea what to look for, you can compare here:

https://github.com/jackaudio/jack1/blob/master/drivers/oss/oss_driver.c
vs https://github.com/jackaudio/jack2/blob/develop/solaris/oss/JackOSSDriver.cpp

> Can we properly upstream this patch?

The branch referenced in the port is in fact a pull request: https://github.com/jackaudio/jack2/pull/400

Feel free to rebase and further improve the patches, I'm not all that interested in that currently
Comment 9 Goran Mekić 2020-11-25 12:40:56 UTC
Created attachment 219959 [details]
jackdbus-dump-values.log

I dumped the values like this: https://github.com/myfreeweb/jack2/compare/master...mekanix:feature/print?expand=1

Hans, I think you know SND_DSP_* familly of ioctl the best. Could you take a peek, please?
Comment 10 Goran Mekić 2020-11-25 12:42:52 UTC
Created attachment 219960 [details]
/etc/rc.conf.d/virtual_oss

As jackdbus configured to run with /dev/dsp as stereo device works, but doesn't with /dev/vdsp (stereo or otherwise), I'm putting this here for reference.
Comment 11 Goran Mekić 2020-11-25 12:54:10 UTC
Instructions for building without ports, assuming you do what "pkg info -D jackit" says:

env CFLAGS=-fPIC python waf
sudo env CFLAGS=-fPIC python waf install
# I'm not glad to admit I have /usr/bin/python3 symlink to /usr/local/bin/python3


Instructions for making sure new binary is loaded and used:
jack_control exit
jack_control start
less ~/.log/jack/jackdbus.log
Comment 12 Hans Petter Selasky freebsd_committer 2020-11-25 13:07:56 UTC
Goran:

This piece should be transformed into a SOUND_PCM_READ_CHANNELS, to get the actual amount of channels, when not specified on the command line:

 if (fCaptureChannels == 0) fCaptureChannels = 2;

SOUND_DSP_CHANNELS is really SOUND_PCM_WRITE_CHANNELS. Check solaris for these definitions too!

--HPS
Comment 13 Goran Mekić 2020-11-27 16:55:33 UTC
Created attachment 220031 [details]
jack-port-no-opus-and-readline.diff

I updated patches to development branch of jack2 and made a port with no opus and readline as I have some trouble making port options work in every combination. My audio in ardour6 is choppy, but in jack1 is even worse as ardour itself doesn't know how to connect inputs/outputs. Two days ago I could play my songs from ardour6 normaly using jack2, so there must be some part of config that is my fault. Anyway, can somebody test so these patches can be ironed out, please? I would love upstream this as soon as possible.
Comment 14 Goran Mekić 2020-11-27 16:56:49 UTC
Also, build port using poudriere works. The command I used to test:

poudriere testport -j 12amd64 -p local audio/jack
Comment 15 Florian Walpen 2020-11-27 21:41:07 UTC
(In reply to Goran Mekić from comment #13)

Hi Goran, thanks for your efforts - i had a quick test run with the patches:
 * Compile, package and installation was ok (synth)
 * USB Soundcard 16bit @ 44100Hz was ok (Hydrogen, MusE)
 * USB Soundcard 24bit @ 48000Hz was just noise (Hydrogen, MusE)

I'm still struggling a bit with the different parameters, i set wordlength and frequency both for "driver" and "internals->audioadapter" - is this correct?
Also tests ran on the "virtual" dsp which does autoconversion to the hardware supported format, would that make any difference?

I'll have more time to look into it this weekend, anything particular i should investigate apart from the 24bit issue?
Comment 16 Goran Mekić 2020-11-27 22:09:06 UTC
(In reply to Florian Walpen from comment #15)
You are most welcome!

What I found is that stereo on my laptop works just fine, but in studio with 18ch, I have all kinds of glitches. In my experience, that usually means some buffers are not aligned or appropriate. The change of channels (and for you, word length) influencing glitches kinda points to these portions of code:

https://github.com/mekanix/jack2/blob/master/solaris/oss/JackOSSAdapter.cpp#L298-L470
https://github.com/mekanix/jack2/blob/master/solaris/oss/JackOSSDriver.cpp#L232-L395

That's as much narrowing as I could possibly do given today's free time. I will report progress as I make it, but I would be really glad if somebody more knowledgable would look at those 4 functions and their ioctl calls.
Comment 17 Hans Petter Selasky freebsd_committer 2020-11-28 00:30:47 UTC
 * USB Soundcard 24bit @ 48000Hz was just noise (Hydrogen, MusE)

You should beware that Linux use 4-bytes for 24-bit samples (There is an own format type for this) and it is not compatible with FreeBSD's 24-bit audio. This should be fixed!

--HPS
Comment 18 Florian Walpen 2020-11-28 14:26:52 UTC
(In reply to Hans Petter Selasky from comment #17)

Thanks for the pointer - trying to deduce the format from the old jack1 patches (is there an official documentation?).

So FreeBSD AFMT_S24... is incompatible with OSSv4 AFMT_S24..., but comparable to AFMT_S24_PACKED as defined here:
http://manuals.opensound.com/developer/formats.html
Is that correct? Which combinations of S24, U24, LE, BE are actually used?
Our jack1 only supports S24_LE, which is also what i have in hardware.

jack2 does not have a 3-byte int conversion from the internal float format, so we have to provide one for input and output. I'll get to that tomorrow.
Comment 19 Hans Petter Selasky freebsd_committer 2020-11-28 16:42:58 UTC
FreeBSD uses what Linux calls 24-bit packed mode always for 24-bit. Maybe a bit misleading, but it has been that way for years, so I'm not sure if it is a good idea to change it, except for using packed mode for Linux.

The only problem I can think of is that buffer size computations can no longer be purely power of two.

We likely need an ifdef for __FreeBSD__ here if we want to use 24-bit mode, which I think is required for bit-perfect support.

--HPS
Comment 20 Hans Petter Selasky freebsd_committer 2020-11-28 16:45:25 UTC
> Which combinations of S24, U24, LE, BE are actually used?
Our jack1 only supports S24_LE, which is also what i have in hardware.

Little endian format is the most common. Our OSS kernel will fix those formats automagically in non-bit-perfect modes. In bit-perfect mode you need to process the format which the audio card is giving you, which theoretically can be any of the LE/BE variants.

--HPS
Comment 21 Goran Mekić 2020-11-28 22:18:39 UTC
As I don't need last two channels, I lowered the number of channels from 18 to 16, and as it is a power of two number, drastically less glitches are present, but they still are there. Anyway, I'm struggling to understand http://manuals.opensound.com/developer/SNDCTL_DSP_SETFRAGMENT.html, so I wonder how FreeBSD implementation handles it. Let me give an example:

int frag = (4 << 16) | 8

Does that mean I will have 4 fragments, each 256 bytes long?
If fragment size is for example (number of channels * sample size / 2), do I need 2 read calls to populate the data of all channels?
Comment 22 Hans Petter Selasky freebsd_committer 2020-11-29 00:14:29 UTC
int frag = (4 << 16) | 8

> Does that mean I will have 4 fragments, each 256 bytes long?

Yes.

> If fragment size is for example (number of channels * sample size / 2), do I need 2 read calls to populate the data of all channels?

You should not do this. You should always write data which is modulus "channels * sample_size_in_bytes".

View the frag = (4 << 16) | 8 as an approximation.

Use:

SNDCTL_DSP_GETBLKSIZE

To get the real blocksize used.

--HPS
Comment 23 Florian Walpen 2020-11-29 01:45:20 UTC
Quick status update from my side:

I have some working code for the 24bit case, we can actually use the conversion functions from jack2 with different parameters - i was misled by a comment in the sources. The produced format will be native endian (AFMT_S24_NE) like in the 16bit and 32bit cases.

That alone still resulted in choppy, slowed down audio. I had to set the frames per period (parameter "period") manually to 1365, which is the number of frames written in one block: 8190 blocksize / 2 channels / 3 bytes sample size. Given this setting audio output was clean. I thought that this value would propagate automatically through JackAudioDriver::SetBufferSize(), but it's somehow not respected.

So patches are not quite ready yet.

The other problem i have is with detection of input channels, my 2-channel USB interface is always detected as mono:
> JackOSSDriver::OpenInput driver forced the number of capture channels 1

I tried to use SOUND_PCM_READ_CHANNELS, same as above, any ideas?
Comment 24 Hans Petter Selasky freebsd_committer 2020-11-29 10:35:42 UTC
> The other problem i have is with detection of input channels, my 2-channel USB
> interface is always detected as mono:
> > JackOSSDriver::OpenInput driver forced the number of capture channels 1

> I tried to use SOUND_PCM_READ_CHANNELS, same as above, any ideas?

Can you check that there are two channels in each direction in dmesg?

--HPS
Comment 25 Florian Walpen 2020-11-29 11:37:14 UTC
(In reply to Hans Petter Selasky from comment #24)

Sure. From dmesg, sysctl and /dev/sndstat:

> uaudio0: Play[0]: 44100 Hz, 2 ch, 24-bit S-LE PCM format, 2x2ms buffer.
> uaudio0: Record[0]: 44100 Hz, 2 ch, 24-bit S-LE PCM format, 2x2ms buffer.
> uaudio0: MIDI sequencer.

> dev.pcm.2.rec.vchanformat: s32le:2.0
> dev.pcm.2.rec.vchanrate: 44100
> dev.pcm.2.rec.vchanmode: fixed
> dev.pcm.2.rec.vchans: 1

> [pcm2:record:dsp2.r0]: spd 44100, fmt 0x00201000/0x00210000, flags 0x00002108, 0x00000007
> interrupts 4950, overruns 0, feed 14850, hfree 4236, sfree 8191 [b:4236/2118/2|bs:8192/4096/2]
> channel flags=0x2108<TRIGGERED,BUSY,HAS_VCHAN>
> {hardware} -> feeder_root(0x00210000) -> feeder_format(0x00210000 -> 0x00201000) -> feeder_mixer(0x00201000) -> {userland}
> pcm2:record:dsp2.r0[pcm2:virtual:dsp2.vr0]: spd 44100, fmt 0x00110000/0x00201000, flags 0x1000112c, 0x00000063, pid 2090 (jackdbus)
> interrupts 0, overruns 0, feed 19800, hfree 0, sfree 8190 [b:0/0/0|bs:8190/4095/2]
> channel flags=0x1000112c<RUNNING,TRIGGERED,SLEEPING,BUSY,HAS_SIZE,VIRTUAL>
> {hardware} -> feeder_root(0x00201000) -> feeder_matrix(2.0 -> 1.0) -> feeder_volume(0x00101000) -> feeder_format(0x00101000 -> 0x00110000) -> {userland}

dev.pcm.2.rec.vchanformat was set via /etc/sysctl.conf
Comment 26 Hans Petter Selasky freebsd_committer 2020-11-29 12:07:20 UTC
Hi,

I think the problem is that SNDCTL_DSP_SETFMT doesn't have the AFMT_STERO bit set, and this forces the audio to 1-ch mono:

    if (ioctl(fInFD, SNDCTL_DSP_SETFMT, &fSampleFormat) == -1) {
        jack_error("JackOSSDriver::OpenInput failed to set format : %s@%i, errno = %d", __FILE__, __LINE__, errno);
        goto error;
    }

Actually you should read the channels first, then SETFMT, then write channels.

--HPS
Comment 27 Goran Mekić 2020-11-29 12:12:48 UTC
I updated the code with (I hope) proper calculation of buffer sizes in https://github.com/mekanix/jack2/tree/feature/freebsd. It is tested only with 32bit samples, as that's the only thing my card supports. It probably need more refinement. The base of the idea is to take http://manuals.opensound.com/developer/SNDCTL_DSP_SETFRAGMENT.html as reference and replace:

int frag = (max_fragments << 16) | (size_selector)

with:

int frag = (channels << 16) | (size_selector)
Comment 28 Goran Mekić 2020-11-29 12:32:18 UTC
(In reply to Hans Petter Selasky from comment #26)
I moved buffer calculation at the end, so order of ioctl calls should be OK now. I still get glitches, but order of magnitude less.
Comment 29 Florian Walpen 2020-11-29 12:59:16 UTC
(In reply to Hans Petter Selasky from comment #26)

Did some more testing, jack2 detects the 2 rec channels just fine when in bitperfect mode. That would explain the difference to Goran's setup.

You mean AFMT_STEREO? Does it imply exactly 2 channels or also >= 2 channels? Anything else we'd need to set, like AFMT_FULLDUPLEX?

I will try Goran's changes first, that should fix the order issue.
Comment 30 Goran Mekić 2020-11-29 13:32:55 UTC
Created attachment 220062 [details]
jack-no-opus-and-readline.diff

Just port update to use latest commit
Comment 31 Florian Walpen 2020-11-29 16:08:09 UTC
(In reply to Goran Mekić from comment #27)

Not sure I can follow your intentions with buffer size calculations. You omit sample size and channel count from the fragment size calculation:

> gFragFormat = (fCaptureChannels << 16) + int2pow2(fEngineControl->fBufferSize);

You will get a much too small block size (fInputBufferSize) and recalculate the internal number of frames (fEngineControl->fBufferSize) from that:

> int new_buffer_size = fInputBufferSize / (fSampleSize * fCaptureChannels);

And then you repeat the whole thing for the output buffers, resulting in an even tinier fragment size and internal number of frames. This is completely unusable and inconsistent for my setup.

Why not just set a smaller number of internal frames ("period") from the start, and then fit the fragment size to that like it was? We are writing the whole data in one go anyway, AFAICT.

Or what are you trying to achieve?
Comment 32 Hans Petter Selasky freebsd_committer 2020-11-29 16:40:53 UTC
> int frag = (channels << 16) | (size_selector)
                ^^ This is wrong.

size_selector should contain log2(samples * channels * samplesize).

Number of fragments should be selectable from the command line, like 2,3,4.

--HPS
Comment 33 Hans Petter Selasky freebsd_committer 2020-11-29 17:17:18 UTC
https://github.com/hselasky/virtual_oss/blob/e8c5ef356b0983d1d264f4657a2000b6dfeaf94d/virtual_main.c#L1130

You may want to have a look how virtual_oss handle these IOCTLs.

Or in the kernel.

--HPS
Comment 34 Goran Mekić 2020-11-29 20:26:41 UTC
(In reply to Florian Walpen from comment #31)
Then the fact that I don't see it in the log and it kinda works indicates the bug is deeper than this. I will have to experiment more.
Comment 35 Florian Walpen 2020-11-29 23:59:34 UTC
Created attachment 220087 [details]
Fix 24bit sample handling in FreeBSD

Based on https://github.com/mekanix/jack2/tree/feature/freebsd, this patch fixes 24bit sample output on FreeBSD and supposedly also input (not tested).

Since these changes are specific to FreeBSD they are guarded by #ifdefs, compatible with upstream.
Comment 36 Florian Walpen 2020-11-30 00:18:14 UTC
(In reply to Goran Mekić from comment #34)

I locally reverted your calculation of the fragment size to the original behaviour, which I think is the correct one. Maybe you could post the log, driver and audioadapter parameters of your current setup? I'd like to have a look at it.
Comment 37 Goran Mekić 2020-11-30 11:33:46 UTC
Created attachment 220102 [details]
jackdbus log (wrong buffer calculation but jackdbus works)

I experimented more and you're definitely right. The question remains why it even works for me. Also, I have no idea how to interpret max_fragments in "int frag = (max_fragments << 16) | (size_selector)". However I change "max_fragments", I get the same fragment size, so I'm confused what that parameter actually is.

Anyway, I'll take your patch, add it to my github and try to run my gear with it. Who knows, maybe it's all perfect. :o)
Comment 38 Hans Petter Selasky freebsd_committer 2020-11-30 12:27:04 UTC
max_fragments doesn't affect the buffer size.

max_fragments indicate how many buffers of size 2**N, that are available.

So the two should be multiplied to get the total buffer size.

In other words, buffer size affects the hardware buffer, and max_fragments doesn't.

--HPS
Comment 39 Florian Walpen 2020-11-30 13:07:54 UTC
Thanks for the log, Goran. For me the block size returned was always the fragment size requested (minus 3 byte rounding for 24bit samples). Obviously the block size of 36864 fits your settings (512 * 4 * 18). But what confuses me is how you get a non-2^N fragment size - maybe it just uses a multiple of the page size for bigger fragments?

Probably HPS knows, or I'll have a look at the source code later on.

What helped in my case was to go the other way round and adjust the period parameter in the jack config (both driver and internals). So you could aim for 2^15 block size, which makes 32768 / (4 * 18) = 455 frames per period. Not sure every jack client will like this odd period size, but in my quick tests Hydrogen, MusE and Ardour6 worked.

Don't forget to revert to the original fragment calculation if you try this.

I will do more testing as soon as I can fetch my oddball gear from the music room - 6 in 6 out 24bit, and 10 in 12 out 32bit.
Comment 40 Goran Mekić 2020-11-30 13:52:15 UTC
(In reply to Florian Walpen from comment #39)
I reverted my patch, applied yours, set the buffer to the size of 455 and made sure internal, driver and engine parameters are in sync. I do get the same results as with my (wrong buffer size) patch applied: it works, but I do hear crackling sounds while ardour is playing (same with muse3). OK, I would call this a progress :o)
Comment 41 Goran Mekić 2020-12-02 23:38:09 UTC
Created attachment 220197 [details]
jack-no-opus-and-readline.diff

I reverted the wrong buffer calculation, added Florian's 24bit patch and moved buffer calculation after getting real channel number in driver. I also learned that "git diff" output is much easier to apply than "git show", so this patch can be applied to official git port repo (I'm using github mirror).

Status is that it's still glitchy on my studio machine, but it could be that after 12 years of use, it's just too old and some electronics is dying out. I will try to get my hands on some other PC and test, but in this time of pandemic it might take a while, so if anyone can test and report if you too get glitches in the output, that would be great!
Comment 42 Florian Walpen 2020-12-04 01:55:21 UTC
Created attachment 220234 [details]
Handle non-power-of-2 sized buffers on the OSS side

Hi Goran, not there yet, but we're getting closer to "generally working" territory. Could you test the above patch applied to your repo, both with 455 and standard periods like 512, and ignorehwbuf disabled?

Background (someone correct me if I'm wrong):
 * Fragment size (== block size) is always 2^N in FreeBSD (minus 3byte rounding)
 * Fragment count is also a 2^N number
 * Some hardware requires odd jack2 periods to achieve 2^N buffer sizes (24bit, 18in18out)
 * Some hardware cannot achieve 2^N buffer sizes (10in12out)
 * Writes (reads) of exactly fragment size may be desirable, but not strictly necessary
 * jack2 setting "ignorehwbuf" is not read from settings, always set to true

The current execution path with "ignorehwbuf" set handles non-2^N buffer sizes by adjusting the jack2 period written per cycle, but fails to update the actual buffer size - thus we write garbage to OSS in that case. Also the adjustment is uncoordinated between in and out channels, fails completely for asymmetric channel counts.

What this patch supposedly does:
 * Preserve current behavior for 2^N buffer sizes
 * Only adjust the jack2 period if "ignorehwbuf" is set (yes, funny name)
   * Update buffer size in that case
   * Asymmetric channel counts are still broken with "ignorehwbuf"
 * Otherwise allow non-2^N buffer sizes
   * Ignore fragment size for reads and writes in that case
   * Request more but smaller fragments (same total size)

Not sure about using smaller fragments, it seemed to help with jitter tolerance, but I'm not totally convinced that the fragment size works like I think it does.
Comment 43 Hans Petter Selasky freebsd_committer 2020-12-04 09:13:29 UTC
 * Fragment size (== block size) is always 2^N in FreeBSD (minus 3byte rounding)
 * Fragment count is also a 2^N number

Totally wrong.

Only the IOCTL API uses a fragment size of 2^N. This is more or less an approximation value. You need to read back the actual fragment size using SNDCTL_DSP_GETBLKSIZE() (which is not power of two) !

The fragment count can be any value N, that fits, usually low, like 2,4,8.

--HPS
Comment 44 Goran Mekić 2020-12-04 13:26:42 UTC
Definitely improvement! I do get glitches, but is like 100 times less, so we're heading there :o) As Hans pointed out, function needs to be refined, but it's starting to look better. I tested with 455 and 512 and ignorehwbuf disabled. It's in my repo for reference.
Comment 45 Florian Walpen 2020-12-04 13:45:24 UTC
(In reply to Hans Petter Selasky from comment #43)

Thanks for the hint. Reading the fragment size is not a problem, we do that, but requesting one that is e.g. a multiple of 512 * 4 * 18. If you know of any way to do this, please tell me.

If I follow the code path from the IOCTLs to chn_resizebuf(...) in sys/dev/sound/pcm/channel.c:

> /*
>  * The application has requested their own blksz/blkcnt.
>  * Just obey with it, and let them toast alone. We can
>  * clamp it to the nearest latency profile, but that would
>  * defeat the purpose of having custom control. The least
>  * we can do is round it to the nearest ^2 and align it.
>  */

And the code around it looks accordingly, which is why I see no way to request a fragment size that matches the period cycle from jack. Not if the channel count or the sample size are non-2^N.

Even if jack cannot obtain a matching fragment size, it's still possible to write and read arbitrary chunks, e.g. 512 * 4 * 18 bytes. That's what my patch is for. In that case my question is: How does the fragment size affect performance and timing tolerance? Are single fragments only processed when they are full, or more continuously like the scatter / gather buffers in read(...) and write(...)?
Comment 46 Hans Petter Selasky freebsd_committer 2020-12-04 13:53:10 UTC
> And the code around it looks accordingly, which is why I see no way to request > a fragment size that matches the period cycle from jack. Not if the channel 
> count or the sample size are non-2^N.

There is:

SNDCTL_DSP_SETBLKSIZE

which can do this. It selects 2 frags as default. Virtual OSS respects the previously set fragcount, only changing the buffer size. Not sure about Linux.

--HPS
Comment 47 Hans Petter Selasky freebsd_committer 2020-12-04 13:58:34 UTC
> How does the fragment size affect performance and timing tolerance?
> Are single fragments only processed when they are full, or more continuously like the scatter / gather buffers in read(...) and write(...)?

Smaller fragment size means more interrupts (CPU usage) and a higher chance of glitches. But also improves the responsiveness of live synth apps.

A fragment is usually the smallest entity the audio driver can process. With USB audio this entity is fixed and can be slightly controlled by hw.usb.uaudio.buffer_ms to 2,3,4,5,6,7 or 8 ms.

Other drivers select the buffer size given by the user. This also depends on the use of vchans. Bitperfect mode is different, as you know.

--HPS
Comment 48 Florian Walpen 2020-12-04 15:17:34 UTC
(In reply to Hans Petter Selasky from comment #46)

> There is:
> 
> SNDCTL_DSP_SETBLKSIZE

I've seen that in the FreeBSD sources, but it sets fragment count to 2 and ends up in the same chn_resizebuf(...) as above, rounding fragment size to 2^N and then aligns it to frame size.

I'm not using Virtual OSS unfortunately, but Goran does, I think - it may be worth a try as Virtual OSS seems to be more flexible in this regard. But this IOCTL is specific to FreeBSD and Virtual OSS I think.

> A fragment is usually the smallest entity the audio driver can process. With
> USB audio this entity is fixed and can be slightly controlled by
> hw.usb.uaudio.buffer_ms to 2,3,4,5,6,7 or 8 ms.

In that case it would be best to leave the predefined fragment size as is, right? But we'd have to make sure that jack doesn't block for too long, there must be enough fragments to hold the jack period (twice). I have to check this.

> Other drivers select the buffer size given by the user. This also depends on
> the use of vchans. Bitperfect mode is different, as you know.

I suppose we should set the fragment size there. A strategy could be to divide the total buffer size into 8 fragments for longer period cycles, and 4 fragments for short periods, if there is no matching fragment size.
Comment 49 Florian Walpen 2020-12-04 15:43:12 UTC
(In reply to Goran Mekić from comment #44)

Hi Goran, nice to hear, I don't get glitches but I didn't test anything demanding yet. If you're on Virtual OSS you could try to use SNDCTL_DSP_SETBLKSIZE, else maybe a different fragment size like:

> gFragFormat = (4 << 16) + int2pow2(fInputBufferSize >> 1);

> gFragFormat = (16 << 16) + int2pow2(fInputBufferSize >> 3);

Are we going to upstream this? We'll probably have to propagate the modifications to JackOSSAdapter.cpp too.
Comment 50 Goran Mekić 2020-12-04 18:30:18 UTC
(In reply to Florian Walpen from comment #49)
Looking at https://www.google.com/search?hl=en&q=SNDCTL_DSP_SETBLKSIZE I guess it is FreeBSD specific thing. If we guard that call with ifdefs, I think it will be OK,  but let me just be clear where SETBLKSIZE should be. I'm guessing before:

if (fIgnoreHW) {

Anyway, I will play with the buffers for a bit tomorrow and report back.


As for the upstreaming, I sure do hope it won't be just a patch in the port (or at least not for long).
Comment 51 Florian Walpen 2020-12-04 21:20:44 UTC
(In reply to Goran Mekić from comment #50)

For testing I would just replace the SNDCTL_DSP_SETFRAGMENT ioctl with SNDCTL_DSP_SETBLKSIZE using fInputBufferSize as parameter. You can see in the jack output whether the fragment size is set to the requested 512 * 18 * 4 or rounded to a different size. If it is the requested size, test again for glitches, otherwise I don't see any use in it.

For an actual FreeBSD-specific implementation we could maybe:
1. Set SNDCTL_DSP_SETBLKSIZE
2. Check the result with SNDCTL_DSP_GETBLKSIZE
3. If not the requested size, use SNDCTL_DSP_SETFRAGMENT with 4 or 8 fragments
Comment 52 Goran Mekić 2020-12-04 22:13:44 UTC
I just realized some of my tracks have glitches recorded! I'll have to test tomorrow all over again. Anyway, there's a chance it works.
Comment 53 Goran Mekić 2020-12-05 11:46:11 UTC
Created attachment 220276 [details]
jack-no-opus-and-readline.diff

I updated Adapter and Driver opening input and output. I would say everything works perfectly. I even ran clean vocal track through hardware processor on full distortion, so if there was any glitch, it would scream. I think I covered all, but having adapter and driver in source and config is really confusing to work with.
Comment 54 Goran Mekić 2020-12-05 11:48:01 UTC
Created attachment 220277 [details]
jackdbus log with standard values and ardour session
Comment 55 Goran Mekić 2020-12-05 14:32:03 UTC
As I suspected, I made few mistakes with combination of adapter/driver/input/output. I hope I fixed that and I can hear audio perfectly, using 18 channels and buffer of 512. What is next? Are we satisfied with how it looks? If yes, should we upstream it (squash it first?), if no, what should we work on?

https://github.com/jackaudio/jack2/compare/master...mekanix:feature/freebsd?expand=1
Comment 57 Florian Walpen 2020-12-05 15:30:02 UTC
(In reply to Goran Mekić from comment #53)

There may have been a misunderstanding about SNDCTL_DSP_SETBLKSIZE. It is only useful to us if it can set an exact fragment size on Virtual OSS - otherwise it does not provide any benefits over SNDCTL_DSP_SETFRAGMENT.

Did you test it? Because your current code won't run the SNDCTL_DSP_SETBLKSIZE part AFAICT. It should produce 2 fragments of size 36864. Just replace the SNDCTL_DSP_SETFRAGMENT ioctl with SNDCTL_DSP_SETBLKSIZE for testing.

But if it works now we can also opt to let it be and just use the SNDCTL_DSP_SETFRAGMENT implementation we had before.
Comment 58 Florian Walpen 2020-12-05 16:27:01 UTC
(In reply to Goran Mekić from comment #55)

> What is next? Are we satisfied with how it looks?
> If yes, should we upstream it (squash it first?), if no, what should we work on?

I'm not a port maintainer, but I would propose to fetch from your repo temporarily for the FreeBSD port - give it some more exposure and testing on the FreeBSD side, make sure we have most use cases covered. It will be hard enough to get it upstream once.

As for commits, I would keep the build fixes separately, and squash the driver fixes to meaningful chunks before going upstream.

What still concerns me is the ordering of the ioctl calls. AFAIK the original order was according to http://manuals.opensound.com/developer/callorder.html, and we should probably keep it that way for non-FreeBSD use. The main conflict lies in SNDCTL_DSP_SETFRAGMENT order, right?

Or is there any other ioctl we should call in different order on FreeBSD? Current implementation does:
SNDCTL_DSP_COOKEDMODE
SNDCTL_DSP_CHANNELS
SNDCTL_DSP_SETFRAGMENT
SNDCTL_DSP_SETFMT
SNDCTL_DSP_SPEED
SNDCTL_DSP_GETBLKSIZE
Comment 59 Goran Mekić 2020-12-05 17:14:17 UTC
(In reply to Florian Walpen from comment #58)
I didn't get good results with SNDCTL_DSP_SETBLKSIZE, but I would live it as it is now, as a last resort, because current code calls that ioctl only on FreeBSD and only if all other methods of setting buffer sizes failed.
Comment 60 Goran Mekić 2020-12-05 19:36:03 UTC
Created attachment 220289 [details]
jack-transport-problem.diff

I added OPUS and READLINE options. When I try poudriere it complains bin/jack_transport is in stage dir but not in pkg-plist, and when I put it in pkg-plist, "make -DBATCH all deinstall install clean" from ports complains there's no bin/jack_transport. I tried setting same options for port and poudriere with no luck. What am I doing wrong?
Comment 61 Florian Walpen 2020-12-05 19:42:28 UTC
(In reply to Goran Mekić from comment #59)

If it didn't help then just scrap the SNDCTL_DSP_SETBLKSIZE part, i'd say. Where it could possibly help is with non-2^N buffer sizes on Virtual OSS. But with your current implementation it will only ever be called for 2^N buffer sizes which never were a problem.
Comment 62 Goran Mekić 2020-12-05 21:36:34 UTC
(In reply to Florian Walpen from comment #61)
I removed it. If I'm not mistaken, bin/jack_transport is the only problem?
Comment 63 Florian Walpen 2020-12-05 22:47:57 UTC
(In reply to Goran Mekić from comment #62)

I think it's ready for the wider FreeBSD audience, it has been at least as robust for me as jack1 ever was. The missing rc.d service could be a regression though for some users, maybe we should provide one. I will try your new port tomorrow, but I have no expertise in portscraft.

The strategy for non-2^N buffer sizes may need some refinement in high speed / low latency situations, but for now it works pretty well in my tests.

Regarding upstream, we first have to resolve the ioctl order issue (see comment #58), and revise what should be #ifdef'ed specific to FreeBSD.
Comment 64 Goran Mekić 2020-12-06 09:15:41 UTC
Could we have two jack implementations? They would, of course, be exclusive: only one can be installed. I'm thinking about this because jackdbus must be started through DBUS and it requires DISPLAY to be set, which makes it hard to write a service. The closest to that is jackdbus autostart. That being said, just upgrading to jack2 is a regression, but I think it's easily rectifiable. If we decide for a single jack package, I would like to put some small howto into pkg-message.
Comment 65 Florian Walpen 2020-12-06 13:28:53 UTC
(In reply to Goran Mekić from comment #64)

I haven't played around yet, but according to the build files and https://github.com/jackaudio/jackaudio.github.com/wiki/JACK-DBus-packaging there are two separate questions:
 * What to build and install - jackd (classic), jackdbus, or both
 * Autostart - either jackd or jackdbus

If the DBUS option is not selected we would configure --classic=yes and --autostart=classic anyway, right?

With DBUS enabled (default) I would prefer to build and install both, but autostart jackdbus because we're able to start classic jackd separately via rc service. Like --classic=yes and --dbus=yes and --autostart=dbus.
Does that make sense?

If that's not possible, just going classic jackd when DBUS is not enabled is fine with me. My main concern is the difficulty of setting rtprio on the jack server - I'm not a huge fan of glitches in recordings, it really complicates software testing ;-)
Comment 66 Florian Walpen 2020-12-06 14:10:13 UTC
(In reply to Goran Mekić from comment #60)

Unfortunately I cannot help you with bin/jack_transport, if I add it to pkg-plist it builds and installs just fine (using synth).
Comment 67 Florian Walpen 2020-12-06 15:41:01 UTC
Created attachment 220316 [details]
Fix port always build classic jackd

Have to take a break now, but this changes the port to:
 * Fix the build with DBUS disabled -> missing libsysinfo dependency, pkg-plist
 * Always build and install classic jackd
 * Autostart is set to dbus mode with DBUS enabled

Still have to check whether the classic jackd can be launched by rc service.
Comment 68 Florian Walpen 2020-12-06 18:13:09 UTC
Created attachment 220321 [details]
Fix port, always build jackd with rc service

Ok, still the fix for Goran's new port to always build the classic jackd, but now includes also the rc service script. Did a quick test with DBUS enabled:
 * rc service script works like with jack1 - service jackd onestart
 * Hydrogen autostarts jackdbus, so users can do both with the default package

May be worth mentioning in pkg-message if we take this route.
Comment 69 Goran Mekić 2020-12-07 18:10:25 UTC
Created attachment 220352 [details]
jack.diff

So jack_transport is readline dependent. I hope this is it as it passes with readline on and off in ports and poudriere.
Comment 70 Florian Walpen 2020-12-07 23:33:37 UTC
(In reply to Goran Mekić from comment #69)

Nice, thank you! Looks good to me (and compiles). One last detail, could we adjust pkg-message, maybe:
 * Add a shortened version of the old pkg-message about classic jackd service
 * Mention jack_dbus to avoid confusion with classic jackd
 * Note about memorylocked limit in login.conf (I always forget about that...)

As of the consumers I compile here locally, only the audio/lash port broke with the new jack port, and needs pthread for linking now:

> LDFLAGS+=     -L${LOCALBASE}/lib -pthread
Comment 71 Goran Mekić 2020-12-09 18:52:39 UTC
Created attachment 220399 [details]
jack.diff

Updated pkg-message. Did we cover all?
Comment 72 Goran Mekić 2020-12-09 18:55:59 UTC
Created attachment 220400 [details]
jack.diff

Sorry, I forgot the explanation of rc vars.
Comment 73 Florian Walpen 2020-12-09 20:55:53 UTC
(In reply to Goran Mekić from comment #72)

I'm fine with this pkg-message, whoever wants to use jack probably has to do some research anyway, but the pointers are there. Thanks for your efforts.

Now we definitely need the attention of someone who can bring this into the ports tree...
Comment 74 Goran Mekić 2020-12-12 21:47:45 UTC
Created attachment 220514 [details]
jack.diff

Improvements suggested by diizzy @ efnet. So just to be explicit, the reason why fork is used to build the port/package is that we intend to upstream changes once more people are exposed to the new jack. Those patches can be applied to development branch of upstream, and once we're satisfied with how it works, send pull request. The changes are minimal as OSS support was mostly present, except it was only build on Solaris, so some glue and patching was needed. Also, 3-byte 24bit support (packed format) and non-power-of-2 number of channels are added through these patches. The reason the celt is disabled is because the upstream is dead. 

QA:

  * portlint: OK (looks fine.)
  * testport: OK (poudriere: 12.2, amd64 tested)