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

handbrake: switch to libav_12 #38811

Merged
merged 2 commits into from Apr 17, 2018
Merged

handbrake: switch to libav_12 #38811

merged 2 commits into from Apr 17, 2018

Conversation

Anton-Latukha
Copy link
Contributor

@Anton-Latukha Anton-Latukha commented Apr 11, 2018

Motivation for this change

Main contributors of HandBrake say that project is developed for libav:
HandBrake/HandBrake#1273 (comment)

It was tested that HandBrake works with never libav_12 in our repositories.
Alternative to use ffmpeg included.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@Anton-Latukha
Copy link
Contributor Author

Since libav is upstream supported - it should be a default choice.

mp4v2, mpeg2dec, x264, x265, libmkv,
fontconfig, freetype, hicolor-icon-theme,
glib, gtk3, intltool, libnotify,
gst_all_1, dbus-glib, udev, libgudev, libvpx,
useGtk ? true, wrapGAppsHook ? null, libappindicator-gtk3 ? null, useFdk ? false, fdk_aac ? null
useGtk ? true, wrapGAppsHook ? null, libappindicator-gtk3 ? null,
useFfmpeg ? false, libav_12 ? null, ffmpeg ? null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the switch.

libgudev
]) ++ (lib.optionals useFdk [fdk_aac]);
libgudev]
++ (if useFfmpeg then [ffmpeg] else [libav_12])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it this way. Is it right way?

Copy link
Member

@Mic92 Mic92 Apr 15, 2018

Choose a reason for hiding this comment

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

if handbrake is not supposed to work with ffmpeg, there is no point in having this as a compilation option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the history of HandBrake through many years. And also FFMpeg -> LibAV fork and their competition and feuds. So I understand their stands and words, who main developers of HandBrake are, and why they use LibAV.

People can use HandBrake with FFMpeg if they would have need, or gains from that. But HB upstream would say to them - "is not supported".

Many people do compile and use it it with FFMpeg.
And developers somehow support FFMpeg (defacto).

They stance is much because of feuds they had in the past, and because the situation is painful for LibAV. And their experience with distribution package maintainers.

Like, current NixOS version works only on FFMpeg, and I do not see people changing that through the years.

Since we do it as nondefault option - it is only an additional option.

If we serious about that, HandBrake distributes with it's own versions of LibAV and all other libraries. We have policy - not to do that, and we successfully use upstream NixOS packages - but that is not supported by HB upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main thing is to make default way - the upstream supported. While allow people the freedom also.

That would allow us to talk to upstream and get more support from upstream.

Because HandBrake otherwise, if we not comply and people would post bugreports to them - they have a pattern of blacklisting distributions and packages.
They would just respond "use official upstream sources, - do not use X package." after some number of bugreports, if our default package would not comply.

@Anton-Latukha
Copy link
Contributor Author

This is a major dependency change. This is probably the right time to call maintainer: @wmertens.

Good day to you, I make my first pull requests to nixpkgs.

I went, and learned some Haskell to get into Nix )

@@ -43,12 +45,13 @@ stdenv.mkDerivation rec {
fribidi fontconfig freetype jansson zlib
libass libiconv libsamplerate libxml2 bzip2
libogg libopus libtheora libvorbis libdvdcss a52dec libmkv
lame ffmpeg libdvdread libdvdnav libbluray mp4v2 mpeg2dec x264 x265 libvpx
] ++ (lib.optionals useGtk [
lame ffmpeg libdvdread libdvdnav libbluray mp4v2 mpeg2dec x264 x265 libvpx]
Copy link
Member

Choose a reason for hiding this comment

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

We keep spaces around items in a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.
Not my style, but I would do in community style to become consistent, on the next time.
I learn.

@Mic92 Mic92 merged commit 7e31e45 into NixOS:master Apr 17, 2018
@Anton-Latukha
Copy link
Contributor Author

Anton-Latukha commented Apr 17, 2018

@Mic92
Thank you.

Now we (I) can continue with talk on HB.
They would listen to our reports now.

@Anton-Latukha Anton-Latukha deleted the handbrake/switch-to-libav_12 branch April 17, 2018 19:42
@peterhoeg
Copy link
Member

My other comment seems to have not made it through. I suggest you add yourself to the maintainer list for HB.

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.

None yet

4 participants