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

Sunshine incorrectly encodes using the BT.601 colourspace instead of BT.709, resulting in very incorrect colour reproduction on any connected client. #300

Closed
akemin-dayo opened this issue Aug 5, 2022 · 12 comments · Fixed by #567
Labels
fixed This issue has been fixed and will be available in the next release. os:Windows OS is Windows

Comments

@akemin-dayo
Copy link

akemin-dayo commented Aug 5, 2022

Describe the Bug

Sunshine incorrectly encodes using the BT.601 colourspace instead of BT.709, resulting in very incorrect colour reproduction on any connected client.

This behaviour does not occur with either Parsec (which I most often use) or NVIDIA GeForce Experience.

Sunshine log output also confirms the issue.

[2022:08:06:04:34:50]: Info:
Device Description : NVIDIA GeForce RTX 2060
Device Vendor ID   : 0x000010DE
Device Device ID   : 0x00001F08
Device Video Mem   : 5980 MiB
Device Sys Mem     : 0 MiB
Share Sys Mem      : 8111 MiB
Feature Level      : 0x0000B100
Capture size       : 1920x1080
Offset             : 0x0
Virtual Desktop    : 1920x1080
[2022:08:06:04:34:50]: Info: Color coding [Rec. 601]
[2022:08:06:04:34:50]: Info: Color range: [MPEG]

Screenshots

Open the Parsec/NVIDIA GFE/Sunshine screenshots in separate tabs and switch between them to see the difference.

Parsec (correct, BT.709)

testpattern-parsec

nami-parsec

win10wallpaper-parsec

sunshinewebui-parsec

Moonlight + NVIDIA GeForce Experience (correct, BT.709)

testpattern-nvidia

nami-nvidia

win10wallpaper-nvidia

sunshinewebui-nvidia

Moonlight + Sunshine (incorrect, BT.601)

testpattern-sunshine

nami-sunshine

win10wallpaper-sunshine

sunshinewebui-sunshine

Sunshine Host Operating System and Version

Windows 10 19044.1865 21H2

Architecture

x86_64

Sunshine Version

0.14.0

GPU

NVIDIA RTX 2060

GPU Driver Version

516.59

@KuleRucket
Copy link
Contributor

Out of interest, could you give some pointers on what's different about the incorrect BT.601 colours? Maybe I'm just old but I can't tell the difference even when putting them side by side.

@akemin-dayo
Copy link
Author

akemin-dayo commented Aug 6, 2022

I… am going to be honest and say that I'm not entirely sure how to answer that question.

Sunshine is performing the RGB→YCbCr colourspace conversion incorrectly (BT.601), while Parsec and NVGFE are performing the RGB→YCbCr colourspace conversion correctly (BT.709).

As an example, let's look at the screenshots I posted above — for demonstration purposes, let's use the winver screenshot.

Open either the Parsec or the NVGFE screenshot above in an image editor, then take a colour picker and get the RGB values of the grey part of the window — it should be #f0f0f0.

Next, take a local screenshot of your own winver window and do the same thing. You should also get the same value of #f0f0f0.

Finally, open the Sunshine screenshot above, and use the colour picker on the same part. You'll get a value of #eef0ed, which is completely incorrect.

Perhaps I may just be more sensitive to colour differences than some people, but to me the issue is immediately noticeable even without having to compare, to the point where Sunshine is pretty much unusable to me because the colour inaccuracy really bothers me ;P


I did spend some time yesterday looking through the codebase a bit (… and staring at a Wireshark packet capture), and it's quite interesting to me that both Sunlight and Moonlight seem to assume BT.601 as the default, when this pretty much (as far as I know) should never be correct when dealing with PC colourspace conversion.

(I've only seen BT.601 be the correct option to use when dealing with SDTV/EDTV analogue video ingest from capture boards, at least in my experience.)

※ Also, I did join the Moonlight Discord server yesterday as well, so feel free to @​mention me there in the #sunshine channel if real-time communication would help in getting this resolved at all.

@ReenigneArcher
Copy link
Member

I found some reference to this in the code. 601 is default.

Sunshine/src/video.cpp

Lines 883 to 913 in 6000b85

int sws_color_space;
switch(config.encoderCscMode >> 1) {
case 0:
default:
// Rec. 601
BOOST_LOG(info) << "Color coding [Rec. 601]"sv;
ctx->color_primaries = AVCOL_PRI_SMPTE170M;
ctx->color_trc = AVCOL_TRC_SMPTE170M;
ctx->colorspace = AVCOL_SPC_SMPTE170M;
sws_color_space = SWS_CS_SMPTE170M;
break;
case 1:
// Rec. 709
BOOST_LOG(info) << "Color coding [Rec. 709]"sv;
ctx->color_primaries = AVCOL_PRI_BT709;
ctx->color_trc = AVCOL_TRC_BT709;
ctx->colorspace = AVCOL_SPC_BT709;
sws_color_space = SWS_CS_ITU709;
break;
case 2:
// Rec. 2020
BOOST_LOG(info) << "Color coding [Rec. 2020]"sv;
ctx->color_primaries = AVCOL_PRI_BT2020;
ctx->color_trc = AVCOL_TRC_BT2020_10;
ctx->colorspace = AVCOL_SPC_BT2020_NCL;
sws_color_space = SWS_CS_BT2020;
break;
}
BOOST_LOG(info) << "Color range: ["sv << ((config.encoderCscMode & 0x1) ? "JPEG"sv : "MPEG"sv) << ']';

I am not sure if this is an undocumented config option or not, since I can't find a reference to it in the config.cpp.

Could you try adding encoderCscMode=1 to your config file to see if it uses the 709?

@akemin-dayo
Copy link
Author

akemin-dayo commented Aug 28, 2022

I am not sure if this is an undocumented config option or not, since I can't find a reference to it in the config.cpp.

It isn't, as far as I'm aware. The config that is being referred to in video.cpp is actually a config_t struct defined here:

Sunshine/src/video.h

Lines 53 to 63 in 6000b85

struct config_t {
int width;
int height;
int framerate;
int bitrate;
int slicesPerFrame;
int numRefFrames;
int encoderCscMode;
int videoFormat;
int dynamicRange;
};
and has no relation to the config file (as you correctly observed).

And just to be sure, I tried what you suggested anyway but there was no change ;P

@psyke83
Copy link
Collaborator

psyke83 commented Sep 2, 2022

The colorspace seems to be specified by the client. Testing on my laptop running Linux connecting to a Windows host using software renderering on an AMD GPU, moonlight-qt's logs shows:

00:00:08 - SDL Info (0): FFmpeg-based video decoder chosen

Looking here, it seems that Rec 601 would always be selected if using this renderer.

@cgutman
Copy link
Collaborator

cgutman commented Sep 3, 2022

Sunshine should be doing the Rec 709 -> Rec 601 conversion in the case that the Moonlight client requests it. Maybe that's what's going wrong here.

Whether Rec 709 or Rec 601 is picked shouldn't make a difference in color reproduction for renderers that support both. When streaming from GFE, Rec 709 and Rec 601 generate identical RGB values in Moonlight. All it does is change the coefficients used for the RGB->YUV and YUV->RGB conversions and you end up with the same color in the end.

@w0utert
Copy link
Contributor

w0utert commented Oct 7, 2022

I can confirm something is not right with colorspace conversion between sunshine/moonlight and the display. Color reproduction is definitely not great.

I put 2 identical screens next to each other, one running a Linux desktop connected natively through displayport, the other connected to a laptop that remotes into it using Moonlight on windows. Comparing side-by-side colors are washed out, which is particularly visible in bright colored text (orange, yellow, green, etc). The problem is not in the Windows laptop because a VNC remote desktop to the same machine has good color reproduction (but terrible performance).

Is there anything anywhere in the chain (host desktop settings - Sunshine - Moonlight - client desktop settings) that could improve this, or does this need to be fixed in either sunshine and/or moonlight?

@w0utert
Copy link
Contributor

w0utert commented Oct 10, 2022

Scrap my last comment about colorspace conversion, the bad color reproduction I was observing was not actually related to color space handling but to bugs in the RGB to NV12 conversion when using NvFBC with CUDA enabled (see #154)

@cgutman
Copy link
Collaborator

cgutman commented Jan 7, 2023

Please try this build from #723 and see if it addresses the issue.

https://github.com/LizardByte/Sunshine/suites/10239661632/artifacts/502111042

@akemin-dayo
Copy link
Author

akemin-dayo commented Jan 8, 2023

It greatly improves the issue, but there is still a noticeable difference, especially for desktop use. (Basically, the image from Sunshine is still shifted slightly more green than it should be.)

In particular, it still doesn't match the output of NVGFE (or Parsec, though that's less direct of a comparison), although it is much closer now.

I think the most noticeable point of comparison here would be to look at the colour of the white background in the Sunshine web UI / conhost, as well as the colour of that one grey-ish colour Windows loves to use for its dialogs, like in winver.


Moonlight + Sunshine #723 (improved, BT.601)

image
image
image
image


※ Ground truth (direct screenshots from OS)

※ These screenshots are for reference only — NVGFE, Parsec, and Sunshine will not be able to achieve a perfect match due to the nature of video compression and colourspace conversion from RGB.

image
image
image
image

@Isalight
Copy link

Isalight commented Jan 8, 2023

The green tone is present with hardware encoding. Not available with x264.

host
monitor test-nvhost

fix (nvenc)
monitor test-nvfix

x264
monitor test-nvfix-x264

@cgutman cgutman mentioned this issue Jan 8, 2023
11 tasks
@ReenigneArcher ReenigneArcher added the fixed This issue has been fixed and will be available in the next release. label Jan 8, 2023
@LizardByte-bot
Copy link
Member

This issue has been fixed and will be available in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed This issue has been fixed and will be available in the next release. os:Windows OS is Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants