[Windows] Use GL_RGBA as backing store internal format #54255
Conversation
loic-sharma
requested review from
cbracken,
chinmaygarde,
jonahwilliams and
yaakovschectman
| gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); | ||
| gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); | ||
| gl_->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, config.size.width, | ||
| gl_->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, config.size.width, |
There was a problem hiding this comment.
I am assuming ES 2.0 capability baselines for my review. But feel free to correct me.
I think I see the bug here. You were attempting to give a sized internal format to TexImage2D. That is only legal in OpenGL ES versions > 2.0.
But you don't check for the presence of the correct extension in GetSupportedTextureFormat. GL_RGBA8 on older ES versions requires OES_rgb8_rgba8.
But, even if you did, there are other issues with this code. Because the presence GL_BGRA8_EXT will cause the internal format and the format you tell Flutter the texture is to be off (see result->open_gl.framebuffer.target = format_;). That is, you will always create an RGBA image but may tell Flutter the image is BGRA.
I guess I would ask why you need to mess with any of the internal formats. Always assume the unsized GL_RGBA, get rid of GetSupportedTextureFormat and the format_ ivar and give 0 as the internal format to Flutter result->open_gl.framebuffer.target = 0;
You delete a bunch of code and make things simpler. And the same code gets run on new and old versions of the platform making testing easier.
There was a problem hiding this comment.
Rename GL_COLOR_ATTACHMENT0_EXT to GL_COLOR_ATTACHMENT0 from the non ext header. Its the same value but available in ES2.
There was a problem hiding this comment.
If I assume ES 2.0 compatibility. the BlitFramebuffer call will fail later because that isn't available in ES 2.0 without a compat shim. So maybe this will fix one issue for the user to run into another later on down the line.
There was a problem hiding this comment.
Thanks for all the pointers!
That is only legal in OpenGL ES versions > 2.0.
From my understanding, we should have ES 3.0 capability baselines:
- ANGLE has "complete" OpenGL ES 3.0 support on Direct3D 11.
- Flutter Windows will use the OpenGL backend only if we can use Direct3D 11 Feature Level 10.0+, Direct3D 11 Feature Level 9.3+, or Direct3D 11 WARP (Window's software rendering fallback). Otherwise, Flutter Windows will use Skia's software backend.
In other words, CompositorOpenGL::CreateBackingStore should only be used if Direct3D 11 is available and ANGLE should have complete OpenGL ES 3.0 support.
I guess I would ask why you need to mess with any of the internal formats. Always assume the unsized GL_RGBA, get rid of GetSupportedTextureFormat and the format_ ivar and give 0 as the internal format to Flutter result->open_gl.framebuffer.target = 0;
Interesting, let me try that! 👀
So maybe this will fix one issue for the user to run into another later on down the line.
Two community members (here and here) reported that this patch fixed the issue. Thankfully, it looks like glBlitFramebuffer works for them :)
There was a problem hiding this comment.
It is then hard to see why the user is running into the error. Do we have a dump of DescriptionGLES::GetString?
There was a problem hiding this comment.
@chinmaygarde I tried this suggestion:
I guess I would ask why you need to mess with any of the internal formats. Always assume the unsized GL_RGBA, get rid of
GetSupportedTextureFormatand theformat_ivar and give0as the internal format to Flutterresult->open_gl.framebuffer.target = 0;You delete a bunch of code and make things simpler. And the same code gets run on new and old versions of the platform making testing easier.
It looks like Skia wants us to specify an internal format: using framebuffer target 0 causes SkSurface creation to fail when it validates the backend render target. Specifically, GrGLCaps::onAreColorTypeAndFormatCompatible returns false as the format info for glFormat of kUnknown has no fColorTypeInfos.
Errors you get with this experiment...
[ERROR:flutter/shell/platform/embedder/embedder.cc(820)] Could not wrap embedder supplied frame-buffer.
[ERROR:flutter/shell/platform/embedder/embedder.cc(1275)] Could not create a surface from an embedder provided render target.
Patch to test this suggestion...
diff --git a/shell/platform/windows/compositor_opengl.cc b/shell/platform/windows/compositor_opengl.cc
index 0b367dd740..a8c87d0d3f 100644
--- a/shell/platform/windows/compositor_opengl.cc
+++ b/shell/platform/windows/compositor_opengl.cc
@@ -20,19 +20,6 @@ struct FramebufferBackingStore {
uint32_t texture_id;
};
-// Based off Skia's logic:
-// https://github.com/google/skia/blob/4738ed711e03212aceec3cd502a4adb545f38e63/src/gpu/ganesh/gl/GrGLCaps.cpp#L1963-L2116
-int GetSupportedTextureFormat(const impeller::DescriptionGLES* description) {
- if (description->HasExtension("GL_EXT_texture_format_BGRA8888")) {
- return GL_BGRA8_EXT;
- } else if (description->HasExtension("GL_APPLE_texture_format_BGRA8888") &&
- description->GetGlVersion().IsAtLeast(impeller::Version(3, 0))) {
- return GL_BGRA8_EXT;
- } else {
- return GL_RGBA8;
- }
-}
-
} // namespace
CompositorOpenGL::CompositorOpenGL(FlutterWindowsEngine* engine,
@@ -58,7 +45,7 @@ bool CompositorOpenGL::CreateBackingStore(
gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
- gl_->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, config.size.width,
+ gl_->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, config.size.width,
config.size.height, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
gl_->BindTexture(GL_TEXTURE_2D, 0);
@@ -68,7 +55,7 @@ bool CompositorOpenGL::CreateBackingStore(
result->type = kFlutterBackingStoreTypeOpenGL;
result->open_gl.type = kFlutterOpenGLTargetTypeFramebuffer;
result->open_gl.framebuffer.name = store->framebuffer_id;
- result->open_gl.framebuffer.target = format_;
+ result->open_gl.framebuffer.target = 0;
result->open_gl.framebuffer.user_data = store.release();
result->open_gl.framebuffer.destruction_callback = [](void* user_data) {
// Backing store destroyed in `CompositorOpenGL::CollectBackingStore`, set
@@ -185,7 +172,6 @@ bool CompositorOpenGL::Initialize() {
return false;
}
- format_ = GetSupportedTextureFormat(gl_->GetDescription());
is_initialized_ = true;
return true;
}
diff --git a/shell/platform/windows/compositor_opengl.h b/shell/platform/windows/compositor_opengl.h
index 8f9e330b48..93f1a02281 100644
--- a/shell/platform/windows/compositor_opengl.h
+++ b/shell/platform/windows/compositor_opengl.h
@@ -48,10 +48,6 @@ class CompositorOpenGL : public Compositor {
// Table of resolved GLES functions. Null until the compositor is initialized.
std::unique_ptr<impeller::ProcTableGLES> gl_ = nullptr;
- // The OpenGL texture target format for backing stores. Invalid value until
- // the compositor is initialized.
- uint32_t format_ = 0;
-
// Initialize the compositor. This must run on the raster thread.
bool Initialize();
Stack trace for the check that fails `SkSurface` validation...
GrGLCaps::onAreColorTypeAndFormatCompatible(GrColorType ct, const GrBackendFormat & format)
GrCaps::areColorTypeAndFormatCompatible(GrColorType grCT, const GrBackendFormat & format)
validate_backend_render_target(const GrCaps * caps, const GrBackendRenderTarget & rt, GrColorType grCT)
SkSurfaces::WrapBackendRenderTarget(...)
MakeSkSurfaceFromBackingStore(...)
CreateEmbedderRenderTarget(...)
InferExternalViewEmbedderFromArgs::void <lambda>(...)<FlutterBackingStoreConfig>(...)
...
flutter::EmbedderExternalViewEmbedder::SubmitFlutterView::<lambda_0>::operator()(...)
...
flutter::EmbedderExternalViewEmbedder::SubmitFlutterView(...)
flutter::Rasterizer::DrawToSurfaceUnsafe(...)
flutter::Rasterizer::DrawToSurfacesUnsafe(...)
flutter::Rasterizer::DrawToSurfaces(...)
flutter::Rasterizer::DoDraw(...)
flutter::Rasterizer::Draw::<lambda>(...)
...
flutter::Pipeline<flutter::FrameItem>::Consume(...)
flutter::Rasterizer::Draw
flutter::Shell::OnAnimatorDraw::<lambda_39>::operator()()
...
But, even if you did, there are other issues with this code. Because the presence GL_BGRA8_EXT will cause the internal format and the format you tell Flutter the texture is to be off (see result->open_gl.framebuffer.target = format_;). That is, you will always create an RGBA image but may tell Flutter the image is BGRA.
Hm interesting. On all my devices, we do indeed tell Flutter the image is BGRA even though we create an RGBA image. This seems to magically work somehow... I'll try some more variants:
- Always tell Flutter RGBA (since the images are always RGBA)
- Create a BGRA image (so that what we tell Flutter matches the image)
FYI, I had copied this logic from the Linux embedder (here and here).
There was a problem hiding this comment.
I tried more variants:
❌ 1. Always tell Flutter RGBA (since the images are always RGBA)
This does not pass Skia's color type validation as our MakeSkSurfaceFromBackingStore gives Skia kN32_SkColorType as the color type, which is actually kBGRA_8888_SkColorType. Do you know why we always use kN32_SkColorType? Should MakeSkSurfaceFromBackingStore detect the color type from the framebuffer target?
Patch...
diff --git a/shell/platform/windows/compositor_opengl.cc b/shell/platform/windows/compositor_opengl.cc
index 0b367dd740..dbac0ea2f6 100644
--- a/shell/platform/windows/compositor_opengl.cc
+++ b/shell/platform/windows/compositor_opengl.cc
@@ -23,14 +23,7 @@ struct FramebufferBackingStore {
// Based off Skia's logic:
// https://github.com/google/skia/blob/4738ed711e03212aceec3cd502a4adb545f38e63/src/gpu/ganesh/gl/GrGLCaps.cpp#L1963-L2116
int GetSupportedTextureFormat(const impeller::DescriptionGLES* description) {
- if (description->HasExtension("GL_EXT_texture_format_BGRA8888")) {
- return GL_BGRA8_EXT;
- } else if (description->HasExtension("GL_APPLE_texture_format_BGRA8888") &&
- description->GetGlVersion().IsAtLeast(impeller::Version(3, 0))) {
- return GL_BGRA8_EXT;
- } else {
- return GL_RGBA8;
- }
+ return GL_RGBA;
}
} // namespace
@@ -58,7 +51,7 @@ bool CompositorOpenGL::CreateBackingStore(
gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
- gl_->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, config.size.width,
+ gl_->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, config.size.width,
config.size.height, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
gl_->BindTexture(GL_TEXTURE_2D, 0);
Error logs...
[ERROR:flutter/shell/platform/embedder/embedder.cc(820)] Could not wrap embedder supplied frame-buffer.
[ERROR:flutter/shell/platform/embedder/embedder.cc(1275)] Could not create a surface from an embedder provided render target.
2. ✅ Create a BGRA image (so that what we tell Flutter matches the image)
This experiment works on my machine!
Patch...
diff --git a/shell/platform/windows/compositor_opengl.cc b/shell/platform/windows/compositor_opengl.cc
index 0b367dd740..52d282e24a 100644
--- a/shell/platform/windows/compositor_opengl.cc
+++ b/shell/platform/windows/compositor_opengl.cc
@@ -20,19 +20,6 @@ struct FramebufferBackingStore {
uint32_t texture_id;
};
-// Based off Skia's logic:
-// https://github.com/google/skia/blob/4738ed711e03212aceec3cd502a4adb545f38e63/src/gpu/ganesh/gl/GrGLCaps.cpp#L1963-L2116
-int GetSupportedTextureFormat(const impeller::DescriptionGLES* description) {
- if (description->HasExtension("GL_EXT_texture_format_BGRA8888")) {
- return GL_BGRA8_EXT;
- } else if (description->HasExtension("GL_APPLE_texture_format_BGRA8888") &&
- description->GetGlVersion().IsAtLeast(impeller::Version(3, 0))) {
- return GL_BGRA8_EXT;
- } else {
- return GL_RGBA8;
- }
-}
-
} // namespace
CompositorOpenGL::CompositorOpenGL(FlutterWindowsEngine* engine,
@@ -58,17 +45,18 @@ bool CompositorOpenGL::CreateBackingStore(
gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
gl_->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
- gl_->TexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, config.size.width,
- config.size.height, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
+ gl_->TexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT, config.size.width,
+ config.size.height, 0, GL_BGRA_EXT, GL_UNSIGNED_BYTE,
+ nullptr);
gl_->BindTexture(GL_TEXTURE_2D, 0);
- gl_->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0_EXT,
+ gl_->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
GL_TEXTURE_2D, store->texture_id, 0);
result->type = kFlutterBackingStoreTypeOpenGL;
result->open_gl.type = kFlutterOpenGLTargetTypeFramebuffer;
result->open_gl.framebuffer.name = store->framebuffer_id;
- result->open_gl.framebuffer.target = format_;
+ result->open_gl.framebuffer.target = GL_BGRA8_EXT;
result->open_gl.framebuffer.user_data = store.release();
result->open_gl.framebuffer.destruction_callback = [](void* user_data) {
// Backing store destroyed in `CompositorOpenGL::CollectBackingStore`, set
@@ -185,7 +173,6 @@ bool CompositorOpenGL::Initialize() {
return false;
}
- format_ = GetSupportedTextureFormat(gl_->GetDescription());
is_initialized_ = true;
return true;
}
diff --git a/shell/platform/windows/compositor_opengl.h b/shell/platform/windows/compositor_opengl.h
index 8f9e330b48..93f1a02281 100644
--- a/shell/platform/windows/compositor_opengl.h
+++ b/shell/platform/windows/compositor_opengl.h
@@ -48,10 +48,6 @@ class CompositorOpenGL : public Compositor {
// Table of resolved GLES functions. Null until the compositor is initialized.
std::unique_ptr<impeller::ProcTableGLES> gl_ = nullptr;
- // The OpenGL texture target format for backing stores. Invalid value until
- // the compositor is initialized.
- uint32_t format_ = 0;
-
// Initialize the compositor. This must run on the raster thread.
bool Initialize();
There was a problem hiding this comment.
From resources I found online, BGRA seems like a better default choice as the backing store format.
-
https://stackoverflow.com/questions/74924790/why-bgra-instead-of-rgba
DXGI_FORMAT_B8G8R8A8_UNORM is therefore the only format that's supported by ALL Direct3D hardware feature levels for the swapchain, which is why a lot of samples and engines default to using it.
-
void DeviceResources::CreateWindowSizeDependentResources() { ... swapChainDesc.Format = DXGI_FORMAT_B8G8R8A8_UNORM; // This is the most common swap chain format. ... } -
¹DXGI 1.1, which is included in the Direct3D 11 runtime, includes BGRA formats. However, support for these formats is optional for Direct3D 10 and 10.1 devices with drivers that are implemented to the Windows Display Driver Model (WDDM) for Windows Vista (WDDM 1.0). Consider using DXGI_FORMAT_R8G8B8A8_UNORM instead. Alternatively, you can create your device with D3D10_CREATE_DEVICE_BGRA_SUPPORT to ensure that you only support computers with the Direct3D 11.0 runtime and a WDDM 1.1 driver or higher installed.Flutter Windows requires Direct3D 11, otherwise it falls back to the software backend.
yaakovschectman
changed the title
|
Closing in favor of: #54329 |
2 participants
This loosens the texture format requirements and increases the number of devices Flutter Windows is compatible with. Kudos to @yaminet1024 for the suggestion.
Potentially fixes: flutter/flutter#150546
Warning
Unfortunately, we are unable to test this.
This bug appears to only affect older devices; none of our devices reproduce this issue.
We also do not have the infrastructure to do a native screenshot test.
I will get a test exemption for this change.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.