multimedia/x264: update to 0.148.2795
ClosedPublic

Authored by jbeich on Wed, Jul 12, 4:16 PM.

Details

Summary

See commit message. Maybe the maintainership should be assigned to multimedia.

Test Plan
  • poudriere bulk -t is green on 10.3 i386/amd64, 11.0 i386/amd64/aarch64, 11.1 aarch64/armv6, 12.0 amd64/armv6
  • poudriere bulk -t on 11.1 armv6 with CFLAGS += -march=armv7-a -mfpu=neon in Makefile.local
  • poudriere bulk -t on 10.3 i386 separately with PGO=on, DEBUG=on, HI10P=on, OPENCL=off, GPAC=on
$ x264 -o bar.mp4 foo.mp4
ffms [info]: 1280x720p 1:1 @ 24000/1001 fps (vfr)
x264 [info]: using SAR=1/1
x264 [info]: using cpu capabilities: ARMv8 NEON
x264 [info]: profile High, level 3.1
[0.0%] 14/32966 frames, 0.72 fps, 1040.07 kb/s, eta 12:47:02
^C
x264 [info]: frame I:1     Avg QP:14.96  size: 48492
x264 [info]: frame P:6     Avg QP:15.38  size:  3824
x264 [info]: frame B:9     Avg QP:20.55  size:   724
x264 [info]: consecutive B-frames:  5.9% 47.1%  0.0% 47.1%
x264 [info]: mb I  I16..4: 18.4% 77.6%  4.0%
x264 [info]: mb P  I16..4:  0.3%  1.0%  0.6%  P16..4: 19.7%  3.0%  2.2%  0.0%  0.0%    skip:73.2%
x264 [info]: mb B  I16..4:  0.0%  0.0%  0.1%  B16..8: 13.6%  0.1%  0.0%  direct: 0.2%  skip:86.0%  L0:14.0% L1:86.0% BI: 0.0%
x264 [info]: 8x8 transform intra:74.2% inter:89.1%
x264 [info]: coded y,uvDC,uvAC intra: 61.6% 85.3% 70.9% inter: 1.6% 6.4% 0.1%
x264 [info]: i16 v,h,dc,p: 62% 19% 16%  2%
x264 [info]: i8 v,h,dc,ddl,ddr,vr,hd,vl,hu: 10% 10% 65%  3%  3%  2%  2%  2%  2%
x264 [info]: i4 v,h,dc,ddl,ddr,vr,hd,vl,hu: 29% 19% 27%  3%  4%  4%  5%  4%  5%
x264 [info]: i8c dc,h,v,p: 63% 16% 17%  4%
x264 [info]: Weighted P-Frames: Y:0.0% UV:0.0%
x264 [info]: ref P L0: 76.1%  1.1% 16.2%  6.6%
x264 [info]: ref B L0: 71.6% 26.5%  1.9%
x264 [info]: ref B L1: 95.5%  4.5%
x264 [info]: kb/s:933.50

aborted at input frame 57, output frame 16
encoded 16 frames, 0.73 fps, 944.79 kb/s

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.
jbeich created this revision.Wed, Jul 12, 4:16 PM
jbeich updated this revision to Diff 30687.Wed, Jul 12, 4:20 PM

s/+=/=/ cruft from before ASM was moved to libx264

@jbeich Thanks for the update, nice work on QA target coverage.

See which commit message?

Please also:

  • Update SUMMARY to include commit log message with rationale/explanation of changes if not self-explanatory
  • Add comments to patches if/where relevant (in particular where they are upstream commit backports)
jbeich added a comment.EditedThu, Jul 13, 12:48 AM

See which commit message?

Revision Contents -> Commits -> Show More. GitHub has better UI (compare), especially for multi-commit reviews.

  • Update SUMMARY to include commit log message with rationale/explanation of changes if not self-explanatory

Are one-line explanations not enough? I guess, I can try to split changes into separate commits to add more verbosity/clarity. However, syncing with SUMMARY is going to be PITA.

  • Add comments to patches if/where relevant (in particular where they are upstream commit backports)

There're no usptream backports atm. Do you mean to add comments to the existing patches, even not mine?

jbeich updated this revision to Diff 30771.Fri, Jul 14, 4:47 AM

Split changes into 3 commits: update, move/limit ASM option, enable more options.

jbeich set the repository for this revision to rP FreeBSD ports repository.Fri, Jul 14, 4:49 AM
jbeich added inline comments.
multimedia/x264/files/patch-armv6
9 ↗(On Diff #30771)

Obsoleted by 0aed59e74. Besides, the port disables NEON by default by dropping -mcpu= from configure.

multimedia/x264/files/patch-configure
21 ↗(On Diff #30771)

Just sorting. Otherwise, see bug 208440 for the proper fix.

30 ↗(On Diff #30771)

Probably cruft from the times uname -p value wasn't stable. config.guess from both x264 distfile and /usr/ports/Templates/ shows aarch64.

48 ↗(On Diff #30771)

Stayed to support real armv6 like RPi (1st generation). Maybe @mikael.urankar_gmail.com can confirm if the hunk is still necassary. Otherwise, it's similar to -march=i686 removal above which is part of respecting CFLAGS.

82 ↗(On Diff #30771)

Probably cruft from when -Wunknown-warning-option was fatal for a brief time (circa Clang 3.6). However, before removing one needs to check the ancient GCC in base.

koobs added a comment.Fri, Jul 14, 8:21 AM

See which commit message?

Revision Contents -> Commits -> Show More. GitHub has better UI (compare), especially for multi-commit reviews.

Ah not many people use/do this given svn. Individual commit messages provided explanations, thanks. Please include them together in the differentials SUMMARY as the proposed commit log (i assume by default that a single review will be committed in one commit rather than separately even if it comprises multiple commits).

I treat code reviews (the differential revision) as a 'commit review', including the commit itself (formatting, meta information, etc), not just the code changes.

  • Update SUMMARY to include commit log message with rationale/explanation of changes if not self-explanatory

Are one-line explanations not enough? I guess, I can try to split changes into separate commits to add more verbosity/clarity. However, syncing with SUMMARY is going to be PITA.

That's fine, I just saw an empty SUMMARY (as in didnt contain a commit log message itself). Now that I can see where were multiple separate commits backing the review, it makes sense. If however, they will be committed together in one commit, not separately, I'd expect the the text in SUMMARY to contain the commit log message for the 'one commit' including all itemized changes, and what/why per change.

  • Add comments to patches if/where relevant (in particular where they are upstream commit backports)

There're no usptream backports atm. Do you mean to add comments to the existing patches, even not mine?

No, only if the new changes in patches weren't explained in the commit log message(s) (SUMMARY), 'and' they came from upstream/other sources: to include references to that source (pr, issue id, mailing list patch, whatever)

koobs added a comment.Fri, Jul 14, 8:27 AM

@jbeich I assume this is going to MFH due to 'bugfixes' in general, as no security context was mentioned (or I missed it)?

@jbeich I assume this is going to MFH due to 'bugfixes' in general, as no security context was mentioned (or I missed it)?

I base MFH requests based on associated risk. In this case we get some optimizations and bugfixes from upstream, bugfixes from downstream and some usability improvement for a leaf package. All of that combined still amounts to small risk. Or do you mean ports-secteam rubberstamps everything? ;)

koobs accepted this revision.EditedFri, Jul 14, 9:23 AM

@jbeich I assume this is going to MFH due to 'bugfixes' in general, as no security context was mentioned (or I missed it)?

I base MFH requests based on associated risk. In this case we get some optimizations and bugfixes from upstream, bugfixes from downstream and some usability improvement for a leaf package. All of that combined still amounts to small risk. Or do you mean ports-secteam rubberstamps everything? ;)

No, I mean that without (rationale) being made explicit its difficult to know/guess why otherwise :) This is particularly the case given (unfortunately) different developers opinions on what/why things should or shouldn't be merged without considering how it might devaluing the common/group good/effort. I merge without waiting for approval on most things because most things don't require it.

To be explicit, I lean towards merging anything other than feature-only, including version updates that are only/primarily bug fixes or 'improvements' But yes, other good ways to think about it is associated risk as you said, or in the context of user value: would users want/need/benefit from it ('latest version desire only' aside), which is my primary consideration.

TLDR: LGTM, (very) well tested, good rationale. syntactically fine. semantics (rationale/meaning/formatting) unclear and not immediately obvious on first read making review non trivial :)

This revision has a positive review.Fri, Jul 14, 9:23 AM
multimedia/x264/files/patch-configure
48 ↗(On Diff #30771)

Yes we still have to support the rpi1, the hunk is needed.

This revision was automatically updated to reflect the committed changes.