Bug 220785

Summary: audio/jack: Add and enable COOKEDMODE option, Use GitHub
Product: Ports & Packages Reporter: Yuri Victorovich <yuri>
Component: Individual Port(s)Assignee: Thomas Zander <riggs>
Status: Closed FIXED    
Severity: Affects Some People CC: hselasky, riggs
Priority: --- Keywords: needs-patch, needs-qa
Version: LatestFlags: riggs: maintainer-feedback+
riggs: merge-quarterly+
Hardware: Any   
OS: Any   
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch
none
patch
none
patch
none
patch
none
patch
none
patch none

Description Yuri Victorovich freebsd_committer 2017-07-17 04:23:14 UTC
Created attachment 184414 [details]
patch

Updated jack:
* Added COOKEDMODE option that turns on SNDCTL_DSP_COOKEDMODE.
  Without the cooked mode Jack's OSS driver currently fails for some soundcards
  (it chooses wrong sample rate). It's better to have COOKEDMODE=on by default since
  it often appears broken otherwise.
  Professionals probably want COOKEDMODE=off, after they make sure their soundcard
  works with jack.
* Switched to github. It is easy to change revisions. Particularly, included the
  merged upstream pull request #60 there enables the realtime mode (-R). It didn't
  make it into the release yet.
* Added USES=autoreconf (github doesn't include configure)
* Also jack now installs more manpages.

Builds in poudriere.
Runs fine. Particularly, it failed for me before, and now it works with COOKEDMODE=on.

Just in case, to run with real-time priority /etc/rc.conf needs lines like these:
> jackd_enable="YES"
> jackd_user="{your-jack-user}"
# jackd_rtprio="YES"
# jackd_args="-R -doss -r{sample-rate} -p1024 -n3 -w16 --capture /dev/dsp{N} --playback /dev/dsp{N}"

- sample-rate can be 44100, 48000, etc.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-17 05:49:18 UTC
Thank you Yuri,

-1 on switching MASTER_SITES, in particular to non-release (commit specific) tags,  given:

- PR's patches can be backported relatively easily (make makepatch, single files/patch-file)

- It becomes less obvious and explicit from which version of the source code the port is ultimately built.

- Once the next version is released, the need to use specific commit hashes/tags is removed

- Given this is a bugfix, it is highly desirable that quarterly users receive the change/benefit. Minimising unrelated changes is good to reduce potential for regressions.

Also, if special instructions are useful/valuable for users, please either add them to a pkg-message, or provide (probably commented out to not affect existing settings) examples in any default configurations files that are installed.

Could you please also elaborate on the details of the QA that was run, mentioning versions/architectures/etc of FreeBSD that were tested.
Comment 2 Yuri Victorovich freebsd_committer 2017-07-17 06:17:04 UTC
Kibilay,

> -1 on switching MASTER_SITES, in particular to non-release (commit specific) tags

Jack's release site is broken for these reasons:
* It is http:// making it vulnerable to MITM attacks.
* It doesn't provide tarball fingerprints (md5 or sha256)
At this point it is much safer to use github, it at least is https://

> - Once the next version is released, the need to use specific commit hashes/tags is removed

They release very rarely, once a year or so. It is much easier to move GH tagname, compared to adding patches.

> Could you please also elaborate on the details of the QA that was run, mentioning versions/architectures/etc of FreeBSD that were tested.

I usually write "Builds in poudriere."
In this case I also ran it for a while.

---

Jack port at this time is basically half-broken. Real-time fails, and samplerate is wrong on many cards. These two problems prevent many users from successfully using Jack. I was struggling with it myself, until I figured out these two problems.
Comment 3 Yuri Victorovich freebsd_committer 2017-07-17 07:22:42 UTC
Created attachment 184418 [details]
patch

* Added package message explaining how to start jack, and explaining COOKEDMODE option.

QAs:
* Builds in poudriere
* passes portlint
* runs fine
Comment 4 Yuri Victorovich freebsd_committer 2017-07-17 09:08:59 UTC
Created attachment 184425 [details]
patch
Comment 5 Yuri Victorovich freebsd_committer 2017-07-17 09:22:50 UTC
Created attachment 184426 [details]
patch
Comment 6 Hans Petter Selasky freebsd_committer 2017-07-17 09:55:30 UTC
Looks good. Maybe you should have a printout when cooked mode is in use?
Comment 7 Yuri Victorovich freebsd_committer 2017-07-17 15:47:15 UTC
(In reply to Hans Petter Selasky from comment #6)

I've added pkg-message with an explanation.

Yuri
Comment 8 Yuri Victorovich freebsd_committer 2017-07-17 16:15:00 UTC
(In reply to Hans Petter Selasky from comment #6)

Ok, I will add the printout in jackd.

Yuri
Comment 9 Yuri Victorovich freebsd_committer 2017-07-17 17:54:31 UTC
Created attachment 184446 [details]
patch

Changes:
* Added COOKEDMODE printout in the log
* Made the log printed through daemon(8), vs. shell
* Added DYNSIMD option to reflect --enable-dynsimd (only for i386/amd64)

QA:
* builds in poudriere
* passes portlint
* runs fine
Comment 10 Yuri Victorovich freebsd_committer 2017-07-17 17:59:10 UTC
Created attachment 184447 [details]
patch
Comment 11 Jan Beich freebsd_committer 2017-07-17 19:57:36 UTC
Comment on attachment 184447 [details]
patch

> +SUB_FILES=	pkg-message

Why? The file doesn't seem to contain any %% macros such e.g., %%PREFIX%%.

> +DYNSIMD_CONFIGURE_ENABLE=dynsimd
> 
>  CFLAGS+=	-I${BDB_INCLUDE_DIR}
> +COOKEDMODE_CFLAGS+=	-DOPTION_COOKEDMODE=1
>  LIBS+=	-L${BDB_LIB_DIR}
>  INSTALL_TARGET=	install-strip

- Rename DYNSIMD to RTCPU which already has default description vi Mk/bsd.options.desc.mk
- Don't mix option helpers and generic definitions/flags. It's confusing and breaks existing style in the file. For one, see how other _CONFIGURE_ENABLE are grouped.
- Maybe replace += with plain = as _CFLAGS helper already appends (not overrides) the value

> +OPTIONS_DEFINE=		ALSA DOXYGEN READLINE SNDIO COOKEDMODE
> +OPTIONS_DEFINE_i386=	ALSA DOXYGEN READLINE SNDIO COOKEDMODE DYNSIMD
> +OPTIONS_DEFINE_amd64=	ALSA DOXYGEN READLINE SNDIO COOKEDMODE DYNSIMD
> +OPTIONS_DEFAULT=	READLINE COOKEDMODE
> +OPTIONS_DEFAULT_i386=	READLINE COOKEDMODE DYNSIMD
> +OPTIONS_DEFAULT_amd64=	READLINE COOKEDMODE DYNSIMD

Can you deduplicate OPTIONS_*_${ARCH} ? Mk/bsd.options.mk already appends (not overrides) arch-specific options.
Comment 12 Jan Beich freebsd_committer 2017-07-17 20:07:30 UTC
(In reply to Jan Beich from comment #11)
> - Rename DYNSIMD to RTCPU which already has default description vi Mk/bsd.options.desc.mk

Ignore, RTCPU isn't in Mk/bsd.options.desc.mk yet. Standardizing on option names makes it easier to enable them globally via OPTIONS_SET.
Comment 13 Yuri Victorovich freebsd_committer 2017-07-17 20:16:37 UTC
Also, RTCPU isn't the same as DYNSIMD.
Changing CFLAGS+= to CFLAGS= strips -O flag from almost all compilation commands.

Will update other items soon.
Comment 14 Yuri Victorovich freebsd_committer 2017-07-17 20:30:05 UTC
Created attachment 184450 [details]
patch

* Removed .in suffix from pkg-message
* Only left DYNSIMD in OPTIONS_DEFINE_{arch} - I previously mistakenly thought that OPTIONS_DEFINE_{arch} overrides, vs. appends to OPTIONS_DEFINE
* Moved DYNSIMD_CONFIGURE_ENABLE below other {opt}_CONFIGURE_{xxx} tags

Thanks Jan for noticing those problems!
Comment 15 Jan Beich freebsd_committer 2017-07-17 20:49:15 UTC
Comment on attachment 184450 [details]
patch

> +COOKEDMODE_CFLAGS+=	-DOPTION_COOKEDMODE

Can you move this one down as well, closer to other option helpers? It only takes effect when both OPTIONS_DEFINE and PORT_OPTIONS (i.e. options selected by user) contain COOKEDMODE.

(In reply to Yuri Victorovich from comment #13)
> Also, RTCPU isn't the same as DYNSIMD.

Looking at the code DYNSIMD seems to use CPUID check to determine which (if any) of SIMD optimizations to use. Not sure how it's different from RTCPU in mplayer, ffmpeg or libvpx.

> Changing CFLAGS+= to CFLAGS= strips -O flag from almost all compilation commands.

_CFLAGS option helper i.e., COOKEDMODE_CFLAGS in this case. Nothing else is expected to modify it unlike CFLAGS which has -O2 via /usr/share/mk/sys.mk, -fstack-protector via $PORTSDIR/Mk/bsd.ssp.mk, etc.
Comment 16 Yuri Victorovich freebsd_committer 2017-07-17 21:02:32 UTC
> Looking at the code DYNSIMD seems to use CPUID check to determine which (if any) of SIMD optimizations to use. Not sure how it's different from RTCPU in mplayer, ffmpeg or libvpx.

RTCPU is "Use runtime CPU detection", DYNSIMD="Use runtime CPU detection for choosing SIMD". So DYNSIMD is more specific. It is logically a subclass of RTCPU.
Comment 17 Yuri Victorovich freebsd_committer 2017-07-17 21:05:22 UTC
Created attachment 184452 [details]
patch

* Moved _CONFIGURE
Comment 18 Yuri Victorovich freebsd_committer 2017-07-17 21:59:30 UTC
Unrelated to this bug, but RTCPU is too generic to be added to Mk/bsd.options.desc.mk IMO. It should have the purpose encoded, ex. RTCPUSIMD, RTCPUHT (hyperthreading), RTCPUHYPERVISOR (running on hypervisor), etc. The purposes are that different, that one flag can't fit all.

Is there a written suggestion for RTCPU online?
Comment 19 Yuri Victorovich freebsd_committer 2017-07-18 02:56:00 UTC
Created attachment 184460 [details]
patch
Comment 20 commit-hook freebsd_committer 2017-08-12 08:27:24 UTC
A commit references this bug:

Author: riggs
Date: Sat Aug 12 08:26:25 UTC 2017
New revision: 447818
URL: https://svnweb.freebsd.org/changeset/ports/447818

Log:
  Enable COOKEDMODE for more versatile soundcard support; use GitHub

  Detailed log:
  * Add COOKEDMODE option that turns on SNDCTL_DSP_COOKEDMODE.
    Without the cooked mode Jack's OSS driver currently fails for some
    soundcards (it chooses wrong sample rate). It's better to have
    COOKEDMODE=on by default since it often appears broken otherwise.
    Professionals probably want COOKEDMODE=off, after they make sure their
    soundcard works with jack.
  * Switch to github for fetching the sources.
  * Include the merged upstream pull request #60 to enable the realtime
    mode (-R).
  * Add USES=autoreconf (github source doesn't include a configure script)
  * Install more manpages.

  PR:		220785
  Submitted by:	yuri@rawbw.com
  MFH:		2017Q3

Changes:
  head/audio/jack/Makefile
  head/audio/jack/distinfo
  head/audio/jack/files/jackd.in
  head/audio/jack/files/patch-configure.ac
  head/audio/jack/files/patch-drivers_oss_oss__driver.c
  head/audio/jack/files/patch-jackd_jackd.c
  head/audio/jack/files/patch-libjack_client.c
  head/audio/jack/pkg-message
  head/audio/jack/pkg-plist
Comment 21 commit-hook freebsd_committer 2017-08-17 18:25:42 UTC
A commit references this bug:

Author: riggs
Date: Thu Aug 17 18:25:22 UTC 2017
New revision: 448166
URL: https://svnweb.freebsd.org/changeset/ports/448166

Log:
  MFH: r447818

  Enable COOKEDMODE for more versatile soundcard support; use GitHub

  Detailed log:
  * Add COOKEDMODE option that turns on SNDCTL_DSP_COOKEDMODE.
    Without the cooked mode Jack's OSS driver currently fails for some
    soundcards (it chooses wrong sample rate). It's better to have
    COOKEDMODE=on by default since it often appears broken otherwise.
    Professionals probably want COOKEDMODE=off, after they make sure their
    soundcard works with jack.
  * Switch to github for fetching the sources.
  * Include the merged upstream pull request #60 to enable the realtime
    mode (-R).
  * Add USES=autoreconf (github source doesn't include a configure script)
  * Install more manpages.

  PR:		220785
  Submitted by:	yuri@rawbw.com

  Approved by:	portmgr (feld)

Changes:
_U  branches/2017Q3/
  branches/2017Q3/audio/jack/Makefile
  branches/2017Q3/audio/jack/distinfo
  branches/2017Q3/audio/jack/files/jackd.in
  branches/2017Q3/audio/jack/files/patch-configure.ac
  branches/2017Q3/audio/jack/files/patch-drivers_oss_oss__driver.c
  branches/2017Q3/audio/jack/files/patch-jackd_jackd.c
  branches/2017Q3/audio/jack/files/patch-libjack_client.c
  branches/2017Q3/audio/jack/pkg-message
  branches/2017Q3/audio/jack/pkg-plist