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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jbeich retitled this revision from to multimedia/x264: update to 0.148.2699.Sep 20 2016, 1:18 AM
jbeich updated this object.
jbeich edited the test plan for this revision. (Show Details)
jbeich added a reviewer: koobs.
jbeich updated this revision to Diff 20511.
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 edited edge metadata.Sep 22 2016, 7:35 AM
koobs requested changes to this revision.

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?

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 ↗(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 edited edge metadata.Sep 22 2016, 5:02 PM
jbeich marked 5 inline comments as done.
jbeich updated this revision to Diff 20622.

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 edited edge metadata.Sep 23 2016, 10:51 AM
koobs accepted this revision.

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.