See commit message. Maybe the maintainership should be assigned to multimedia.
Details
Details
- Reviewers
koobs - Commits
- rP446081: MFH: r445836 r445837 r445838 r445839 r445840
rP445840: multimedia/x264: r445837 forgot to move option helpers
rP445838: multimedia/libx264: yasm is only used on x86, other archs use GNU as
rP445839: multimedia/x264: enable FFMS, LAVF, SWSCALE by default
rP445837: multimedia/x264: yasm is only used by libx264, so move ASM option there
rP445836: multimedia/{lib,}x264: update to 0.148.2795
- 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
Diff Detail
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 10492 Build 10902: arc lint + arc unit
Event Timeline
Comment Actions
@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)
Comment Actions
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?
| multimedia/x264/files/patch-armv6 | ||
|---|---|---|
| 9 | Obsoleted by 0aed59e74. Besides, the port disables NEON by default by dropping -mcpu= from configure. | |
| multimedia/x264/files/patch-configure | ||
| 21 | Just sorting. Otherwise, see bug 208440 for the proper fix. | |
| 30 | Probably cruft from the times uname -p value wasn't stable. config.guess from both x264 distfile and /usr/ports/Templates/ shows aarch64. | |
| 48 | 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 | 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. | |
Comment Actions
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)
Comment Actions
@jbeich I assume this is going to MFH due to 'bugfixes' in general, as no security context was mentioned (or I missed it)?
Comment Actions
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? ;)
Comment Actions
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 :)
| multimedia/x264/files/patch-configure | ||
|---|---|---|
| 48 | Yes we still have to support the rpi1, the hunk is needed. | |