[Impeller] ensure that OpenGL "flipped" textures do not leak via texture readback. #163501
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
February 17, 2025 16:10
github-actions
bot
added
engine
chinmaygarde
approved these changes
Feb 18, 2025
Member
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
A suggestion to use just one call to use ReadPixels with an inplace swap to avoid potential driver shenanigans. Perhaps its pinning memory weirdly.
OTOH, feel free to ignore. ReadPixels is slow either way.
| case TextureCoordinateSystem::kRenderToTexture: | ||
| // The texture is upside down, and must be inverted when copying | ||
| // byte data out. | ||
| size_t offset = destination_offset; |
Member
There was a problem hiding this comment.
I have nothing to provide as evidence for this other than perhaps an irrational fear that multiple calls to ReadPixels will make some driver do the most insane thing possible. But can we read in one shot and then flip the image in place after instead?
Iterate over the rows picking two from each end and std::swap_ranges on each perhaps.
Contributor
Author
There was a problem hiding this comment.
This is a reasonable fear, but I also doubt my ability to correctly use std::swap_ranges so it might be 6 of 1 and half a dozen of another...
Member
There was a problem hiding this comment.
static void FlipImage(uint8_t* buffer,
size_t width,
size_t height,
size_t stride) {
if (buffer == nullptr || stride == 0) {
return;
}
const auto byte_width = width * stride;
for (size_t top = 0; top < height; top++) {
size_t bottom = height - top - 1;
if (top >= bottom) {
break;
}
auto* top_row = buffer + byte_width * top;
auto* bottom_row = buffer + byte_width * bottom;
std::swap_ranges(top_row, top_row + byte_width, bottom_row);
}
}
Contributor
Author
There was a problem hiding this comment.
Will update the PR to use this.
| case TextureCoordinateSystem::kRenderToTexture: | ||
| // The texture is upside down, and must be inverted when copying | ||
| // byte data out. | ||
| FlipImage(data + destination_offset, 4, source_region.GetHeight(), |
Member
There was a problem hiding this comment.
These arguments aren't right. The stride goes last. It's the root buffer address, image width, image height, and the pixel stride.
Member
There was a problem hiding this comment.
Oh, lol. The order doesn't matter since its its multiplied. Anyway, not how I meant the arguments would be interpreted 🙃
Contributor
Author
There was a problem hiding this comment.
oops...
But thanks to associativity it is mathematically equivalent 😆
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
jonahwilliams
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
jonahwilliams
pushed a commit
to jonahwilliams/flutter
that referenced
this pull request
Feb 19, 2025
…ure readback. (flutter#163501) Fixes flutter#163315 Fixes flutter#163521 Fixes flutter#142641 OpenGL has an inverted coordinate system (bottom left is zero) compared to Metal/Vulkan (top left is zero). We handle this by rendering things upside down on OpenGL. Unfortunately this can leak out of the renderer via readback (toImage), so we need to make sure to undo the inversion. This is not performed for the "TextureCoordinateSystem::kUploadFromHost" state as that indicates the texture is already "right side up".
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 20, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 20, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 20, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 20, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 21, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 21, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 21, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 21, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 21, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 22, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 22, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 22, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 22, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 23, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 23, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 24, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 24, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 24, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 24, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 24, 2025
…via texture readback. (flutter/flutter#163501)
auto-submit bot
pushed a commit
that referenced
this pull request
Feb 26, 2025
…texture readback. (#163501) (#163667) Fixes #163315 Fixes #163521 Fixes #142641 OpenGL has an inverted coordinate system (bottom left is zero) compared to Metal/Vulkan (top left is zero). We handle this by rendering things upside down on OpenGL. Unfortunately this can leak out of the renderer via readback (toImage), so we need to make sure to undo the inversion. This is not performed for the "TextureCoordinateSystem::kUploadFromHost" state as that indicates the texture is already "right side up".
9 tasks
Fintasys
pushed a commit
to Fintasys/flutter
that referenced
this pull request
May 14, 2025
…texture readback. (flutter#163501) (flutter#163667) Fixes flutter#163315 Fixes flutter#163521 Fixes flutter#142641 OpenGL has an inverted coordinate system (bottom left is zero) compared to Metal/Vulkan (top left is zero). We handle this by rendering things upside down on OpenGL. Unfortunately this can leak out of the renderer via readback (toImage), so we need to make sure to undo the inversion. This is not performed for the "TextureCoordinateSystem::kUploadFromHost" state as that indicates the texture is already "right side up".
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 20, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 20, 2025
…via texture readback. (flutter/flutter#163501)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 21, 2025
…via texture readback. (flutter/flutter#163501)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #163315
Fixes #163521
Fixes #142641
OpenGL has an inverted coordinate system (bottom left is zero) compared to Metal/Vulkan (top left is zero). We handle this by rendering things upside down on OpenGL. Unfortunately this can leak out of the renderer via readback (toImage), so we need to make sure to undo the inversion.
This is not performed for the "TextureCoordinateSystem::kUploadFromHost" state as that indicates the texture is already "right side up".