multimedia/x264: update to 0.148.2708

[jbeich]

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

[jbeich]

• 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]

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

[jbeich]

• 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]

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

[koobs]

Please also update SUMMARY to include itemized changes to patches incl. rationale/reasons. Hard to understand change context without them.

multimedia/libx264/Makefile

6 ↗

(On Diff #20597)

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 ↗

(On Diff #20597)

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 ↗

(On Diff #20597)

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?

20–22 ↗

(On Diff #20597)

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 ↗

(On Diff #20597)

Remove trailing \

multimedia/x264/files/patch-configure

21 ↗

(On Diff #20597)

Was this upstreamed?

[koobs]

In D7958#165387, @jbeich wrote:

• 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]

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 ↗

(On Diff #20597)

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 ↗

(On Diff #20597)

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 ↗

(On Diff #20597)

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

20–22 ↗

(On Diff #20597)

• 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 ↗

(On Diff #20597)

I thought it was intentional...

multimedia/x264/files/patch-configure

21 ↗

(On Diff #20597)

Yes, superseded by ce0757d9d277.

49 ↗

(On Diff #20597)

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]

Addressed review points. See my reply before for details.

[jbeich]

Herald is a bit too pessimistic. According to Mk/bsd.options.desc.mk:

# - This file is maintained by [e-mail] so that entries can be added
#   to it easily.  Any sweeping changes should be approved by portmgr@.

Options_Desc_MAINTAINER=        [e-mail]

[koobs]

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.

[koobs]

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)

[koobs]

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

[jbeich]

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.