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

Revert "build: require our own ffmpeg repo" #5058

Closed
wants to merge 1 commit into from

Conversation

orbea
Copy link
Contributor

@orbea orbea commented Oct 30, 2017

This reverts commit 83d44ac.

Lets try this again...

"This was a really terrible idea and essentially a "F*** you" to any and all distros that support mpv. I get that ffmpeg can be frustrating, but only mpv and its users are hurt by this commit, not ffmpeg. Lets please revert it and come up with a proper solution."

Ideally, you shouldn't need enter any text here, and your commit messages should
explain your changes sufficiently (especially why they are needed). Read
https://github.com/mpv-player/mpv/blob/master/DOCS/contribute.md for coding
style and development conventions. Remove this text block, but if you haven't
agreed to it before, leave the following sentence in place:

I agree that my changes can be relicensed to LGPL 2.1 or later.

@midnightpizza
Copy link

I second this, forcing a, fork of a probably within months insecure libary is a bad idea.

Perhaps someday soon, distros have to include instead of mpv a wrapper:

function mpv() {
youtube-dl -o - "$1" | mplayer -
}

@CounterPillow
Copy link
Contributor

dumb shit like this (resubmitting the PR when it was rejected, and stupid doomsaying comments) are exactly how you make sure none of your opinions will ever be considered.

@orbea
Copy link
Contributor Author

orbea commented Oct 30, 2017

Its a valid problem, shoving it under the rug will not make it go away.

@Hrxn
Copy link
Contributor

Hrxn commented Oct 30, 2017

I don't think it's really that much of a valid problem, to be honest..

@orbea
Copy link
Contributor Author

orbea commented Oct 30, 2017

In which way? Distros are not going to support the ffmpeg fork and @wm4 apparently won't support ffmpeg so that means either most mpv users are going to be using an unsupported mpv build or another program like MPlayer as has already been demonstrated in this PR...

@CounterPillow
Copy link
Contributor

@orbea it's amazing you didn't even read the commit message of the commit you're trying to revert.

@orbea
Copy link
Contributor Author

orbea commented Oct 30, 2017

I did read it. I don't think I follow your line of thought, could you elaborate?

@midnightpizza
Copy link

midnightpizza commented Oct 30, 2017

Forking ffmpeg over an issue, instead of throwing a few rocks upstream does not solve it, forking does not bring the rest of the herd with it, thereof forking is the enemy of community and just causes issues in the end. - see libav
And blocking compile to force such fork is anoying.

@CounterPillow
Copy link
Contributor

@orbea

Upstream FFmpeg is of course still supported

whereas you wrote:

Distros are not going to support the ffmpeg fork and @wm4 apparently won't support ffmpeg

your claim of

most mpv users are going to be using an unsupported mpv build

hinges on the assumption that the majority of mpv's users are on Linux, and of those, are using one of the few distributions that package a recent enough version to be supported. Which is not the case. Most Linux mpv users are probably on Debian or Ubuntu, and so they're using an mpv build that already isn't supported anyway.

@orbea
Copy link
Contributor Author

orbea commented Oct 31, 2017

I see you did not read the entire commit message...

"Official support is only with the master branch of our own repo."

I presume this means that anyone that reports an issue with upstream ffmpeg and mpv will have their issue closed and told how its their fault or at best the issue will be ignored.

Also I do not use debian or ubuntu and they have no bearing on this conversation.

@CounterPillow
Copy link
Contributor

My interpretation is that it means wm4 won't add mpv-side workarounds if FFmpeg randomly breaks the API again. E.g. third-party PPAs aren't officially supported either, but you see people reporting valid issues even though they're using those builds, and they get treated as such.

Also I do not use debian or ubuntu and they have no bearing on this conversation.

when you use words such as "most mpv users", they do have bearing on this conversation. So either don't appeal to statistics you do not have, or accept when you're being corrected.

@orbea
Copy link
Contributor Author

orbea commented Oct 31, 2017

You're right there, "Most" was not the correct word, "Many" is a better word here. Ubuntu and debian the exceptions and not the rule, most distros are more respectful towards upstream projects and they do not deserve this.

That said this PR's biggest problem is that I apparently can't python...

@CounterPillow
Copy link
Contributor

Okay, so you're claiming that

  1. Issues opened by people who built against ffmpeg instead of ffmpeg-mpv will be closed as invalid or ignored
    • I base this on your statement "I presume this means that anyone that reports an issue with upstream ffmpeg and mpv will have their issue closed and told how its their fault or at best the issue will be ignored."
  2. This somehow will lead to downstream distributions or users no longer using mpv
    • I base this on your statement "so that means either most mpv users are going to be using an unsupported mpv build or another program like MPlayer as has already been demonstrated in this PR..."
  3. This somehow gives you a right to ragespam revert pull-requests

I disagree with all three of those points.

  1. See previous point about PPAs or various other unsupported platforms. wm4 saying he's not responsible for distro packages that have bugs due to a broken FFmpeg upstream is not the same as wm4 saying everyone using FFmpeg upstream will not be allowed on the issue tracker.
  2. No. If that was the case, Debian and Ubuntu wouldn't package mpv, and users on those distributions wouldn't still use mpv either through official builds in the repos or through PPAs/their own builds.
  3. I doubt it, but I'm not part of the mpv organisation.

@Artoriuz
Copy link

Artoriuz commented Oct 31, 2017

I'm not a mpv developer, and I'm not a distro maintainer either. I'm just a mpv user who has used it for a few years now, and sincerely speaking I wonder why this is causing so much drama.
What's so different about what mpv is doing compared to let's say, Firefox or Chrome? Don't they also ship their own versions of FFmpeg?

Just look at the issue from wm4's point of view, upstream FFmpeg broke mpv and it stayed broken for a while. He's just making sure this doesn't happen again by requiring mpv's own FFmpeg, which will obviously make it easier for him to revert problematic changes, or include important patches, or even diverge slightly from upstream for a while if needed to keep mpv working. This is also a change that allows mpv to target a single FFmpeg build without having to worry about compatibility hacks.

Distro maintainers can always patch their mpvs or FFmpegs if they really, really oppose to build it the recommended way. Most distro packaged mpv builds were never even supported to begin with. They can do whatever they want with their own builds, but expecting mpv to accept their bug reports when most problems will come from not building it the recommended way is just insane.

I don't think wm4 ever intended distros to ship mpv's version of ffmpeg instead of upstream ffmpeg as their system's ffmpeg. This is a change that simply helps keeping mpv stable, prevents sudden breakage and gives mpv devs more control over their own software. From a development point of view there's literally nothing bad about it.

Maintainers will defend their point of view, wm4 will defend his developer point of view. You'll probably never agree on it, there's a clear divergence on what's important to each of you.

As an user I can only say that preventing breakage and being able to ship new features faster only benefits me. Which seems to contradict arguments from distro maintainers claiming this change hurts mpv's users.

@perfect7gentleman
Copy link

patch doesn't apply, and modified patch doesn't work anyway.
IMO, mpv should either stop using its fork or bye-bye mpv.
I don't think the distors developers will patch the other packages to be able build against ffmpeg-mpv.

@CounterPillow
Copy link
Contributor

@perfect7gentleman cya

I don't think the distors developers will patch the other packages to be able build against ffmpeg-mpv.

That was never even suggested. Stop making shit up.

@perfect7gentleman
Copy link

@CounterPillow, was never suggested what?

@CounterPillow
Copy link
Contributor

@perfect7gentleman why would a distro want to build other packages against ffmpeg-mpv? ffmpeg-mpv is for mpv. You wouldn't build other packages against chromium or firefox's ffmpeg either.

Besides, distributions can still build mpv against regular upstream ffmpeg. It just needs to have the build script patched, which distros already do for tons of software, which you would know if you actually knew anything about packaging.

@perfect7gentleman
Copy link

perfect7gentleman commented Oct 31, 2017

@CounterPillow,
I patched the wscript, built ffpmeg-git. it does not f*cking build. Only against ffmpeg-mpv.

[158/226] Compiling common/tags.c
18:43:01 runner ['x86_64-pc-linux-gnu-gcc', '-march=native', '-mtune=native', '-O2', '-pipe', '-fomit-frame-pointer', '-fno-stack-protector', '-ftree-vectorize', '-D_ISOC99_SOURCE', '-D_GNU_SOURCE', '-D_LARGEFILE_SOURCE', '-D_FILE_OFFSET_BITS=64', '-D_LARGEFILE64_SOURCE', '-std=c99', '-Wall', '-O2', '-Werror=implicit-function-declaration', '-Wno-error=deprecated-declarations', '-Wno-error=unused-function', '-Wempty-body', '-Wstrict-prototypes', '-Wno-format-zero-length', '-Werror=format-security', '-Wno-redundant-decls', '-Wvla', '-Wall', '-Wundef', '-Wmissing-prototypes', '-Wshadow', '-Wno-switch', '-Wparentheses', '-Wpointer-arith', '-Wno-pointer-sign', '-Wno-unused-result', '-pthread', '-I.', '-I..', '-I/usr/include/freetype2', '-I/usr/include/harfbuzz', '-I/usr/include/glib-2.0', '-I/usr/lib64/glib-2.0/include', '-I/usr/include/fribidi', '-I/usr/include/uchardet', '-I/usr/include/luajit-2.0', '-I/usr/include/alsa', '-D_REENTRANT', '../common/tags.c', '-c', '-o/tmp/portage/media-video/mpv-9999/work/mpv-9999/build/common/tags.c.19.o']
../video/decode/vd_lavc.c: In function ‘init_generic_hwaccel’:
../video/decode/vd_lavc.c:744:9: error: implicit declaration of function ‘avcodec_get_hw_frames_parameters’; did you mean ‘avcodec_get_frame_class’? [-Werror=implicit-function-declaration]
     if (avcodec_get_hw_frames_parameters(ctx->avctx,
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         avcodec_get_frame_class
cc1: some warnings being treated as errors

Waf: Leaving directory `/tmp/portage/media-video/mpv-9999/work/mpv-9999/build'
Build failed
 -> task in 'objects' failed with exit status 1: 
        {task 140278929615032: c vd_lavc.c -> vd_lavc.c.19.o}
['x86_64-pc-linux-gnu-gcc', '-march=native', '-mtune=native', '-O2', '-pipe', '-fomit-frame-pointer', '-fno-stack-protector', '-ftree-vectorize', '-D_ISOC99_SOURCE', '-D_GNU_SOURCE', '-D_LARGEFILE_SOURCE', '-D_FILE_OFFSET_BITS=64', '-D_LARGEFILE64_SOURCE', '-std=c99', '-Wall', '-O2', '-Werror=implicit-function-declaration', '-Wno-error=deprecated-declarations', '-Wno-error=unused-function', '-Wempty-body', '-Wstrict-prototypes', '-Wno-format-zero-length', '-Werror=format-security', '-Wno-redundant-decls', '-Wvla', '-Wall', '-Wundef', '-Wmissing-prototypes', '-Wshadow', '-Wno-switch', '-Wparentheses', '-Wpointer-arith', '-Wno-pointer-sign', '-Wno-unused-result', '-pthread', '-I.', '-I..', '-I/usr/include/freetype2', '-I/usr/include/harfbuzz', '-I/usr/include/glib-2.0', '-I/usr/lib64/glib-2.0/include', '-I/usr/include/fribidi', '-I/usr/include/uchardet', '-I/usr/include/luajit-2.0', '-I/usr/include/alsa', '-D_REENTRANT', '../video/decode/vd_lavc.c', '-c', '-o/tmp/portage/media-video/mpv-9999/work/mpv-9999/build/video/decode/vd_lavc.c.19.o']

@CounterPillow
Copy link
Contributor

because it currently relies on a feature in Libav/ffmpeg-mpv which has not yet been merged into ffmpeg. Either patch that out (this is the reason why this PR is stupid anyway, it doesn't fix shit) or accelerate the process to get avcodec_get_hw_frames_parameters into upstream ffmpeg.

@kkkrackpot
Copy link
Contributor

kkkrackpot commented Oct 31, 2017

@perfect7gentleman It's not enough to patch the waf script only. You have to revert all commits in mpv's code which rely on custom mpv-ffmpeg's commits (i.e. the ones that are absent in upstream ffmpeg)...

Regardless to this PR, I'd say that the both ways are "worse"...

Custom mpv-ffmpeg does have improvements that benefit to mpv functionality. However, it now puts additional stress to package maintainers. Besides, it puts additional stress to mpv people if they want to keep mpv-ffmpeg more or less in sync with upstream ffmpeg.

On the other hand, I don't know why ffmpeg people refuse to accept patches that fix ffmpeg's bugs and enhance its functionality. Using a buggy upstream ffmpeg is no better than having a statically linked fixed custom ffmpeg.

PS. Besides, I'd say that pushing PRs back and forth is hardly beneficial either, as well as lack of explanation from mpv people about their future plans regarding breaking up with upstream ffmpeg.

@orbea
Copy link
Contributor Author

orbea commented Nov 2, 2017

The point of re-opening this PR has expired. If more changes should be made a new PR should be opened.

@orbea orbea closed this Nov 2, 2017
@orbea orbea deleted the ffmpeg-revert branch November 2, 2017 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants