Bug 238773 - multimedia/x265: Only highest bit-depth profile is built when multiple (bit-depth OPTIONS) are selected
Summary: multimedia/x265: Only highest bit-depth profile is built when multiple (bit-d...
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-ports-bugs mailing list
Keywords: easy, needs-patch
Depends on:
Reported: 2019-06-23 05:20 UTC by Jamie Landeg-Jones
Modified: 2020-01-18 08:13 UTC (History)
6 users (show)

See Also:
daniel.engberg.lists: maintainer-feedback+

Patch for x265 (3.95 KB, patch)
2019-12-30 16:30 UTC, daniel.engberg.lists
no flags Details | Diff
Poudriere log (203.91 KB, text/plain)
2019-12-30 16:32 UTC, daniel.engberg.lists
no flags Details
Patch for x265 (3.98 KB, patch)
2020-01-09 08:54 UTC, daniel.engberg.lists
no flags Details | Diff
Support multiple pixel-widths (2.70 KB, patch)
2020-01-10 03:45 UTC, Mikhail Teterin
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jamie Landeg-Jones 2019-06-23 05:20:53 UTC
The build config menu contains the following checkboxes:

│ │ [x] HI10P       Enable HI10P Support (64-bit only)        │ │
│ │ [x] HI12P       Enable HI12P Support (64-bit only)        │ │                                                                                                             

This implies additional profiles, but as it stands, only the highest is built (so chosing 12 means 8 and 10 aren't built).

Example code to enable multi-profiles is in ....multimedia/x265/work/x265_3.0/build/libux/multilib.sh

As per https://x265.readthedocs.io/en/default/api.html#packaging-and-distribution :

"We recommend that packagers distribute a single combined shared/static library build which includes all the bit depth libraries linked together. See the multilib scripts in our build/ subdirectories for examples of how to affect these combined library builds. It is the packager’s discretion which bit-depth exports the public C functions and thus becomes the default bit-depth for the combined library."

Cheers, Jamie
Comment 1 Jamie Landeg-Jones 2019-06-23 05:51:25 UTC
Or, if that seems like overkill, may I suggest changing the options to an 8/10/12 bit radio-button selection?

Comment 2 Mikhail Teterin freebsd_committer 2019-06-23 18:26:01 UTC
Thanks for pointing this out, Jamie. If you can make a patch for the port, I can transfer maintainership to you as well...
Comment 3 Wade 2019-11-13 07:19:51 UTC
I'm also trying to sort this out, but have no idea how to craft a ports Makefile to do what I think is expected. Is there documentation which would help with understanding its structure?

From build/linux/multilib.sh in the x265 source archive it builds individual 8-, 10- and 12-bit binaries when selected via configuration then bundles them together using libtool.

Another example is the homebrew build formula:

Comment 4 Jamie Landeg-Jones 2019-12-22 16:18:28 UTC
Ahh.. Yeah, I've been meaning to do this, but real life keeps getting in the way...

Give me a few days.

Watch this space!
Comment 5 Kubilay Kocak freebsd_committer freebsd_triage 2019-12-25 07:04:25 UTC
Pending the bigger change (see below), changing the options to an OPTIONS_SINGLE group seems the most desirable short-term change proposal to resolve the issue "as reported", given the current behaviour that only the highest is actually built, which is not expected by user.

We can resolve this issue with that change, after creating a new/separate issue to "Add multiple-bit-depth profile linking' mechanism/feature to the port if someone would like to take that on.
Comment 6 Wade 2019-12-25 07:15:27 UTC
I'm a complete novice regarding this, but I tried to manually bundle the 3x binaries using libtool as per the Linux examples, but it appeared to not perform this function as expected.

Is there any documentation, or a similar existing example, I could refer to which would help explain how this is expected to be performed for this environment?
Comment 7 daniel.engberg.lists 2019-12-30 10:27:01 UTC
I have somewhat of a working Makefile but I'm not sure how to handle install-do section.
Also, this might be a horrible idea so suggestions are welcome.
I don't see the harm in providing 8,10,12 support by default as that seems to be what other distributions do. Test needs to be fixed but I'll tackle that later.

Suggestions are welcome!
Comment 8 daniel.engberg.lists 2019-12-30 16:30:35 UTC
Created attachment 210333 [details]
Patch for x265

Fixes following issues:
Only require NASM on i386 and amd64
No need to pass the same switches using both CFLAGS and CPPFLAGS
Compile 8,10,12-bit support unconditionally on 64-bit platforms

Compile and run-tested on aarch64 and amd64

Supersides PR#239510
Comment 9 daniel.engberg.lists 2019-12-30 16:32:02 UTC
Created attachment 210334 [details]
Poudriere log
Comment 10 Jamie Landeg-Jones 2020-01-05 15:02:55 UTC
(In reply to daniel.engberg.lists from comment #8)

Apologies for not having the time to get back to this as I promised, but brilliant job sorting it out!

I haven't tried running it yet, but it looks good.

As for your original question about whether to install 8, 10, 12 by default. Originally, I was going to make each one optional - installing the 3 basically triples compile time / library disk usage.

However, when the coffee finally kicked in, I realised that this will only be installed for encoding x265. Any machine that has problems with a slightly bigger library, and the extended compile time won't be suitable for x265 encoding!

Also, it will make support, and the binary package easier to maintain bundling the whole lot together.

My only slight concern is about changed behavior. Before your patch, as the highest quality profile selected was the only one installed, it therefore was used as the default profile.

With your patch, the default encoding will be the 8bit profile. My personal feeling is that again, for support and maintainance, this is the correct approach to follow.

However anyone who previously used a high profile at default - without specifying it specifically in their configuration - will now have their code changed to use the 8 bit profile. They will need to alter their config to explicitly select the 10bit/12bit profiles as appropriate.

I'm just wondering whether this (admitedly probably rare) situation should be warned about when a user upgrades - either via a pkg-message or an entry in ports/UPDATING?

Cheers again! Jamie

P.S. Mikhail offered ownership of this port to me - I think instead you should take it, if that's ok with you!
Comment 11 Jamie Landeg-Jones 2020-01-05 15:07:28 UTC
(In reply to Wade from comment #3)

Hey Wade. Apologies for the delay in replying. It's good that you attempted this, so I hope you don't give up in future!

There is quite detailed information on building ports - including the Makefile - in the porters handbook : https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/

Also, the mailing lists and the forum https://forums.freebsd.org/ can be used for further help.

Cheers, Jamie
Comment 12 daniel.engberg.lists 2020-01-05 20:04:58 UTC

Splitting it up just makes it unnecessarily complicated for no obvious benefit. I just made this patch for a small project and it seems to work fine on my end. I think it's a better idea for you to maintain this port if you're interested as I'm  only using x265 a few times a year at best.

As far as the changed behaviour goes I don't think it's needed as it's clearly stated what settings are being used during encoding.
Comment 13 Jamie Landeg-Jones 2020-01-07 15:13:23 UTC
(In reply to daniel.engberg.lists from comment #12)

Ah ok, I didn't remember that it showed the settings.. I've rarely used x265 encoding myself up to now!

Anyway, I've applied your patch, and it installed without issue, and I've tested it in operation, coding at various profiles, and it all works perfectly.

Mikhail, I'd recommend Daniels patch as it stands. If you'd also still like to make me the maintainer, feel free to do so, if no-one else wants it.

Cheers, Jamie
Comment 14 daniel.engberg.lists 2020-01-09 08:54:20 UTC
Created attachment 210557 [details]
Patch for x265

Maintainer updated updated to Jamie and minor style fix
Comment 15 Mikhail Teterin freebsd_committer 2020-01-10 03:45:20 UTC
Created attachment 210581 [details]
Support multiple pixel-widths

Gentlemen, it occurred to me, we can fix the original problem without changing the promised behavior. The high-bit options can even be ON by default -- if we don't care for different arches having different functionality -- but it should still be possible to turn them off, I think.

Could you review this new patch, please?

It adds the build of the higher-resolution modes, while still keeping them optional. It also updates the pkg-plist...

I would've committed this already, except, when I compile with -march=sandybridge, the test-executable crashes halfway through the run. Compiling with -march=core2 tests fine -- and I'd like to spend some more time investigating the crash.

Another remaining item is to enable to the higher-bit options for other 64-bit systems (besides amd64). How can this be done elegantly -- and who can test the builds?
Comment 16 daniel.engberg.lists 2020-01-10 08:26:30 UTC
Hi, this doesn't work as intended and you shouldn't have multiple libraries.

x265 [info]: build info [Unk-OS][clang 9.0.1][64 bit] 8bit

This is with both 10 and 12-bit selected.

Does the binary using the previous patch also crash on your end?
Comment 17 Mikhail Teterin freebsd_committer 2020-01-11 03:29:37 UTC
(In reply to daniel.engberg.lists from comment #16)
> Hi, this doesn't work as intended and you shouldn't have multiple libraries.

Uhm, sorry, what?

> x265 [info]: build info [Unk-OS][clang 9.0.1][64 bit] 8bit
> This is with both 10 and 12-bit selected.

How did you get the above output?

> Does the binary using the previous patch also crash on your end?

No... I filed the https://bitbucket.org/multicoreware/x265/issues/531/crash-in-x265_pelfilterlumastrong_h_sse4
Comment 18 Jamie Landeg-Jones 2020-01-11 04:11:34 UTC
(In reply to Mikhail Teterin from comment #15)

Hi Mikhail!

The "promised behavior" was never correct. It implied 8 bit build, along with optional 10bit and 12bit.

It never stated what the default bitdepth would be if not specified on the command line.

As it stood, only the highest bitdepth selected was actually build, and hence, it became the defacto default.

Daniels patch does indeed build all bitdepths into one library. The default bitdepth will now be 8bit, with 10 and 12 bit optional on the x265 command line.

I agree with Daniel that this is the best solution - if you want a higher bit depth, it's easily selectable. (I think the bitdepth should be specified regardless anyway..)

To emulate the previous behaviour you'd need to have a compile time option to configure it at build time within the ports Makefile. I think this is overkill, and will make support/pkg harder to manage.

I did suggest mentioning that this may make an unexpected change for some use cases, but as Daniel pointed out, it's clear at use time what bit depth is being used.

I'm not sure where the extra libraries and include files comes from - Daniels patch simply bundles the whole lot into one (ie. the plist doesn't change)

As for the output, use "x265 --version".

This is what I get with Daniels patch applied:

% x265 --version
x265 [info]: HEVC encoder version 3.2.1+1-b5c86a64bbbe
x265 [info]: build info [Unk-OS][clang 7.0.1][64 bit] 8bit+10bit+12bit
x265 [info]: using cpu capabilities: MMX2 SSE2Fast LZCNT SSSE3 SSE4.2 AVX FMA3 BMI2 AVX2

, and a quick test encode of all three profiles worked, producing different binaries, that ffprobe showed had the correct bit depth selected.

As for the architecture issues, I must admit I did noting other than see if it compiled on my box, which is Freebsd12 with the following flags:
04:09 (7) "/tmp" jamie@thompson% show-clang-native-features
Clang "native" target for this machine: skylake  (Triple: x86_64-unknown-freebsd12.0)

Features enabled  (+) : sse2, cx16, sahf, prfchw, bmi2, fsgsbase, xsavec, popcnt, aes, xsaves, mmx, rdseed, clflushopt, xsave, invpcid, avx, fma, bmi, rdrnd, sse4.1, sse4.2, avx2, sse, lzcnt, pclmul, f16c, ssse3, sgx, cmov, movbe, xsaveopt, adx, sse3
Features disabled (-) : tbm, avx512ifma, sha, gfni, fma4, vpclmulqdq, cldemote, ptwrite, avx512bitalg, movdiri, avx512er, avx512vnni, avx512vpopcntdq, pconfig, clwb, avx512f, clzero, pku, lwp, rdpid, xop, waitpkg, movdir64b, sse4a, avx512bw, avx512vbmi2, avx512vl, avx512cd, vaes, rtm, mwaitx, wbnoinvd, prefetchwt1, shstk, avx512vbmi, avx512dq, avx512pf
04:09 (8) "/tmp" jamie@thompson% cc
+ cc -Wall -std=gnu99 -O2 -pipe -march=native -mtune=native -ftree-vectorize -fstack-protector-strong

Hope this helps!

Comment 19 daniel.engberg.lists 2020-01-18 08:13:36 UTC

I gave this a go on 11.3 which is the latest and only supported release version of 11.X according to https://www.freebsd.org/security/ (you're listing 11.2 in your bug report to upstream). Using my patch "make test" passes using both O2 and O3 (OPTIMIZED_CLAGS) setting CPUTYPE?=sandybridge in /etc/make.conf which results in -march=sandybridge when building x265.

This is on a fresh 11.3 install (AMD64) with a newly pulled ports tree using portsnap.

$ clang --version
FreeBSD clang version 8.0.0 (tags/RELEASE_800/final 356365) (based on LLVM 8.0.0)
Target: x86_64-unknown-freebsd11.3
Thread model: posix
InstalledDir: /usr/bin

$ nasm --version
NASM version 2.14.02 compiled on Jan 18 2020

$ x265 --version
x265 [info]: HEVC encoder version 3.2.1+1-b5c86a64bbbe
x265 [info]: build info [Unk-OS][clang 8.0.0][64 bit] 8bit+10bit+12bit
x265 [info]: using cpu capabilities: MMX2 SSE2Fast SSSE3 SSE4.2 AVX

I've tested this on VM with the host running an Ivy bridge CPU as I don't have a Sandybox based machine available. Can you please retest using 11.3 as it would be nice to get this in tree in a fully working state.