https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #1 from Nicolas Chauvet <kwizart(a)gmail.com&gt; --- Few comments: - Does it really uses the bundled fdk-aac instead of the internal ffmpeg aac encoder ? # Use system libraries in place of bundled ones for module in a52dec %{?_with_fdk:fdk-aac} ... - We cannot rely on this currently (we might need to restart the discussion) At least it should be dlopenable. Requires: libdvdcss%{_isa} - What is the point to have -gui -tui separation without any main package ? I would use handbrake (main) as the default (-gui) package and use -core or keep -tui as a sub-package. (even using a virtual provide for lower case handbrake compatibility if not handled by dnf already). - Using , %make_build, %make_install, missing BR gcc, gcc-c++ - Remove dependency that are in the default BR (m4, patch, Makefile, etc) - No appdata ? Thx for this submit. -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #3 from Dominik 'Rathann' Mierzejewski <dominik(a)greysector.net&gt; --- Spec URL: https://www.greysector.net/~rathann/review/HandBrake/HandBrake.spec SRPM URL: https://www.greysector.net/~rathann/review/HandBrake/HandBrake-1.0-34.201... Rebased my changes on top of Simone's 1.0-32.20161129gitfac5e0e: - Update to latest sources. - Use make_build macro. - Fix BRs and drop redundant ones. - libva is required to build with QSV. - Soft dependency on gstreamer1-plugins-good for live preview. - Change hard dependency on libdvdcss to soft. - libmfx is x86-only. - Move -cli to the main package. - Drop support for building with libav. -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #5 from Dominik 'Rathann' Mierzejewski <dominik(a)greysector.net&gt; --- (In reply to Nicolas Chauvet from comment #4) Yes, it's called that way just about everywhere, including their website and github repo. However, now that I went back and re-checked their website, I see there's finally been a 1.0.x release, so I'll roll a package based on that. Sure thing. -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #7 from Dominik 'Rathann' Mierzejewski <dominik(a)greysector.net&gt; --- (In reply to Simone Caronni from comment #6) It seems to be referenced here unconditionally if QSV is enabled: test/module.defs: ifeq (1,$(FEATURE.qsv)) TEST.GCC.D += USE_QSV HAVE_THREADS=1 ifeq ($(BUILD.system),linux) TEST.GCC.l += va va-drm endif endif I don't see any option to disable it via configure command line and since it doesn't hurt anything apart from a small additional build dependency, I put it in. If you want, feel free to patch it out. Sure. -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #9 from Simone Caronni <negativo17(a)gmail.com&gt; --- Sorry for the delay, I've been busy. Unfortunately even with the latest commits from the master branch or the 1.0.2 release, any UTF-8 subtitle is not detected. All my Anime collection is pretty much off. Looking at the libav/libmfx part. -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #11 from Dominik 'Rathann' Mierzejewski <dominik(a)greysector.net&gt; --- Spec URL: https://www.greysector.net/~rathann/review/HandBrake/HandBrake.spec SRPM URL: https://www.greysector.net/~rathann/review/HandBrake/HandBrake-1.0.2-1.fc... - update to 1.0.2 - avoid unnecessary libva dependency I have one problem, though. The -debuginfo package is empty and I'm not sure why. I can see the following errors during build: + /usr/lib/rpm/find-debuginfo.sh --strict-build-id -m --run-dwz --dwz-low-mem-die-limit 10000000 --dwz-max-die-limit 110000000 /builddir/build/BUILD/HandBrake-1.0.2 extracting debug info from /builddir/build/BUILDROOT/HandBrake-1.0.2-1.fc24.x86_64/usr/bin/ghb readelf: Error: the dynamic segment offset + size exceeds the size of the file extracting debug info from /builddir/build/BUILDROOT/HandBrake-1.0.2-1.fc24.x86_64/usr/bin/HandBrakeCLI readelf: Error: the dynamic segment offset + size exceeds the size of the file Looks like https://bugzilla.redhat.com/show_bug.cgi?id=1412348 ... -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #13 from Xavier Bachelot <xavier(a)bachelot.org&gt; --- A couple notes before a full review (fedora-review doesn't like me for whatever reason...) - Version 1.0.3 available - HandBrake-debuginfo.x86_64: E: debuginfo-without-sources - Turn "Recommends: libdvdcss" into "%{?fedora:Recommends: libdvdcss}" to make it work for EL. - Same for "Recommends: gstreamer1-plugins-good" Also, the libbluray patch is in libbluray 1.0.0 which has just been released, but this release bumps the soname. -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #15 from Xavier Bachelot <xavier(a)bachelot.org&gt; --- Created attachment 1749 --> https://bugzilla.rpmfusion.org/attachment.cgi?id=1749&action=edit Full review This is the edited review.txt generated with fedora-review. -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #17 from Dominik 'Rathann' Mierzejewski <dominik(a)greysector.net&gt; --- (In reply to Xavier Bachelot from comment #16) That was brilliant! I blame the HB's custom configure script written in python. We can also drop the clip_id patch after libbluray is updated to 1.0.0. Regarding BuildRequires: gcc make, I don't think I should drop them. The fedora-review recommendation is out of date. See https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_2 . The link to https://fedorahosted.org/FedoraReview/wiki/AutoTools doesn't exist anymore, so I'm not sure what the recommendation is. Anyway, here's an updated RPM with your patch applied. Spec URL: https://www.greysector.net/~rathann/review/HandBrake/HandBrake.spec SRPM URL: https://www.greysector.net/~rathann/review/HandBrake/HandBrake-1.0.3-1.fc... -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 Xavier Bachelot <xavier(a)bachelot.org&gt; changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flags| |fedora-review+ --- Comment #19 from Xavier Bachelot <xavier(a)bachelot.org&gt; --- (In reply to Dominik 'Rathann' Mierzejewski from comment #17) It took my quite some time and several attempts to figure it out. I was really puzzled at this issue and the solution came from the specfile included in HandBrake. I think it will need to be conditionally applied to %fedora >= 26, as I'll keep libbluray 0.9.3 for %fedora <= 25 and %rhel. Yes, agreed. It doesn't hurt anyway and seems more future proof. Probably the best to do is just file a bug upstream. Thanks, I will take another look this evening but consider the package APPROVED. Sorry it took me so long to do the review. And also, hopefully 1.0.3 helps with Simone's subtitle issue, but this is upstream's job to fix it, and it should not hold us from importing the package (but given the writing on downstream distribution of HandBrake on their website, I don't expect much from them... What a pity...). -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #21 from Dominik 'Rathann' Mierzejewski <dominik(a)greysector.net&gt; --- I opened new package requests (for master and f25) in pkgdb, but syncing from bugzilla doesn't work. I get an error about invalid title (no - present). -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #23 from Richard <hobbes1069(a)gmail.com&gt; --- I just rebuilt my version of the package which uses all bundled versions of libraries and encoding works... -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #25 from Dominik 'Rathann' Mierzejewski <dominik(a)greysector.net&gt; --- It does seem to work on rawhide with FFmpeg 3.2.4. So I'm guessing this is a bug in FFmpeg. I'll retry with FFmpeg 3.2.x on F25 later. -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 --- Comment #27 from Nicolas Chauvet <kwizart(a)gmail.com&gt; --- (In reply to Dominik 'Rathann' Mierzejewski from comment #26) If yoou don't test on ffmpeg-3.1.x that is in f25 I'm going to discard the update on Handbrake build on f25 -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 Nicolas Chauvet <kwizart(a)gmail.com&gt; changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|33 | Referenced Bugs: https://bugzilla.rpmfusion.org/show_bug.cgi?id=33 [Bug 33] Tracker: Packages awaitting CVS Administration -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=4361 Nicolas Chauvet <kwizart(a)gmail.com&gt; changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|2 | Referenced Bugs: https://bugzilla.rpmfusion.org/show_bug.cgi?id=2 [Bug 2] Tracker: New packages awaiting review -- You are receiving this mail because: You are on the CC list for the bug. You are the assignee for the bug.