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

[Impeller] Enable MSAA for OpenGLES: Take 2. #47030

Merged
matanlurey merged 17 commits into from
Oct 24, 2023
Merged

[Impeller] Enable MSAA for OpenGLES: Take 2. #47030

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
08f4b93
Fix MSAA implementation.
matanlurey Oct 17, 2023
1b2b898
Checkpoint.
matanlurey Oct 17, 2023
10389b3
Print debugging.
matanlurey Oct 18, 2023
d0f1d7c
Working with serious hacks.
matanlurey Oct 19, 2023
99e9c68
Revert impeller/core/formats.cc, it's a NOOP.
matanlurey Oct 20, 2023
ce1ffeb
Remove unused code.
matanlurey Oct 20, 2023
ad5c75b
Merge remote-tracking branch 'upstream/main' into engine-impeller-gle…
matanlurey Oct 20, 2023
5964853
Still working.
matanlurey Oct 20, 2023
118ccd8
Working mostly.
matanlurey Oct 20, 2023
64cf63c
Revert kDevicePrivate.
matanlurey Oct 20, 2023
29fc11a
if -> dcheck
matanlurey Oct 20, 2023
29da282
Discard is back, baby.
matanlurey Oct 20, 2023
8082acb
WIP still not working.
matanlurey Oct 23, 2023
fcefcf6
Finish and remove logging.
matanlurey Oct 23, 2023
b7b5333
Remove comment.
matanlurey Oct 23, 2023
c502394
Add TODOs.
matanlurey Oct 23, 2023
f47324e
Change capabilities_gles.:
matanlurey Oct 24, 2023
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
17 changes: 16 additions & 1 deletion impeller/renderer/backend/gles/capabilities_gles.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,21 @@ CapabilitiesGLES::CapabilitiesGLES(const ProcTableGLES& gl) {
gl.GetDescription()->HasExtension(kOESTextureBorderClampExt)) {
supports_decal_sampler_address_mode_ = true;
}

if (gl.GetDescription()->HasExtension(
"GL_EXT_multisampled_render_to_texture") &&
// The current implementation of MSAA support in Impeller GLES requires
// the use of glBlitFramebuffer, which is not available on all GLES
// implementations. We can't use MSAA on these platforms yet.
gl.BlitFramebuffer.IsAvailable()) {
supports_offscreen_msaa_ = true;

// We hard-code 4x MSAA, so let's make sure it's supported.
GLint value = 0;
gl.GetIntegerv(GL_MAX_SAMPLES_EXT, &value);
FML_DCHECK(value == 4) << "Unexpected max samples value: " << value << ". "
Copy link
Member

@chinmaygarde chinmaygarde Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am still reading up on the extensions spec but this check seems dodgy. We want to use 4 even when the max is higher. Now per the dashboard, ~83% support the 4 max. But the rest support higher numbers. In those cases, we still need to use 4.

Screenshot 2023-10-23 at 2 53 26 PM

Copy link
Member

@chinmaygarde chinmaygarde Oct 23, 2023

Choose a reason for hiding this comment

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

I suggest something like supports_offscreen_msaa_ = value >= 4.

<< "Only 4x MSAA is currently supported.";
}
}

size_t CapabilitiesGLES::GetMaxTextureUnits(ShaderStage stage) const {
Expand All @@ -124,7 +139,7 @@ size_t CapabilitiesGLES::GetMaxTextureUnits(ShaderStage stage) const {
}

bool CapabilitiesGLES::SupportsOffscreenMSAA() const {
return false;
return supports_offscreen_msaa_;
}

bool CapabilitiesGLES::SupportsSSBO() const {
Expand Down
1 change: 1 addition & 0 deletions impeller/renderer/backend/gles/capabilities_gles.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class CapabilitiesGLES final
private:
bool supports_framebuffer_fetch_ = false;
bool supports_decal_sampler_address_mode_ = false;
bool supports_offscreen_msaa_ = false;
};

} // namespace impeller
2 changes: 1 addition & 1 deletion impeller/renderer/backend/gles/formats_gles.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ constexpr std::optional<GLenum> ToTextureTarget(TextureType type) {
case TextureType::kTexture2D:
return GL_TEXTURE_2D;
Copy link
Member

@chinmaygarde chinmaygarde Oct 23, 2023

Choose a reason for hiding this comment

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

Nit: Just a [[fallthrough]] here?

case TextureType::kTexture2DMultisample:
return std::nullopt;
return GL_TEXTURE_2D;
case TextureType::kTextureCube:
return GL_TEXTURE_CUBE_MAP;
case TextureType::kTextureExternalOES:
Expand Down
69 changes: 59 additions & 10 deletions impeller/renderer/backend/gles/render_pass_gles.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "flutter/fml/trace_event.h"
#include "fml/closure.h"
#include "fml/logging.h"
#include "impeller/base/validation.h"
#include "impeller/renderer/backend/gles/context_gles.h"
#include "impeller/renderer/backend/gles/device_buffer_gles.h"
Expand Down Expand Up @@ -125,6 +126,7 @@ struct RenderPassData {
Scalar clear_depth = 1.0;

std::shared_ptr<Texture> color_attachment;
std::shared_ptr<Texture> resolve_attachment;
std::shared_ptr<Texture> depth_attachment;
std::shared_ptr<Texture> stencil_attachment;

Expand Down Expand Up @@ -186,17 +188,23 @@ struct RenderPassData {
return false;
}
}

if (auto depth = TextureGLES::Cast(pass_data.depth_attachment.get())) {
if (!depth->SetAsFramebufferAttachment(
GL_FRAMEBUFFER, TextureGLES::AttachmentPoint::kDepth)) {
return false;
}
}
if (auto stencil = TextureGLES::Cast(pass_data.stencil_attachment.get())) {
if (!stencil->SetAsFramebufferAttachment(
GL_FRAMEBUFFER, TextureGLES::AttachmentPoint::kStencil)) {
return false;
}
// TODO: Re-enable before finalizing PR.
// Causes "Fatal GL Error GL_INVALID_OPERATION(1282) encountered on call
// to glFramebufferTexture2DMultisampleEXT'". We probably don't want to be
// using MSAA for the stencil buffer anyway.

// if (!stencil->SetAsFramebufferAttachment(
// GL_FRAMEBUFFER, TextureGLES::AttachmentPoint::kStencil)) {
// return false;
// }
}

auto status = gl.CheckFramebufferStatus(GL_FRAMEBUFFER);
Expand Down Expand Up @@ -470,6 +478,51 @@ struct RenderPassData {
}
}

// When we have a resolve_attachment, MSAA is being used. We blit from the
// MSAA FBO to the resolve FBO, otherwise the resolve FBO ends up being
// incomplete (because it has no attachments).
//
// Note that this only works on OpenGLES 3.0+, or put another way, in older
// versions of OpenGLES, MSAA is not currently supported by Impeller. It's
// possible to work around this issue a few different ways (not yet done):
//
// Do not use the resolve_texture, and instead use the MSAA texture only.
// To preview what this would look like (but should not be implemented this
// way - would impact the HAL):
//
// @@ render_target.cc @@ CreateOffscreenMSAA
// - color0.resolve_texture = color0_resolve_tex;
// + color0.resolve_texture = color0_msaa_tex;
Copy link
Contributor

@jonahwilliams jonahwilliams Oct 23, 2023

Choose a reason for hiding this comment

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

nit: I would remove this part.

Copy link
Contributor Author

@matanlurey matanlurey Oct 23, 2023

Choose a reason for hiding this comment

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

This is mostly a reminder to myself 😅 but I can put it in the issue I end up filing instead:

  // Do not use the resolve_texture, and instead use the MSAA texture only.
  // To preview what this would look like (but should not be implemented this
  // way - would impact the HAL):
  //
  //    @@ render_target.cc @@ CreateOffscreenMSAA
  //    - color0.resolve_texture = color0_resolve_tex;
  //    + color0.resolve_texture = color0_msaa_tex;

if (!is_default_fbo && pass_data.resolve_attachment) {
Copy link
Member

@chinmaygarde chinmaygarde Oct 23, 2023

Choose a reason for hiding this comment

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

A missing cap check here for SupportsOffscreenMSAA for paranoia? Instead of the DCHECK for the blit.

Copy link
Contributor

@jonahwilliams jonahwilliams Oct 23, 2023

Choose a reason for hiding this comment

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

We already have cap checks in render target construction. We could add an FML CHECK here but I'd be worried that too many layers of error checking is going to make it hard to find/fix problems.

// MSAA should not be enabled if BlitFramebuffer is not available.
FML_DCHECK(gl.BlitFramebuffer.IsAvailable());

GLuint draw_fbo = GL_NONE;
gl.GenFramebuffers(1u, &draw_fbo);
gl.BindFramebuffer(GL_FRAMEBUFFER, draw_fbo);

auto resolve = TextureGLES::Cast(pass_data.resolve_attachment.get());
if (!resolve->SetAsFramebufferAttachment(
GL_FRAMEBUFFER, TextureGLES::AttachmentPoint::kColor0)) {
return false;
}

gl.BindFramebuffer(GL_DRAW_FRAMEBUFFER, draw_fbo);
gl.BindFramebuffer(GL_READ_FRAMEBUFFER, fbo);
auto size = pass_data.resolve_attachment->GetSize();
gl.BlitFramebuffer(0, // srcX0
0, // srcY0
size.width, // srcX1
size.height, // srcY1
0, // dstX0
0, // dstY0
size.width, // dstX1
size.height, // dstY1
GL_COLOR_BUFFER_BIT, // mask
GL_NEAREST // filter
);
}

if (gl.DiscardFramebufferEXT.IsAvailable()) {
std::vector<GLenum> attachments;

Expand Down Expand Up @@ -498,11 +551,6 @@ struct RenderPassData {
attachments.data() // size
);
}
#ifdef IMPELLER_DEBUG
if (is_default_fbo) {
tracer->MarkFrameEnd(gl);
}
#endif // IMPELLER_DEBUG

return true;
}
Expand Down Expand Up @@ -531,6 +579,7 @@ bool RenderPassGLES::OnEncodeCommands(const Context& context) const {
/// Setup color data.
///
pass_data->color_attachment = color0.texture;
pass_data->resolve_attachment = color0.resolve_texture;
pass_data->clear_color = color0.clear_color;
pass_data->clear_color_attachment = CanClearAttachment(color0.load_action);
pass_data->discard_color_attachment =
Expand All @@ -548,7 +597,7 @@ bool RenderPassGLES::OnEncodeCommands(const Context& context) const {
}

//----------------------------------------------------------------------------
/// Setup depth data.
/// Setup stencil data.
///
if (stencil0.has_value()) {
pass_data->stencil_attachment = stencil0->texture;
Expand Down
60 changes: 21 additions & 39 deletions impeller/renderer/backend/gles/texture_gles.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "impeller/base/allocation.h"
#include "impeller/base/validation.h"
#include "impeller/core/formats.h"
#include "impeller/core/texture_descriptor.h"
#include "impeller/renderer/backend/gles/formats_gles.h"

namespace impeller {
Expand All @@ -21,19 +22,20 @@ static TextureGLES::Type GetTextureTypeFromDescriptor(
const auto usage = static_cast<TextureUsageMask>(desc.usage);
const auto render_target =
static_cast<TextureUsageMask>(TextureUsage::kRenderTarget);
if (usage == render_target) {
const auto is_msaa = desc.sample_count == SampleCount::kCount4;
if (usage == render_target && !is_msaa) {
return TextureGLES::Type::kRenderBuffer;
Copy link
Contributor

@jonahwilliams jonahwilliams Oct 23, 2023

Choose a reason for hiding this comment

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

Maybe file a bug to investigate if we should investigate RenderBuffer multisampled (now that we have something working)

Copy link
Contributor Author

@matanlurey matanlurey Oct 23, 2023

Choose a reason for hiding this comment

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

Added.

Copy link
Member

@chinmaygarde chinmaygarde Oct 23, 2023

Choose a reason for hiding this comment

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

I thought we were just getting rid of kRenderBuffer altogether.

Copy link
Contributor

@jonahwilliams jonahwilliams Oct 23, 2023

Choose a reason for hiding this comment

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

We shouldn't try to mix refactors and feature work.

}
return TextureGLES::Type::kTexture;
return is_msaa ? TextureGLES::Type::kTextureMultisampled
: TextureGLES::Type::kTexture;
}

HandleType ToHandleType(TextureGLES::Type type) {
switch (type) {
case TextureGLES::Type::kTexture:
case TextureGLES::Type::kTextureMultisampled:
return HandleType::kTexture;
case TextureGLES::Type::kRenderBuffer:
// MSAA textures are treated as render buffers.
case TextureGLES::Type::kRenderBufferMultisampled:
return HandleType::kRenderBuffer;
}
FML_UNREACHABLE();
Expand Down Expand Up @@ -362,7 +364,8 @@ void TextureGLES::InitializeContentsIfNecessary() const {
}

switch (type_) {
case Type::kTexture: {
case Type::kTexture:
case Type::kTextureMultisampled: {
TexImage2DData tex_data(GetTextureDescriptor().format);
if (!tex_data.IsValid()) {
VALIDATION_LOG << "Invalid format for texture image.";
Expand All @@ -382,7 +385,6 @@ void TextureGLES::InitializeContentsIfNecessary() const {
nullptr // data
);
}

} break;
case Type::kRenderBuffer: {
auto render_buffer_format =
Expand All @@ -401,26 +403,6 @@ void TextureGLES::InitializeContentsIfNecessary() const {
);
}
} break;
case Type::kRenderBufferMultisampled: {
auto render_buffer_msaa =
ToRenderBufferFormat(GetTextureDescriptor().format);
if (!render_buffer_msaa.has_value()) {
VALIDATION_LOG << "Invalid format for render-buffer MSAA image.";
return;
}
gl.BindRenderbuffer(GL_RENDERBUFFER, handle.value());
{
TRACE_EVENT0("impeller", "RenderBufferStorageInitialization");
gl.RenderbufferStorageMultisampleEXT(
GL_RENDERBUFFER, // target
4, // samples
render_buffer_msaa.value(), // internal format
size.width, // width
size.height // height
);
}
break;
}
}
}

Expand All @@ -438,7 +420,8 @@ bool TextureGLES::Bind() const {
}
const auto& gl = reactor_->GetProcTable();
switch (type_) {
case Type::kTexture: {
case Type::kTexture:
case Type::kTextureMultisampled: {
const auto target = ToTextureTarget(GetTextureDescriptor().type);
if (!target.has_value()) {
VALIDATION_LOG << "Could not bind texture of this type.";
Expand All @@ -447,8 +430,6 @@ bool TextureGLES::Bind() const {
gl.BindTexture(target.value(), handle.value());
} break;
case Type::kRenderBuffer:
// MSAA textures are treated as render buffers.
case Type::kRenderBufferMultisampled:
gl.BindRenderbuffer(GL_RENDERBUFFER, handle.value());
break;
}
Expand Down Expand Up @@ -516,6 +497,7 @@ bool TextureGLES::SetAsFramebufferAttachment(GLenum target,
return false;
}
const auto& gl = reactor_->GetProcTable();

switch (type_) {
case Type::kTexture:
gl.FramebufferTexture2D(target, // target
Expand All @@ -525,16 +507,7 @@ bool TextureGLES::SetAsFramebufferAttachment(GLenum target,
0 // level
);
break;
case Type::kRenderBuffer:
gl.FramebufferRenderbuffer(target, // target
ToAttachmentPoint(point), // attachment
GL_RENDERBUFFER, // render-buffer target
handle.value() // render-buffer
);
break;
case Type::kRenderBufferMultisampled:
// Assume that when MSAA is enabled, we're using 4x MSAA.
FML_DCHECK(GetTextureDescriptor().sample_count == SampleCount::kCount4);
case Type::kTextureMultisampled:
gl.FramebufferTexture2DMultisampleEXT(
target, // target
ToAttachmentPoint(point), // attachment
Expand All @@ -544,7 +517,16 @@ bool TextureGLES::SetAsFramebufferAttachment(GLenum target,
4 // samples
);
break;
case Type::kRenderBuffer:
gl.FramebufferRenderbuffer(target, // target
ToAttachmentPoint(point), // attachment
GL_RENDERBUFFER, // render-buffer target
handle.value() // render-buffer
);
gl.BindRenderbuffer(GL_RENDERBUFFER, GL_NONE);
break;
}

return true;
}

Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/gles/texture_gles.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ class TextureGLES final : public Texture,
public:
enum class Type {
kTexture,
kTextureMultisampled,
kRenderBuffer,
kRenderBufferMultisampled,
};

enum class IsWrapped {
Expand Down