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

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?

[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

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]

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.