It/no http #2051

darkdarkdragon · 2021-10-04T02:22:22Z

What does this pull request do? Explain your changes. (required)
Removes HTTP and HTTPS protocols from FFmpeg build.

Specific updates (required)

How did you test each of these updates (required)

Does this pull request close any open issues?
Fixes #2040

Checklist:

Read the contribution guide
make runs successfully
All tests in ./test.sh pass
README and other documentation updated
Pending changelog updated

prepare_mingw64.sh

iameli

No opinion on the Go changes, one comment on the build

iameli · 2021-10-12T22:17:49Z

prepare_mingw64.sh

-done
-# And one more dynamic library to remove, this one in the system itself...
-rm -rfv /mingw64/x86_64-w64-mingw32/lib/libwinpthread.dll.a
+echo "removing all dlls"


Nice. I guess I just assumed that something would break when you did this, but I guess .dll.a files are only needed for making new dynamic binaries, which we're not doing? Cool.

Looks like it 😄 Without that it was linking some dynamic libs, and it was not clear where it finds those .dll.a so I've just removed everything. And it works 😃

iameli · 2021-10-12T22:20:33Z

install_ffmpeg.sh

@@ -37,8 +44,7 @@ if [ $(uname) != "Darwin" ]; then
  fi
 fi

-# Static linking of gnutls on Linux/Mac
-if [[ $(uname) != *"MSYS"* ]]; then
+if [[ $(uname) != *"MSYS"* ]] && [[ ! $IS_M1 ]]; then
  if [ ! -e "$ROOT/nasm-2.14.02" ]; then


So how do we handle NASM on the M1? Homebrew? Is it only a build-time dependency?

Is it only a build-time dependency?

If I understand it correctly, nasm only needed to build x264. And newest x264 builds correctly on M1 without nasm installed (I suspect it uses MacOs native assembler - at least configure of x264 showed that it found assembler).

victorges

No opinion about the go changes either as I don't think I have context on the code. Wasn't able to tell if it was just a refactor or a functional change, or if it is related to the HTTP(S) removal from ffmpeg.

The ffmpeg part LGTM with a small suggestion

install_ffmpeg.sh

server/ot_rpc.go

leszko

LGTM 👍

Added some minor comments, other than that, it looks good.

leszko · 2021-12-08T11:02:44Z

docker/Dockerfile.build-linux

@@ -13,7 +13,7 @@ RUN apt-get update \
  && apt-key adv --keyserver keyserver.ubuntu.com --recv 15CF4D18AF4F7421 \
  && add-apt-repository "deb [arch=amd64] http://apt.llvm.org/xenial/ llvm-toolchain-xenial-8 main" \
  && apt-get update \
-  && apt-get -y install clang-8 clang-tools-8 build-essential pkg-config autoconf gnutls-dev sudo git python docker-ce-cli


Don't you need to also update the official Livepeer doc that gnutls is not needed anymore?

https://livepeer.org/docs/installation/install-livepeer

leszko · 2021-12-08T11:27:04Z

server/ot_rpc.go


 	md, err := coreSegMetadata(notify.SegData)
 	if err != nil {
-		glog.Error("Unable to parse segData err=", err)


I see you changed some error handling. E.g. here, before the md struct was sent to n.Transcoder.Transcode(md) and now, in case of this error, we don't even call n.Transcoder.Transcode(md). I guess the change is good. Just wanted to point it out.

leszko · 2021-12-08T11:34:05Z

server/ot_rpc.go

+	}
+	// Create input file from segment. Removed after transcoding done
+	fname = path.Join(n.WorkDir, common.RandName()+".tempfile")
+	if err = ioutil.WriteFile(fname, data, 0644); err != nil {


Wouldn't 0600 be enough, do we need other users to access this file?

leszko · 2021-12-08T11:37:51Z

prepare_mingw64.sh


 DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

 export MINGW_INSTALLS=mingw64
-pacman -S --noconfirm --noprogressbar --ask=20 --needed mingw-w64-x86_64-binutils mingw-w64-x86_64-gcc mingw-w64-x86_64-pkg-config mingw-w64-x86_64-go mingw-w64-x86_64-nasm mingw-w64-x86_64-clang git make autoconf automake patch libtool texinfo gtk-doc zip
+pacman -S --noconfirm --noprogressbar --ask=20 --needed mingw-w64-x86_64-binutils mingw-w64-x86_64-gcc mingw-w64-x86_64-pkg-config mingw-w64-x86_64-go mingw-w64-x86_64-nasm mingw-w64-x86_64-clang git make autoconf automake patch libtool texinfo gtk-doc zip zlib


Why do we need zlib now?

Don't remember 😄 Removed.

iameli · 2021-12-08T21:49:15Z

Tested on Windows 10 with split O/T and NVENC enabled, looking good! No zlib required apparently.

in Go code instead of providing direct http url to lpms.

Remove http and https protocols from FFmpeg configuration in install_ffmpeg.sh Use newer x264 lib on Apple Silicon

darkdarkdragon force-pushed the it/no-http branch from 3677f95 to fede962 Compare October 6, 2021 03:56

iameli reviewed Oct 6, 2021

View reviewed changes

prepare_mingw64.sh Outdated Show resolved Hide resolved

darkdarkdragon force-pushed the it/no-http branch from fede962 to 62d28bf Compare October 6, 2021 21:01

darkdarkdragon marked this pull request as ready for review October 7, 2021 00:43

darkdarkdragon requested review from iameli, jailuthra, kyriediculous and yondonfu October 7, 2021 00:43

iameli reviewed Oct 13, 2021

View reviewed changes

darkdarkdragon force-pushed the it/no-http branch from 62d28bf to 044b9d7 Compare October 29, 2021 13:27

iameli mentioned this pull request Nov 1, 2021

wip: arm64 Docker builds #2082

Closed

darkdarkdragon force-pushed the it/no-http branch from 044b9d7 to 7f116f0 Compare November 17, 2021 03:35

darkdarkdragon force-pushed the it/no-http branch from 7f116f0 to be5eb18 Compare November 30, 2021 21:58

iameli requested a review from victorges December 3, 2021 21:21

yondonfu requested review from leszko and removed request for kyriediculous December 7, 2021 18:03

victorges approved these changes Dec 7, 2021

View reviewed changes

install_ffmpeg.sh Outdated Show resolved Hide resolved

victorges reviewed Dec 7, 2021

View reviewed changes

server/ot_rpc.go Show resolved Hide resolved

leszko approved these changes Dec 8, 2021

View reviewed changes

leszko reviewed Dec 8, 2021

View reviewed changes

darkdarkdragon added 3 commits December 9, 2021 00:15

In standalone transcoder download segment to disk

a3c0459

in Go code instead of providing direct http url to lpms.

Remove GnuTLS from install_ffmpeg.sh

fab8162

Remove http and https protocols from FFmpeg configuration in install_ffmpeg.sh Use newer x264 lib on Apple Silicon

use lpms 644122186d24b45060e56af7fdaad04c0f00950b

464084d

darkdarkdragon force-pushed the it/no-http branch from e2c68b3 to 464084d Compare December 8, 2021 22:15

CHANGELOG_PENDING: Removes HTTP and HTTPS protocols from FFmpeg build

fa26d74

darkdarkdragon merged commit 9b40b94 into master Dec 8, 2021

darkdarkdragon deleted the it/no-http branch December 8, 2021 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

It/no http #2051

It/no http #2051

darkdarkdragon commented Oct 4, 2021

iameli left a comment

iameli Oct 12, 2021

darkdarkdragon Oct 13, 2021

iameli Oct 12, 2021

darkdarkdragon Oct 13, 2021

victorges left a comment

leszko left a comment

leszko Dec 8, 2021

leszko Dec 8, 2021

leszko Dec 8, 2021

darkdarkdragon Dec 8, 2021

leszko Dec 8, 2021

darkdarkdragon Dec 8, 2021

iameli commented Dec 8, 2021 •

edited

Loading

It/no http #2051

It/no http #2051

Conversation

darkdarkdragon commented Oct 4, 2021

iameli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorges left a comment

Choose a reason for hiding this comment

leszko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iameli commented Dec 8, 2021 • edited Loading

iameli commented Dec 8, 2021 •

edited

Loading