Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Windows] Use GL_RGBA as backing store internal format #54255

Closed
loic-sharma wants to merge 1 commit into from
Closed

[Windows] Use GL_RGBA as backing store internal format #54255

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shell/platform/windows/compositor_opengl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,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,
Copy link
Contributor

@chinmaygarde chinmaygarde Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@chinmaygarde chinmaygarde Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename GL_COLOR_ATTACHMENT0_EXT to GL_COLOR_ATTACHMENT0 from the non ext header. Its the same value but available in ES2.

Copy link
Contributor

@chinmaygarde chinmaygarde Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@loic-sharma loic-sharma Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. ANGLE has "complete" OpenGL ES 3.0 support on Direct3D 11.
  2. 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 :)

Copy link
Contributor

@chinmaygarde chinmaygarde Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is then hard to see why the user is running into the error. Do we have a dump of DescriptionGLES::GetString?

Copy link
Member Author

@loic-sharma loic-sharma Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 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.

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:

  1. Always tell Flutter RGBA (since the images are always RGBA)
  2. 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).

Copy link
Member Author

@loic-sharma loic-sharma Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Member Author

@loic-sharma loic-sharma Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From resources I found online, BGRA seems like a better default choice as the backing store format.

  1. 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.

  2. https://learn.microsoft.com/en-us/previous-versions/windows/apps/hh465096(v=win.10)?redirectedfrom=MSDN

    void DeviceResources::CreateWindowSizeDependentResources()
{
    ...
    swapChainDesc.Format = DXGI_FORMAT_B8G8R8A8_UNORM; // This is the most common swap chain format.
    ...
}

  3. https://learn.microsoft.com/en-us/windows/win32/direct3d10/d3d10-graphics-programming-guide-d3d9-to-d3d10-considerations?redirectedfrom=MSDN#mapping-texture-formats

    ¹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.

config.size.height, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
gl_->BindTexture(GL_TEXTURE_2D, 0);

Expand Down