Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
33 changes: 26 additions & 7 deletions common/graphics/texture.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Texture::Texture(int64_t id) : id_(id) {}

Texture::~Texture() = default;

TextureRegistry::TextureRegistry() = default;
TextureRegistry::TextureRegistry() : image_counter_(0) {}

void TextureRegistry::RegisterTexture(const std::shared_ptr<Texture>& texture) {
if (!texture) {
Expand All @@ -26,7 +26,13 @@ void TextureRegistry::RegisterTexture(const std::shared_ptr<Texture>& texture) {
void TextureRegistry::RegisterContextListener(
uintptr_t id,
std::weak_ptr<ContextListener> image) {
images_[id] = std::move(image);
size_t next_id = image_counter_.fetch_add(1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

C++ operator overloading of ++ does this for you already. IMO that is more readable that fetch_add.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Besides, does this need to be an atomic? I thought all texture registry operations were raster thread only.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess it doesn't need to be atomic. I was being overly cautious :)

auto const result = image_indices_.insert({id, next_id});
if (!result.second) {
ordered_images_.erase(result.first->second);
result.first->second = next_id;
}
ordered_images_[next_id] = {next_id, std::move(image)};
}

void TextureRegistry::UnregisterTexture(int64_t id) {
Expand All @@ -39,19 +45,29 @@ void TextureRegistry::UnregisterTexture(int64_t id) {
}

void TextureRegistry::UnregisterContextListener(uintptr_t id) {
images_.erase(id);
ordered_images_.erase(image_indices_[id]);
image_indices_.erase(id);
}

void TextureRegistry::OnGrContextCreated() {
for (auto& it : mapping_) {
it.second->OnGrContextCreated();
}

for (const auto& [id, weak_image] : images_) {
// Calling OnGrContextCreated may result in a subsequent call to
// RegisterContextListener from the listener, which may modify the map.
std::vector<
std::pair<size_t, std::pair<uintptr_t, std::weak_ptr<ContextListener>>>>
ordered_images(ordered_images_.begin(), ordered_images_.end());

for (const auto& [id, pair] : ordered_images) {
auto index_id = pair.first;
auto weak_image = pair.second;
if (auto image = weak_image.lock()) {
image->OnGrContextCreated();
} else {
images_.erase(id);
image_indices_.erase(index_id);
ordered_images_.erase(id);
}
}
}
Expand All @@ -61,11 +77,14 @@ void TextureRegistry::OnGrContextDestroyed() {
it.second->OnGrContextDestroyed();
}

for (const auto& [id, weak_image] : images_) {
for (const auto& [id, pair] : ordered_images_) {
auto index_id = pair.first;
auto weak_image = pair.second;
if (auto image = weak_image.lock()) {
image->OnGrContextDestroyed();
} else {
images_.erase(id);
image_indices_.erase(index_id);
ordered_images_.erase(id);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere, mutation while enumeration makes me nervous. Are iterators on ordered_images_ invalidated when the erase happens here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Used to doing this safely by using iterators explicitly and assigning the iterator to the return call of erase. I am not sure I have ever done it this way. So want to double check this is safe.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll change this. It seems to work but it'd be safer to use the iterator.

I had another issue with mutation during iteration that I fixed by copying the map into a vector, but that would be overkill here.

}
}
}
Expand Down
10 changes: 9 additions & 1 deletion common/graphics/texture.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,15 @@ class TextureRegistry {

private:
std::map<int64_t, std::shared_ptr<Texture>> mapping_;
std::map<uintptr_t, std::weak_ptr<ContextListener>> images_;
std::atomic_size_t image_counter_;
// This map keeps track of registered context listeners by their own
// externally provided id. It indexes into ordered_images_.
std::map<uintptr_t, size_t> image_indices_;
// This map makes sure that iteration of images happens in insertion order
// (managed by image_counter_) so that images which depend on other images get
// re-created in the right order.
std::map<size_t, std::pair<uintptr_t, std::weak_ptr<ContextListener>>>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess this makes sense but is a bit rough to read. How about we take advantage of the fact that the map is ordered already and store the existing key and the order as a key type. Then you can have a custom std::less on the key type and typedef the whole thing for more readibility. Iterating over the map elements will be order then. I suppose you will need a separate map for the removal (a unintptr to order association).

Just a suggestion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I considered that but it comes down to either storing extra data in the key or extra data in the value.

This makes the key easier to hash and avoids the need for a custom comparer that ignores part of the data. I'll use the typedef though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(if I didn't need to worry about updating the order when someone re-registers I could just use a std::vector)

ordered_images_;

FML_DISALLOW_COPY_AND_ASSIGN(TextureRegistry);
};
Expand Down
43 changes: 43 additions & 0 deletions flow/texture_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,36 @@

#include "flutter/common/graphics/texture.h"

#include <functional>

#include "flutter/flow/testing/mock_texture.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"

namespace flutter {
namespace testing {

namespace {
using int_closure = std::function<void(int)>;

struct TestContextListener : public ContextListener {
TestContextListener(uintptr_t p_id,
int_closure p_create,
int_closure p_destroy)
: id(p_id), create(p_create), destroy(p_destroy) {}

virtual ~TestContextListener() = default;

const uintptr_t id;
int_closure create;
int_closure destroy;

void OnGrContextCreated() override { create(id); }

void OnGrContextDestroyed() override { destroy(id); }
};
} // namespace

TEST(TextureRegistryTest, UnregisterTextureCallbackTriggered) {
TextureRegistry registry;
auto mock_texture1 = std::make_shared<MockTexture>(0);
Expand Down Expand Up @@ -95,5 +119,24 @@ TEST(TextureRegistryTest, ReuseSameTextureSlot) {
ASSERT_TRUE(mock_texture2->unregistered());
}

TEST(TextureRegistryTest, CallsOnGrContextCreatedInInsertionOrder) {
TextureRegistry registry;
std::vector<int> create_order;
std::vector<int> destroy_order;
auto create = [&](int id) { create_order.push_back(id); };
auto destroy = [&](int id) { destroy_order.push_back(id); };
auto a = std::make_shared<TestContextListener>(5, create, destroy);
auto b = std::make_shared<TestContextListener>(4, create, destroy);
auto c = std::make_shared<TestContextListener>(3, create, destroy);
registry.RegisterContextListener(a->id, a);
registry.RegisterContextListener(b->id, b);
registry.RegisterContextListener(c->id, c);
registry.OnGrContextDestroyed();
registry.OnGrContextCreated();

EXPECT_THAT(create_order, ::testing::ElementsAre(5, 4, 3));
EXPECT_THAT(destroy_order, ::testing::ElementsAre(5, 4, 3));
}

} // namespace testing
} // namespace flutter
1 change: 0 additions & 1 deletion lib/ui/painting/display_list_deferred_image_gpu_skia.cc
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ void DlDeferredImageGPUSkia::ImageWrapper::OnGrContextCreated() {

void DlDeferredImageGPUSkia::ImageWrapper::OnGrContextDestroyed() {
FML_DCHECK(raster_task_runner_->RunsTasksOnCurrentThread());

DeleteTexture();
}

Expand Down