- 2016Q4 is around the corner but our x264 is more than a year old
- upstream requires SSE2 on i386 which may crash ffmpeg built against libx264 (default)
- OpenCL was explicitly disabled in rP329667 but builds fine nowadays
- Style can be improved
Details
Details
- Reviewers
koobs - Commits
- rP413179: multimedia/{,lib}x264: modernize
rP422674: multimedia/x264: switch to _ENABLE option helper
rP422672: multimedia/x264: update to 0.148.2708
rP422671: multimedia/x264: add OPENCL option, enabled by default
rP422670: multimedia/x264: don't require SSE on i386 with ASM=on (default)
rP422673: bsd.options.desc.mk: add common OPENCL option description
poudriere bulk -t is green on 93i386, 93amd64, 101i386, 103amd64, 110i386, for all (bumped) consumers and inversed options. No QA was done for non-x86 architectures.
Tested by transcoding a video to avoid stutter from CPU decoding on Skylake (hybrid decoding is Windows-only). *facepalm*
$ fetch http://demo-uhd3d.com/files/uhd4k/Samsung_UHD_7Wonders_of_the_World_Italy.ts $ mpv --no-config --msg-level ffmpeg=v \ --ovc libx264 --ovcopts-add tune=film,crf=18 \ --oacopts-add flags=+qscale,global_quality=500 \ -o test.mp4 Samsung_UHD_7Wonders_of_the_World_Italy.ts ... [ffmpeg] libx264: using cpu capabilities: MMX2 SSE2Fast SSSE3 SSE4.2 AVX FMA3 AVX2 LZCNT BMI2 ...
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 5256 Build 5417: arc lint + arc unit
Event Timeline
Comment Actions
- Inversed options are green on 93i386, 93amd64, 101i386, 103amd64, 110i386. DEBUG tested separetely.
- All (bumped) libx264.so consumers are green on 93i386, 93amd64, 101i386, 103amd64, 110i386
Comment Actions
New snapshot (9 commits). Green on 93i386, 93amd64, 101i386, 103amd64, 110i386 and for ffmpeg{,0}.
Comment Actions
- New snapshot (0 commits).
- Add OPENCL option, enabled by default. Green on 93i386, 93amd64, 101i386, 103amd64, 110i386.
- Stop requiring SSE2 support on i386 with ASM option enabled.
Comment Actions
Oops, phabricator didn't split commits that had PORTREVISION bumps but creating separate reviews would make reviewer lose the big picture.
Comment Actions
Please also update SUMMARY to include itemized changes to patches incl. rationale/reasons. Hard to understand change context without them.
| multimedia/libx264/Makefile | ||
|---|---|---|
| 6 | I prefer and have always removed PORTREVISION for PORTVERSION updates, but I also like explicit >> implicit (PEP20). *brain divides by zero* I'll leave this up to you | |
| 12 | Can you add a # comment above this line to note that this is to null out LIB_DEPENDS from the x264 port without having to use an if/endif block. I couldn't remember why I did this originally, so a note is good for our future selves and other readers. | |
| 17 | Not 100% on this being a default option (yet)...
Perhaps its worth trialing as a separate port (libx264-opencl) so that we can get it user QA'd "in the field", thoughts? | |
| 20–22 | I think they call this (OPENCL_DESC) "OpenCL Lookahead", or "Enable OpenCL Lookahead (Hardware Acceleration)" +LIB_DEPENDS if either {lib}x264 depend on the library at run time Does OPENCL_CONFIGURE_ENABLE=opencl not work? Prefer explicit enable/disable which doesn't rely on upstream defaults, or break if upstream defaults change | |
| multimedia/x264/Makefile | ||
| 106 | Remove trailing \ | |
| multimedia/x264/files/patch-configure | ||
| 21 | Was this upstreamed? | |
Comment Actions
Ah, I note now in the above comment that OPENCL was tested for compilation(implicitly given it was OPTIONS_DEFAULT).
For future reviews please include all changes/tests in review SUMMARY/TEST PLANS sections including after followup commits (updates)
Comment Actions
I didn't expect such a detailed review after being desensitized by rubberstamp approvals. Thanks.
I've made rationale a bit more verbose in commit messages. Ultimately, they're more important (but rarely reviewed) than SUMMARY which is left with terse one-liners, or the big picture.
| multimedia/libx264/Makefile | ||
|---|---|---|
| 6 | PORTREVISION=0 serves as a reminder when to bump only master, only slave or both. It also protects against accidental bumps in slaves. However, I'm not trying to become the maintainer here. Removed. | |
| 12 | According to rP413179 the line is a slave helper to kill .include <bsd.port.options.mk>. Ideally, libx264 should have been the master port. Current separation isn't ideal as some libx264-specific stuff is still in x264 port. If you don't mind the risk (and churn) I can swap them but not in this review request. | |
| 17 |
| |
| 20–22 |
[1] Build with default options incurs the following warnings but the rest is identical according to diff(1). # multimedia/libx264 Unknown option --enable-asm, ignored Unknown option --disable-debug, ignored Unknown option --enable-opencl, ignored # multimedia/x264 Unknown option --enable-asm, ignored Unknown option --disable-debug, ignored Unknown option --enable-lsmash, ignored | |
| multimedia/x264/Makefile | ||
| 106 | I thought it was intentional... | |
| multimedia/x264/files/patch-configure | ||
| 21 | Yes, superseded by ce0757d9d277. | |
| 49 | Note, this may need upstreaming. I miss the point why they need to limit OpenCL availability per platform or why LoadLibraryW() cannot fail gracefully like dlopen(). | |
Comment Actions
Herald is a bit too pessimistic. According to Mk/bsd.options.desc.mk:
# - This file is maintained by ports@FreeBSD.org so that entries can be added # to it easily. Any sweeping changes should be approved by portmgr@. Options_Desc_MAINTAINER= ports@FreeBSD.org
Comment Actions
You're welcome Jan. Rubberstamp approvals are worse than no reviews at all. I review "commits requests", not just code changes. I can't wait for us as a project to be/do better.
Your attention to detail, doing the right thing and having done your due diligence on proposed changes is noted, please commit as you see fit (including the PORTREVISION=0 if so desired) and when you're confident of the change.
Comment Actions
Additionally I welcome updates to improve consistency of the port that should (rightly) be in separate commits. You have my blessing to make those changes without review (though I am happy to review if you would like to)