[Impeller] Update BlitPass::AddCopy to use destination_region instead of origin for buffer to texture copies. #52555
Conversation
| // For non cubemap textures, 0 indicates uninitialized and 1 indicates | ||
| // initialized. For cubemap textures, each face is initialized separately with | ||
| // each bit tracking the initialization of the corresponding slice. | ||
| void MarkSliceInitialized(size_t slice) const; |
There was a problem hiding this comment.
This is what I was missing last time. For the GLES cube map textures we need to tracking initialization per slice
|
|
||
| [encoder copyFromBuffer:source_mtl | ||
| sourceOffset:source.range.offset | ||
| sourceBytesPerRow:destination_bytes_per_row | ||
| sourceBytesPerImage:destination_bytes_per_image | ||
| sourceBytesPerImage:0 |
There was a problem hiding this comment.
according to https://developer.apple.com/documentation/metal/mtlblitcommandencoder/1400752-copyfrombuffer , we can set sourceBytesPerImage to 0 for 2d textures
There was a problem hiding this comment.
Let's add an inline comment to that effect? That just reads wrong lol.
There was a problem hiding this comment.
oops, I mean "Done"
|
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. |
|
Goldens look good:
|
impeller/aiks/aiks_unittests.cc
Outdated
| blit_pass->AddCopy(DeviceBuffer::AsBufferView(device_buffer), bridge, | ||
| IRect::MakeLTRB(50, 50, 150, 150)); | ||
|
|
||
| if (!blit_pass->EncodeCommands(GetContext()->GetResourceAllocator()) || |
There was a problem hiding this comment.
Just curious. Why not ASSERT_TRUE? The GTEST_FAIL doesn't print out the assertion.
|
|
||
| private: | ||
| ReactorGLES::Ref reactor_; | ||
| const Type type_; | ||
| HandleGLES handle_; | ||
| mutable bool contents_initialized_ = false; | ||
| mutable uint32_t contents_initialized_ = 0; |
There was a problem hiding this comment.
Consider using std::bitset<6> and renaming to slices_initialized_. That gives you nice utilites to set and check these things.
|
|
||
| [encoder copyFromBuffer:source_mtl | ||
| sourceOffset:source.range.offset | ||
| sourceBytesPerRow:destination_bytes_per_row | ||
| sourceBytesPerImage:destination_bytes_per_image | ||
| sourceBytesPerImage:0 |
There was a problem hiding this comment.
Let's add an inline comment to that effect? That just reads wrong lol.
| std::string label, | ||
| uint32_t slice) { | ||
| auto command = std::make_unique<BlitCopyBufferToTextureCommandGLES>(); | ||
| command->label = label; | ||
| command->source = std::move(source); | ||
| command->destination = std::move(destination); | ||
| command->destination_origin = destination_origin; | ||
| command->destination_region = destination_region; | ||
| command->label = label; | ||
| command->slice = slice; |
There was a problem hiding this comment.
Here and elsewhere, we should check that slices are from 0 to 5. And, if not, either return false or clamp to the values. I prefer the former.
There was a problem hiding this comment.
I added this check it blit_pass.cc and a test case.
jonahwilliams
added
the
autosubmit
…on instead of origin for buffer to texture copies. (flutter/engine#52555)
…on instead of origin for buffer to texture copies. (flutter/engine#52555)
…147994) flutter/engine@f3ce5a5...9cb60de 2024-05-08 [email protected] Roll Skia from c7794fee8063 to d8427844b843 (2 revisions) (flutter/engine#52671) 2024-05-08 [email protected] Manual roll Dart SDK from a9cf6a411c71 to b7cad2edae4b (7 revisions) (flutter/engine#52669) 2024-05-08 [email protected] [Impeller] Update BlitPass::AddCopy to use destination_region instead of origin for buffer to texture copies. (flutter/engine#52555) 2024-05-08 [email protected] DisplayListBuilder internal reorganization with better rendering op overlap detection (flutter/engine#52646) 2024-05-08 [email protected] Roll reclient forward (flutter/engine#52632) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Based on #52510
Work towards flutter/flutter#138798
Change IPoint destination_origin to IRect destination_region, which allows us to specify an area smaller than the entire texture to replace. This will eventually allow us to only upload individual glyphs. This fixes the cubemap issue I previously hit: each face needs to track initialization separately.