Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addition of package HandBrake CLI #3937

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jvega1976
Copy link

Motivation: I have been able to build the command line program of the famous open-source video transcoder program: HandBrake. I also builded it with the Intel Quick Sync option. I build it specifically for the ApolloLake architecture, but I assume it should work OK with the other x86_64 architectures. So far it works for me like a charm, and the QSV encoding works very fast and a produce files with a very good quality. Specially, it make very easy the encoding of DVD's and Blue-rays.
Linked issues: To accomplish the HandBrake compilation I had to update the FFmpeg configuration options (HandBrake require to build FFmpeg with a very specific set of options), for this, I made use of a new variable (HANDBRAKE) to determine what configurations options should be used, this variable is automatically assigned when HandBrake is being build.
It also required the addition of the new packages: libdvdread, Libdvdnav, dav1d, harfbuzz and jansson.
Package FFmpeg is updated to 4.2.2

Checklist

  • Build rule all-supported completed successfully
  • Package upgrade completed successfully
  • New installation of package completed successfully

jvega1976 and others added 6 commits October 12, 2018 23:16
dependencies:
libdvdread
libdvdnav
dav1d
harfbuff
jansson
ffmpeg updated to 4.2.2
addition of new package HandBrake CLI (Command Line Interface)
update of ffmpeg configuration options according to HandBrake package requirement.
Update of ffmpeg to version 4.2.2
@hgy59
Copy link
Contributor

hgy59 commented Apr 4, 2020

@jvega1976 Welcome and thanks for this contribution.

Please create a new branch and do not create a pull request from your master branch. Otherwise you will get troubles to update your fork from the upstream repo and to create other (independent) PRs from your fork.
I just updated the Pull Request section on the CONTRIBUTING page, to give this information and a link to an external page on How to make a clean pull request.

@hgy59
Copy link
Contributor

hgy59 commented Apr 4, 2020

Please contact @th0ma7 on updating ffmpeg. There exists PR #3873 for this.

Copy link
Contributor

@hgy59 hgy59 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this package is for x64 archs only, this is obsolete. IMHO it would be better to use a docker container for this.

This PR changes some common cross modules that are related to other packages that need to be verified with build and functional testing.

  • cross/bzip2 (this change breaks all 32-bit archs if not adjusted)
  • cross/ffmpeg (revert changes and wait for official update with PR FFMPEG 4.2.2: Update, fixes and cleanup #3873)
  • cross/fontconfig
  • cross/freetype
  • cross/glib (this is a breaking change for all dependent packages, as introduces depependency on native/python3)
  • cross/gmmlib
  • cross/intel-media-driver
  • cross/intel-media-sdk
  • cross/libaom
  • cross/libva-utils
  • cross/libxml2
  • cross/util-linux
  • cross/x265

General request:
revert the remove of targets in cross module PLIST files, as other packages may depend on it.

  • cross/libaom
  • cross/libva-utils

@@ -15,7 +15,7 @@ CONFIGURE_TARGET = nop
COMPILE_TARGET = bzip2_compile
INSTALL_TARGET = bzip2_install

ADDITIONAL_CFLAGS = -fpic -fPIC -O3 -D_FILE_OFFSET_BITS=64
ADDITIONAL_CFLAGS = -m64 -fpic -fPIC -O3 -D_FILE_OFFSET_BITS=64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the flag -m64 for 64-bit archs only

@@ -75,7 +75,7 @@ tc_vars: patch
@echo TC_ENV += CPPFLAGS=\"$(CPPFLAGS) $$\(ADDITIONAL_CPPFLAGS\)\"
@echo TC_ENV += CXXFLAGS=\"$(CXXFLAGS) $$\(ADDITIONAL_CXXFLAGS\)\"
@echo TC_ENV += LDFLAGS=\"$(LDFLAGS) $$\(ADDITIONAL_LDFLAGS\)\"
@echo TC_CONFIGURE_ARGS := --host=$(TC_TARGET) --build=i686-pc-linux
@echo TC_CONFIGURE_ARGS := --host=$(TC_TARGET) --build=x86_64-pc-linux
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change.
The official development environment uses i686 and not x64 for toolchain builds.

STARTABLE = no

HOMEPAGE = https://handbrake.fr
LICENSE = GPLv3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please set the LICENSE according to cross/HandBrakeCLI/Makefile
without ENABLE_FDK_AAC=YES: LICENSE = GPLv2
with ENABLE_FDK_AAC=YES:
this should be something like: LICENSE = for personal use of <your name and email>

A better approach would be to isolate the build with ENABLE_FDK_AAC=YES to /diyspk/HandBrakeCLI/Makefile, as this is for do it yourself builds only.
And /spk/HandBrakeCLI/Makefile with ENABLE_FDK_AAC=YES must not build and terminate with an appropriate error.

@@ -0,0 +1,206 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file must be empty as all target files are taken from the dependent cross modules.

@@ -0,0 +1,203 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file.
All targets are normally included by dependent cross modules

Comment on lines +44 to +46
CONFIGURE_TARGET = myConfigure
COMPILE_TARGET = myCompile
INSTALL_TARGET = myInstall
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use correct naming <packagename>_target as:
HandBrake_configure
HandBrake_compile
HandBrake_install

Please apply this to all your new Makefiles

PKG_VERS = 1.3.1
PKG_EXT = tar.bz2
PKG_DIST_NAME = $(PKG_NAME)-$(PKG_VERS)-source.$(PKG_EXT)
PKG_DIST_SITE = https://github.com/HandBrake/HandBrake/releases/download/1.3.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use $(PKG_VERS) in url to avoid conflicts.

Comment on lines +78 to +81
.PHONY: myCompile
myCompile:
$(RUN) DESTDIR=$(INSTALL_DIR) ./configure $(CONFIGURE_ARGS)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use HandBreak_configure for calling ./configure
And define HandBreak_compile = nop to suppress the compile step, instead of defining a target which does nothing.

Probably you have to use $(REAL_CONFIGURE_ARGS) instead of $(CONFIGURE_ARGS) to include toolchain specific args.

May be you can use the default configure task with additional definition of CONFIGURE_ARGS += DESTDIR=$(INSTALL_DIR)? (didn't try myself).

Comment on lines +22 to +25
.PHONY: myPreCopy
myPreCopy:
cp PLIST $(WORK_DIR)/PLIST

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PLIST is handled by build framework and must not be copied like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants