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

Screenshot color isn't consistent with displayed color #7840

Open
rcombs opened this issue Jun 18, 2020 · 8 comments
Open

Screenshot color isn't consistent with displayed color #7840

rcombs opened this issue Jun 18, 2020 · 8 comments

Comments

@rcombs
Copy link
Contributor

rcombs commented Jun 18, 2020

Important Information

  • ae5ac7e
  • macOS 10.15.5
  • Local mpv build

Reproduction steps

  • Take a screenshot of some particularly vibrant content
  • View the screenshot in a different application

Expected behavior

  • Screenshot colors match displayed video

Actual behavior

  • Screenshot colors appear muted, apparently due to gamma error

Log file

mpv-tagged-screenshot-output.txt

Sample files

mpv screenshot taken with screenshot-tag-colorspace=yes:
tagged

mpv screenshot taken with screenshot-tag-colorspace=no:
untagged

(Both taken with zimg-fast=no, though I'm not sure if it matters currently)

OS-level screenshot:
macos-screenshot

The issue is most clearly visible in the bright red areas in this screenshot (particularly the hair).
Test results with these 3 images:

  • mpv displays all 3 identically (and also identically to the original video); I'm not entirely sure why that is and it seems like it might indicate a bug in the handling of the untagged image
  • Firefox displays the tagged screenshot identically to the OS screenshot, and the untagged screenshot differently
  • Safari, Chrome, macOS Preview, and macOS Quick Look display the tagged and untagged screenshots identically, and the OS screenshot differently

The tagged image can be used as a sample file to take additional screenshots of, amusingly enough; the behavior is identical to using the original source file, so I don't think there's any need for me to upload a separate sample.

I think the problem here is that screenshots are being saved with the original video gamma, and most applications expect sRGB gamma. Some (e.g. Firefox) support tagged gamma, but it's not widespread enough to rely on that (which is why screenshot-tag-colorspace was set to no by default in the first place). I've been poking around trying to get mpv to convert to sRGB gamma when taking screenshots, but I haven't had any luck so far; not entirely sure what's going wrong.

Assigning @haasn for (hopefully) some more expertise on the intricacies here.

@haasn
Copy link
Member

haasn commented Jun 18, 2020

So, there are two separate issues here:

  1. Why does mpv show what it shows when viewing those three images?
  2. Why does mpv output what it outputs when taking those three images?

Question 1 is the easier to answer. Let me first note that all three images contain the same pixel data, i.e. the only difference is down to metadata. And in terms of that, all three are different:

  1. untagged.png is indeed untagged, which the PNG specification leaves undefined but mpv defaults to assuming sRGB in this case (as it does for all untagged RGB content)
  2. tagged.png is indeed tagged, with an sRGB tag - this means that the correct interpretation is as sRGB
  3. oslevel.png contains an embedded ICC profile of some rather wide gamut color space, the copyright is set to Apple Inc., 2020. Since the space is wider than sRGB the resulting image should appear much more saturated as a result

Note that mpv ignores embedded ICC profiles unless you also have a display ICC profile set, which is probably why 3 renders the same as 1 and 2 for you - it's different on my end!

From this we can infer:

  1. Your display seems to have a non-sRGB, wide gamut color space (or at least Apple thinks so)
  2. This means you should probably be using icc-profile-auto, because all colors will be wrong out of the box otherwise - i.e. the actual image is not as saturated as it appears on your display

I assume that if you use icc-profile-auto, mpv will pick up the correct display profile and adapt the window output to it, so an OS-level screenshot of that window (with the same ICC profile embedded) should render identically to the screenshots taken with mpv.

As for why mpv creates the other two files it creates should be rather obvious from just the explanation alone that mpv considers untagged RGB to be sRGB by default, and importantly, these files should also both roundtrip back to the same thing when viewed in mpv (or other software making the same assumption).

@haasn
Copy link
Member

haasn commented Jun 18, 2020

Also, side note, it's worth exploring what happens in the current screenshot code:

  1. struct ra_fbo's color_space member gets zero-initialized (by gl_video_screenshot), which is essentially setting all fields to "auto"
  2. This gets inspected by pass_colormanage, which consults the target-trc and target-prim fields as explicit overrides and otherwise falls back to the values specified in the ra_fbo
  3. The code further down in pass_colormanage infers AUTO as being the same color space as the source, but this is never communicated back to the resulting mp_image - leaving the image_writer code to always write out sRGB tags even if the original data was not sRGB (bug!)

And for what it's worth, target-trc etc. affecting even non-window screenshots is definitely a bug, although in the past, screenshots taken via the VO were tagged by the same colorimetry data as the renderer was using - this seems to have been deleted in f17246f and never reintroduced, the function gl_video_get_output_colorspace is currently defined and unused - another bug!

@haasn
Copy link
Member

haasn commented Jun 18, 2020

It seems to me as the solution is multitude, and involves fixing all of the following bugs:

  1. Make screenshot window tag the resulting mp_image with gl_video_get_output_colorspace again
  2. Make regular screenshot ignore target-trc/target-prim when rendering
  3. Forward the mp_image color metadata from the source to the destination when taking non-window screenshots

@rcombs
Copy link
Contributor Author

rcombs commented Jun 18, 2020

Heh, yup, this is a laptop with a P3 monitor; I didn't realize mpv displayed incorrectly in that case by default (I thought macOS automatically handled color conversion in the compositor). I guess it was just expanding the BT709 input into a P3 output? Kind of a pity it was wrong, I was kinda fond of that bright shade.

icc-profile-auto does indeed make mpv's display output match its own screenshots:

image

Oddly, I tested in QuickTime and got yet another slightly different result; not sure what's going on here:

image

Wonder if it'd be worth enabling icc-profile-auto by default on macOS, then? All iMacs since 2015 and MacBook Pros since 2016 have had P3 displays with well-calibrated built-in default profiles, so they're currently oversaturating by default on BT709 content. I'd just note that the "ICC profile detected contrast very high" warning would probably need to be dealt with, as the built-in profiles specify infinite contrast; not sure how that can best be addressed.

@haasn
Copy link
Member

haasn commented Jun 18, 2020

Kind of a pity it was wrong, I was kinda fond of that bright shade.

For subjective aesthetic reasons it's definitely an effect you can mimick using a custom "gamut expansion" shader. I do agree that saturated colors make anime etc. look very nice, the main three issues afaict are skin tones, grass and sky. Maybe we can develop a shader that tries preserving those three hue types while oversaturating everything else (especially red)?

Almost like a sort of "inverse gamut mapping", i.e. mostly linear response with "un-knee" function close to the gamut boundary.

@haasn
Copy link
Member

haasn commented Jun 18, 2020

Wonder if it'd be worth enabling icc-profile-auto by default on macOS, then? All iMacs since 2015 and MacBook Pros since 2016 have had P3 displays with well-calibrated built-in default profiles, so they're currently oversaturating by default on BT709 content. I'd just note that the "ICC profile detected contrast very high" warning would probably need to be dealt with, as the built-in profiles specify infinite contrast; not sure how that can best be addressed.

I would enable it by default on all platforms...

@haasn
Copy link
Member

haasn commented Jun 18, 2020

For the "infinite contrast" warning, re-analyzing this problem I'm not sure if what we're doing currently is even correct. For "perceptual" (black-scaled) profiles, we probably don't want to also be performing contrast scaling in the BT.1886 implementation, so rather than treating it as 1000:1 contrast we should probably treat it more like infinite contrast, either at gamma 2.4 or possibly at gamma 2.2? (Or indeed, the dreaded 1.961!)

I unfortunately don't have any perceptual profiles to test with, so I can't say what preserves the overall intent the best, end-to-end. Feel free to open a new issue for that. Maybe I can get around to dusting off my colorimeter and improving the status quo of perceptual profile support.

It would be nice if apple shipped both perceptual and colorimetric profiles for their displays, or something..

@SingleRottenChips
Copy link

SingleRottenChips commented Feb 29, 2024

In my case (macOS 14.2.1), screenshots match mpv's display with icc-profile-auto, but only when they are untagged, i.e. without screenshot-tag-colorspace, which took me a long time to figure out.


Update:
Sorry, it's got nothing to do with "tag" but with target-trc which should be set to srgb, at least for SDR content.

But I feel like the display is over-saturated in dark scenes.

icc-profile-auto off:
sc_upscaled--

icc-profile-auto on:
0001

off:
large_phantom_of_the_paradise_02_blu-ray_

on:
Phantom of the Paradise 1974 720p BluRay (00:20:54 462) 0002

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
@Akemi @rcombs @haasn @SingleRottenChips and others