From 5fcc7b719029f44233dd624309290e4385f9ce0d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 18 Apr 2023 13:11:05 -0700 Subject: [PATCH] [Impeller] Gpu model information to Skia gold (#41216) Does two things: - Exposes a string about the GPU model on `impeller::Context` - passes that string on to Skia gold when we add a test image. This should help reduce noise/flakiness in golden images. --- impeller/golden_tests/golden_digest.cc | 29 ++++++++++++++-- impeller/golden_tests/golden_digest.h | 4 +++ .../golden_playground_test_mac.cc | 3 ++ impeller/golden_tests/golden_tests.cc | 6 ++++ .../bin/golden_tests_harvester.dart | 34 ++++++++++++++----- .../lib/golden_tests_harvester.dart | 20 ++++------- .../renderer/backend/gles/context_gles.cc | 5 +++ impeller/renderer/backend/gles/context_gles.h | 3 ++ impeller/renderer/backend/metal/context_mtl.h | 3 ++ .../renderer/backend/metal/context_mtl.mm | 5 +++ .../renderer/backend/vulkan/context_vk.cc | 10 ++++++ impeller/renderer/backend/vulkan/context_vk.h | 4 +++ impeller/renderer/context.h | 2 ++ impeller/renderer/testing/mocks.h | 2 ++ lib/ui/painting/image_decoder_unittests.cc | 2 ++ 15 files changed, 107 insertions(+), 25 deletions(-) diff --git a/impeller/golden_tests/golden_digest.cc b/impeller/golden_tests/golden_digest.cc index 9dde1bdd9b14e..3dcb9be9b8a7a 100644 --- a/impeller/golden_tests/golden_digest.cc +++ b/impeller/golden_tests/golden_digest.cc @@ -5,6 +5,7 @@ #include "impeller/golden_tests/golden_digest.h" #include +#include static const double kMaxDiffPixelsPercent = 0.01; static const int32_t kMaxColorDelta = 8; @@ -23,6 +24,13 @@ GoldenDigest* GoldenDigest::Instance() { GoldenDigest::GoldenDigest() {} +void GoldenDigest::AddDimension(const std::string& name, + const std::string& value) { + std::stringstream ss; + ss << "\"" << value << "\""; + dimensions_[name] = ss.str(); +} + void GoldenDigest::AddImage(const std::string& test_name, const std::string& filename, int32_t width, @@ -38,15 +46,28 @@ bool GoldenDigest::Write(WorkingDirectory* working_directory) { return false; } - fout << "[" << std::endl; + fout << "{" << std::endl; + fout << " \"dimensions\": {" << std::endl; bool is_first = true; + for (const auto& dimension : dimensions_) { + if (!is_first) { + fout << "," << std::endl; + } + is_first = false; + fout << " \"" << dimension.first << "\": " << dimension.second; + } + fout << std::endl << " }," << std::endl; + fout << " \"entries\":" << std::endl; + + fout << " [" << std::endl; + is_first = true; for (const auto& entry : entries_) { if (!is_first) { fout << "," << std::endl; } is_first = false; - fout << " { " + fout << " { " << "\"testName\" : \"" << entry.test_name << "\", " << "\"filename\" : \"" << entry.filename << "\", " << "\"width\" : " << entry.width << ", " @@ -64,7 +85,9 @@ bool GoldenDigest::Write(WorkingDirectory* working_directory) { fout << "\"maxColorDelta\":" << entry.max_color_delta << " "; fout << "}"; } - fout << std::endl << "]" << std::endl; + fout << std::endl << " ]" << std::endl; + + fout << "}" << std::endl; fout.close(); return true; diff --git a/impeller/golden_tests/golden_digest.h b/impeller/golden_tests/golden_digest.h index a018ac1b9c7d8..3bdee0002f6b4 100644 --- a/impeller/golden_tests/golden_digest.h +++ b/impeller/golden_tests/golden_digest.h @@ -4,6 +4,7 @@ #pragma once +#include #include #include @@ -18,6 +19,8 @@ class GoldenDigest { public: static GoldenDigest* Instance(); + void AddDimension(const std::string& name, const std::string& value); + void AddImage(const std::string& test_name, const std::string& filename, int32_t width, @@ -42,6 +45,7 @@ class GoldenDigest { static GoldenDigest* instance_; std::vector entries_; + std::map dimensions_; }; } // namespace testing } // namespace impeller diff --git a/impeller/golden_tests/golden_playground_test_mac.cc b/impeller/golden_tests/golden_playground_test_mac.cc index 21323d9887d03..fae5c3984377c 100644 --- a/impeller/golden_tests/golden_playground_test_mac.cc +++ b/impeller/golden_tests/golden_playground_test_mac.cc @@ -78,6 +78,9 @@ void GoldenPlaygroundTest::SetUp() { "GoldenPlaygroundTest doesn't support interactive playground tests " "yet."); } + + testing::GoldenDigest::Instance()->AddDimension( + "gpu_string", GetContext()->DescribeGpuModel()); } PlaygroundBackend GoldenPlaygroundTest::GetBackend() const { diff --git a/impeller/golden_tests/golden_tests.cc b/impeller/golden_tests/golden_tests.cc index a54353fa0114a..c7ea062d79d95 100644 --- a/impeller/golden_tests/golden_tests.cc +++ b/impeller/golden_tests/golden_tests.cc @@ -52,6 +52,12 @@ class GoldenTests : public ::testing::Test { MetalScreenshoter& Screenshoter() { return *screenshoter_; } + void SetUp() override { + testing::GoldenDigest::Instance()->AddDimension( + "gpu_string", + Screenshoter().GetContext().GetContext()->DescribeGpuModel()); + } + private: std::unique_ptr screenshoter_; }; diff --git a/impeller/golden_tests_harvester/bin/golden_tests_harvester.dart b/impeller/golden_tests_harvester/bin/golden_tests_harvester.dart index 8b5d2a2b55ba1..21524bf9bc4da 100644 --- a/impeller/golden_tests_harvester/bin/golden_tests_harvester.dart +++ b/impeller/golden_tests_harvester/bin/golden_tests_harvester.dart @@ -2,9 +2,11 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:convert'; import 'dart:io'; import 'package:golden_tests_harvester/golden_tests_harvester.dart'; +import 'package:path/path.dart' as p; import 'package:process/src/interface/process_manager.dart'; import 'package:skia_gold_client/skia_gold_client.dart'; @@ -14,21 +16,25 @@ bool get isLuciEnv => Platform.environment.containsKey(_kLuciEnvName); /// Fake SkiaGoldClient that is used if the harvester is run outside of Luci. class FakeSkiaGoldClient implements SkiaGoldClient { - FakeSkiaGoldClient(this._workingDirectory); + FakeSkiaGoldClient(this._workingDirectory, {this.dimensions}); final Directory _workingDirectory; + @override + final Map? dimensions; + @override Future addImg(String testName, File goldenFile, {double differentPixelsRate = 0.01, int pixelColorDelta = 0, required int screenshotSize}) async { - Logger.instance.log('addImg testName:$testName goldenFile:${goldenFile.path} screenshotSize:$screenshotSize differentPixelsRate:$differentPixelsRate pixelColorDelta:$pixelColorDelta'); + Logger.instance.log( + 'addImg testName:$testName goldenFile:${goldenFile.path} screenshotSize:$screenshotSize differentPixelsRate:$differentPixelsRate pixelColorDelta:$pixelColorDelta'); } @override Future auth() async { - Logger.instance.log('auth'); + Logger.instance.log('auth dimensions:${dimensions ?? 'null'}'); } @override @@ -36,9 +42,6 @@ class FakeSkiaGoldClient implements SkiaGoldClient { throw UnimplementedError(); } - @override - Map? get dimensions => throw UnimplementedError(); - @override List getCIArguments() { throw UnimplementedError(); @@ -80,9 +83,22 @@ Future main(List arguments) async { } final Directory workDirectory = Directory(arguments[0]); + + final File digest = File(p.join(workDirectory.path, 'digest.json')); + if (!digest.existsSync()) { + Logger.instance + .log('Error: digest.json does not exist in ${workDirectory.path}.'); + return; + } + final Object? decoded = jsonDecode(digest.readAsStringSync()); + final Map data = (decoded as Map?)!; + final Map dimensions = + (data['dimensions'] as Map?)!.cast(); + final List entries = (data['entries'] as List?)!; + final SkiaGoldClient skiaGoldClient = isLuciEnv - ? SkiaGoldClient(workDirectory) - : FakeSkiaGoldClient(workDirectory); + ? SkiaGoldClient(workDirectory, dimensions: dimensions) + : FakeSkiaGoldClient(workDirectory, dimensions: dimensions); - await harvest(skiaGoldClient, workDirectory); + await harvest(skiaGoldClient, workDirectory, entries); } diff --git a/impeller/golden_tests_harvester/lib/golden_tests_harvester.dart b/impeller/golden_tests_harvester/lib/golden_tests_harvester.dart index 144bd1562dbb7..dfdc647c7c033 100644 --- a/impeller/golden_tests_harvester/lib/golden_tests_harvester.dart +++ b/impeller/golden_tests_harvester/lib/golden_tests_harvester.dart @@ -2,7 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import 'dart:convert'; import 'dart:io'; import 'package:path/path.dart' as p; @@ -14,30 +13,25 @@ export 'logger.dart'; /// Reads the digest inside of [workDirectory], sending tests to /// [skiaGoldClient]. -Future harvest( - SkiaGoldClient skiaGoldClient, Directory workDirectory) async { +Future harvest(SkiaGoldClient skiaGoldClient, Directory workDirectory, + List entries) async { await skiaGoldClient.auth(); - final File digest = File(p.join(workDirectory.path, 'digest.json')); - if (!digest.existsSync()) { - Logger.instance - .log('Error: digest.json does not exist in ${workDirectory.path}.'); - return; - } - final Object? decoded = jsonDecode(digest.readAsStringSync()); - final List entries = (decoded as List?)!; final List> pendingComparisons = >[]; for (final Object? entry in entries) { final Map map = (entry as Map?)!; final String filename = (map['filename'] as String?)!; final int width = (map['width'] as int?)!; final int height = (map['height'] as int?)!; - final double maxDiffPixelsPercent = (map['maxDiffPixelsPercent'] as double?)!; + final double maxDiffPixelsPercent = + (map['maxDiffPixelsPercent'] as double?)!; final int maxColorDelta = (map['maxColorDelta'] as int?)!; final File goldenImage = File(p.join(workDirectory.path, filename)); final Future future = skiaGoldClient .addImg(filename, goldenImage, - screenshotSize: width * height, differentPixelsRate: maxDiffPixelsPercent, pixelColorDelta: maxColorDelta) + screenshotSize: width * height, + differentPixelsRate: maxDiffPixelsPercent, + pixelColorDelta: maxColorDelta) .catchError((dynamic err) { Logger.instance.log('skia gold comparison failed: $err'); throw Exception('Failed comparison: $filename'); diff --git a/impeller/renderer/backend/gles/context_gles.cc b/impeller/renderer/backend/gles/context_gles.cc index 4d3207d36cc86..fb42495e36a13 100644 --- a/impeller/renderer/backend/gles/context_gles.cc +++ b/impeller/renderer/backend/gles/context_gles.cc @@ -104,6 +104,11 @@ bool ContextGLES::IsValid() const { return is_valid_; } +// |Context| +std::string ContextGLES::DescribeGpuModel() const { + return reactor_->GetProcTable().GetDescription()->GetString(); +} + // |Context| std::shared_ptr ContextGLES::GetResourceAllocator() const { return resource_allocator_; diff --git a/impeller/renderer/backend/gles/context_gles.h b/impeller/renderer/backend/gles/context_gles.h index 0c3336d5de3a2..5387f9aa908e3 100644 --- a/impeller/renderer/backend/gles/context_gles.h +++ b/impeller/renderer/backend/gles/context_gles.h @@ -47,6 +47,9 @@ class ContextGLES final : public Context, std::unique_ptr gl, const std::vector>& shader_libraries); + // |Context| + std::string DescribeGpuModel() const override; + // |Context| bool IsValid() const override; diff --git a/impeller/renderer/backend/metal/context_mtl.h b/impeller/renderer/backend/metal/context_mtl.h index acd046b1e8465..86f91140e482b 100644 --- a/impeller/renderer/backend/metal/context_mtl.h +++ b/impeller/renderer/backend/metal/context_mtl.h @@ -36,6 +36,9 @@ class ContextMTL final : public Context, id GetMTLDevice() const; + // |Context| + std::string DescribeGpuModel() const override; + // |Context| bool IsValid() const override; diff --git a/impeller/renderer/backend/metal/context_mtl.mm b/impeller/renderer/backend/metal/context_mtl.mm index 7af8367a35b69..5976c5b9c7477 100644 --- a/impeller/renderer/backend/metal/context_mtl.mm +++ b/impeller/renderer/backend/metal/context_mtl.mm @@ -225,6 +225,11 @@ static bool DeviceSupportsComputeSubgroups(id device) { ContextMTL::~ContextMTL() = default; +// |Context| +std::string ContextMTL::DescribeGpuModel() const { + return std::string([[device_ name] UTF8String]); +} + // |Context| bool ContextMTL::IsValid() const { return is_valid_; diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index e0d65d0f7634b..157aa76f6370b 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -344,6 +344,10 @@ void ContextVK::Setup(Settings settings) { return; } + VkPhysicalDeviceProperties physical_device_properties; + dispatcher.vkGetPhysicalDeviceProperties(physical_device.value(), + &physical_device_properties); + //---------------------------------------------------------------------------- /// All done! /// @@ -358,6 +362,7 @@ void ContextVK::Setup(Settings settings) { queues_ = std::move(queues); device_capabilities_ = std::move(caps); fence_waiter_ = std::move(fence_waiter); + device_name_ = std::string(physical_device_properties.deviceName); is_valid_ = true; //---------------------------------------------------------------------------- @@ -367,6 +372,11 @@ void ContextVK::Setup(Settings settings) { SetDebugName(device_.get(), device_.get(), "ImpellerDevice"); } +// |Context| +std::string ContextVK::DescribeGpuModel() const { + return device_name_; +} + bool ContextVK::IsValid() const { return is_valid_; } diff --git a/impeller/renderer/backend/vulkan/context_vk.h b/impeller/renderer/backend/vulkan/context_vk.h index b7dcc820679f8..9b95214f83466 100644 --- a/impeller/renderer/backend/vulkan/context_vk.h +++ b/impeller/renderer/backend/vulkan/context_vk.h @@ -49,6 +49,9 @@ class ContextVK final : public Context, public BackendCast { // |Context| ~ContextVK() override; + // |Context| + std::string DescribeGpuModel() const override; + // |Context| bool IsValid() const override; @@ -130,6 +133,7 @@ class ContextVK final : public Context, public BackendCast { std::shared_ptr swapchain_; std::shared_ptr device_capabilities_; std::shared_ptr fence_waiter_; + std::string device_name_; bool is_valid_ = false; diff --git a/impeller/renderer/context.h b/impeller/renderer/context.h index 37968bc975526..f270b0033eeb3 100644 --- a/impeller/renderer/context.h +++ b/impeller/renderer/context.h @@ -23,6 +23,8 @@ class Context : public std::enable_shared_from_this { public: virtual ~Context(); + virtual std::string DescribeGpuModel() const = 0; + virtual bool IsValid() const = 0; virtual const std::shared_ptr& GetCapabilities() diff --git a/impeller/renderer/testing/mocks.h b/impeller/renderer/testing/mocks.h index 527f2d077df11..4df48f285410c 100644 --- a/impeller/renderer/testing/mocks.h +++ b/impeller/renderer/testing/mocks.h @@ -85,6 +85,8 @@ class MockCommandBuffer : public CommandBuffer { class MockImpellerContext : public Context { public: + MOCK_CONST_METHOD0(DescribeGpuModel, std::string()); + MOCK_CONST_METHOD0(IsValid, bool()); MOCK_CONST_METHOD0(GetResourceAllocator, std::shared_ptr()); diff --git a/lib/ui/painting/image_decoder_unittests.cc b/lib/ui/painting/image_decoder_unittests.cc index 439e17d20036c..571b96c7d4a84 100644 --- a/lib/ui/painting/image_decoder_unittests.cc +++ b/lib/ui/painting/image_decoder_unittests.cc @@ -113,6 +113,8 @@ class TestImpellerContext : public impeller::Context { public: TestImpellerContext() = default; + std::string DescribeGpuModel() const override { return "TestGpu"; } + bool IsValid() const override { return true; } const std::shared_ptr& GetCapabilities() const override {