Bug 194817 – [EGL] Runtime support for RGB565 pixel layout

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Philippe Normand]

Currently our graphics pipeline always relies on a ARGB8888 (32 bpp) pixel configuration. On some low-end (old) embedded platforms the graphics driver is sometimes optimized for 16 bpp configurations, such as RGB565. So it would be nice if there was a runtime setting that could be used on these platforms.

I started a patch...

[Philippe Normand]

http://www.barth-dev.de/online/rgb565-color-picker/

Especially (cheap) screens used with embedded devices do not provide 24 bit color-depth. Moreover, storing and/or transmitting 3 bytes per pixel is consuming quite some memory and creates latency. RGB565 requires only 16 (5+6+5) bits/2 bytes and is commonly used with embedded screens.

[Philippe Normand]

Created attachment 362385 [details]
Patch

[Philippe Normand]

I'm not really happy with the added optional arguments, I can change them to use WTF::Optional instead, if deemed better approach :)

[Carlos Garcia Campos]

Comment on attachment 362385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362385&action=review

Why do we need a setting for this? Can't it be detected somehow?

> Source/WebCore/platform/graphics/GLContext.cpp:79
> +std::unique_ptr<GLContext> GLContext::createContextForWindow(GLNativeWindowType windowHandle, PlatformDisplay* platformDisplay, uint32_t eglConfigBitsPerPixel)

If this value is set in the platform display, we wouldsn't need to pass it everywhere, we could get it from the display.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:161
> +#if !PLATFORM(WPE)
> +    bitsPerPixel = 32;
> +#endif

Why is this specific to WPE?

> Source/WebCore/platform/graphics/egl/GLContextEGL.h:92
> +    static std::unique_ptr<GLContextEGL> createSurfacelessContext(PlatformDisplay&, EGLContext sharingContext = nullptr, uint32_t bitsPerPixel = 42);

why 42 here?

> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:2123
> +#if PLATFORM(WPE)
> +void WKPreferencesSetEGLConfigBitsPerPixel(WKPreferencesRef preferencesRef, int bitsPerPixel)
> +{
> +    toImpl(preferencesRef)->setEglConfigBitsPerPixel(bitsPerPixel);
> +}
> +
> +bool WKPreferencesGetEGLConfigBitsPerPixel(WKPreferencesRef preferencesRef)
> +{
> +    return toImpl(preferencesRef)->eglConfigBitsPerPixel();
> +}
> +#endif

Please do not add anew C API unless it's used by WKTR

[Philippe Normand]

Comment on attachment 362385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362385&action=review

>> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:161
>> +#endif
> 
> Why is this specific to WPE?

The whole patch is specific to WPE because I assumed the new setting would be useful for this port only, as it's the preferred port nowadays for embedded devices.

>> Source/WebCore/platform/graphics/egl/GLContextEGL.h:92
>> +    static std::unique_ptr<GLContextEGL> createSurfacelessContext(PlatformDisplay&, EGLContext sharingContext = nullptr, uint32_t bitsPerPixel = 42);
> 
> why 42 here?

Typo :)

>> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:2123
>> +#endif
> 
> Please do not add anew C API unless it's used by WKTR

Hum ok, but can I still have a web-setting without C API ?

[Philippe Normand]

(In reply to Carlos Garcia Campos from comment #4)
> Comment on attachment 362385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362385&action=review
> 
> Why do we need a setting for this? Can't it be detected somehow?
> 

I'll let others chime in on this.
For my case (i.MX51 board) it can't be detected because most formats are already advertised as supported (from 16bpp to 32bpp) but only I know that 16bpp should be preferred. AFAIK the EGL config attributes provide no support pixel format "hinting".

[Philippe Normand]

Comment on attachment 362385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362385&action=review

>>> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:2123
>>> +#endif
>> 
>> Please do not add anew C API unless it's used by WKTR
> 
> Hum ok, but can I still have a web-setting without C API ?

The answer is 42^Wyes

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #7)
> Comment on attachment 362385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362385&action=review
> 
> >>> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:2123
> >>> +#endif
> >> 
> >> Please do not add anew C API unless it's used by WKTR
> > 
> > Hum ok, but can I still have a web-setting without C API ?
> 
> The answer is 42^Wyes

Yes, I only complained about the C API. But I'm not sure about the glib one either. This is very specific use-case for a particular hw/driver, and it's not something we might want to configure per web view. Maybe an env var would be more appropriate.

[Philippe Normand]

(In reply to Carlos Garcia Campos from comment #8)
> (In reply to Philippe Normand from comment #7)
> > Comment on attachment 362385 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=362385&action=review
> > 
> > >>> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:2123
> > >>> +#endif
> > >> 
> > >> Please do not add anew C API unless it's used by WKTR
> > > 
> > > Hum ok, but can I still have a web-setting without C API ?
> > 
> > The answer is 42^Wyes
> 
> Yes, I only complained about the C API. But I'm not sure about the glib one
> either. This is very specific use-case for a particular hw/driver, and it's
> not something we might want to configure per web view. Maybe an env var
> would be more appropriate.

Before I update the patch I would like to know which approach should be used...
An env var would be easier to support than a web-setting, in the end it's mostly a question about semantics. So... do we use an env-var or a web-setting for this?

[Adrian Perez]

(In reply to Philippe Normand from comment #6)
> (In reply to Carlos Garcia Campos from comment #4)
> > Comment on attachment 362385 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=362385&action=review
> > 
> > Why do we need a setting for this? Can't it be detected somehow?
> > 
> 
> I'll let others chime in on this.
> For my case (i.MX51 board) it can't be detected because most formats are
> already advertised as supported (from 16bpp to 32bpp) but only I know that
> 16bpp should be preferred. AFAIK the EGL config attributes provide no
> support pixel format "hinting".

I have also seen some systems in which the GPU is perfectly capable
of rendering in 32bpp modes, but the display attached (typically some
kind of TFT LCD) only supports 16bpp. I have been working with one
where the framebuffer device (correctly) allows only configuring 16bpp
modes but WebKit still needs to be patched to allow choosing an EGLConfig
where color components can have less than 8bit. This patch (or similar)
would help and avoid needing to apply patches in my case as well.

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #10)
> (In reply to Philippe Normand from comment #6)
> > (In reply to Carlos Garcia Campos from comment #4)
> > > Comment on attachment 362385 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=362385&action=review
> > > 
> > > Why do we need a setting for this? Can't it be detected somehow?
> > > 
> > 
> > I'll let others chime in on this.
> > For my case (i.MX51 board) it can't be detected because most formats are
> > already advertised as supported (from 16bpp to 32bpp) but only I know that
> > 16bpp should be preferred. AFAIK the EGL config attributes provide no
> > support pixel format "hinting".
> 
> I have also seen some systems in which the GPU is perfectly capable
> of rendering in 32bpp modes, but the display attached (typically some
> kind of TFT LCD) only supports 16bpp. I have been working with one
> where the framebuffer device (correctly) allows only configuring 16bpp
> modes but WebKit still needs to be patched to allow choosing an EGLConfig
> where color components can have less than 8bit. This patch (or similar)
> would help and avoid needing to apply patches in my case as well.

I'm only concerned (not even opposed) to the setting, not the patch itself.

[Philippe Normand]

Yeah I think we all agree the patch makes sense. The dilemma is about env-var vs setting...

An env-var would simplify the patch, a lot. But is less discoverable from the user POV, unless we have a place in the docs documenting env-vars?

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #12)
> Yeah I think we all agree the patch makes sense. The dilemma is about
> env-var vs setting...
> 
> An env-var would simplify the patch, a lot. But is less discoverable from
> the user POV, unless we have a place in the docs documenting env-vars?

We don't, but we should.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #13)
> (In reply to Philippe Normand from comment #12)
> > Yeah I think we all agree the patch makes sense. The dilemma is about
> > env-var vs setting...
> > 
> > An env-var would simplify the patch, a lot. But is less discoverable from
> > the user POV, unless we have a place in the docs documenting env-vars?
> 
> We don't, but we should.

This is a good point. I have found myself now and then grepping
the WebKit sources to find out which env-var we had for this or
that (e.g. like the one for pausing the WebProcess to give time
to attach the debugger).

Not all environment variables need to be in the documentation,
for example the ones used for development probably would fit better
somewhere in the wiki.

[Carlos Garcia Campos]

Comment on attachment 362385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362385&action=review

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:110
> +        EGL_SAMPLES, 0,

Why do you add EGL_SAMPLES too?

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:118
> +    EGLint bufferSize = bitsPerPixel;
> +    if (bufferSize == 16) {
> +        attributeList[9] = 0;
> +        attributeList[11] = 0;
> +    }

I don't understand this. 9 is EGL_STENCIL_SIZE and 11 is EGL_ALPHA_SIZE. Why do we only change them if bitsPerPixel is 16, don't we want to support other depth sizes? like 24 for example? And why do we set the values to 0?

[Philippe Normand]

Comment on attachment 362385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362385&action=review

>> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:110
>> +        EGL_SAMPLES, 0,
> 
> Why do you add EGL_SAMPLES too?

Perhaps it can be unset, I just followed the same code as weston's simple-egl.

>> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:118
>> +    }
> 
> I don't understand this. 9 is EGL_STENCIL_SIZE and 11 is EGL_ALPHA_SIZE. Why do we only change them if bitsPerPixel is 16, don't we want to support other depth sizes? like 24 for example? And why do we set the values to 0?

stencil and alpha are disabled here so that we have a chance to reach 16bpp... Disabling alpha means we use an RGB format, like RGB565, which uses 16 bits for storage.
For 24bpp, I think additional changes might be needed, I haven't investigated this.

[Carlos Garcia Campos]

Comment on attachment 362385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362385&action=review

>>> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:110
>>> +        EGL_SAMPLES, 0,
>> 
>> Why do you add EGL_SAMPLES too?
> 
> Perhaps it can be unset, I just followed the same code as weston's simple-egl.

But 0 is the default for both depth size and samples.

>>> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:118
>>> +    }
>> 
>> I don't understand this. 9 is EGL_STENCIL_SIZE and 11 is EGL_ALPHA_SIZE. Why do we only change them if bitsPerPixel is 16, don't we want to support other depth sizes? like 24 for example? And why do we set the values to 0?
> 
> stencil and alpha are disabled here so that we have a chance to reach 16bpp... Disabling alpha means we use an RGB format, like RGB565, which uses 16 bits for storage.
> For 24bpp, I think additional changes might be needed, I haven't investigated this.

That means we are exposing a setting to set the depth size, but that only works with value 16 and what it does is disabling alpha, instead of changing the depth size in the config. It's very confusing.

[Philippe Normand]

Comment on attachment 362385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362385&action=review

>>>> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:110
>>>> +        EGL_SAMPLES, 0,
>>> 
>>> Why do you add EGL_SAMPLES too?
>> 
>> Perhaps it can be unset, I just followed the same code as weston's simple-egl.
> 
> But 0 is the default for both depth size and samples.

I'll revert this change in the next patch iteration.

>>>> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:118
>>>> +    }
>>> 
>>> I don't understand this. 9 is EGL_STENCIL_SIZE and 11 is EGL_ALPHA_SIZE. Why do we only change them if bitsPerPixel is 16, don't we want to support other depth sizes? like 24 for example? And why do we set the values to 0?
>> 
>> stencil and alpha are disabled here so that we have a chance to reach 16bpp... Disabling alpha means we use an RGB format, like RGB565, which uses 16 bits for storage.
>> For 24bpp, I think additional changes might be needed, I haven't investigated this.
> 
> That means we are exposing a setting to set the depth size, but that only works with value 16 and what it does is disabling alpha, instead of changing the depth size in the config. It's very confusing.

I think you're confusing pixel formats with depth testing... Anyway I'll let actual experts step in here :)

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #18)
> Comment on attachment 362385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362385&action=review
> 
> >>>> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:110
> >>>> +        EGL_SAMPLES, 0,
> >>> 
> >>> Why do you add EGL_SAMPLES too?
> >> 
> >> Perhaps it can be unset, I just followed the same code as weston's simple-egl.
> > 
> > But 0 is the default for both depth size and samples.
> 
> I'll revert this change in the next patch iteration.
> 
> >>>> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:118
> >>>> +    }
> >>> 
> >>> I don't understand this. 9 is EGL_STENCIL_SIZE and 11 is EGL_ALPHA_SIZE. Why do we only change them if bitsPerPixel is 16, don't we want to support other depth sizes? like 24 for example? And why do we set the values to 0?
> >> 
> >> stencil and alpha are disabled here so that we have a chance to reach 16bpp... Disabling alpha means we use an RGB format, like RGB565, which uses 16 bits for storage.
> >> For 24bpp, I think additional changes might be needed, I haven't investigated this.
> > 
> > That means we are exposing a setting to set the depth size, but that only works with value 16 and what it does is disabling alpha, instead of changing the depth size in the config. It's very confusing.
> 
> I think you're confusing pixel formats with depth testing... Anyway I'll let
> actual experts step in here :)

Yes, probably, this is indeed confusing, it's weird to expose a setting that allows to set any bpp but only works if 0 or 16 is passed. Whatever bpp means, that should be an enum and not a integer, I would say.

[Miguel Gomez]

Couple of comments related to what you're talking:

- EGL_DEPTH_SIZE is not set because we don't use depth testing so we want it to be 0 (default value). Same with EGL_SAMPLES.

- EGL_STENCIL_SIZE needs to be 8 cause we need it to do stencil clipping. That's not related to bpp as that's not transferred to the display.

- When setting the values of EGL_RED_SIZE, EGL_BLUE_SIZE and EGL_GREEN_SIZE to 1 you are saying that you want the smallest size available for those. But that could be RGB444 if that's supported and not the RGB565 that you're expecting. If you want RGB565 specifically, you should set those values to 5, 6, and 5 respectively.

[Miguel Gomez]

Maybe instead of receiving the bpp, what we need to receive is a format. For example WPE_EGL_FORMAT and allow RGB565 for starters, and then fallback to RGBA8888 if that's not supported. Later we can add more formats if we want to.

[Philippe Normand]

(In reply to Miguel Gomez from comment #20)
> Couple of comments related to what you're talking:
> 
> - EGL_DEPTH_SIZE is not set because we don't use depth testing so we want it
> to be 0 (default value). Same with EGL_SAMPLES.
> 
> - EGL_STENCIL_SIZE needs to be 8 cause we need it to do stencil clipping.
> That's not related to bpp as that's not transferred to the display.
> 
> - When setting the values of EGL_RED_SIZE, EGL_BLUE_SIZE and EGL_GREEN_SIZE
> to 1 you are saying that you want the smallest size available for those. But
> that could be RGB444 if that's supported and not the RGB565 that you're
> expecting. If you want RGB565 specifically, you should set those values to
> 5, 6, and 5 respectively.

But RGB444 isn't a 16bpp format, is it?
As I understand it, setting RGB channels to 1 constitutes a low-barrier for filtering on EGL_BUFFER_SIZE later on.

(In reply to Miguel Gomez from comment #21)
> Maybe instead of receiving the bpp, what we need to receive is a format. For
> example WPE_EGL_FORMAT and allow RGB565 for starters, and then fallback to
> RGBA8888 if that's not supported. Later we can add more formats if we want
> to.

Yes... that's an option too. Explicit is better than implicit... But I'm not sure there's a use for say, 24bpp formats... As mentioned in previous comments there's quite a few setups where 16bpp is the preferred format.

In this patch I basically copied the behaviour of weston's simple-egl client, which has a runtime option for 16bpp formats...

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #22)
> (In reply to Miguel Gomez from comment #20)
> > Couple of comments related to what you're talking:
> > 
> > - EGL_DEPTH_SIZE is not set because we don't use depth testing so we want it
> > to be 0 (default value). Same with EGL_SAMPLES.
> > 
> > - EGL_STENCIL_SIZE needs to be 8 cause we need it to do stencil clipping.
> > That's not related to bpp as that's not transferred to the display.
> > 
> > - When setting the values of EGL_RED_SIZE, EGL_BLUE_SIZE and EGL_GREEN_SIZE
> > to 1 you are saying that you want the smallest size available for those. But
> > that could be RGB444 if that's supported and not the RGB565 that you're
> > expecting. If you want RGB565 specifically, you should set those values to
> > 5, 6, and 5 respectively.
> 
> But RGB444 isn't a 16bpp format, is it?
> As I understand it, setting RGB channels to 1 constitutes a low-barrier for
> filtering on EGL_BUFFER_SIZE later on.
> 
> (In reply to Miguel Gomez from comment #21)
> > Maybe instead of receiving the bpp, what we need to receive is a format. For
> > example WPE_EGL_FORMAT and allow RGB565 for starters, and then fallback to
> > RGBA8888 if that's not supported. Later we can add more formats if we want
> > to.
> 
> Yes... that's an option too. Explicit is better than implicit... But I'm not
> sure there's a use for say, 24bpp formats... As mentioned in previous
> comments there's quite a few setups where 16bpp is the preferred format.
> 
> In this patch I basically copied the behaviour of weston's simple-egl
> client, which has a runtime option for 16bpp formats...

Could we simplify it then as use-rgba-egl-config (or something similar) in the public API?

[Carlos Garcia Campos]

This is what gtk does in gdkglcontext-wayland.c:

use_rgba = (visual == gdk_screen_get_rgba_visual (gdk_display_get_default_screen (display)));

if (use_rgba)
  {
    attrs[i++] = EGL_ALPHA_SIZE;
    attrs[i++] = 1;
  }
else
  {
    attrs[i++] = EGL_ALPHA_SIZE;
    attrs[i++] = 0;
  }

So, for the GTK port we could get the web view visual and send use_rgba to the web process without any new public API (I'm not asking to do this as port of this patch, though)

[Philippe Normand]

Disabling alpha is not enough because it might lead to RGB888 being selected.

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #25)
> Disabling alpha is not enough because it might lead to RGB888 being selected.

GTK also uses 1 for EGL_RED_SIZE, EGL_BLUE_SIZE and EGL_GREEN_SIZE

[Philippe Normand]

So which approach should I use? Env var or setting? I'd like to get this in WPE 2.24 if possible, so we need to settle on the approach :)

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #27)
> So which approach should I use? Env var or setting? I'd like to get this in
> WPE 2.24 if possible, so we need to settle on the approach :)

I think it's too late for new API in 2.24, specially when it's not clear enough how the thing should be exposed in the API.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #26)
> (In reply to Philippe Normand from comment #25)
> > Disabling alpha is not enough because it might lead to RGB888 being selected.
> 
> GTK also uses 1 for EGL_RED_SIZE, EGL_BLUE_SIZE and EGL_GREEN_SIZE

And allows to set a different visual per window, so maybe we should also allow to use a different config per web view in the end.

[Philippe Normand]

Created attachment 362894 [details]
Patch

[Philippe Normand]

Comment on attachment 362894 [details]
Patch

Oops,  this is incomplete

[Philippe Normand]

Created attachment 362895 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 362895 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362895&action=review

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:114
> +    const char* environmentVariable = getenv("WEBKIT_EGL_PIXEL_FORMAT");
> +    if (environmentVariable && *environmentVariable) {

I don't think we need to check it's not empty, you could simply:

if (const char* pixelFormat = getenv("WEBKIT_EGL_PIXEL_FORMAT")) {

}

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:120
> +            attributeList[3] = 5;
> +            attributeList[5] = 6;
> +            attributeList[7] = 5;
> +            attributeList[9] = 0;

This is confusing, please add a comment before every line saying with the value this is for:

// EGL_RED_SIZE
attributeList[3] = 5;
// EGL_GREEN_SIZE
attributeList[5] = 6;
.....

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:122
> +            g_warning("Value supplied for WEBKIT_EGL_PIXEL_FORMAT should be RGB565 or the environment variable should not be set at all.");

Do not use g_warning() in this file that is not glib specific. You can use WTFLogAlways. I would say something like: "Unknown pixel format %s, falling back to RGBA8888".

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:141
> +    EGLint count;
> +    if (!eglGetConfigs(display, nullptr, 0, &count) || !count)
> +        return false;

I don't think we want all display configs, but only the ones matching the attributes. You can use eglChooseConfig() passing nullptr as configs to get the numbers of configs.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:143
> +    EGLConfig* configs = reinterpret_cast<EGLConfig*>(fastCalloc(sizeof(EGLConfig), count));

Use unique_ptr here, this is leaked if eglChooseConfig fails below.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:146
>      EGLint numberConfigsReturned;
> -    return eglChooseConfig(display, attributeList, config, 1, &numberConfigsReturned) && numberConfigsReturned;
> +    if (!eglChooseConfig(display, attributeList, configs, count, &numberConfigsReturned) || !numberConfigsReturned)
> +        return false;

I think we can keep using 1 config only for the non rgb565 case, since we don'tm need to iterate configs in such case. We could do something like:

if (!eglChooseConfig(display, attributeList, isRGB565 ? configs : config,  isRGB565 ? count : 1, &numberConfigsReturned) || !numberConfigsReturned)
    return false;

if (!isRGB565)
    return true;

// Iterate configs to find the one matching.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:150
> +        eglGetConfigAttrib(display, configs[i], EGL_BUFFER_SIZE, &size);

Is really buffer size enough? couldn't be other formats with 16 that are not RGB565? I think it would be safer to get EGL_RED_SIZE, EGL_GREEN_SIZE, EGL_BLUE_SIZE, and EGL_ALPHA_SIZE, and check they are 5, 6, 5, 0.

[Philippe Normand]

Created attachment 362974 [details]
Patch

[Philippe Normand]

Created attachment 362975 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 362974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362974&action=review

> Source/WebCore/ChangeLog:3
> +        [WPE][EGL] Runtime support for RGB565 pixel layout

This is not WPE specific anymore

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:150
> +    auto configs = std::make_unique<Vector<EGLConfig>>(count);

Using Vector is a good idea, but then you don't need to use std::unique_ptr, just keep the vector stack allocated.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:162
> +        return (redSize == 5 && greenSize == 6 && blueSize == 5);

Don't we need to check alpha is 0 too?

[Philippe Normand]

Created attachment 362978 [details]
Patch

[Philippe Normand]

Committed r242084: <https://trac.webkit.org/changeset/242084>

[Michael Catanzaro]

(In reply to Philippe Normand from comment #27)
> So which approach should I use? Env var or setting? I'd like to get this in
> WPE 2.24 if possible, so we need to settle on the approach :)

We shouldn't add environment variables that must be set for WebKit to work properly, unless there's also corresponding API. If applications rely on setting the environment variable, they will -- almost guaranteed -- do so in an unsafe manner. But if we provide a corresponding API, then the environment variable becomes OK, since applications will very likely use the API, and the environment variable becomes just a convenience for debugging.

If there's really absolutely no way for WebKit to know the right pixel layout to use on its own, then we must add new API. That's better than requiring applications set an environment variable to work. But it would also be sad, because we try hard to avoid exposing low-level details like this in our API. Maybe it should be exposed only for WPE.

[Philippe Normand]

(In reply to Michael Catanzaro from comment #39)
> (In reply to Philippe Normand from comment #27)
> > So which approach should I use? Env var or setting? I'd like to get this in
> > WPE 2.24 if possible, so we need to settle on the approach :)
> 
> We shouldn't add environment variables that must be set for WebKit to work
> properly, 

s/must/may/
You once again exaggerated things.

This patch is about optimizing rendering, if the var is not set rendering will still work, although in some platforms, not optimally.

> unless there's also corresponding API. If applications rely on
> setting the environment variable, they will -- almost guaranteed -- do so in
> an unsafe manner. But if we provide a corresponding API, then the
> environment variable becomes OK, since applications will very likely use the
> API, and the environment variable becomes just a convenience for debugging.
> 
> If there's really absolutely no way for WebKit to know the right pixel
> layout to use on its own, then we must add new API. That's better than
> requiring applications set an environment variable to work. But it would
> also be sad, because we try hard to avoid exposing low-level details like
> this in our API. Maybe it should be exposed only for WPE.

I agree it doesn't make much sense to have GTK API for this.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #39)
> (In reply to Philippe Normand from comment #27)
> > So which approach should I use? Env var or setting? I'd like to get this in
> > WPE 2.24 if possible, so we need to settle on the approach :)
> 
> We shouldn't add environment variables that must be set for WebKit to work
> properly, unless there's also corresponding API.

That's not the case here.

> If applications rely on
> setting the environment variable, they will -- almost guaranteed -- do so in
> an unsafe manner. 

Sure, you know it.

> But if we provide a corresponding API, then the
> environment variable becomes OK, since applications will very likely use the
> API, and the environment variable becomes just a convenience for debugging.

Please, read the other comments in this bug. The plan is to add API for this, but we haven't agreed on how to expose it yet. Phil needs this in 2.24 branch and I don't want to add new API in 2.24 at this point without being sure about the best approach to expose this. So, the env var allows to add the functionality while we decide on the API for 2.26. 

> If there's really absolutely no way for WebKit to know the right pixel
> layout to use on its own, then we must add new API.

Again, that's the plan.

> That's better than
> requiring applications set an environment variable to work.

Again, that's not the case.

> But it would
> also be sad, because we try hard to avoid exposing low-level details like
> this in our API. Maybe it should be exposed only for WPE.

In gtk we might try to guess the right pixel layout from the GdkWindow visual.

[Michael Catanzaro]

OK, that seems perfectly fine! Just need to follow-up with new API for 2.26. And document it here:

https://trac.webkit.org/wiki/EnvironmentVariables

(In reply to Carlos Garcia Campos from comment #41)
> > We shouldn't add environment variables that must be set for WebKit to work
> > properly, unless there's also corresponding API.
> 
> That's not the case here.

It could be restated as: we shouldn't add environment variables that applications will be tempted to set.

[Robert Junior]

Created attachment 280450 [details]
Patched on https://emp3juice.blog/ https://tubidy.diy/ https://y2mate.diy/