Skip to content

Commit

Permalink
[Impeller] Gpu model information to Skia gold (flutter#41216)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dnfield committed Apr 18, 2023
1 parent b71dd7e commit 5fcc7b7
Show file tree
Hide file tree
Showing 15 changed files with 107 additions and 25 deletions.
29 changes: 26 additions & 3 deletions impeller/golden_tests/golden_digest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "impeller/golden_tests/golden_digest.h"

#include <fstream>
#include <sstream>

static const double kMaxDiffPixelsPercent = 0.01;
static const int32_t kMaxColorDelta = 8;
Expand All @@ -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,
Expand All @@ -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 << ", "
Expand All @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions impeller/golden_tests/golden_digest.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#pragma once

#include <map>
#include <string>
#include <vector>

Expand All @@ -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,
Expand All @@ -42,6 +45,7 @@ class GoldenDigest {

static GoldenDigest* instance_;
std::vector<Entry> entries_;
std::map<std::string, std::string> dimensions_;
};
} // namespace testing
} // namespace impeller
3 changes: 3 additions & 0 deletions impeller/golden_tests/golden_playground_test_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 6 additions & 0 deletions impeller/golden_tests/golden_tests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<MetalScreenshoter> screenshoter_;
};
Expand Down
34 changes: 25 additions & 9 deletions impeller/golden_tests_harvester/bin/golden_tests_harvester.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -14,31 +16,32 @@ 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<String, String>? dimensions;

@override
Future<void> 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<void> auth() async {
Logger.instance.log('auth');
Logger.instance.log('auth dimensions:${dimensions ?? 'null'}');
}

@override
String cleanTestName(String fileName) {
throw UnimplementedError();
}

@override
Map<String, String>? get dimensions => throw UnimplementedError();

@override
List<String> getCIArguments() {
throw UnimplementedError();
Expand Down Expand Up @@ -80,9 +83,22 @@ Future<void> main(List<String> 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<String?, Object?> data = (decoded as Map<String?, Object?>?)!;
final Map<String, String> dimensions =
(data['dimensions'] as Map<String, Object?>?)!.cast<String, String>();
final List<Object?> entries = (data['entries'] as List<Object?>?)!;

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);
}
20 changes: 7 additions & 13 deletions impeller/golden_tests_harvester/lib/golden_tests_harvester.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -14,30 +13,25 @@ export 'logger.dart';

/// Reads the digest inside of [workDirectory], sending tests to
/// [skiaGoldClient].
Future<void> harvest(
SkiaGoldClient skiaGoldClient, Directory workDirectory) async {
Future<void> harvest(SkiaGoldClient skiaGoldClient, Directory workDirectory,
List<Object?> 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<Object?> entries = (decoded as List<Object?>?)!;
final List<Future<void>> pendingComparisons = <Future<void>>[];
for (final Object? entry in entries) {
final Map<String, Object?> map = (entry as Map<String, Object?>?)!;
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<void> 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');
Expand Down
5 changes: 5 additions & 0 deletions impeller/renderer/backend/gles/context_gles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<Allocator> ContextGLES::GetResourceAllocator() const {
return resource_allocator_;
Expand Down
3 changes: 3 additions & 0 deletions impeller/renderer/backend/gles/context_gles.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ class ContextGLES final : public Context,
std::unique_ptr<ProcTableGLES> gl,
const std::vector<std::shared_ptr<fml::Mapping>>& shader_libraries);

// |Context|
std::string DescribeGpuModel() const override;

// |Context|
bool IsValid() const override;

Expand Down
3 changes: 3 additions & 0 deletions impeller/renderer/backend/metal/context_mtl.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ class ContextMTL final : public Context,

id<MTLDevice> GetMTLDevice() const;

// |Context|
std::string DescribeGpuModel() const override;

// |Context|
bool IsValid() const override;

Expand Down
5 changes: 5 additions & 0 deletions impeller/renderer/backend/metal/context_mtl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,11 @@ static bool DeviceSupportsComputeSubgroups(id<MTLDevice> device) {

ContextMTL::~ContextMTL() = default;

// |Context|
std::string ContextMTL::DescribeGpuModel() const {
return std::string([[device_ name] UTF8String]);
}

// |Context|
bool ContextMTL::IsValid() const {
return is_valid_;
Expand Down
10 changes: 10 additions & 0 deletions impeller/renderer/backend/vulkan/context_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,10 @@ void ContextVK::Setup(Settings settings) {
return;
}

VkPhysicalDeviceProperties physical_device_properties;
dispatcher.vkGetPhysicalDeviceProperties(physical_device.value(),
&physical_device_properties);

//----------------------------------------------------------------------------
/// All done!
///
Expand All @@ -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;

//----------------------------------------------------------------------------
Expand All @@ -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_;
}
Expand Down
4 changes: 4 additions & 0 deletions impeller/renderer/backend/vulkan/context_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ class ContextVK final : public Context, public BackendCast<ContextVK, Context> {
// |Context|
~ContextVK() override;

// |Context|
std::string DescribeGpuModel() const override;

// |Context|
bool IsValid() const override;

Expand Down Expand Up @@ -130,6 +133,7 @@ class ContextVK final : public Context, public BackendCast<ContextVK, Context> {
std::shared_ptr<SwapchainVK> swapchain_;
std::shared_ptr<const Capabilities> device_capabilities_;
std::shared_ptr<FenceWaiterVK> fence_waiter_;
std::string device_name_;

bool is_valid_ = false;

Expand Down
2 changes: 2 additions & 0 deletions impeller/renderer/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class Context : public std::enable_shared_from_this<Context> {
public:
virtual ~Context();

virtual std::string DescribeGpuModel() const = 0;

virtual bool IsValid() const = 0;

virtual const std::shared_ptr<const Capabilities>& GetCapabilities()
Expand Down
2 changes: 2 additions & 0 deletions impeller/renderer/testing/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<Allocator>());
Expand Down
2 changes: 2 additions & 0 deletions lib/ui/painting/image_decoder_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<const Capabilities>& GetCapabilities() const override {
Expand Down

0 comments on commit 5fcc7b7

Please sign in to comment.