Preserve order when regenerating GPU images #40268
Conversation
common/graphics/texture.cc
Outdated
| uintptr_t id, | ||
| std::weak_ptr<ContextListener> image) { | ||
| images_[id] = std::move(image); | ||
| size_t next_id = image_counter_.fetch_add(1); |
There was a problem hiding this comment.
C++ operator overloading of ++ does this for you already. IMO that is more readable that fetch_add.
There was a problem hiding this comment.
Besides, does this need to be an atomic? I thought all texture registry operations were raster thread only.
There was a problem hiding this comment.
I guess it doesn't need to be atomic. I was being overly cautious :)
common/graphics/texture.cc
Outdated
| } else { | ||
| images_.erase(id); | ||
| image_indices_.erase(index_id); | ||
| ordered_images_.erase(id); |
There was a problem hiding this comment.
Here and elsewhere, mutation while enumeration makes me nervous. Are iterators on ordered_images_ invalidated when the erase happens here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
common/graphics/texture.h
Outdated
| // 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>>> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(if I didn't need to worry about updating the order when someone re-registers I could just use a std::vector)
dnfield
added
the
autosubmit
2 participants
Fixes flutter/flutter#122566
The cause of that bug is that multiple dependent
toImageSynccalls are made such that each successive image depends on the previous one.In the current implementation, a
std::mapis causing theOnGrContextCreatedcalls to happen in sort-order based on pointer values. This patch adds another map to track the insertion order and a test to make sure that the callbacks respect insertion order.