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

mpv, ffmpeg: add some removed options as dependencies #36327

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@deus0ww
Copy link
Contributor

deus0ww commented Jan 23, 2019

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@fxcoudert

This comment has been minimized.

Copy link
Member

fxcoudert commented Jan 23, 2019

Based on what? Why those options and not others?

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Jan 23, 2019

I'm ok with adding all the previous options as hard dependencies if that's what it takes to un-!@#$ my build.

@fxcoudert

This comment has been minimized.

Copy link
Member

fxcoudert commented Jan 23, 2019

Please give context.

@SMillerDev

This comment has been minimized.

Copy link
Contributor

SMillerDev commented Jan 23, 2019

Dependencies shouldn't be added to "un-!@#$ my build" but because there's a distinct advantage to all homebrew users weighed against the implications of pulling in extra dependencies imho.

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Jan 23, 2019

Context: I'm using MPV and FFmpeg on my HTPC. Why these options are important:

FFmpeg:

  • AOM - I need AV1 support (for files or youtube-dl)
  • soxr - Higher quality audio sample rate conversion
  • rubberband - Higher quality audio speed change (this one is less important than the rest)

MPV:

  • uchardet - Support for non-UTF-8 international subtitles (like Thai, which I use).
  • libarchive - So I can open files in archives without extracting
@fxcoudert

This comment has been minimized.

Copy link
Member

fxcoudert commented Jan 23, 2019

From what I can see, libarchive, uchardet, libsoxr and aom have low disk cost. rubberband has recursive dependencies, though.

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Jan 23, 2019

I can live with just libarchive, uchardet, libsoxr and aom.

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Jan 23, 2019

Can I get a verdict on this? I'm not asking for when but if the PR will be accepted. In the short-term, I'll create a tap. However, if there's no interest at all, I might as well start looking for more permanent alternative solutions now.

@SMillerDev

This comment has been minimized.

Copy link
Contributor

SMillerDev commented Jan 24, 2019

Can you squash everything into one commit, ffmpeg: add libarchive, uchardet, libsoxr and aom as dependencies

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Jan 24, 2019

This is my first time doing a squash. Is the result what you're looking for?

@SMillerDev SMillerDev force-pushed the deus0ww:master branch from f495495 to 65637de Jan 24, 2019

@SMillerDev

This comment has been minimized.

Copy link
Contributor

SMillerDev commented Jan 24, 2019

It wasn't, I fixed it for you now.

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Jan 24, 2019

Thank you. My git skill is sorely lacking.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Jan 25, 2019

Please add back fdk-aac. AAC is one of the (two) most widely audio codecs and fdk-aac was the de facto standard of AAC encoding with FFmpeg for years (see https://trac.ffmpeg.org/wiki/Encode/AAC, also search for AAC on https://ffmpeg.org/index.html#news). FFmpeg's builtin AAC encoder only moved out of experimental stage in December 2015, and fdk-aac is still the go-to for HE-AAC. Removing fdk-aac breaks a hell lot of encoding/transcoding scripts.

EDIT: To save you some time reading the Wiki, let me quote this:

The Fraunhofer FDK AAC codec library. This is currently the highest-quality AAC encoder available with ffmpeg.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Jan 25, 2019

By the way, I'm toying with the idea to start a separate org to host more advanced formulae and keep options alive. If there are similar-minded past maintainer / power users out there, feel free to ping me (email's on my profile).

@mlindner

This comment was marked as disruptive content.

Copy link

mlindner commented Jan 28, 2019

fxcoudert Please don't remove options from mpv. Your previous commit here is BAD! 1fdd9b1

Please don't remove working options just to suit your personal fantasies. I don't know what the hell you were thinking.

@mlindner

This comment has been minimized.

Copy link

mlindner commented Jan 28, 2019

This pull request doesn't go far enough. Please revert all the changes in 1fdd9b1 as well please.

@SMillerDev

This comment was marked as off-topic.

Copy link
Contributor

SMillerDev commented Jan 28, 2019

@mlindner please read the code of conduct. This isn't the issue for this and #31510 has the explanation you are looking for.

@mlindner

This comment was marked as off-topic.

Copy link

mlindner commented Jan 28, 2019

@SMillerDev That's not a satisfactory answer. Homebrew has been making many bad decisions like this of late. I'm frustrated because I can't go back to my previous situation because homebrew breaks all the previous dependencies because the previously built version of mpv now has symbol errors and there's no way to easily revert back to that version.

@mlindner

This comment was marked as off-topic.

Copy link

mlindner commented Jan 28, 2019

@MikeMcQuaid Made an unfortunate decision.

@SMillerDev

This comment was marked as off-topic.

Copy link
Contributor

SMillerDev commented Jan 28, 2019

You're more than welcome to maintain your own tap where you can decide what the direction is. Please stop hijacking threads because you have a complaint though.

@mlindner

This comment was marked as off-topic.

Copy link

mlindner commented Jan 28, 2019

@SMillerDev Bifurcating the entire community is a great response. /s If I maintain a tap then I am now off of mainline. I don't want to maintain a tap, I just want compiler flags to be able to be passed through.

@SMillerDev SMillerDev force-pushed the deus0ww:master branch from b5c0eba to f3bbec0 Jan 28, 2019

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Jan 28, 2019

@mlindner Read #31510 and various other threads about the options removal. You will come to realize that it's pointless to try to reverse this. In the short-term, I've created a tap here: https://github.com/deus0ww/homebrew-tap. You're welcome to use it. Note that 'HEAD' for mpv and libass are pointing to my forks.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 28, 2019

Please add back fdk-aac

This should definitely not be added back (see also: openssl). From the formula before this was removed:

# These librares are GPL-incompatible, and require ffmpeg be built with
# the "--enable-nonfree" flag, which produces unredistributable libraries
args << "--enable-nonfree" if build.with?("fdk-aac") || build.with?("openssl")

We cannot legally distribute bottles with FDK AAC so we will not do so.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Jan 28, 2019

Please add back fdk-aac

This should definitely not be added back (see also: openssl). From the formula before this was removed:

# These librares are GPL-incompatible, and require ffmpeg be built with
# the "--enable-nonfree" flag, which produces unredistributable libraries
args << "--enable-nonfree" if build.with?("fdk-aac") || build.with?("openssl")

We cannot legally distribute bottles with FDK AAC so we will not do so.

Yes, I realized that soon after posting the comment, but forgot to edit it. Which is why options need to be kept alive, somewhere.

@JuPlutonic

This comment has been minimized.

Copy link
Contributor

JuPlutonic commented Jan 31, 2019

On openssl. We need something to handle (e.g. in mpv, youtube-viewer+mpv) https-addresses/SSL protocol.
I recommend to put in dependencies gnutls GNU Transport Layer Security (TLS) Library.
But it up to you...
PS New ffmpeg sources and parameters (--enable-videotoolbox) uses corevideo coremedia corefoundation libraries - not implemented for Linux, absent in Linuxbrew.
PPS Fraunhofer?! Remembered epopee with their MP3 codec licence

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 1, 2019

I recommend to put in dependencies gnutls GNU Transport Layer Security (TLS) Library.
But it up to you...

If we can do this without --enable-nonfree it seems like a good idea. Could you submit a pull request @JuPlutonic?

@retokromer

This comment has been minimized.

Copy link
Contributor

retokromer commented Feb 1, 2019

PPS Fraunhofer?! Remembered epopee with their MP3 codec licence

I don’t guess that Fraunhofer is a problem. Today the FFmpeg’s own AAC encoder is excellent. More problematic is OpenSSL, because – to my knowledge – it’s widely used.

@JuPlutonic

This comment has been minimized.

Copy link
Contributor

JuPlutonic commented Feb 2, 2019

I recommend to put in dependencies gnutls GNU Transport Layer Security (TLS) Library.
But it up to you...

If we can do this without --enable-nonfree it seems like a good idea. Could you submit a pull request @JuPlutonic?
@MikeMcQuaid , #36650

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Feb 6, 2019

Are there reasons why #36663 and #36730 got merged and this one did not? I should note that #36730, which includes dep to rubberband, ballooned the dependencies tree way more than this PR.

@SMillerDev

This comment has been minimized.

Copy link
Contributor

SMillerDev commented Feb 6, 2019

conflicts and failing tests would be the main reason I assume.

@SMillerDev

This comment has been minimized.

Copy link
Contributor

SMillerDev commented Feb 6, 2019

@BrewTestBot test this please!

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Feb 6, 2019

@SMillerDev It doesn't help that ffmpeg.rb keeps getting changed from other PR's and the revolving conflicts on github doesn't appear to be working without causing build failure.

@MikeMcQuaid MikeMcQuaid force-pushed the deus0ww:master branch 2 times, most recently from e7b98aa to ce337f4 Feb 6, 2019

@MikeMcQuaid MikeMcQuaid changed the title mpv, ffmpeg: Added removed options as dependencies mpv, ffmpeg: add some removed options as dependencies Feb 6, 2019

deus0ww and others added some commits Feb 6, 2019

@MikeMcQuaid MikeMcQuaid force-pushed the deus0ww:master branch from ce337f4 to 2d01fd0 Feb 7, 2019

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 7, 2019

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @deus0ww!

Sorry for the delay! This reproduced an issue with our CI system that I had to fix before this could be merged (Homebrew/homebrew-test-bot#231). I also wanted to help resolve the merge conflicts for you and split your commits to one-per-formula (our preferred approach).

@deus0ww

This comment has been minimized.

Copy link
Contributor Author

deus0ww commented Feb 8, 2019

No problem and thank you for merging this. Sorry about the mess with git (I'm sure some of the blame is with me and not just that bug with the CI... my git-fu is weak).

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 8, 2019

@deus0ww You're very welcome! ❤️

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