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

[Impeller] Ensure that overlay surfaces are constructed with wide gamut settings. #48190

Merged
merged 16 commits into from
Nov 21, 2023
Merged
1 change: 1 addition & 0 deletions ci/licenses_golden/excluded_files
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@
../../../flutter/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc
../../../flutter/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc
../../../flutter/impeller/renderer/backend/vulkan/test
../../../flutter/impeller/renderer/blit_pass_unittests.cc
../../../flutter/impeller/renderer/capabilities_unittests.cc
../../../flutter/impeller/renderer/compute_subgroup_unittests.cc
../../../flutter/impeller/renderer/compute_unittests.cc
Expand Down
1 change: 1 addition & 0 deletions impeller/renderer/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ impeller_component("renderer_unittests") {
testonly = true

sources = [
"blit_pass_unittests.cc",
"capabilities_unittests.cc",
"device_buffer_unittests.cc",
"host_buffer_unittests.cc",
Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/metal/context_mtl.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class ContextMTL final : public Context,
private:
class SyncSwitchObserver : public fml::SyncSwitch::Observer {
public:
SyncSwitchObserver(ContextMTL& parent);
explicit SyncSwitchObserver(ContextMTL& parent);
virtual ~SyncSwitchObserver() = default;
void OnSyncSwitchUpdate(bool new_value) override;

Expand Down
1 change: 1 addition & 0 deletions impeller/renderer/backend/metal/context_mtl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "flutter/fml/logging.h"
#include "flutter/fml/paths.h"
#include "flutter/fml/synchronization/sync_switch.h"
#include "impeller/core/formats.h"
#include "impeller/core/sampler_descriptor.h"
#include "impeller/renderer/backend/metal/gpu_tracer_mtl.h"
#include "impeller/renderer/backend/metal/sampler_library_mtl.h"
Expand Down
11 changes: 11 additions & 0 deletions impeller/renderer/blit_pass.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "impeller/base/strings.h"
#include "impeller/base/validation.h"
#include "impeller/core/formats.h"
#include "impeller/core/host_buffer.h"
#include "impeller/renderer/blit_command.h"

Expand Down Expand Up @@ -52,6 +53,16 @@ bool BlitPass::AddCopy(std::shared_ptr<Texture> source,
static_cast<int>(destination->GetTextureDescriptor().sample_count));
return false;
}
if (source->GetTextureDescriptor().format !=
destination->GetTextureDescriptor().format) {
VALIDATION_LOG << SPrintF(
"The source pixel format (%s) must match the destination pixel format "
"(%s) "
"for blits.",
PixelFormatToString(source->GetTextureDescriptor().format),
PixelFormatToString(destination->GetTextureDescriptor().format));
return false;
}

if (!source_region.has_value()) {
source_region = IRect::MakeSize(source->GetSize());
Expand Down
61 changes: 61 additions & 0 deletions impeller/renderer/blit_pass_unittests.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "gtest/gtest.h"
#include "impeller/base/validation.h"
#include "impeller/core/formats.h"
#include "impeller/core/texture_descriptor.h"
#include "impeller/playground/playground_test.h"
#include "impeller/renderer/command_buffer.h"

// TODO(zanderso): https://github.com/flutter/flutter/issues/127701
// NOLINTBEGIN(bugprone-unchecked-optional-access)

namespace impeller {
namespace testing {

using BlitPassTest = PlaygroundTest;
INSTANTIATE_PLAYGROUND_SUITE(BlitPassTest);

TEST_P(BlitPassTest, BlitAcrossDifferentPixelFormatsFails) {
ScopedValidationDisable scope; // avoid noise in output.
auto context = GetContext();
auto cmd_buffer = context->CreateCommandBuffer();
auto blit_pass = cmd_buffer->CreateBlitPass();

TextureDescriptor src_desc;
src_desc.format = PixelFormat::kA8UNormInt;
src_desc.size = {100, 100};
auto src = context->GetResourceAllocator()->CreateTexture(src_desc);

TextureDescriptor dst_format;
dst_format.format = PixelFormat::kR8G8B8A8UNormInt;
dst_format.size = {100, 100};
auto dst = context->GetResourceAllocator()->CreateTexture(dst_format);

EXPECT_FALSE(blit_pass->AddCopy(src, dst));
}

TEST_P(BlitPassTest, BlitAcrossDifferentSampleCountsFails) {
ScopedValidationDisable scope; // avoid noise in output.
auto context = GetContext();
auto cmd_buffer = context->CreateCommandBuffer();
auto blit_pass = cmd_buffer->CreateBlitPass();

TextureDescriptor src_desc;
src_desc.format = PixelFormat::kR8G8B8A8UNormInt;
src_desc.sample_count = SampleCount::kCount4;
src_desc.size = {100, 100};
auto src = context->GetResourceAllocator()->CreateTexture(src_desc);

TextureDescriptor dst_format;
dst_format.format = PixelFormat::kR8G8B8A8UNormInt;
dst_format.size = {100, 100};
auto dst = context->GetResourceAllocator()->CreateTexture(dst_format);

EXPECT_FALSE(blit_pass->AddCopy(src, dst));
}

} // namespace testing
} // namespace impeller
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef SHELL_PLATFORM_IOS_FRAMEWORK_SOURCE_FLUTTER_OVERLAY_VIEW_H_
#define SHELL_PLATFORM_IOS_FRAMEWORK_SOURCE_FLUTTER_OVERLAY_VIEW_H_

#include <Metal/Metal.h>
#include <UIKit/UIKit.h>

#include <memory>
Expand Down Expand Up @@ -32,7 +33,8 @@
- (instancetype)initWithCoder:(NSCoder*)aDecoder NS_UNAVAILABLE;

- (instancetype)init NS_DESIGNATED_INITIALIZER;
- (instancetype)initWithContentsScale:(CGFloat)contentsScale;
- (instancetype)initWithContentsScale:(CGFloat)contentsScale
pixelFormat:(MTLPixelFormat)pixelFormat;

@end

Expand Down
16 changes: 14 additions & 2 deletions shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// found in the LICENSE file.

#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterOverlayView.h"
#include <CoreGraphics/CGColorSpace.h>
#include <Metal/Metal.h>

#include "flutter/common/settings.h"
#include "flutter/common/task_runners.h"
Expand Down Expand Up @@ -42,15 +44,25 @@ - (instancetype)init {
return self;
}

- (instancetype)initWithContentsScale:(CGFloat)contentsScale {
- (instancetype)initWithContentsScale:(CGFloat)contentsScale
pixelFormat:(MTLPixelFormat)pixelFormat {
self = [self init];

if ([self.layer isKindOfClass:NSClassFromString(@"CAMetalLayer")]) {
self.layer.allowsGroupOpacity = NO;
self.layer.contentsScale = contentsScale;
self.layer.rasterizationScale = contentsScale;
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunguarded-availability-new"
CAMetalLayer* layer = (CAMetalLayer*)self.layer;
#pragma clang diagnostic pop
layer.pixelFormat = pixelFormat;
if (pixelFormat == MTLPixelFormatRGBA16Float) {
auto srgb = CGColorSpaceCreateWithName(kCGColorSpaceExtendedSRGB);
layer.colorspace = srgb;
CGColorSpaceRelease(srgb);
}
}

return self;
}

Expand Down
36 changes: 23 additions & 13 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include <Metal/Metal.h>
#import <UIKit/UIGestureRecognizerSubclass.h>

#include <list>
Expand All @@ -14,6 +15,7 @@
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterChannels.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterOverlayView.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterView.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h"
#import "flutter/shell/platform/darwin/ios/ios_surface.h"

Expand Down Expand Up @@ -85,13 +87,15 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,

std::shared_ptr<FlutterPlatformViewLayer> FlutterPlatformViewLayerPool::GetLayer(
GrDirectContext* gr_context,
const std::shared_ptr<IOSContext>& ios_context) {
const std::shared_ptr<IOSContext>& ios_context,
MTLPixelFormat pixel_format) {
if (available_layer_index_ >= layers_.size()) {
std::shared_ptr<FlutterPlatformViewLayer> layer;
fml::scoped_nsobject<UIView> overlay_view;
fml::scoped_nsobject<UIView> overlay_view_wrapper;

if (!gr_context) {
bool impeller_enabled = !!ios_context->GetImpellerContext();
if (!gr_context && !impeller_enabled) {
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like the intention of this code was to run when the software backend was enabled, but it was running with Impeller enabled. I don't even know who to FYI, maybe @dnfield @chinmaygarde ?

overlay_view.reset([[FlutterOverlayView alloc] init]);
overlay_view_wrapper.reset([[FlutterOverlayView alloc] init]);

Expand All @@ -104,8 +108,10 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
std::move(surface));
} else {
CGFloat screenScale = [UIScreen mainScreen].scale;
overlay_view.reset([[FlutterOverlayView alloc] initWithContentsScale:screenScale]);
overlay_view_wrapper.reset([[FlutterOverlayView alloc] initWithContentsScale:screenScale]);
overlay_view.reset([[FlutterOverlayView alloc] initWithContentsScale:screenScale
pixelFormat:pixel_format]);
overlay_view_wrapper.reset([[FlutterOverlayView alloc] initWithContentsScale:screenScale
pixelFormat:pixel_format]);

auto ca_layer = fml::scoped_nsobject<CALayer>{[[overlay_view.get() layer] retain]};
std::unique_ptr<IOSSurface> ios_surface = IOSSurface::Create(ios_context, ca_layer);
Expand Down Expand Up @@ -735,13 +741,15 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
// on the overlay layer.
background_canvas->ClipRect(joined_rect, DlCanvas::ClipOp::kDifference);
// Get a new host layer.
std::shared_ptr<FlutterPlatformViewLayer> layer = GetLayer(gr_context, //
ios_context, //
slice, //
joined_rect, //
current_platform_view_id, //
overlay_id //
);
std::shared_ptr<FlutterPlatformViewLayer> layer =
GetLayer(gr_context, //
ios_context, //
slice, //
joined_rect, //
current_platform_view_id, //
overlay_id, //
((FlutterView*)flutter_view_.get()).pixelFormat //
);
did_submit &= layer->did_submit_last_frame;
platform_view_layers[current_platform_view_id].push_back(layer);
overlay_id++;
Expand Down Expand Up @@ -811,9 +819,11 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
EmbedderViewSlice* slice,
SkRect rect,
int64_t view_id,
int64_t overlay_id) {
int64_t overlay_id,
MTLPixelFormat pixel_format) {
FML_DCHECK(flutter_view_);
std::shared_ptr<FlutterPlatformViewLayer> layer = layer_pool_->GetLayer(gr_context, ios_context);
std::shared_ptr<FlutterPlatformViewLayer> layer =
layer_pool_->GetLayer(gr_context, ios_context, pixel_format);

UIView* overlay_view_wrapper = layer->overlay_view_wrapper.get();
auto screenScale = [UIScreen mainScreen].scale;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef FLUTTER_SHELL_PLATFORM_DARWIN_IOS_FRAMEWORK_SOURCE_FLUTTERPLATFORMVIEWS_INTERNAL_H_
#define FLUTTER_SHELL_PLATFORM_DARWIN_IOS_FRAMEWORK_SOURCE_FLUTTERPLATFORMVIEWS_INTERNAL_H_

#include <Metal/Metal.h>
#include "flutter/flow/embedded_views.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"
#include "flutter/shell/common/shell.h"
Expand Down Expand Up @@ -167,9 +168,9 @@ class FlutterPlatformViewLayerPool {

// Gets a layer from the pool if available, or allocates a new one.
// Finally, it marks the layer as used. That is, it increments `available_layer_index_`.
std::shared_ptr<FlutterPlatformViewLayer> GetLayer(
GrDirectContext* gr_context,
const std::shared_ptr<IOSContext>& ios_context);
std::shared_ptr<FlutterPlatformViewLayer> GetLayer(GrDirectContext* gr_context,
const std::shared_ptr<IOSContext>& ios_context,
MTLPixelFormat pixel_format);

// Gets the layers in the pool that aren't currently used.
// This method doesn't mark the layers as unused.
Expand Down Expand Up @@ -320,7 +321,8 @@ class FlutterPlatformViewsController {
EmbedderViewSlice* slice,
SkRect rect,
int64_t view_id,
int64_t overlay_id);
int64_t overlay_id,
MTLPixelFormat pixel_format);
// Removes overlay views and platform views that aren't needed in the current frame.
// Must run on the platform thread.
void RemoveUnusedLayers();
Expand Down
2 changes: 2 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterView.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef SHELL_PLATFORM_IOS_FRAMEWORK_SOURCE_FLUTTER_VIEW_H_
#define SHELL_PLATFORM_IOS_FRAMEWORK_SOURCE_FLUTTER_VIEW_H_

#include <Metal/Metal.h>
#import <UIKit/UIKit.h>

#include <memory>
Expand Down Expand Up @@ -47,6 +48,7 @@
enableWideGamut:(BOOL)isWideGamutEnabled NS_DESIGNATED_INITIALIZER;

- (UIScreen*)screen;
- (MTLPixelFormat)pixelFormat;

// Set by FlutterEngine or FlutterViewController to override software rendering.
@property(class, nonatomic) BOOL forceSoftwareRendering;
Expand Down
14 changes: 12 additions & 2 deletions shell/platform/darwin/ios/framework/Source/FlutterView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterView.h"
#include <Metal/Metal.h>

#include "flutter/common/settings.h"
#include "flutter/common/task_runners.h"
Expand All @@ -19,6 +20,7 @@
@implementation FlutterView {
id<FlutterViewEngineDelegate> _delegate;
BOOL _isWideGamutEnabled;
MTLPixelFormat _pixelFormat;
}

- (instancetype)init {
Expand All @@ -43,6 +45,10 @@ - (UIScreen*)screen {
return UIScreen.mainScreen;
}

- (MTLPixelFormat)pixelFormat {
return _pixelFormat;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a new ivar? Can't we use self.layer.pixelFormat?

Copy link
Member Author

Choose a reason for hiding this comment

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

the layer ivar isn't exposed, this seemed more restricted.

Copy link
Member

Choose a reason for hiding this comment

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

Right, you should keep your accessor. Just use:

- (MTLPixelFormat)pixelFormat {
  return self.layer.pixelFormat;
}

By duplicating this value we are opening ourselves up to bugs where the layer's pixel format does not match the FlutterView's pixel format.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, I think!

}

- (BOOL)isWideGamutSupported {
if (![_delegate isUsingImpeller]) {
return NO;
Expand Down Expand Up @@ -115,8 +121,12 @@ - (void)layoutSubviews {
// F16 was chosen over BGRA10_XR since Skia does not support decoding
// BGRA10_XR.
layer.pixelFormat = MTLPixelFormatRGBA16Float;
} else if (_isWideGamutEnabled && !isWideGamutSupported) {
PrintWideGamutWarningOnce();
_pixelFormat = MTLPixelFormatRGBA16Float;
} else {
if (_isWideGamutEnabled && !isWideGamutSupported) {
PrintWideGamutWarningOnce();
}
_pixelFormat = layer.pixelFormat;
}
}

Expand Down