Page MenuHomeFreeBSD

multimedia/x264: update to 0.148.2708
ClosedPublic

Authored by jbeich on Sep 20 2016, 1:18 AM.

Details

Summary
  • 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
Test Plan

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

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5247
Build 5403: arc lint + arc unit

Event Timeline

jbeich updated this revision to Diff 20511.Sep 20 2016, 1:18 AM
jbeich retitled this revision from to multimedia/x264: update to 0.148.2699.
jbeich updated this object.
jbeich edited the test plan for this revision. (Show Details)
jbeich added a reviewer: koobs.
jbeich edited the test plan for this revision. (Show Details)Sep 20 2016, 1:21 AM
  • 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
jbeich updated this revision to Diff 20578.Sep 21 2016, 12:16 PM

New snapshot (9 commits). Green on 93i386, 93amd64, 101i386, 103amd64, 110i386 and for ffmpeg{,0}.

jbeich updated this revision to Diff 20597.Sep 21 2016, 9:40 PM
  • 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.
jbeich added a comment.EditedSep 21 2016, 9:53 PM

Oops, phabricator didn't split commits that had PORTREVISION bumps but creating separate reviews would make reviewer lose the big picture.

koobs requested changes to this revision.Sep 22 2016, 7:35 AM
koobs edited edge metadata.

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)...

  • Is it enabled in a default build? (when just running ./configure)
  • Was an OpenCL build tested? Test plan doesn't list that specifically

Perhaps its worth trialing as a separate port (libx264-opencl) so that we can get it user QA'd "in the field", thoughts?

19–21

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–107

Remove trailing \

multimedia/x264/files/patch-configure
21

Was this upstreamed?

This revision now requires changes to proceed.Sep 22 2016, 7:35 AM
koobs added a comment.Sep 22 2016, 7:39 AM
  • 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.

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)

jbeich retitled this revision from multimedia/x264: update to 0.148.2699 to multimedia/x264: update to 0.148.2708.Sep 22 2016, 4:58 PM
jbeich updated this object.
jbeich edited the test plan for this revision. (Show Details)
jbeich edited edge metadata.
jbeich marked 5 inline comments as done.

I didn't expect such a detailed review after being desensitized by rubberstamp approvals. Thanks.

In D7958#165452, @koobs wrote:

Please also update SUMMARY to include itemized changes to patches incl. rationale/reasons.

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
  • OpenCL is enabled upstream by default if the headers are available, before the port explicitly disabled it.
  • Sure. Runtime testing is more tricky as lang/beignet appears to be buggy on drm-next-4.7 while nvidia-driver only supports for OpenCL for linux binaries.
  • Separate ports only make sense for options that cannot be disabled at runtime or carry a lot of bloat.
19–21
  • OPENCL_DESC was meant to be generic. The next step is moving into Mk/bsd.options.desc.mk. OpenCL isn't limited to GPU (see devel/pocl) while SSE2, AVX, FMA3 would be also hardware acceleration.
  • No! Let dlopen(3) (see 3aa9a67b6d62) take care of choosing OpenCL implementation: ocl-ocd or freeocl.
  • _ENABLE does work[1] but applying only to OPENCL wouldn't be consistent with x264/Makefile.

[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–107

I thought it was intentional...

multimedia/x264/files/patch-configure
21

Yes, superseded by ce0757d9d277.

36

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().

jbeich updated this revision to Diff 20622.Sep 22 2016, 5:02 PM
jbeich edited edge metadata.
jbeich marked 5 inline comments as done.

Addressed review points. See my reply before for details.

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
jbeich edited the test plan for this revision. (Show Details)Sep 22 2016, 5:14 PM
koobs accepted this revision.Sep 23 2016, 10:51 AM
koobs edited edge metadata.

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.

This revision is now accepted and ready to land.Sep 23 2016, 10:51 AM

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)

This revision was automatically updated to reflect the committed changes.

Did you intend to close this with the commit only changing bsd.options.desc.mk ?

No. If you check rP422670, rP422671, rP422672, rP422674, rP422675 they all had the same Differential Revision line as rP422673. It appears Phabricator doesn't auto-close review unless the line is the last in a commit message.