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.

Sign up for GitHub

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

Jump to bottom

mpv: enable sixel support #104865

Merged
merged 1 commit into from Nov 25, 2020
Merged

mpv: enable sixel support #104865

merged 1 commit into from Nov 25, 2020

Conversation

berbiche
Copy link
Member

Motivation for this change

The newest release of mpv added support for sixel output.

Test:

$ nix run 'nixpkgs#mlterm' 
# Inside mlterm
$ mpv --vo=sixel path-to-a-video

image

Tested using a 4K 60fps video from YouTube.

Closure:

  • From: /nix/store/xbxpclh4pbvx5is95935y9vhnvdbky7b-mpv-with-scripts-0.33.0 426.6M
  • To: /nix/store/iddyc4azrdd334lkiilmmpmc48fb3qdv-mpv-with-scripts-0.33.0 427.2M

cc: @Ma27

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.

prusnak
prusnak requested changes Nov 25, 2020
View reviewed changes
Copy link
Member

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

Looks good. One little nitpick below.

pkgs/applications/video/mpv/default.nix Outdated
@@ -45,6 +45,7 @@
, swiftSupport ? false, swift ? null
, theoraSupport ? true, libtheora ? null
, vaapiSupport ? stdenv.isLinux, libva ? null
, sixelSupport ? true, libsixel ? null
Copy link
Member

Choose a reason for hiding this comment

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

To keep the list sorted, we could put this a few lines above, just below sdl2Support

Copy link
Member

Choose a reason for hiding this comment

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

I would like to disable sixelSupport by default cause not every terminal supports and some like Kitty even do a better job themselves.

Copy link
Member

Choose a reason for hiding this comment

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

@SuperSandro2000 I'm actually wondering if we want to provide a second package (e.g. mpvFull) that has all features enabled for people who don't care about size etc., but about having all features. Something similar happens e.g. with diffoscope, curl or python. Just grep for e.g. Full on all-packages.nix to get a few more examples.

@prusnak while I appreciate everyone who invests free time in helping out maintaining nixpkgs, I'd like to mention here that I'm not a big fan of those low-impact nitpicks that don't really have an actual benefit and can be taken up by either a tool or the next time someone touches the package. You may have noticed that we had several policy discussions in the past regarding the contribution process and one of the issues a lot of people have is that our PR queue is steadily growing and a lot of PRs stall because of that. Feel free to disagree (I mean, I want to mention it here, but I don't intend to speak speak for the project and there's no actual policy around that), but I think that those sort of reviews kind of help slowing down the merge process.

Copy link
Member Author

@berbiche berbiche Nov 25, 2020 •
edited

Choose a reason for hiding this comment

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

@prusnak Reordered the lines

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to disable sixelSupport by default cause not every terminal supports and some like Kitty even do a better job themselves.

I tested Kitty 0.19.0 (with no user configuration) and mpv's sixel output didn't work with it ¯\_(ツ)_/¯

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 25, 2020 •
edited

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

1 package failed to build:
  • mpc-qt
16 packages built:
  • celluloid
  • curseradio
  • jellyfin-mpv-shim
  • jftui
  • minitube
  • mpv
  • mpv-unwrapped
  • mpvScripts.mpris
  • plex-media-player
  • plex-mpv-shim
  • python37Packages.mpv
  • python38Packages.mpv
  • qimgv
  • somafm-cli
  • sublime-music
  • tuijam

mpc-qr is already broken on master. Sending a PR to fix that.

@SuperSandro2000 SuperSandro2000 mentioned this pull request Nov 25, 2020
Merged
10 tasks
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 25, 2020 •
edited

Result of nixpkgs-review pr 104865 run on x86_64-darwin 1

2 packages marked as broken and skipped:
  • jellyfin-mpv-shim
  • sublime-music
1 package failed to build:
  • mpc-qt
7 packages built:
  • curseradio
  • mpv-unwrapped
  • plex-mpv-shim
  • python37Packages.mpv
  • python38Packages.mpv
  • somafm-cli
  • tuijam

mpc-qt failure is mac related and does not block this PR.

@berbiche
Copy link
Member Author

Defaulted sixelSupport to false. Ready to be merged I think?

AndersonTorres
AndersonTorres suggested changes Nov 25, 2020
View reviewed changes
Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

LGTM, LGTBorg.

@AndersonTorres AndersonTorres merged commit f9677ba into NixOS:master Nov 25, 2020
@ofborg ofborg bot requested a review from AndersonTorres November 25, 2020 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@prusnak prusnak prusnak approved these changes

@tadeokondrak tadeokondrak Awaiting requested review from tadeokondrak

@globin globin Awaiting requested review from globin

@Ma27 Ma27 Awaiting requested review from Ma27

@fpletz fpletz Awaiting requested review from fpletz

@AndersonTorres AndersonTorres Awaiting requested review from AndersonTorres

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@berbiche @SuperSandro2000 @prusnak @AndersonTorres @Ma27