Stellarium / stellarium

Stellarium is a free GPL software which renders realistic skies in real time with OpenGL. It is available for Linux/Unix, Windows and macOS. With Stellarium, you really see what you can see with your eyes, binoculars or a small telescope.
https://stellarium.org
GNU General Public License v2.0
7.88k stars 837 forks source link

Color banding artifacts make atmosphere ugly in low light #131

Closed 10110111 closed 6 years ago

10110111 commented 6 years ago

See the following screenshot in 1:1 scale to understand the problem: screenshot of the problem

This banding can be smoothed by application of dithering. I've done a quick-and-dirty implementation of it just to see which places would need it, and below is a proof-of-concept patch I've come up with. I'm sure it can be improved, including a switch to toggle it on/off as well as enable/disable 6 BPP monitor support (which is currently an #ifdef). Also it might be better to simply do all the rendering to a higher-precision FBO texture (e.g. with GL_RGBA32F format) and then simply dither this render target when blitting to screen. But at least this patch makes the image much nicer (see the screenshot after the diff).

diff --git a/src/core/StelPainter.cpp b/src/core/StelPainter.cpp
index 529407e826..f3d66e5cc3 100644
--- a/src/core/StelPainter.cpp
+++ b/src/core/StelPainter.cpp
@@ -2027,12 +2027,36 @@ void StelPainter::initGLShaders()

    QOpenGLShader fshader2(QOpenGLShader::Fragment);
    const char *fsrc2 =
+"#version 120\n"
+"vec3 dither(vec3 c)\n"
+"{\n"
+"    const float bayerPattern[] = float[](\n"
+"        0,  32,  8, 40,  2, 34, 10, 42,  /* 8x8 Bayer ordered dithering  */\n"
+"        48, 16, 56, 24, 50, 18, 58, 26,  /* pattern.  Each input pixel   */\n"
+"        12, 44,  4, 36, 14, 46,  6, 38,  /* is scaled to the 0..63 range */\n"
+"        60, 28, 52, 20, 62, 30, 54, 22,  /* before looking in this table */\n"
+"        3,  35, 11, 43,  1, 33,  9, 41,  /* to determine the action.     */\n"
+"        51, 19, 59, 27, 49, 17, 57, 25,\n"
+"        15, 47,  7, 39, 13, 45,  5, 37,\n"
+"        63, 31, 55, 23, 61, 29, 53, 21);\n"
+"    float bayer=bayerPattern[int(mod(int(gl_FragCoord.x),8)+8*mod(int(gl_FragCoord.y),8))]/64;\n"
+#ifdef MONITOR_6_BPP
+        "    const float rgbByteMax=63.;\n"
+#else
+        "    const float rgbByteMax=255.;\n"
+#endif
+"    vec3 rgb=c*rgbByteMax;\n"
+"    vec3 head=floor(rgb);\n"
+"    vec3 tail=fract(rgb);\n"
+"    return (head+step(bayer,tail))/rgbByteMax;\n"
+"}\n"
+"vec4 dither(vec4 c) { return vec4(dither(c.xyz),c.w); }\n"
        "varying mediump vec2 texc;\n"
        "uniform sampler2D tex;\n"
        "uniform mediump vec4 texColor;\n"
        "void main(void)\n"
        "{\n"
-       "    gl_FragColor = texture2D(tex, texc)*texColor;\n"
+       "    gl_FragColor = dither(texture2D(tex, texc)*texColor);\n"
        "}\n";
    fshader2.compileSourceCode(fsrc2);
    if (!fshader2.log().isEmpty()) { qWarning() << "StelPainter: Warnings while compiling fshader2: " << fshader2.log(); }
@@ -2067,12 +2091,36 @@ void StelPainter::initGLShaders()

    QOpenGLShader fshader4(QOpenGLShader::Fragment);
    const char *fsrc4 =
+"#version 120\n"
+"vec3 dither(vec3 c)\n"
+"{\n"
+"    const float bayerPattern[] = float[](\n"
+"        0,  32,  8, 40,  2, 34, 10, 42,  /* 8x8 Bayer ordered dithering  */\n"
+"        48, 16, 56, 24, 50, 18, 58, 26,  /* pattern.  Each input pixel   */\n"
+"        12, 44,  4, 36, 14, 46,  6, 38,  /* is scaled to the 0..63 range */\n"
+"        60, 28, 52, 20, 62, 30, 54, 22,  /* before looking in this table */\n"
+"        3,  35, 11, 43,  1, 33,  9, 41,  /* to determine the action.     */\n"
+"        51, 19, 59, 27, 49, 17, 57, 25,\n"
+"        15, 47,  7, 39, 13, 45,  5, 37,\n"
+"        63, 31, 55, 23, 61, 29, 53, 21);\n"
+"    float bayer=bayerPattern[int(mod(int(gl_FragCoord.x),8)+8*mod(int(gl_FragCoord.y),8))]/64;\n"
+#ifdef MONITOR_6_BPP
+        "    const float rgbByteMax=63.;\n"
+#else
+        "    const float rgbByteMax=255.;\n"
+#endif
+"    vec3 rgb=c*rgbByteMax;\n"
+"    vec3 head=floor(rgb);\n"
+"    vec3 tail=fract(rgb);\n"
+"    return (head+step(bayer,tail))/rgbByteMax;\n"
+"}\n"
+"vec4 dither(vec4 c) { return vec4(dither(c.xyz),c.w); }\n"
        "varying mediump vec2 texc;\n"
        "varying mediump vec4 outColor;\n"
        "uniform sampler2D tex;\n"
        "void main(void)\n"
        "{\n"
-       "    gl_FragColor = texture2D(tex, texc)*outColor;\n"
+       "    gl_FragColor = dither(texture2D(tex, texc)*outColor);\n"
        "}\n";
    fshader4.compileSourceCode(fsrc4);
    if (!fshader4.log().isEmpty()) { qWarning() << "StelPainter: Warnings while compiling fshader4: " << fshader4.log(); }
diff --git a/src/core/modules/Atmosphere.cpp b/src/core/modules/Atmosphere.cpp
index 12953ed9ed..20798f554f 100644
--- a/src/core/modules/Atmosphere.cpp
+++ b/src/core/modules/Atmosphere.cpp
@@ -58,10 +58,34 @@ Atmosphere::Atmosphere(void)
    }
    QOpenGLShader fShader(QOpenGLShader::Fragment);
    if (!fShader.compileSourceCode(
+"#version 120\n"
+"vec3 dither(vec3 c)\n"
+"{\n"
+"    const float bayerPattern[] = float[](\n"
+"        0,  32,  8, 40,  2, 34, 10, 42,  /* 8x8 Bayer ordered dithering  */\n"
+"        48, 16, 56, 24, 50, 18, 58, 26,  /* pattern.  Each input pixel   */\n"
+"        12, 44,  4, 36, 14, 46,  6, 38,  /* is scaled to the 0..63 range */\n"
+"        60, 28, 52, 20, 62, 30, 54, 22,  /* before looking in this table */\n"
+"        3,  35, 11, 43,  1, 33,  9, 41,  /* to determine the action.     */\n"
+"        51, 19, 59, 27, 49, 17, 57, 25,\n"
+"        15, 47,  7, 39, 13, 45,  5, 37,\n"
+"        63, 31, 55, 23, 61, 29, 53, 21);\n"
+"    float bayer=bayerPattern[int(mod(int(gl_FragCoord.x),8)+8*mod(int(gl_FragCoord.y),8))]/64;\n"
+#ifdef MONITOR_6_BPP
+        "    const float rgbByteMax=63.;\n"
+#else
+        "    const float rgbByteMax=255.;\n"
+#endif
+"    vec3 rgb=c*rgbByteMax;\n"
+"    vec3 head=floor(rgb);\n"
+"    vec3 tail=fract(rgb);\n"
+"    return (head+step(bayer,tail))/rgbByteMax;\n"
+"}\n"
+"vec4 dither(vec4 c) { return vec4(dither(c.xyz),c.w); }\n"
                    "varying mediump vec3 resultSkyColor;\n"
                    "void main()\n"
                    "{\n"
-                    "   gl_FragColor = vec4(resultSkyColor, 1.);\n"
+                    "   gl_FragColor = vec4(dither(resultSkyColor), 1.);\n"
                     "}"))
    {
        qFatal("Error while compiling atmosphere fragment shader: %s", fShader.log().toLatin1().constData());

Screenshot of the same scene with the above patch: screenshot with patch

gzotti commented 6 years ago

This looks excellent, thank you! However, I see a #version 120 in the GLSL code. The code must be compatible to GLSL ES 1.0, or there must be a way to run dithered on "true" OpenGL and (if not possible) the old undithered version for OpenGL ES2 (e.g. ANGLE on Windows, or the small ARM SBCs systems which would even profit more when running 16bit colors.

Another element where dithering would really help would be the Zodiacal light.

Unfortunately even some ANGLE version that came with later Qt editions (5.9) is worse than earlier (5.6). See https://bugs.launchpad.net/stellarium/+bug/1731788. If you are familiar with GLSL, maybe you can find what's wrong in our shaders (if it's not really an ANGLE bug?)?

10110111 commented 6 years ago

I think we can make it GLES-compatible if we replace the bayerPattern array with an additional texture supplied to the shader.

gzotti commented 6 years ago

We will possibly take any working solution which you can give! It just has to work on all platforms. Is this MONITOR_6_BPP a compile-time switch, and could this be configured at runtime?

10110111 commented 6 years ago

Is this MONITOR_6_BPP a compile-time switch, and could this be configured at runtime?

As the patch is simply a proof of concept, this is a compile-time switch. But in final implementation I think it can be a uniform.

gzotti commented 6 years ago

Are you able to implement the required things:

It really looks good, and you appear to have the necessary knowledge for this. I can test on Windows (OpenGL and ANGLE/GL ES2) and on 2 ARM boards (GL ES2). (Not daily, but weekends should be ok).

10110111 commented 6 years ago

code to find out current color mode (is this an OpenGL (ES) query working everywhere or operating system dependent (Windows/X11/MacOS?)), add a uniform switch to the shader

Do you mean the monitor color depth — 6 vs 8 BPP? Not sure. Maybe it's doable on mobile devices, but there are some desktop monitors with 6 BPP TN matrices (e.g. old but still used by me SyncMaster 193p Plus). For a beginning I'd make the whole thing configurable like

After implementing this we might try to choose a custom default on devices which can provide color depth of their monitor.

develop this required Bayer texture

Yes, I guess this shouldn't be hard.

Maybe even introduce a config option for 10bit colors?

Do you mean native 10-BPP monitors? I've not even seen such, not even speaking of being able to test, but I guess simply adding an rgbByteMax=1023/10 BPP option to the list of dithering modes should be enough for this to work.

or the small ARM SBCs systems which would even profit more when running 16bit colors

What is the RGBX layout of 16-bit there? 5-6-5 or maybe 5-5-5-1? For the former rgbByteMax would need to be a bit trickier, like vec2(31,63,31).

gzotti commented 6 years ago

Hm, I had actually assumed 6bit means 5-6-5 ("64k color mode"), did not think of mobile devices. I have not used anything but 8-8-8 for many years now, but some people seem to still use 64k colors on lesser systems.

On 10bit colors, I also have no such device, but some new monitors seem to have introduced it. We should be prepared for those if possible, then. (Or maybe 10bit colors can just work undithered already?)

RGBX layout: I know I have last seen 16bit colors on some X11 system, also years ago, and you can set it in some xorg.conf files. I googled around a bit, people seem to agree that all are using (at least) 24 bit now. Apparently there are no more 16bit colors available in Win/Mac (?) So I am not sure if it is indeed relevant, OTOH dithering is all the more relevant on low-color systems. Is there any speed gain in 16bit mode? Or was that a framebuffer memory question? One discussion I found was about remote desktops which may be shown in 16bit colors. Now, I am not an expert in X11 settings, and use remote desktops only in rare cases. Would that even matter here (@alex-w @guillaumechereau any opinions?)? A remote desktop system packs the final frame whatever its mode and may color-compress it to 16 bits, right? So in this case likely 16bit-aware dithering is not required.

On Ubuntu Mate 16.04 on a Raspberry Pi 3 (with Mesa17 and VC4 driver) I ran GLXInfo. It lists 216 GLX Visuals with colorbuffer values all 8/8/8/8 or 8/8/8/0. Then there is a list of 270 GLXFBConfigs with colorbuffers 8/8/8/8, 8/8/8/0 and 5/6/5/0. I have not yet reconfigured to 16bit mode to see how this looks, or how Stellarium would look.

10110111 commented 6 years ago

Here's a WIP next version of the patch. Now it:

Could you give me some pointers how (where) to better implement the config option for dithering mode (which will affect rgbMaxValue)?

diff --git a/src/bayer-pattern.hpp b/src/bayer-pattern.hpp
new file mode 100644
index 0000000000..dcd62fec21
--- /dev/null
+++ b/src/bayer-pattern.hpp
@@ -0,0 +1,12 @@
+static const float bayerPattern[8*8] =
+{
+   // 8x8 Bayer ordered dithering pattern.
+    0/64.f, 32/64.f,  8/64.f, 40/64.f,  2/64.f, 34/64.f, 10/64.f, 42/64.f,
+   48/64.f, 16/64.f, 56/64.f, 24/64.f, 50/64.f, 18/64.f, 58/64.f, 26/64.f,
+   12/64.f, 44/64.f,  4/64.f, 36/64.f, 14/64.f, 46/64.f,  6/64.f, 38/64.f,
+   60/64.f, 28/64.f, 52/64.f, 20/64.f, 62/64.f, 30/64.f, 54/64.f, 22/64.f,
+    3/64.f, 35/64.f, 11/64.f, 43/64.f,  1/64.f, 33/64.f,  9/64.f, 41/64.f,
+   51/64.f, 19/64.f, 59/64.f, 27/64.f, 49/64.f, 17/64.f, 57/64.f, 25/64.f,
+   15/64.f, 47/64.f,  7/64.f, 39/64.f, 13/64.f, 45/64.f,  5/64.f, 37/64.f,
+   63/64.f, 31/64.f, 55/64.f, 23/64.f, 61/64.f, 29/64.f, 53/64.f, 21/64.f
+};
diff --git a/src/core/StelPainter.cpp b/src/core/StelPainter.cpp
index 529407e826..420211bc71 100644
--- a/src/core/StelPainter.cpp
+++ b/src/core/StelPainter.cpp
@@ -2027,12 +2027,13 @@ void StelPainter::initGLShaders()

    QOpenGLShader fshader2(QOpenGLShader::Fragment);
    const char *fsrc2 =
+#include "dither.glsl"
        "varying mediump vec2 texc;\n"
        "uniform sampler2D tex;\n"
        "uniform mediump vec4 texColor;\n"
        "void main(void)\n"
        "{\n"
-       "    gl_FragColor = texture2D(tex, texc)*texColor;\n"
+       "    gl_FragColor = dither(texture2D(tex, texc)*texColor);\n"
        "}\n";
    fshader2.compileSourceCode(fsrc2);
    if (!fshader2.log().isEmpty()) { qWarning() << "StelPainter: Warnings while compiling fshader2: " << fshader2.log(); }
@@ -2046,6 +2047,8 @@ void StelPainter::initGLShaders()
    texturesShaderVars.vertex = texturesShaderProgram->attributeLocation("vertex");
    texturesShaderVars.texColor = texturesShaderProgram->uniformLocation("texColor");
    texturesShaderVars.texture = texturesShaderProgram->uniformLocation("tex");
+   texturesShaderVars.bayerPattern = texturesShaderProgram->uniformLocation("bayerPattern");
+   texturesShaderVars.rgbMaxValue = texturesShaderProgram->uniformLocation("rgbMaxValue");

    // Texture shader program + interpolated color per vertex
    QOpenGLShader vshader4(QOpenGLShader::Vertex);
@@ -2067,12 +2070,13 @@ void StelPainter::initGLShaders()

    QOpenGLShader fshader4(QOpenGLShader::Fragment);
    const char *fsrc4 =
+#include "dither.glsl"
        "varying mediump vec2 texc;\n"
        "varying mediump vec4 outColor;\n"
        "uniform sampler2D tex;\n"
        "void main(void)\n"
        "{\n"
-       "    gl_FragColor = texture2D(tex, texc)*outColor;\n"
+       "    gl_FragColor = dither(texture2D(tex, texc)*outColor);\n"
        "}\n";
    fshader4.compileSourceCode(fsrc4);
    if (!fshader4.log().isEmpty()) { qWarning() << "StelPainter: Warnings while compiling fshader4: " << fshader4.log(); }
@@ -2086,6 +2090,8 @@ void StelPainter::initGLShaders()
    texturesColorShaderVars.vertex = texturesColorShaderProgram->attributeLocation("vertex");
    texturesColorShaderVars.color = texturesColorShaderProgram->attributeLocation("color");
    texturesColorShaderVars.texture = texturesColorShaderProgram->uniformLocation("tex");
+   texturesColorShaderVars.bayerPattern = texturesColorShaderProgram->uniformLocation("bayerPattern");
+   texturesColorShaderVars.rgbMaxValue = texturesColorShaderProgram->uniformLocation("rgbMaxValue");
 }

@@ -2146,6 +2152,19 @@ void StelPainter::drawFromArray(DrawingMode mode, int count, int offset, bool do
    const Mat4f& m = getProjector()->getProjectionMatrix();
    const QMatrix4x4 qMat(m[0], m[4], m[8], m[12], m[1], m[5], m[9], m[13], m[2], m[6], m[10], m[14], m[3], m[7], m[11], m[15]);

+   const auto makeBayerPatternTexture=[this]{
+       GLuint tex;
+       glGenTextures(1, &tex);
+       glBindTexture(GL_TEXTURE_2D, tex);
+       glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+       glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+       glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_REPEAT);
+       glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT);
+#include "bayer-pattern.hpp"
+       glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, 8, 8, 0, GL_RED, GL_FLOAT, bayerPattern);
+       return tex;
+   };
+   const auto rgbMaxValue=Vec3f(255,255,255); // TODO: to the options
    if (!texCoordArray.enabled && !colorArray.enabled && !normalArray.enabled)
    {
        pr = basicShaderProgram;
@@ -2166,6 +2185,12 @@ void StelPainter::drawFromArray(DrawingMode mode, int count, int offset, bool do
        pr->setAttributeArray(texturesShaderVars.texCoord, texCoordArray.type, texCoordArray.pointer, texCoordArray.size);
        pr->enableAttributeArray(texturesShaderVars.texCoord);
        //pr->setUniformValue(texturesShaderVars.texture, 0);    // use texture unit 0
+       glActiveTexture(GL_TEXTURE1);
+       if(!bayerPatternTex)
+           bayerPatternTex=makeBayerPatternTexture();
+       glBindTexture(GL_TEXTURE_2D, bayerPatternTex);
+       pr->setUniformValue(texturesShaderVars.bayerPattern, 1);
+       pr->setUniformValue(texturesShaderVars.rgbMaxValue, rgbMaxValue[0], rgbMaxValue[1], rgbMaxValue[2]);
    }
    else if (texCoordArray.enabled && colorArray.enabled && !normalArray.enabled)
    {
@@ -2179,6 +2204,12 @@ void StelPainter::drawFromArray(DrawingMode mode, int count, int offset, bool do
        pr->setAttributeArray(texturesColorShaderVars.color, colorArray.type, colorArray.pointer, colorArray.size);
        pr->enableAttributeArray(texturesColorShaderVars.color);
        //pr->setUniformValue(texturesShaderVars.texture, 0);    // use texture unit 0
+       glActiveTexture(GL_TEXTURE1);
+       if(!bayerPatternTex)
+           bayerPatternTex=makeBayerPatternTexture();
+       glBindTexture(GL_TEXTURE_2D, bayerPatternTex);
+       pr->setUniformValue(texturesColorShaderVars.bayerPattern, 1);
+       pr->setUniformValue(texturesColorShaderVars.rgbMaxValue, rgbMaxValue[0], rgbMaxValue[1], rgbMaxValue[2]);
    }
    else if (!texCoordArray.enabled && colorArray.enabled && !normalArray.enabled)
    {
diff --git a/src/core/StelPainter.hpp b/src/core/StelPainter.hpp
index e08f8e2724..3889963ac5 100644
--- a/src/core/StelPainter.hpp
+++ b/src/core/StelPainter.hpp
@@ -402,6 +402,8 @@ private:
        int vertex;
        int texColor;
        int texture;
+       int bayerPattern;
+       int rgbMaxValue;
    };
    static TexturesShaderVars texturesShaderVars;
    static QOpenGLShaderProgram* texturesColorShaderProgram;
@@ -411,9 +413,12 @@ private:
        int vertex;
        int color;
        int texture;
+       int bayerPattern;
+       int rgbMaxValue;
    };
    static TexturesColorShaderVars texturesColorShaderVars;

+   GLuint bayerPatternTex=0;

    //! The descriptor for the current opengl vertex array
    ArrayDesc vertexArray;
diff --git a/src/core/modules/Atmosphere.cpp b/src/core/modules/Atmosphere.cpp
index 12953ed9ed..46cfd21af7 100644
--- a/src/core/modules/Atmosphere.cpp
+++ b/src/core/modules/Atmosphere.cpp
@@ -58,10 +58,11 @@ Atmosphere::Atmosphere(void)
    }
    QOpenGLShader fShader(QOpenGLShader::Fragment);
    if (!fShader.compileSourceCode(
+#include "dither.glsl"
                    "varying mediump vec3 resultSkyColor;\n"
                    "void main()\n"
                    "{\n"
-                    "   gl_FragColor = vec4(resultSkyColor, 1.);\n"
+                    "   gl_FragColor = vec4(dither(resultSkyColor), 1.);\n"
                     "}"))
    {
        qFatal("Error while compiling atmosphere fragment shader: %s", fShader.log().toLatin1().constData());
@@ -76,6 +77,8 @@ Atmosphere::Atmosphere(void)
    StelPainter::linkProg(atmoShaderProgram, "atmosphere");

    atmoShaderProgram->bind();
+   shaderAttribLocations.bayerPattern = atmoShaderProgram->uniformLocation("bayerPattern");
+   shaderAttribLocations.rgbMaxValue = atmoShaderProgram->uniformLocation("rgbMaxValue");
    shaderAttribLocations.alphaWaOverAlphaDa = atmoShaderProgram->uniformLocation("alphaWaOverAlphaDa");
    shaderAttribLocations.oneOverGamma = atmoShaderProgram->uniformLocation("oneOverGamma");
    shaderAttribLocations.term2TimesOneOverMaxdLpOneOverGamma = atmoShaderProgram->uniformLocation("term2TimesOneOverMaxdLpOneOverGamma");
@@ -368,6 +371,28 @@ void Atmosphere::draw(StelCore* core)
    const Mat4f& m = sPainter.getProjector()->getProjectionMatrix();
    atmoShaderProgram->setUniformValue(shaderAttribLocations.projectionMatrix,
        QMatrix4x4(m[0], m[4], m[8], m[12], m[1], m[5], m[9], m[13], m[2], m[6], m[10], m[14], m[3], m[7], m[11], m[15]));
+
+   const auto rgbMaxValue=Vec3f(255,255,255); // TODO: to the options
+   atmoShaderProgram->setUniformValue(shaderAttribLocations.rgbMaxValue, rgbMaxValue[0], rgbMaxValue[1], rgbMaxValue[2]);
+   const auto makeBayerPatternTexture=[&sPainter]{
+       GLuint tex;
+       const auto& gl=sPainter.glFuncs();
+       gl->glGenTextures(1, &tex);
+       gl->glBindTexture(GL_TEXTURE_2D, tex);
+       gl->glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+       gl->glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+       gl->glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_REPEAT);
+       gl->glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT);
+#include "bayer-pattern.hpp"
+       gl->glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, 8, 8, 0, GL_RED, GL_FLOAT, bayerPattern);
+       return tex;
+   };
+   const auto& gl=sPainter.glFuncs();
+   gl->glActiveTexture(GL_TEXTURE1);
+   if(!bayerPatternTex)
+       bayerPatternTex=makeBayerPatternTexture();
+   gl->glBindTexture(GL_TEXTURE_2D, bayerPatternTex);
+   atmoShaderProgram->setUniformValue(shaderAttribLocations.bayerPattern, 1);

    colorGridBuffer.bind();
    atmoShaderProgram->setAttributeBuffer(shaderAttribLocations.skyColor, GL_FLOAT, 0, 4, 0);
diff --git a/src/core/modules/Atmosphere.hpp b/src/core/modules/Atmosphere.hpp
index 9553008929..18e697cbc5 100644
--- a/src/core/modules/Atmosphere.hpp
+++ b/src/core/modules/Atmosphere.hpp
@@ -103,6 +103,8 @@ private:
    //! Vertex shader used for xyYToRGB computation
    class QOpenGLShaderProgram* atmoShaderProgram;
    struct {
+       int bayerPattern;
+       int rgbMaxValue;
        int alphaWaOverAlphaDa;
        int oneOverGamma;
        int term2TimesOneOverMaxdLpOneOverGamma;
@@ -114,6 +116,8 @@ private:
        int skyVertex;
        int skyColor;
    } shaderAttribLocations;
+
+   GLuint bayerPatternTex=0;
 };

 #endif // _ATMOSTPHERE_HPP_
diff --git a/src/dither.glsl b/src/dither.glsl
new file mode 100644
index 0000000000..b86fc52979
--- /dev/null
+++ b/src/dither.glsl
@@ -0,0 +1,13 @@
+R"(uniform vec3 rgbMaxValue;
+uniform sampler2D bayerPattern;
+vec3 dither(vec3 c)
+{
+    float bayer=texture2D(bayerPattern,gl_FragCoord.xy/8.).r;
+
+    vec3 rgb=c*rgbMaxValue;
+    vec3 head=floor(rgb);
+    vec3 tail=fract(rgb);
+    return (head+step(bayer,tail))/rgbMaxValue;
+}
+vec4 dither(vec4 c) { return vec4(dither(c.xyz),c.w); }
+)"
gzotti commented 6 years ago

For configurations: Look e.g. into StelCore.cpp. In its constructor you see config handling. A similar construct should then go into StelPainter's constructor. The setting could be called "video/dithering" with default=true. Either a Boolean with number of bits detected from OpenGL environment (Format?) or a number of bits? I think it should best be auto-detected, with a fallback to "false" if something goes wrong. The setting does not have to become a full "Q_PROPERTY" of StelPainter, just a regular private variable, as it will likely not be changed by user action at runtime. (Unless you want to show the effect?)

Please move any .glsl files into data/shaders.

I have not tested your code so far, but avoiding 1.20 should help in ES2 modes, thanks. I will try to configure a Raspberry to 16bit mode to see whether this even works, and whether this gives any performance bonus. (But probably the whole OpenGL setup must be changed then? It is possible we don't need 5/6/5. Else if there is a significant fps increase, dithering is a big plus to hide its ugliness.)

gzotti commented 6 years ago

Seems I cannot find the proper xorg.conf lines for a Screen section that defines 16bit colors. Ubuntu Mate 16.04 does not boot, neither with the default nor the VC4 driver overlay. Who is willing to test some 16bit layout? Or else we skip that. It seems nobody ever bothered about 5/6/5 on an SBC (?) But 10bit colors may be come a topic of relevance within 0-5 years.

10110111 commented 6 years ago

Just tried it with my Intel(R) HD Graphics 4600 by simply running a new Xorg instance like

startx `which xterm` -- :1 -depth 16

After changing rgbMaxValue=Vec3f(255,255,255); to rgbMaxValue=Vec3f(31,63,31); in the two files containing this, I got quite a nice picture (as compared to non-dithered version).

10110111 commented 6 years ago

On nvidia, though, I get black window in 16-bit mode, even without dithering...

gzotti commented 6 years ago

OK, that means support is hardware dependent and cannot be guaranteed, but your dithering runs when available, good! Now you would just have to find out color depth in the StelPainter constructor (or elsewhere) to set the rgbMaxValue.

Is there any noticeable difference in frame rate between 24bit and 16bit depth on the HD4600? Or is it both 60frames by sync rate? I just want to estimate whether struggling to set some other ARM system (with Mali) into 16bit can make any (positive) difference.

10110111 commented 6 years ago

OK, that means support is hardware dependent and cannot be guaranteed

Not quite. Other OpenGL programs, including GLSL-intensive ones like Precomputed Atmospheric Scattering demo work perfectly (and PAS looks great when I add dithering to it). At the same time, Stellarium just draws black frames. But that's another issue.

Is there any noticeable difference in frame rate between 24bit and 16bit depth on the HD4600?

I failed to see any subjectively noticeable difference. And the FPS counter at the bottom panel appears unreliable: when I don't move the camera, it simply decays to 18.2 FPS regardless of the mode, although it does sometimes go higher than 50 FPS when I rotate/zoom etc.. So I don't know how to do a reliable benchmark.

gzotti commented 6 years ago

That's a feature: Stellarium falls back to a low rate to conserve energy. You can increase [video]/minimum_fps in config.ini. High framerates are of course best-effort.

Wow, the PAS is what we need as next step over the current atmosphere model! How does it behave at night? (twilight with earth shadow is great!)

10110111 commented 6 years ago

How does it behave at night?

Well, unsurprisingly, it becomes dark :) . In the demo you can increase exposure to see more, but it'll simply be the scattering of sunlight. PAS doesn't model airglow nor Aurorae, so it might be not an ideal fit for Stellarium (does Stellarium model them, though?).

10110111 commented 6 years ago

Please move any .glsl files into data/shaders.

Are you sure dither.glsl should be moved there? It's actually a C++ source file, containing the raw literal with the GLSL source. Maybe it should be simply renamed to .cpp or .hpp?

gzotti commented 6 years ago

PAS: I meant how does it look in deep twilight? I should try this myself, obviously, if I find time. Stellarium has airglow brightness from Schaefer's sky brightness model, but no aurorae. The physics-based values have to be checked and compared/recalibrated/better documented in the process.

Ah right, it's rather a header-type file to be included. @alex-w what is the best place then? Just src/core?

10110111 commented 6 years ago

I think it should best be auto-detected, with a fallback to "false" if something goes wrong. The setting does not have to become a full "Q_PROPERTY" of StelPainter, just a regular private variable, as it will likely not be changed by user action at runtime. (Unless you want to show the effect?)

Actually this will look like garbage on TN+film monitors, which work in 24-bit mode (as far as video card is concerned), thus giving OpenGL app 8/8/8 GLX visuals, but only have 6 BPP matrix in reality. This may work nicely if the video card in use supports temporal dithering by itself and has it enabled, but e.g. Intel video chips don't, and on nvidia it's not the default mode AFAIR.

Are you still sure you don't want to let the user choose dithering mode?

gzotti commented 6 years ago

Phew, good question. Regular users don't want to tweak settings, but they also don't like unpleasant surprises. But I was not aware of this TN 6bit issue, I had an IPS following my CRT... If this cannot be detected, dithering should default to off, and modes only edited manually.

Is this a setting that should be set interactively to see the effect immediately? Then we should be able to add a combo none/6bit/8bit/10bit into the configuration panel. It is enough if you work with the flag from config.ini, we will do the GUI stuff.

video/dithering should in this case be an int with usable values 0/6/8/10. I have no TN panel to test.

alex-w commented 6 years ago

@gzotti Yes, just src/core will be best place

10110111 commented 6 years ago

@gzotti To see how much of an issue it is, here's a pair of photos of the same scene displayed on SyncMaster 193P Plus: rgbMaxValue=vec3(255) (which would be based on GLX visual): maxval255 rgbMaxValue=vec3(63) (actual value for the monitor, despite current 24-bit video mode): maxval63

10110111 commented 6 years ago

Is this a setting that should be set interactively to see the effect immediately?

I suppose yes, the user should be able to see whether current setting is better and easily revert if it's not.

gzotti commented 6 years ago

Thanks for the images, very instructive. OK, let's make this interactive, default none, with Q_PROPERTY and GUI stuff. But you can leave this to us, just the 6/8 and if you can envision it, 10bit modes please, settable in config.ini.

10110111 commented 6 years ago

Current incarnation of the patch:

Is this work with the settings what you intended it to be at this stage?

diff --git a/src/core/Dithering.hpp b/src/core/Dithering.hpp
new file mode 100644
index 0000000..3ccfc6f
--- /dev/null
+++ b/src/core/Dithering.hpp
@@ -0,0 +1,70 @@
+#ifndef DITHERING_HPP
+#define DITHERING_HPP
+
+#include "StelOpenGL.hpp"
+#include "StelPainter.hpp"
+#include "VecMath.hpp"
+
+inline GLuint makeBayerPatternTexture(QOpenGLFunctions& gl)
+{
+    GLuint tex;
+    gl.glGenTextures(1, &tex);
+    gl.glBindTexture(GL_TEXTURE_2D, tex);
+    gl.glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+    gl.glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+    gl.glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_REPEAT);
+    gl.glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT);
+    static constexpr float bayerPattern[8*8] =
+    {
+        // 8x8 Bayer ordered dithering pattern.
+         0/64.f, 32/64.f,  8/64.f, 40/64.f,  2/64.f, 34/64.f, 10/64.f, 42/64.f,
+        48/64.f, 16/64.f, 56/64.f, 24/64.f, 50/64.f, 18/64.f, 58/64.f, 26/64.f,
+        12/64.f, 44/64.f,  4/64.f, 36/64.f, 14/64.f, 46/64.f,  6/64.f, 38/64.f,
+        60/64.f, 28/64.f, 52/64.f, 20/64.f, 62/64.f, 30/64.f, 54/64.f, 22/64.f,
+         3/64.f, 35/64.f, 11/64.f, 43/64.f,  1/64.f, 33/64.f,  9/64.f, 41/64.f,
+        51/64.f, 19/64.f, 59/64.f, 27/64.f, 49/64.f, 17/64.f, 57/64.f, 25/64.f,
+        15/64.f, 47/64.f,  7/64.f, 39/64.f, 13/64.f, 45/64.f,  5/64.f, 37/64.f,
+        63/64.f, 31/64.f, 55/64.f, 23/64.f, 61/64.f, 29/64.f, 53/64.f, 21/64.f
+    };
+    gl.glTexImage2D(GL_TEXTURE_2D, 0, GL_RED, 8, 8, 0, GL_RED, GL_FLOAT, bayerPattern);
+    return tex;
+};
+
+inline Vec3f calcRGBMaxValue(StelPainter::DitheringMode mode)
+{
+    switch(mode)
+    {
+    default:
+    case StelPainter::DitheringMode::Disabled:
+        return Vec3f(0.);
+    case StelPainter::DitheringMode::Color666:
+        return Vec3f(63);
+    case StelPainter::DitheringMode::Color565:
+        return Vec3f(31,63,31);
+    case StelPainter::DitheringMode::Color888:
+        return Vec3f(255);
+    case StelPainter::DitheringMode::Color101010:
+        return Vec3f(1024);
+    }
+};
+
+inline QString makeDitheringShader()
+{
+    return
+R"(uniform vec3 rgbMaxValue;
+uniform sampler2D bayerPattern;
+vec3 dither(vec3 c)
+{
+    if(rgbMaxValue.r==0.) return c;
+    float bayer=texture2D(bayerPattern,gl_FragCoord.xy/8.).r;
+
+    vec3 rgb=c*rgbMaxValue;
+    vec3 head=floor(rgb);
+    vec3 tail=fract(rgb);
+    return (head+step(bayer,tail))/rgbMaxValue;
+}
+vec4 dither(vec4 c) { return vec4(dither(c.xyz),c.w); }
+)";
+}
+
+#endif
diff --git a/src/core/StelPainter.cpp b/src/core/StelPainter.cpp
index 529407e..535551c 100644
--- a/src/core/StelPainter.cpp
+++ b/src/core/StelPainter.cpp
@@ -24,6 +24,7 @@
 #include "StelProjector.hpp"
 #include "StelProjectorClasses.hpp"
 #include "StelUtils.hpp"
+#include "Dithering.hpp"

 #include <QDebug>
 #include <QString>
@@ -110,6 +111,17 @@ bool StelPainter::linkProg(QOpenGLShaderProgram* prog, const QString& name)
    return ret;
 }

+StelPainter::DitheringMode StelPainter::parseDitheringMode(QString const& str)
+{
+    const auto s=str.trimmed().toLower();
+    if(s=="disabled"   ) return DitheringMode::Disabled;
+    if(s=="color565"   ) return DitheringMode::Color565;
+    if(s=="color666"   ) return DitheringMode::Color666;
+    if(s=="color888"   ) return DitheringMode::Color888;
+    if(s=="color101010") return DitheringMode::Color101010;
+    return DitheringMode::Disabled;
+}
+
 StelPainter::StelPainter(const StelProjectorP& proj) : QOpenGLFunctions(QOpenGLContext::currentContext()), glState(this)
 {
    Q_ASSERT(proj);
@@ -136,6 +148,9 @@ StelPainter::StelPainter(const StelProjectorP& proj) : QOpenGLFunctions(QOpenGLC
    glStencilMask(0x11111111);
    glState.apply(); //apply default OpenGL state
    setProjector(proj);
+
+    QSettings*const conf = StelApp::getInstance().getSettings();
+   ditheringMode = parseDitheringMode(conf->value("video/dithering_mode").toString());
 }

 void StelPainter::setProjector(const StelProjectorP& p)
@@ -2026,13 +2041,14 @@ void StelPainter::initGLShaders()
    if (!vshader2.log().isEmpty()) { qWarning() << "StelPainter: Warnings while compiling vshader2: " << vshader2.log(); }

    QOpenGLShader fshader2(QOpenGLShader::Fragment);
-   const char *fsrc2 =
+   const auto fsrc2 =
+        makeDitheringShader()+
        "varying mediump vec2 texc;\n"
        "uniform sampler2D tex;\n"
        "uniform mediump vec4 texColor;\n"
        "void main(void)\n"
        "{\n"
-       "    gl_FragColor = texture2D(tex, texc)*texColor;\n"
+       "    gl_FragColor = dither(texture2D(tex, texc)*texColor);\n"
        "}\n";
    fshader2.compileSourceCode(fsrc2);
    if (!fshader2.log().isEmpty()) { qWarning() << "StelPainter: Warnings while compiling fshader2: " << fshader2.log(); }
@@ -2046,6 +2062,8 @@ void StelPainter::initGLShaders()
    texturesShaderVars.vertex = texturesShaderProgram->attributeLocation("vertex");
    texturesShaderVars.texColor = texturesShaderProgram->uniformLocation("texColor");
    texturesShaderVars.texture = texturesShaderProgram->uniformLocation("tex");
+   texturesShaderVars.bayerPattern = texturesShaderProgram->uniformLocation("bayerPattern");
+   texturesShaderVars.rgbMaxValue = texturesShaderProgram->uniformLocation("rgbMaxValue");

    // Texture shader program + interpolated color per vertex
    QOpenGLShader vshader4(QOpenGLShader::Vertex);
@@ -2066,13 +2084,14 @@ void StelPainter::initGLShaders()
    if (!vshader4.log().isEmpty()) { qWarning() << "StelPainter: Warnings while compiling vshader4: " << vshader4.log(); }

    QOpenGLShader fshader4(QOpenGLShader::Fragment);
-   const char *fsrc4 =
+   const auto fsrc4 =
+        makeDitheringShader()+
        "varying mediump vec2 texc;\n"
        "varying mediump vec4 outColor;\n"
        "uniform sampler2D tex;\n"
        "void main(void)\n"
        "{\n"
-       "    gl_FragColor = texture2D(tex, texc)*outColor;\n"
+       "    gl_FragColor = dither(texture2D(tex, texc)*outColor);\n"
        "}\n";
    fshader4.compileSourceCode(fsrc4);
    if (!fshader4.log().isEmpty()) { qWarning() << "StelPainter: Warnings while compiling fshader4: " << fshader4.log(); }
@@ -2086,6 +2105,8 @@ void StelPainter::initGLShaders()
    texturesColorShaderVars.vertex = texturesColorShaderProgram->attributeLocation("vertex");
    texturesColorShaderVars.color = texturesColorShaderProgram->attributeLocation("color");
    texturesColorShaderVars.texture = texturesColorShaderProgram->uniformLocation("tex");
+   texturesColorShaderVars.bayerPattern = texturesColorShaderProgram->uniformLocation("bayerPattern");
+   texturesColorShaderVars.rgbMaxValue = texturesColorShaderProgram->uniformLocation("rgbMaxValue");
 }

@@ -2146,6 +2167,7 @@ void StelPainter::drawFromArray(DrawingMode mode, int count, int offset, bool do
    const Mat4f& m = getProjector()->getProjectionMatrix();
    const QMatrix4x4 qMat(m[0], m[4], m[8], m[12], m[1], m[5], m[9], m[13], m[2], m[6], m[10], m[14], m[3], m[7], m[11], m[15]);

+   const auto rgbMaxValue=calcRGBMaxValue(ditheringMode);
    if (!texCoordArray.enabled && !colorArray.enabled && !normalArray.enabled)
    {
        pr = basicShaderProgram;
@@ -2166,6 +2188,12 @@ void StelPainter::drawFromArray(DrawingMode mode, int count, int offset, bool do
        pr->setAttributeArray(texturesShaderVars.texCoord, texCoordArray.type, texCoordArray.pointer, texCoordArray.size);
        pr->enableAttributeArray(texturesShaderVars.texCoord);
        //pr->setUniformValue(texturesShaderVars.texture, 0);    // use texture unit 0
+       glActiveTexture(GL_TEXTURE1);
+       if(!bayerPatternTex)
+           bayerPatternTex=makeBayerPatternTexture(*this);
+       glBindTexture(GL_TEXTURE_2D, bayerPatternTex);
+       pr->setUniformValue(texturesShaderVars.bayerPattern, 1);
+       pr->setUniformValue(texturesShaderVars.rgbMaxValue, rgbMaxValue[0], rgbMaxValue[1], rgbMaxValue[2]);
    }
    else if (texCoordArray.enabled && colorArray.enabled && !normalArray.enabled)
    {
@@ -2179,6 +2207,12 @@ void StelPainter::drawFromArray(DrawingMode mode, int count, int offset, bool do
        pr->setAttributeArray(texturesColorShaderVars.color, colorArray.type, colorArray.pointer, colorArray.size);
        pr->enableAttributeArray(texturesColorShaderVars.color);
        //pr->setUniformValue(texturesShaderVars.texture, 0);    // use texture unit 0
+       glActiveTexture(GL_TEXTURE1);
+       if(!bayerPatternTex)
+           bayerPatternTex=makeBayerPatternTexture(*this);
+       glBindTexture(GL_TEXTURE_2D, bayerPatternTex);
+       pr->setUniformValue(texturesColorShaderVars.bayerPattern, 1);
+       pr->setUniformValue(texturesColorShaderVars.rgbMaxValue, rgbMaxValue[0], rgbMaxValue[1], rgbMaxValue[2]);
    }
    else if (!texCoordArray.enabled && colorArray.enabled && !normalArray.enabled)
    {
diff --git a/src/core/StelPainter.hpp b/src/core/StelPainter.hpp
index e08f8e2..612ba2c 100644
--- a/src/core/StelPainter.hpp
+++ b/src/core/StelPainter.hpp
@@ -63,6 +63,15 @@ public:
        TriangleFan                 = 0x0006  //!< GL_TRIANGLE_FAN
    };

+    enum class DitheringMode
+    {
+        Disabled,    //!< Dithering disabled, will leave the infamous color bands
+        Color565,    //!< 16-bit color (AKA High color) with R5_G6_B5 layout
+        Color666,    //!< TN+film typical color depth in TrueColor mode
+        Color888,    //!< 24-bit color (AKA True color)
+        Color101010, //!< 30-bit color (AKA Deep color)
+    };
+
    explicit StelPainter(const StelProjectorP& prj);
    ~StelPainter();

@@ -306,6 +315,8 @@ public:
    //! @return true if the link was successful.
    static bool linkProg(class QOpenGLShaderProgram* prog, const QString& name);

+    DitheringMode getDitheringMode() const { return ditheringMode; }
+
 private:

    friend class StelTextureMgr;
@@ -402,6 +413,8 @@ private:
        int vertex;
        int texColor;
        int texture;
+       int bayerPattern;
+       int rgbMaxValue;
    };
    static TexturesShaderVars texturesShaderVars;
    static QOpenGLShaderProgram* texturesColorShaderProgram;
@@ -411,9 +424,14 @@ private:
        int vertex;
        int color;
        int texture;
+       int bayerPattern;
+       int rgbMaxValue;
    };
    static TexturesColorShaderVars texturesColorShaderVars;

+   GLuint bayerPatternTex=0;
+    DitheringMode ditheringMode;
+    static DitheringMode parseDitheringMode(QString const& s);

    //! The descriptor for the current opengl vertex array
    ArrayDesc vertexArray;
@@ -425,5 +443,7 @@ private:
    ArrayDesc colorArray;
 };

+Q_DECLARE_METATYPE(StelPainter::DitheringMode)
+
 #endif // _STELPAINTER_HPP_

diff --git a/src/core/modules/Atmosphere.cpp b/src/core/modules/Atmosphere.cpp
index 12953ed..eaff332 100644
--- a/src/core/modules/Atmosphere.cpp
+++ b/src/core/modules/Atmosphere.cpp
@@ -25,6 +25,7 @@
 #include "StelCore.hpp"
 #include "StelPainter.hpp"
 #include "StelFileMgr.hpp"
+#include "Dithering.hpp"

 #include <QDebug>
 #include <QSettings>
@@ -58,10 +59,11 @@ Atmosphere::Atmosphere(void)
    }
    QOpenGLShader fShader(QOpenGLShader::Fragment);
    if (!fShader.compileSourceCode(
+                    makeDitheringShader()+
                    "varying mediump vec3 resultSkyColor;\n"
                    "void main()\n"
                    "{\n"
-                    "   gl_FragColor = vec4(resultSkyColor, 1.);\n"
+                    "   gl_FragColor = vec4(dither(resultSkyColor), 1.);\n"
                     "}"))
    {
        qFatal("Error while compiling atmosphere fragment shader: %s", fShader.log().toLatin1().constData());
@@ -76,6 +78,8 @@ Atmosphere::Atmosphere(void)
    StelPainter::linkProg(atmoShaderProgram, "atmosphere");

    atmoShaderProgram->bind();
+   shaderAttribLocations.bayerPattern = atmoShaderProgram->uniformLocation("bayerPattern");
+   shaderAttribLocations.rgbMaxValue = atmoShaderProgram->uniformLocation("rgbMaxValue");
    shaderAttribLocations.alphaWaOverAlphaDa = atmoShaderProgram->uniformLocation("alphaWaOverAlphaDa");
    shaderAttribLocations.oneOverGamma = atmoShaderProgram->uniformLocation("oneOverGamma");
    shaderAttribLocations.term2TimesOneOverMaxdLpOneOverGamma = atmoShaderProgram->uniformLocation("term2TimesOneOverMaxdLpOneOverGamma");
@@ -368,6 +372,15 @@ void Atmosphere::draw(StelCore* core)
    const Mat4f& m = sPainter.getProjector()->getProjectionMatrix();
    atmoShaderProgram->setUniformValue(shaderAttribLocations.projectionMatrix,
        QMatrix4x4(m[0], m[4], m[8], m[12], m[1], m[5], m[9], m[13], m[2], m[6], m[10], m[14], m[3], m[7], m[11], m[15]));
+
+   const auto rgbMaxValue=calcRGBMaxValue(sPainter.getDitheringMode());
+   atmoShaderProgram->setUniformValue(shaderAttribLocations.rgbMaxValue, rgbMaxValue[0], rgbMaxValue[1], rgbMaxValue[2]);
+   auto& gl=*sPainter.glFuncs();
+   gl.glActiveTexture(GL_TEXTURE1);
+   if(!bayerPatternTex)
+       bayerPatternTex=makeBayerPatternTexture(*sPainter.glFuncs());
+   gl.glBindTexture(GL_TEXTURE_2D, bayerPatternTex);
+   atmoShaderProgram->setUniformValue(shaderAttribLocations.bayerPattern, 1);

    colorGridBuffer.bind();
    atmoShaderProgram->setAttributeBuffer(shaderAttribLocations.skyColor, GL_FLOAT, 0, 4, 0);
diff --git a/src/core/modules/Atmosphere.hpp b/src/core/modules/Atmosphere.hpp
index 9553008..18e697c 100644
--- a/src/core/modules/Atmosphere.hpp
+++ b/src/core/modules/Atmosphere.hpp
@@ -103,6 +103,8 @@ private:
    //! Vertex shader used for xyYToRGB computation
    class QOpenGLShaderProgram* atmoShaderProgram;
    struct {
+       int bayerPattern;
+       int rgbMaxValue;
        int alphaWaOverAlphaDa;
        int oneOverGamma;
        int term2TimesOneOverMaxdLpOneOverGamma;
@@ -114,6 +116,8 @@ private:
        int skyVertex;
        int skyColor;
    } shaderAttribLocations;
+
+   GLuint bayerPatternTex=0;
 };

 #endif // _ATMOSTPHERE_HPP_
gzotti commented 6 years ago

Looks nice and clean, but for some reason I could not get the QVariant to read a useful settings value from config.ini, neither int nor names. (Maybe a stupid copying error on my side, though.) I turned the StelPainter into a Q_GADGET and added the Q_ENUM(DitheringMode), now it reads/writes the enum names, and I see differences (some small pattern in 565 mode). So, manual configuration is possible!

Is there anything you think is unfinished and you want to polish further, or else I would happily accept&commit this solution, it solves a long-standing wish (see LP bug/wishlist entry, I assigned its solving to you.) Our GUI needs a bit attention, I would do that in the next few days.

10110111 commented 6 years ago

for some reason I could not get the QVariant to read a useful settings value from config.ini, neither int nor names

Yeah, that's QTBUG-53384. See the latest edit of my previous comment, it has another approach with QVariant::toString, followed by manual parsing (and then the config values will have to be the same as enumerator names instead of numbers).

Is there anything you think is unfinished and you want to polish further, or else I would happily accept&commit this solution

If the latest version of the diff above is acceptable, I'll put it into a pull request, so that you can merge it.

gzotti commented 6 years ago

@10110111 I had to make a slight change to get it to run on OpenGL ES2 (ANGLE and Raspberry Pi), explicitly declaring mediump (or highp, but it does not change the result). However, this seems to brighten all textured rectangles significantly so they become visible, which looks more ugly than ever before. Any idea what is the difference in the two in this respect, and how to fix that? (Some 0/1 offset, or rounding in different directions?) Screenshot from ANGLE, in OpenGL the texture rectangles are never visible. stellarium-075 klein

10110111 commented 6 years ago

Is there any way I could reproduce this on x86_64 Ubuntu?

10110111 commented 6 years ago

A wild guess: what if you replace vec3 tail=fract(rgb); with vec3 tail=rgb-head;?

gzotti commented 6 years ago

I am afraid no, this seems to be a GL ES2 problem. (I can show working result in OpenGL and bad in GL ES2/ANGLE on Windows and bad in GL ES2 on Raspberry Pi3.) But I don't know whether there is a way to force a desktop GL system into GL ES2 mode on Ubuntu. You see, this is one of our problems, multiplatform, many devices, all are slightly different, and we have far from all devices available.

I tried rgb-head, does not make a difference. :-(

What about the maxRGB, should this be 2^n-1 or 2^n? It seems the value for Color101010 is off by one?

10110111 commented 6 years ago

What about the maxRGB, should this be 2^n-1 or 2^n? It seems the value for Color101010 is off by one?

Ah, right. It's the maximum value for color channel, so indeed Color101010 should have 1023 instead of 1024.

I can show working result in OpenGL and bad in GL ES2/ANGLE on Windows and bad in GL ES2 on Raspberry Pi3.

Please do, with the same color depth mode, and please mention which color depth it actually is.

10110111 commented 6 years ago

Also, it'd be more useful to see an isolated bad texture, preferably on black background (turning off atmosphere?). This way I'd be able to at least compare pixel values and possibly make some better guesses.

gzotti commented 6 years ago

OK, here is a series, which just revealed another bug around our reworked screenshots (What you see in white here is actually transparent, but should be initialized to black before export, obviously. Usually the Milky Way covers that... ) But let's concentrate on the topic here.

All made with ANGLE, showing Sculptor and surroundings, stars, atmosphere, milky way and Zodiacal light switched off. Artwork brightness=1. You see some DSO (galaxies) as little specks, they are additional visible texture squares.

es2dith-none

es2dith-565

es2dith-666

es2dith-888

es2dith-101010

gzotti commented 6 years ago

Same for OpenGL native (Geforce 960M). The 6bit modes have a tiny dot pattern, but no visible texture squares.

10110111 commented 6 years ago

Let's stick to 565 dithering mode for this experiment. Could you please extract the actual values of bayer, head, tail and step(bayer,tail) from a point of the screenshot's dark area where currently the RGB value of 8,4,8 repeats? You can do this using this GLSL code for toColor function. Namely, copy-paste the implementation to the beginning of the dithering shader (adding highp where appropriate), and then e.g. for bayer variable add a return toColor(bayer).rgb; statement (and .a for the LSB).

For vector variables only red channel would be enough for now. It'll also be good enough if you simply provide the RGBA values returned from toColor, I'll convert them back to float myself.

gzotti commented 6 years ago

Sorry, seems I cannot continue today on this, hopefully in the next days or on the weekend.

10110111 commented 6 years ago

Same for OpenGL native (Geforce 960M). The 6bit modes have a tiny dot pattern, but no visible texture squares

The dot pattern instead of true blackness is an off-by-ULP error due to not quite correct usage of GLSL step function. I'll make a PR to fix this.

10110111 commented 6 years ago

As for the problem on GLES2, I've failed to reproduce it with Mesa 17.1 software renderer and Qt configured with -opengl es2 option. I get nice rendering there. So I'll still need your results of extraction of values.

gzotti commented 6 years ago

I tried to activate this toColor output (very interesting!) and changed all floats to highp but on ANGLE(GLES2) I get

ERROR: 0:82: 'mod' : no matching overloaded function found

This refers to binary32.y+=128*int(mod(e,2));

integer mod is not available on GLES2. Is it useful to try casting to float? (And how?)

10110111 commented 6 years ago

Yes, simply cast both arguments to float, like mod(float(e),2.) instead of mod(e,2).

gzotti commented 6 years ago

Next issue:

QOpenGLShader::compile(Fragment): ERROR: 0:41: 'n' : Loop index cannot be compared with non-constant expression

This is in int part(), the line after "// Multiplication is exact since it's just an increase of exponent by 8"

Maybe the number has to be split and exponent increased some other way?

10110111 commented 6 years ago

Mm... I think this

for(int n=0;n<N;++n)
    x*=byteShift;

would be equivalent to

for(int n=0;n<3;++n)
    if(n<N)
        x*=byteShift;

, since N<=2 as specified in all the callers.

gzotti commented 6 years ago

Yes, result looks identical on OpenGL. I am seeing toColor(bayer).rgb; now, a blue dot pattern. In GLES there is nothing, simple black. I will test the next values...

10110111 commented 6 years ago

Simple black... that looks like a driver bug. I had black result from older Mesa on an Intel Haswell chip. Could you check that simply returning vec3(bayer) without additional processing gives you constant color?

gzotti commented 6 years ago

vec3(bayer) shows the texture squares with a white dither pattern on OpenGL, but nothing (black) on ES2/ANGLE. As we saw in that other bug report (planets), ANGLE with Qt5.9.0 seems worse than some earlier version. I have no shader compile warning, but cannot make screenshots due to some Attachment problem. (I make screenshots with Windows' own Shift-Print and store from clipboard). The screenshot with OpenGL works. If this is only an ANGLE bug, we should not waste too much time on that. The values for rgb, head and tail look pretty much the same, I can send them if you want. I may be able to test on weekend with Raspberry (native ES2). Sorry, I must quit for today.

gzotti commented 6 years ago

Alright @10110111 , I can continue today.

Results with dither 5/6/5 from Qt5.9/Windows, Driver version string: "OpenGL ES 2.0 (ANGLE 2.1.0.8613f4946861)" return of vec3(bayer) is black, nothing. Of course, also toColor(bayer).rgb. Running with OpenGL looks more meaningful, white (or bluish for toColor()) pattern.

I just ran the vec3(bayer) on a Raspberry Pi3 with VC4 driver (Mesa 17). Driver version string "OpenGL ES 2.0 Mesa 17.4.0 devel (git-7983adc60f)". Also here I have a black screen. I will try 2 other PCs with (hopefully) different Qt /ANGLE versions.

To make sure about mediump and highp, my Dithering.hpp is (sorry for the formatting, why is the start part wrong?):

inline QString makeDitheringShader() { return R"( const int emax=127; // Input: x>=0 // Output: base 2 exponent of x if (x!=0 && !isnan(x) && !isinf(x)) // -emax if x==0 // emax+1 otherwise int floorLog2(highp float x) { if(x==0.) return -emax; // NOTE: there exist values of x, for which floor(log2(x)) will give wrong // (off by one) result as compared to the one calculated with infinite precision. // Thus we do it in a brute-force way. for(int e=emax;e>=1-emax;--e) if(x>=exp2(float(e))) return e; // If we are here, x must be infinity or NaN return emax+1; } // Input: any x // Output: IEEE 754 biased exponent with bias=emax int biasedExp(highp float x) { return emax+floorLog2(abs(x)); }

        // Input: any x such that (!isnan(x) && !isinf(x))
        // Output: significand AKA mantissa of x if !isnan(x) && !isinf(x)
        //         undefined otherwise
        highp float significand(highp float x)
        {
            // converting int to float so that exp2(genType) gets correctly-typed value
            highp float expo=float(floorLog2(abs(x)));
            return abs(x)/exp2(expo);
        }

        // Input: x\in[0,1)
        //        N>=0
        // Output: Nth byte as counted from the highest byte in the fraction
        int part(highp float x,int N)
        {
            // All comments about exactness here assume that underflow and overflow don't occur
            const highp float byteShift=256.;
            // Multiplication is exact since it's just an increase of exponent by 8 == LINE 40
            //for(int n=0;n<N;++n)
            //x*=byteShift;
        for(int n=0;n<3;++n)
            if(n<N)
            x*=byteShift;

            // Cut higher bits away.
            // $q \in [0,1) \cap \mathbb Q'.$
            highp float q=fract(x);

            // Shift and cut lower bits away. Cutting lower bits prevents potentially unexpected
            // results of rounding by the GPU later in the pipeline when transforming to TrueColor
            // the resulting subpixel value. 
            // $c \in [0,255] \cap \mathbb Z.$
            // Multiplication is exact since it's just and increase of exponent by 8
            highp float c=floor(byteShift*q);
            return int(c);
        }

        // Input: any x acceptable to significand()
        // Output: significand of x split to (8,8,8)-bit data vector  
        ivec3 significandAsIVec3(highp float x)
        {
            ivec3 result;
            highp float sig=significand(x)/2.; // shift all bits to fractional part
            result.x=part(sig,0);
            result.y=part(sig,1);
            result.z=part(sig,2);
            return result;
        }

        // Input: any x such that !isnan(x)
        // Output: IEEE 754 defined binary32 number, packed as ivec4(byte3,byte2,byte1,byte0) 
        ivec4 packIEEE754binary32(highp float x)
        {
            int e = biasedExp(x);
            // sign to bit 7
            int s = x<0. ? 128 : 0;

            ivec4 binary32;
            binary32.yzw=significandAsIVec3(x);
            // clear the implicit integer bit of significand
            if(binary32.y>=128) binary32.y-=128;
            // put lowest bit of exponent into its position, replacing just cleared integer bit
            binary32.y+=128*int(mod(float(e),2.));
            // prepare high bits of exponent for fitting into their positions
            e/=2;
            // pack highest byte
            binary32.x=e+s;

            return binary32;
        }

        highp vec4 toColor(highp float x)
        {
            ivec4 binary32=packIEEE754binary32(x);
            // Transform color components to [0,1] range.
            // Division is inexact, but works reliably for all integers from 0 to 255 if
            // the transformation to TrueColor by GPU uses rounding to nearest or upwards.
            // The result will be multiplied by 255 back when transformed
            // to TrueColor subpixel value by OpenGL.
            return vec4(binary32)/255.;
        }

        uniform mediump vec3 rgbMaxValue;
        uniform sampler2D bayerPattern;
        highp  vec3 dither(highp  vec3 c)
        {
        if(rgbMaxValue.r==0.) return c;
        highp float bayer=texture2D(bayerPattern,gl_FragCoord.xy/8.).r;

        //return vec3(bayer);
        return toColor(bayer).rgb;
        //highp vec3 rgb=c*rgbMaxValue;
        //return rgb;
        //highp vec3 head=floor(rgb);
        //return head;
        //highp vec3 tail=rgb-head;
        //return tail;
        //return (head+1.-step(tail,vec3(bayer)))/rgbMaxValue;
        }
        highp  vec4 dither(highp  vec4 c) { return vec4(dither(c.xyz),c.w); }
        )";

}

In the last block of course I return what's needed.

gzotti commented 6 years ago

Interestingly, on the RasPi3, the final image seems OK. No brightened texture squares:

rpi_dith565

(Image made with scrot, currently we have some WIP issues around built-in screenshots...)

10110111 commented 6 years ago

sorry for the formatting, why is the start part wrong?

The code here is formatted either when each line is prepended with four spaces like

    someCode()

or when the snipped is enclosed in triple backticks ```, like

```
someCode();
```

You can edit your post to fix it.

10110111 commented 6 years ago

I just ran the vec3(bayer) on a Raspberry Pi3 with VC4 driver (Mesa 17). Driver version string "OpenGL ES 2.0 Mesa 17.4.0 devel (git-7983adc60f)". Also here I have a black screen.

What if you set LIBGL_ALWAYS_SOFTWARE=1 environment variable before running the test?