[Impeller] Add Context::GetTextureContents for readback. #46394
Conversation
jonahwilliams
changed the title
impeller/renderer/context.cc
Outdated
| return false; | ||
| } | ||
|
|
||
| void Context::GetTextureContents(const std::shared_ptr<Texture>& texture, |
There was a problem hiding this comment.
We have something similar implemented in image_decode_impeller.cc, eventually we need to get rid of that and replace with our own logic to convert pixel formats. I'm not sure if the Skia implementation is unintentionally pulling in the software renderer.
|
Maybe we can do something like an MPSImageConversion on iOS, but probably want to avoid really slow pixel conversions. |
|
Actually we can just do the conversion in the blit pass. |
|
Looks like Metal blit passes wont do format conversions so I'd need to do a render pass. Maybe a separate helper to convert texture format a to format b, then we can have platform specific implementations. |
|
Filled flutter/flutter#135764 |
| return; | ||
| } | ||
|
|
||
| if (!blit_pass->AddCopy(texture, device_buffer)) { |
There was a problem hiding this comment.
Does a texture->device buffer blit actually force a transfer from the device to the host? If it does, I really think that behavior should be decoupled (not now, but later). But if it doesn't force a host transfer, I don't think this patch will work?
There was a problem hiding this comment.
Also, no cap check needed here (like SupportsBufferToTextureBlits)?
There was a problem hiding this comment.
No, this is supported on all backends from my inspection. If it didn't work, then the test would be failing, no?
There was a problem hiding this comment.
For fixing up the blits (later), I think a good solution would be to have two different AddCopy overloads: One that only copies to the host, and another that only copies to a new device allocation. Copying to both should never be necessary.
bool AddCopy(std::shared_ptr<Texture> source,
std::vector<uint8_t>& destination,
std::optional<IRect> source_region = std::nullopt,
IPoint destination_origin = {},
std::string label = "");
There was a problem hiding this comment.
Made a follow-up issue about this here: flutter/flutter#135783
impeller/renderer/context.cc
Outdated
| return false; | ||
| } | ||
|
|
||
| void Context::GetTextureContents(const std::shared_ptr<Texture>& texture, |
There was a problem hiding this comment.
As implemented, a more appropriate name would be CopyToDeviceBuffer.
impeller/renderer/context.h
Outdated
| virtual std::shared_ptr<CommandBuffer> CreateCommandBuffer() const = 0; | ||
|
|
||
| using ReadbackCallback = | ||
| std::function<void(std::shared_ptr<const DeviceBuffer> buffer)>; |
There was a problem hiding this comment.
Should this API emit a constant copy of the host byte data rather than a DeviceBuffer? I believe later we we can remove the unnecessary blit on all of the backends and even support onscreen textures on GLES.
There was a problem hiding this comment.
I believe later we we can remove the unnecessary blit on all of the backends and even support onscreen textures on GLES.
What?
There was a problem hiding this comment.
Sorry, I meant we can get rid of the device->device copy, as blitting to a DeviceBuffer implies.
There was a problem hiding this comment.
Ahh yeah makes sense. I went back and forth a few times on what the API should be. This actually needs to end up as. a unique_ptr of something, so that I can use the Dart typed data APIs for readback (eventually). So If I change this to be something like std::unique_ptr<const uint8_t[]> or whatever, I don't know C++ , that should all work nicely right?
There was a problem hiding this comment.
Yeah, we could also just return an std::vector<uint8_t> and just use move semantics to avoid copies when it makes sense, especially since we're getting pretty good help from clang tidy in that department at this point.
There was a problem hiding this comment.
This kind of thing would jive well with a non-DeviceBuffer variant of AddCopy I mentioned above. Might as well eat an extra copy on the host for now and fix both the copies later. :)
chinmaygarde
changed the title
|
Moving to WIP while I iterate on some stuff |
impeller/core/device_buffer.cc
Outdated
| auto size = GetDeviceBufferDescriptor().size; | ||
| auto raw_data = static_cast<uint8_t*>(malloc(size)); | ||
| memcpy(raw_data, OnGetContents(), size); | ||
| return std::unique_ptr<const uint8_t[]>(raw_data); |
There was a problem hiding this comment.
This is leaking right now because I need to give it a release proc
There was a problem hiding this comment.
Rather than lugging around a device allocation ref, you could just eat an extra host copy now and we can repair this later when we fix up the blits.
|
I think I might find a way to move this to texture.h too. |
|
actually it looks like only vulkan textures have a weak ptr to their context, gles and mtl textures do not. I could add more weak ptrs, but it would really only be in service of making the API slightly more ergonomic. |
|
The failing tests are entirely mock based. We should migrate them to dart:ui tests once flutter_tester supports impeller, but until then I'll leave the old readback approach. |
|
@jonahwilliams What's the status of this PR? |
|
This is still a WIP |
|
Closing as I don't have time to work on this right now |
3 participants
Fixes flutter/flutter#133101
Adds an API for reading the base mip level in whatever format a texture happens to be in.