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

ardour: add a videoSupport option, harvid: init at 0.8.3, xjadeo: init at 0.8.10 #115121

Merged
merged 5 commits into from Mar 19, 2021

Conversation

mitchmindtree
Copy link
Member

@mitchmindtree mitchmindtree commented Mar 4, 2021

Motivation

Video support in Adour with videoSupport

This adds a bool option that, when set to true, enables video
timeline support for the Ardour DAW. This is commonly useful for
soundtrack composition, sound design for film, etc.

When enabled, videoSupport ensures that both harvid and xjadeo are
available to the ardour6 exe via the PATH. harvid decodes the video
stream in real-time to produce still images (I think for thumbnail
support for the timeline?). xjadeo acts as a video monitoring window
that whose playback position is synchronised to the Ardour playhead.

videoSupport remains disabled by default, preserving the original
behaviour.

Video support can be added to ardour in your system or home
configuration package list with:

(ardour.override { videoSupport = true; })

harvid and xjadeo

harvid decodes still images from movie files and serves them via HTTP.
xjadeo is an "X Jack Video Monitor".
Both executables must be available via PATH in order for ardour's video
timeline support to work. You can read more about both tools in their
respective meta attributes.

Tested builds of ardour both with and without videoSupport and both
appear to work nicely. Both harvid and xjadeo also appear to work
as intended from their respective CLIs.


Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @cillianderoiste @magnetophon, current maintainers of the ardour package.

In preparation for the following commits adding ardour-related video
tooling and new `videoSupport` option.
@r-rmcgibbo
Copy link

r-rmcgibbo commented Mar 4, 2021

Result of nixpkgs-review pr 115121 at 65ee58f1 run on x86_64-linux 1

3 packages built:
5 suggestions:
  • warning: maintainers-missing

    Package does not have a maintainer. Consider adding yourself?

    Near pkgs/tools/video/xjadeo/default.nix:33:3:

       |
    33 |   meta = with lib; {
       |   ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/tools/video/harvid/default.nix:17:3:

       |
    17 |   installPhase = ''
       |   ^
    
  • warning: unclear-gpl

    gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

    Near pkgs/applications/audio/ardour/default.nix:189:5:

        |
    189 |     license = licenses.gpl2;
        |     ^
    
  • warning: unclear-gpl

    gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

    Near pkgs/tools/video/harvid/default.nix:36:5:

       |
    36 |     license = licenses.gpl2;
       |     ^
    
  • warning: unclear-gpl

    gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

    Near pkgs/tools/video/xjadeo/default.nix:42:5:

       |
    42 |     license = licenses.gpl2;
       |     ^
    

Result of nixpkgs-review pr 115121 at 65ee58f1 run on aarch64-linux 1

3 packages built:
5 suggestions:
  • warning: maintainers-missing

    Package does not have a maintainer. Consider adding yourself?

    Near pkgs/tools/video/xjadeo/default.nix:33:3:

       |
    33 |   meta = with lib; {
       |   ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/tools/video/harvid/default.nix:17:3:

       |
    17 |   installPhase = ''
       |   ^
    
  • warning: unclear-gpl

    gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

    Near pkgs/applications/audio/ardour/default.nix:189:5:

        |
    189 |     license = licenses.gpl2;
        |     ^
    
  • warning: unclear-gpl

    gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

    Near pkgs/tools/video/harvid/default.nix:36:5:

       |
    36 |     license = licenses.gpl2;
       |     ^
    
  • warning: unclear-gpl

    gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

    Near pkgs/tools/video/xjadeo/default.nix:42:5:

       |
    42 |     license = licenses.gpl2;
       |     ^
    

pkgs/applications/audio/ardour/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/ardour/default.nix Outdated Show resolved Hide resolved
pkgs/tools/video/harvid/default.nix Outdated Show resolved Hide resolved
pkgs/tools/video/harvid/default.nix Outdated Show resolved Hide resolved
pkgs/tools/video/harvid/default.nix Outdated Show resolved Hide resolved
pkgs/tools/video/xjadeo/default.nix Outdated Show resolved Hide resolved
pkgs/tools/video/xjadeo/default.nix Outdated Show resolved Hide resolved
pkgs/tools/video/xjadeo/default.nix Outdated Show resolved Hide resolved
pkgs/tools/video/xjadeo/default.nix Outdated Show resolved Hide resolved
pkgs/tools/video/xjadeo/default.nix Outdated Show resolved Hide resolved
@mitchmindtree
Copy link
Member Author

Thanks for the thorough review @SuperSandro2000!

I believe I've addressed what I can - the changes have been addressed via a rebase of the original commits. Let me know if these sorts of small changes are better addressed with follow-up commits - I assumed I was better off editing the existing commits to make sure the PR still adheres to the CONTRIBUTING.md.

@SuperSandro2000

This comment has been minimized.

@mitchmindtree mitchmindtree force-pushed the ardour-video branch 2 times, most recently from 9fff389 to 3e76170 Compare March 7, 2021 17:17
@mitchmindtree
Copy link
Member Author

OK, second round of review should now be addressed.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Mar 8, 2021

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 115121 run on x86_64-linux 1

3 packages built:
  • ardour
  • harvid
  • xjadeo

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

ardour:

warning: unclear-gpl
gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

Near pkgs/applications/audio/ardour/default.nix:186:5:

    |
186 |     license = licenses.gpl2;
    |     ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/unclear-gpl.md

harvid decodes still images from movie files and serves them via HTTP.
Please refer to the meta attribute for more information.

This commit is part of a PR aimed at enabling video support in ardour.
xjadeo is the X Jack Video Monitor.

Please refer to the meta attribute for more information on xjadeo.

This commit is part of a PR aimed at enabling video support in ardour.
This adds a `bool` option that, when set to `true`, enables video
timeline support for the Ardour DAW. This is commonly useful for
soundtrack composition, sound design for film, etc.

When enabled, `videoSupport` ensures that both `harvid` and `xjadeo` are
available to the `ardour6` exe via the PATH. `harvid` decodes the video
stream in real-time to produce still images (I think for thumbnail
support for the timeline?). `xjadeo` acts as a video monitoring window
that whose playback position is synchronised to the Ardour playhead.

`videoSupport` remains disabled by default, preserving the original
behaviour.

Video support can be added to ardour in your system or home
configuration package list with:

```
(ardour.override { videoSupport = true; })
```
@mitchmindtree
Copy link
Member Author

OK, I've addressed all mentioned formatting nits and added a new commit changing the ardour meta license attribute from gpl2 to gpl2Plus as requested by the automated check.

@risicle
Copy link
Contributor

risicle commented Mar 14, 2021

Builds for me, both with video enabled and disabled, linux x86_64.

Comment on lines +28 to +29
description =
"Decodes still images from movie files and serves them via HTTP";
Copy link
Member

Choose a reason for hiding this comment

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

nixfmt is not the official formatter for nixpkgs and we do not have a ~80 line length limit. It is totally fine to not linebreak a 120 or even 240 character URL and in this example the line break only stretches the file instead of making it more reable.

@@ -158,6 +163,10 @@ stdenv.mkDerivation rec {
"$out/share/icons/hicolor/''${size}x''${size}/apps/ardour6.png"
done
install -vDm 644 "ardour.1"* -t "$out/share/man/man1"
'' + lib.optionalString videoSupport ''
# `harvid` and `xjadeo` must be accessible in `PATH` for video to work.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# `harvid` and `xjadeo` must be accessible in `PATH` for video to work.
# `harvid` and `xjadeo` must be accessible in $PATH for video to work.

@SuperSandro2000 SuperSandro2000 merged commit 8fe2456 into NixOS:master Mar 19, 2021
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

5 participants