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

Screenshots do not preserve GPU-accelerated features such as HDR. #5498

Closed
winneon opened this Issue Feb 7, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@winneon

winneon commented Feb 7, 2018

mpv version and platform

mpv 0.28.0-226-g9282a34fbf Copyright © 2000-2018 mpv/MPlayer/mplayer2 projects
 built on Wed Feb  7 02:46:11 CST 2018
ffmpeg library versions:
   libavutil       56.7.100
   libavcodec      58.10.100
   libavformat     58.9.100
   libswscale      5.0.101
   libavfilter     7.11.101
   libswresample   3.0.101
ffmpeg version: N-89971-g8318bf1751
ffmpeg version N-89971-g8318bf1751 Copyright (c) 2000-2018 the FFmpeg developers
  built with gcc 7.3.0 (GCC)
  configuration: --prefix=/usr --extra-cflags= --extra-ldflags= --disable-rpath --enable-gpl --enable-version3 --enable-nonfree --enable-shared --disable-static --enable-gray --enable-avresample --enable-alsa --enable-avisynth --enable-bzlib --enable-chromaprint --enable-frei0r --enable-gcrypt --enable-gmp --enable-gnutls --enable-iconv --enable-ladspa --enable-libass --enable-libbluray --enable-libbs2b --enable-libcaca --enable-libcelt --enable-libcdio --enable-libdc1394 --enable-libfdk-aac --disable-libflite --enable-fontconfig --enable-libfreetype --enable-libfribidi --enable-libgme --enable-libgsm --enable-libiec61883 --enable-libilbc --enable-libjack --enable-libkvazaar --enable-libmodplug --enable-libmp3lame --enable-libopencore-amrnb --enable-libopencore-amrwb --enable-libopencv --enable-libopenh264 --enable-libopenjpeg --enable-libopenmpt --enable-libopus --enable-libpulse --enable-librsvg --enable-librubberband --enable-librtmp --enable-libshine --enable-libsmbclient --enable-libsnappy --enable-libsoxr --enable-libspeex --enable-libssh --enable-libtesseract --enable-libtheora --disable-libtls --enable-libtwolame --enable-libv4l2 --enable-libvidstab --enable-libvmaf --enable-libvo-amrwbenc --enable-libvorbis --enable-libvpx --enable-libwavpack --enable-libwebp --enable-libx264 --enable-libx265 --enable-libxavs --enable-libxcb --enable-libxcb-shm --enable-libxcb-xfixes --enable-libxcb-shape --enable-libxvid --enable-libxml2 --enable-libzimg --enable-libzmq --enable-libzvbi --enable-lv2 --enable-lzma --enable-decklink --enable-libmysofa --enable-openal --enable-opencl --enable-opengl --disable-openssl --enable-sndio --enable-sdl2 --enable-xlib --enable-zlib --enable-libdrm --enable-libmfx --enable-nvdec --enable-nvenc --enable-omx --enable-omx-rpi --enable-rkmpp --enable-v4l2-m2m --enable-vaapi --enable-vdpau
  libavutil      56.  7.100 / 56.  7.100
  libavcodec     58. 10.100 / 58. 10.100
  libavformat    58.  9.100 / 58.  9.100
  libavdevice    58.  1.100 / 58.  1.100
  libavfilter     7. 11.101 /  7. 11.101
  libavresample   4.  0.  0 /  4.  0.  0
  libswscale      5.  0.101 /  5.  0.101
  libswresample   3.  0.101 /  3.  0.101
  libpostproc    55.  0.100 / 55.  0.100

Running on Arch Linux with version 4.14.15-1 of the Linux kernel. The VO driver used is gpu, the default with the master branch as of this issue.

Explanation

Currently, screenshots do not properly output and preserve GPU-accelerated feature(s) such as HDR. Instead, the screenshot that is outputted is an 8-bit version of the 10-bit HDR frame, resulting in a very washed-out image that is not representative of the image displayed in mpv in the slightest. This happens on both the subtitles and video screenshot options. The window option is unaffected because it just literally takes a screenshot of the window, not the video file (at least, I'm assuming that's how it works). The window option also scales the image and isn't a proper screenshot of the current frame anyway, so it's not a viable workaround.

I'm unsure if there are other features not present in the outputted image, as I've only tested HDR as of this time. Below are a few examples I tested with "Life of Pi" sample HDR footage, and a ripped UHD BD HDR copy of the animated film "Your Name". All comparison images have been downscaled to 1280x720 for smaller file sizes, and all tone-mapping & related HDR options have been kept to default. The window option comparison was not downscaled manually, but by mpv itself by keeping the window at 1280x720.

"Life of Pi" sample HDR footage

subtitles & video screenshot options

subtitle option

window screenshot option

window option

"Your Name" UHD BD copy

subtitles & video screenshot options

subtitle option

window screenshot option

window option


Reproduction steps

  1. Load provided sample "Life of Pi" HDR footage into mpv.
  2. Take a screenshot using the subtitle option (default is the s key).

Expected behavior

The outputted screenshot looks like what you're viewing in mpv.

Actual behavior

The outputted screenshot is incredibly washed-out and entirely unrepresentative of what you are watching. It's even more unrepresentative of non-HDR copies of the same film.

Log file

mpv -v --log-file=out.txt /media/alpha/kiminonawa/old/title01.mkv

http://0x0.st/scuL.txt

Sample files

"Life of Pi" sample HDR footage

@haasn

This comment has been minimized.

Member

haasn commented Feb 7, 2018

“Regular” screenshots and “window” screenshots go through very different code paths.

Regular screenshots take the frame, pass them through libswscale to convert to RGB, and then save this to disk. The data never touches the VO, or the GPU for that matter.

Window screenshots go through the VO's rendering path - as you said, they're basically hard screenshots of what's visible on-screen. Since the vo_gpu rendering path is the only part of mpv that has HDR tone mapping implemented, this is the only way to get HDR->SDR conversion out of mpv, currently.

There are a couple of possible work-arounds and long-term solutions:

  1. You can forcibly insert some of lavf's conversion filters (e.g. zimg + tonemap) which are designed to produce a better result on HDR content. (The quality won't be the same as mpv's built-in algorithm, but it also won't be as horribly washed out as your examples). You can even do this right now, perhaps even inserting the correct filters before taking a screenshot and removing them afterwards. (Hack)

  2. Mpv could become smart enough to recognize HDR content when taking screenshots and apply the correct zimg/tonemap filters itself.

  3. We could implement software tonemapping in mpv (perhaps using the same algorithm?), and apply this where needed to convert from HDR to SDR. This would require duplication of code, however, and it would surely also be very slow unless somebody sits down and writes SIMD for it or whatever.

  4. We can perhaps add an optional dependency on libplacebo and use this for the VO-agnostic screenshot path where available/supported. This library basically implements the same algorithms as mpv, including high quality tone mapping. As a bonus, we could even make an effort to apply e.g. the same --scale when invoking libplacebo. This would have the benefit of being fairly orthogonal to the VO used (and even work when you have no VO at all).

  5. We could redesign the VO to also allow taking window-agnostic screenshots (by basically re-rendering the image to an FBO and saving that) using the same rendering algorithms. This would, however, require potentially reinitializing a lot of state on every screenshot due to limitations of the current vo_gpu design. It also wouldn't be trivial, and it wouldn't work on platforms without e.g. FBO support. It also wouldn't work if you were using another VO.

@winneon

This comment has been minimized.

winneon commented Feb 7, 2018

The fourth solution seems to be the one with the most benefits with the least disadvantages.

  • The first one seems too much of a "workaround" and not a practical solution to the problem (as you mentioned).
  • The second one seems a little better but the fourth option seems like the same idea but implemented better.
  • The third option is also interesting and could work, but if there were to be future vo_gpu related features added, they would also have to be added to the software algorithm, not to mention the duplication of code as you mentioned.
  • The fourth option does add yet another dependency to mpv, but it would be optional, it would offload the tone-mapping and other related features to libplacebo, and as you mentioned, it would work even without a VO.
  • The fifth option seems too long-term and would lack support for for certain platforms and VOs.

Do you have any specific opinions as well?

@haasn

This comment has been minimized.

Member

haasn commented Feb 7, 2018

The fourth would probably be the easiest for me to get going, it would also be a good test case for libplacebo's APIs without having to write a full VO for it, so I'd have a personal interest in trying it out.

On the other hand, further downsides are the fact that libplacebo is still WIP/beta and therefore might not be the best fit for an on-by-default solution. It'd be more of an experimental branch for now, I think. libplacebo also only currently supports vulkan, so there would be little hope for getting this running on a system that doesn't, at least for the immediate foreseeable future. There is no CPU-emulation of the libplacebo algorithms planned. So with that in mind, especially on systems with older or weaker GPUs or where vulkan is not available, the zimg/tonemap approach might be more reasonable.

That said, we could also do both: I wouldn't be opposed to adding libplacebo as a strictly optional dependency that users who do meet all the prerequisites can use it for higher quality screenshots, but mpv will otherwise fall back to zimg/tonemap's CPU algorithms.

My main issue is that I don't really know my way around FFmpeg code at all, so I'd have trouble working on any sort of zimg/tonemap based approach.

tl;dr I could implement 3. 4. and with difficulty 5., but not 2.

@winneon

This comment has been minimized.

winneon commented Feb 7, 2018

You could always try out the fourth option for now and tag it under "experimental" features, and then when an alternative for lower-end users like the second option comes around, likely developed by someone else, then both features could move out of the "experimental" label. Whatever "experimental" ends up being, that is.

@wm4

This comment has been minimized.

Contributor

wm4 commented Feb 7, 2018

The easiest solution would be just adding it to the current renderer, which would be relatively trivial.

@winneon

This comment has been minimized.

winneon commented Feb 8, 2018

That would be the easiest solution, but would it be practical in the long run? It seems to fix the main issue here but it's incompatible with other GPU-related options.

@kevmitch

This comment has been minimized.

Member

kevmitch commented Feb 8, 2018

I'd tend to agree that the current renderer should be used or be fixed so that it can be used, otherwise there are two code-paths that do the same thing and we'd have to fight to get the results consistent between them. If someone has spent 3 hours tweaking their mpv.conf vo options, those should be honoured in making the screenshot as applicable.

kevmitch added a commit that referenced this issue Feb 12, 2018

vo_gpu: make screenshots use the GL renderer
Using the GL renderer for color conversion will make sure screenshots
will use the same conversion as normal video rendering. It can do this
for all types of screenshots.

The logic when to write 16 bit PNGs changes. To approximate the old
behavior, we decide by looking whether the source video format has more
than 8 bits per component. We apply this logic even for window
screenshots. Also, 16 bit PNGs now always include an unused alpha
channel. The reason is that FFmpeg has RGB48 and RGBA64 formats, but no
RGB064. RGB48 is 3 bytes and usually not supported by GPUs for
rendering, so we have to use RGBA64, which forces an alpha channel.

Will break for users who use --target-trc and similar options.

I considered creating a new gl_video context, but it could double GPU
memory use, so I didn't.

This uses FBOs instead of glGetTexImage(), because that increases the
chance it could work on GLES (e.g. ANGLE). Untested. No support for the
Vulkan and D3D11 backends yet.

Fixes #5498. Also fixes #5240, because the code for reading back is not
used with the new code path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment