I think this PR is clean and feature-complete enough for review now....

Damn that's cool

a0bc7be updates the configure script to check for boost::process, which requires boost 1.64, released April 2017, seems safe enough.

e31a33e updates the configure script to check for boost::json in order to use json serialization and ueberzug's json parser. From ueberzug readme:

simple:
Key-value pairs seperated by a tab,
pairs are also seperated by a tab
warning ONLY FOR TESTING!
Simple was never intended for the usage in production!
It doesn't support paths containing tabs or line breaks
which makes it error prone.

boost::json requires boost 1.75, released December 2020. I'm not comfortable escaping json strings manually, given all the unicode stuff; I'd rather give it to a library to deal with. If that's too new then we can revert the last commit and use the simple ueberzug parser. If you use tabs or newlines, then you probably have bigger issues than ncmppcpp not working. MPD itself doesn't see paths with newlines on my system.

Not to mention json has its own problems. Linux doesn't require that filenames be valid unicode. json doesn't support non-unicode strings. We can construct a path that cannot be passed successfully to either ueberzug's simple or json parsers by creating a filename with both tabs and non-unicode characters (please don't do this).

$ cp img.jpg "$(printf "\t\xbd\xb2.jpg")"

boost::json requires boost 1.75, released December 2020. I'm not comfortable escaping json strings manually, given all the unicode stuff; I'd rather give it to a library to deal with. If that's too new then we can revert the last commit and use the simple ueberzug parser.

There is also the possibility of using Boost.PropertyTree for reading/writing JSON. I would also prefer to work with Boost.JSON, but Boost.PropertyTree has the advantage to be around since Boost 1.41.

Edit: I need to try out this PR. :)

@evanc577 Thanks!

Would it be a lot of work to use PropertyTree instead? Considering that boost::json is very new.

Easy enough, switched to property_tree and lowered min boost version to 1.64

Do you think it's worth adding support for fetching covers from the Internet (and storing them in ~/.cache)? AFAIK MPDroid (android mpd client) does that and it's quite neat.

I don't know where it fetches them from though.

Fetching art from the internet might be for another PR, this one's scope is already getting pretty big.

I've been playing around with libmpdclient, and even though it doesn't have direct support for the albumart API, I can manually call it and receive the data. Maybe this is a better idea than finding the art within ncmpcpp?

Pros:

• Works with remote MPD servers.
• Support for some embedded album art. Since there are a million different standards for embedded artwork, better to have MPD do it for us to avoid duplicating efforts.

Cons:

• Since the artwork data is coming over the network now, it can potentially block the main thread for a long time.
  • Spawn a dedicated worker thread for reading the artwork data from MPD and displaying it. Need to open another connection to MPD to avoid races. Copy host, port, and password from main MPD connection.
• MPD sends binary data in 8192B chunks by default. This can result thousands of network round-trips for large images and unacceptable performance.
  • Worked around by setting MPD binarylimit added in 0.22.4 to a larger value, such as 1MB. MPD seems to only send a max chuck size of around 200KB. A 10MB image requires around 40 round-trips, which is acceptable.

I've created a new branch https://github.com/evanc577/ncmpcpp/tree/artwork-mpd with a working implementation. It's only been tested with a local MPD server and no password.

I have added support for getting artwork via the mpd protocol and displaying images via the Kitty backend.

There is one issue that needs to be addressed for the Kitty backend (and probably any other backend with draws directly to the terminal, rather than create a new X window) which I am currently stuck on.... When displaying the artwork window side by side with a window that can scroll, the artwork also scrolls up and down when scrolling the other window. I believe this is an issue with ncurses scrolling the physical terminal as an optimization, but I can't figure out how to disable this behavior at all times.

If the scrollable window has many duplicated lines, sometimes the artwork does not scroll, so I know that "technically" it should be fixable. Whether ncurses has a way to change this behavior or not I don't know. Any ideas are appreciated, thanks.

@arybczak

I have not been working on this PR because I don't know of a solution to the Kitty backend problem I described in my last comment. The Ueberzug backend seems to work fine though.

SInce I think this PR is going to be stuck here, how about I hide the Kitty backend by removing it from the docs but otherwise merge it as-is, so we can at least get one functional backend. Then in the future, if I (or someone else) wants to finish the Kitty or Sixel backends, it can go in as a separate PR.

Thoughts?

Sounds good 👍

OK, this should be ready now.

• Removed Kitty backend from docs, it can still be activated by setting albumart_backend = kitty
• Some terminal emulators (such as VTE ones in particular) don't support detecting windows size in pixels, allow manually setting font size in this case.
• Add global config toggle for album art.
• Allow alignment to 8 cardinal/intercardinal directions, or center.
• Add support for mpd's readpicture and albumart protocols, in addition to the previously mentioned local artwork.
• Ueberzug only supports X11.
• Requires Imagemagick headers to compile album art support.

Tested working terminals on my machine (KDE Plasma X11, Arch Linux):

• Kitty
• Alacritty
• Gnome Terminal (VTE, requires setting font_width/font_height and albumart_xoffset/albumart_yoffset)
• Termite (VTE, requires setting font_width/font_height)
• Konsole (VTE, requires setting font_width/font_heightand albumart_xoffset/albumart_yoffset, but alignment is still wonky)

Example config

mpd_music_dir = "/path/to/library"
startup_screen = artwork
startup_slave_screen = playlist
startup_slave_screen_focus = yes
locked_screen_width_part = 25
albumart = yes

Screenshots

If I've followed correctly, this PR pulls the album art from online sources? Or online in the sense of a remote mpd server? Out of curiosity, how would something like mopidy with the mopidy-spotify plugin work (obviously with mopidy-mpd as well)? Will it work with the current state of the PR, or is this a future PR?

In case anyone else wanted to get a taste for this feature before it gets merged, checkout ncmpcpp-ueberzug. I've been using it for a while and it works well.

If I've followed correctly, this PR pulls the album art from online sources? Or online in the sense of a remote mpd server?

Online in the sense of a remote (or local) mpd server. It can also fetch art from the local filesystem (relative to the mpd_music_dir config variable). It does not make connections to any external website or service other than the connected mpd server.

Out of curiosity, how would something like mopidy with the mopidy-spotify plugin work (obviously with mopidy-mpd as well)? Will it work with the current state of the PR, or is this a future PR?

I haven't tested with mopidy at all, but if/when mopidy-mpd implements the albumart or readpicture mpd commands, then it should work.

It doesn't look like mopidy-mpd has support today. mopidy/mopidy-mpd#17

Thanks for the clarification! It's unfortunate that mopidy doesn't have the support (yet, at least).

I suppose as an alternative, it could be possible to extract albumart URLs through mpris (which is exposed through mopidy-mpris), but I have no idea how feasible that would be to include here.

Is there a branch for this I could try out?

Is there a branch for this I could try out?

The source branch for this PR is https://github.com/evanc577/ncmpcpp/tree/artwork but review progress seems to have stalled

Just checked out your fork (amazing by the way) and was wondering: Does local have any point now that mpd_albumart does essentially the same thing? If the difference is in back end, then should there be an albumart_backend option?

@evanc577 Is it possible to use an alternative backend other than ueberzug or kitty? For example, chafa is a fantastic tool for displaying images directly in the terminal either using ascii, blocks, or sixel format. An ability to use something like chafa, imgcat, or caca, etc. is a viable choice for folks that are running wayland (therefore can't use ueberzug) or any other terminal besides kitty (e.g. foot, alacritty).

@evanc577 Is it possible to use an alternative backend other than ueberzug or kitty? For example, chafa is a fantastic tool for displaying images directly in the terminal either using ascii, blocks, or sixel format. An ability to use something like chafa, imgcat, or caca, etc. is a viable choice for folks that are running wayland (therefore can't use ueberzug) or any other terminal besides kitty (e.g. foot, alacritty).

There aren't any alternatives implemented, but they shouldn't be difficult to implement. Assuming they play well with ncurses.

@arybczak thoughts about merging this PR?

@evanc577 Can't build this PR on Ubuntu 20.04.

./configure --enable-artwork PATH=$PATH:/usr/lib/x86_64-linux-gnu/ImageMagick-6.9.10/bin-q16
…
make
…
g++ -DHAVE_CONFIG_H -I. -I..   -DU_USING_ICU_NAMESPACE=0  -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -I/usr/include/x86_64-linux-gnu -I/usr/include/taglib -fopenmp -DMAGICKCORE_HDRI_ENABLE=0 -DMAGICKCORE_QUANTUM_DEPTH=16 -fopenmp -DMAGICKCORE_HDRI_ENABLE=0 -DMAGICKCORE_QUANTUM_DEPTH=16 -fopenmp -DMAGICKCORE_HDRI_ENABLE=0 -DMAGICKCORE_QUANTUM_DEPTH=16 -I/usr/include/x86_64-linux-gnu//ImageMagick-6 -I/usr/include/ImageMagick-6 -I/usr/include/x86_64-linux-gnu//ImageMagick-6 -I/usr/include/ImageMagick-6 -I/usr/include/x86_64-linux-gnu//ImageMagick-6 -I/usr/include/ImageMagick-6  -g -O2 -flto -ftree-vectorize -ffast-math -std=c++14 -MT screens/artwork.o -MD -MP -MF $depbase.Tpo -c -o screens/artwork.o screens/artwork.cpp &&\
mv -f $depbase.Tpo $depbase.Po
screens/artwork.cpp: In member function 'std::vector<unsigned char> KittyBackend::writeChunked(std::map<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> >, const Magick::Blob&)':
screens/artwork.cpp:695:42: error: passing 'const Magick::Blob' as 'this' argument discards qualifiers [-fpermissive]
  695 |   std::string b64_data_str = data.base64();
      |                                          ^
In file included from /usr/include/ImageMagick-6/Magick++/Image.h:15,
                 from /usr/include/ImageMagick-6/Magick++.h:12,
                 from ./screens/artwork.h:28,
                 from screens/artwork.cpp:21:
/usr/include/ImageMagick-6/Magick++/Blob.h:48:17: note:   in call to 'std::string Magick::Blob::base64()'
   48 |     std::string base64(void);
      |                 ^~~~~~
screens/artwork.cpp: In constructor 'Artwork::Artwork()':
screens/artwork.cpp:133:6: warning: ignoring return value of 'int pipe(int*)', declared with attribute warn_unused_result [-Wunused-result]
  133 |  pipe(pipefd);
      |  ~~~~^~~~~~~~
screens/artwork.cpp: In member function 'void Artwork::drawToScreen()':
screens/artwork.cpp:169:6: warning: ignoring return value of 'ssize_t read(int, void*, size_t)', declared with attribute warn_unused_result [-Wunused-result]
  169 |  read(pipefd_read, buf, BUF_SIZE);
      |  ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
screens/artwork.cpp: In member function 'virtual void KittyBackend::setOutput(std::vector<unsigned char>, int, int)':
screens/artwork.cpp:732:7: warning: ignoring return value of 'ssize_t write(int, const void*, size_t)', declared with attribute warn_unused_result [-Wunused-result]
  732 |  write(pipefd_write, dummy, 1);
      |  ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [Makefile:768: screens/artwork.o] Error 1
make[2]: Leaving directory '/tmp/build/ncmpcpp/src'
make[1]: *** [Makefile:499: all-recursive] Error 1
make[1]: Leaving directory '/tmp/build/ncmpcpp'
make: *** [Makefile:410: all] Error 2

@arybczak In the latest commit 80f755e, I've changed the Artwork::resetArtworkPosition() logic. Anytime Window::operator<<() prints a character to screen via an ncurses function, set a flag to reset the artwork position later. After Screen::update() has been called on all windows, reset the artwork position if the previously mentioned flag was set. Performance looks good with this strategy and it removes the artwork reset call from the visualizer.

Let me know if you think this is a better solution.

This is fantastic, would love to see it merged :)