Page MenuHomeFreeBSD

multimedia/x264: update to 0.148.2795
ClosedPublic

Authored by jbeich on Jul 12 2017, 4:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 18 2024, 8:15 AM
Unknown Object (File)
Feb 11 2024, 11:45 PM
Unknown Object (File)
Feb 6 2024, 1:59 PM
Unknown Object (File)
Jan 26 2024, 5:14 PM
Unknown Object (File)
Jan 15 2024, 3:21 AM
Unknown Object (File)
Dec 20 2023, 6:56 AM
Unknown Object (File)
Dec 9 2023, 6:49 PM
Unknown Object (File)
Dec 9 2023, 6:37 PM
Subscribers

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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)

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?

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

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.

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)

@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? ;)

@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 is now accepted and ready to land.Jul 14 2017, 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.