Update multimedia/handbrake to 1.1.0
ClosedPublic

Authored by fernape on Apr 13 2018, 10:17 PM.

Details

Summary

Related to PR 227374
Submitter (naito.yuichiro@gmail.com) wants to take maintainership of the port

Test Plan
  • portlint -AC OK
  • poudriere builds {10.3,10.4,11.1}{amd64,i386}, 12i386 OK
  • run test in 12i386 OK

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.
fernape created this revision.Apr 13 2018, 10:17 PM
tcberner added inline comments.Apr 14 2018, 6:30 AM
multimedia/handbrake/Makefile
5 ↗(On Diff #41454)

^ as the line already changes, you can switch it to DISTVERSION.

14 ↗(On Diff #41454)

ONLY_FOR_ARCH=i386 amd64 seems more idiomatic to express that.
Porters Handbook

101 ↗(On Diff #41454)

This whole option block should be changed to use the options helpers

fernape updated this revision to Diff 41488.Apr 15 2018, 5:22 PM

Adressing issues pointed out by tcberner:

  • Use ONLY_FOR_ARCHS
  • Convert to use option helpers

Can you help me out with this? What am I doing wrong?
The --disable-gst and --disable-gtk-update-checks are not added to
CONFIGURE_ARGS. I didn't use OPT_CONFIGURE_ENABLE because I want the exact
opposite logic, this is to disable when X11 is set. portlint doesn't complain
and the build logs don't show any obviuos errors :S

tcberner added a comment.EditedApr 15 2018, 6:00 PM

The options are X11, not MX11 :), so it should be X11_USES and so on

The M came from the :M expansion of make: Variable modifiers

fernape updated this revision to Diff 41492.Apr 15 2018, 9:04 PM
  • Fix X11 option block.
  • Use DISTVERSION instead of PORTVERSION
naito.yuichiro_gmail.com added inline comments.
multimedia/handbrake/Makefile
100 ↗(On Diff #41492)

Excuse me.
I'm Yuichiro NAITO, submitter of this patch.
Why did you add --disable-gst ?
It disables preview feature of HandBrake.
Are there any problem for this feature?

tcberner added inline comments.Apr 16 2018, 2:56 PM
multimedia/handbrake/Makefile
100 ↗(On Diff #41492)

It seems to be already there in the current version in the ports tree:
CONFIGURE_ARGS+= --disable-gtk-update-checks --disable-gst

fernape added inline comments.Apr 16 2018, 5:48 PM
multimedia/handbrake/Makefile
100 ↗(On Diff #41492)

Hi,

I compiled on 11.1 with and without --disable-gst. In both cases, the preview of the main window show a frame of the video. Clicking on "show preview" plays that only frame for the length of the video (mp4). It also warns about some gstreamer missing plugins.

Can you confirm this behavior?

multimedia/handbrake/Makefile
100 ↗(On Diff #41492)

No, I can't see any warnings on my HandBrake except of 'inhibited 0'.
'inhibited 0' is a debug message that upsteram missed deleting before 1.1.0 release.
Could you show me the message?
I believe preview requires gstreamer1-plugins-gdkpixbuf.
I want to check if another plugin is required or not.

fernape added inline comments.Apr 17 2018, 4:00 PM
multimedia/handbrake/Makefile
100 ↗(On Diff #41492)

This is the first msg:
https://www.dropbox.com/s/nadocvs1cwoiof1/msg.png?dl=0

Then, a second one shows up:
https://www.dropbox.com/s/7jut7j2bw2c5n4c/msg2.png?dl=0

In this system I have the following gstreamer packages:

$ pkg info -x 'gstreamer.*'
gstreamer-0.10.36_6
gstreamer-plugins-0.10.36_8,3
gstreamer1-1.12.3
gstreamer1-plugins-1.12.3
gstreamer1-plugins-gdkpixbuf-1.12.3
gstreamer1-plugins-good-1.12.3

In the console I see only this message:
$ ./HandBrake
Xlib: extension "GLX" missing on display ":0.0".

multimedia/handbrake/Makefile
100 ↗(On Diff #41492)

Thank you so much.
I found gstreamer1-libav plugin is needed for preview.
Gstreamer1-libav plugin depends on ffmpeg package.
It seems redundant because HandBrake is a video encoder, too.
But I don't mind the redundancy.
GUI interface includes preview feature is important.
So please include libav plugin like as following line.

"X11_USE= gstreamer1=gdkpixbuf,libav"

If someone claims the redundancy,
I will make a port option to switch preview on/off.
It's a future work not now.

fernape updated this revision to Diff 41627.Apr 18 2018, 4:22 PM
  • Remove --disable-gst option
  • Add gstreamer libav dependency
fernape added inline comments.Apr 18 2018, 5:33 PM
multimedia/handbrake/Makefile
100 ↗(On Diff #41492)

It works for me now.

Thanks!

tcberner accepted this revision.Apr 18 2018, 5:53 PM

Looks good to me. If you are also fine with it @naito.yuichiro_gmail.com as the maintainer, this is good to go.

This revision is now accepted and ready to land.Apr 18 2018, 5:53 PM
This revision was automatically updated to reflect the committed changes.
mat added inline comments.Mon, May 14, 10:26 PM
head/multimedia/handbrake/Makefile
15

Add ONLY_FOR_ARCHS_REASON with what was in all the BROKEN_arch before.

fernape reopened this revision.Tue, May 15, 3:57 PM

Reopening to address mat's comment.

fernape updated this revision to Diff 42580.Tue, May 15, 4:08 PM

Add ONLY_FOR_ARCHS_REASON.

Pointed out by mat@

tcberner accepted this revision.Wed, May 16, 5:42 AM

Looks good.

This revision is now accepted and ready to land.Wed, May 16, 5:42 AM
This revision was automatically updated to reflect the committed changes.