Oh thanks guys, I was planning on adding reviewers manually once all the CI lights had turned green. I'll get someone on the engine team to take a look.

@b-luk expressed some concerns about these textures getting changed under the covers and thus not having the same semantics as other Images. We actually had to copy the external textures to get them to work so I don't think that is a concern. You can see it really clearly in opengles's implementation where we had to copy them because opengles has limitations on how oes textures can be used.

I think with DlImageTextureRegistry, the texture is copied whenever skia_image() or impeller_texture() is called, right? So the DlImage's underlying SkImage/impeller::Texture that gets returned is not the same static image with each call. And that's intended, because the purpose of this is to allow a non-static DlImage (like from a camera or video playback) to be used with fragment shaders, right? I think that's different from the semantics as other DlImages, where their returned skia_image() or impeller_texture() is a static image.

Another question I have after looking at this for a while: With DlImageTextureRegistry, skia_image() and impeller_texture() directly uses snapshot_delegate and calls texture->GetTextureImage(). I think this is only allowed on the raster thread. So skia_image() and impeller_texture() can only be called from the raster thread. Is this a problem? I think skia_image() and impeller_texture() do currently get called from the UI thread.

The other DlImage subclasses seem to have this whole ImageWrapper pattern which contains their usage of snapshot_delegate to be within a raster thread task runner. The resulting SkImage/impeller::Texture gets cached so it can be returned in calls to skia_image() and impeller_texture() from the UI thread. But DlImageTextureRegistry doesn't do this, and uses snapshot_delegate and calls texture->GetTextureImage() directly from skia_image() and impeller_texture().

@b-luk

I think with DlImageTextureRegistry, the texture is copied whenever skia_image() or impeller_texture() is called, right? So the DlImage's underlying SkImage/impeller::Texture that gets returned is not the same static image with each call. And that's intended, because the purpose of this is to allow a non-static DlImage (like from a camera or video playback) to be used with fragment shaders, right? I think that's different from the semantics as other DlImages, where their returned skia_image() or impeller_texture() is a static image.

Yes, this is correct. It creates an image each time. I don't think that's an issue, but we could cache the result if we wanted to to make sure it always returns the same image.

Another question I have after looking at this for a while: With DlImageTextureRegistry, skia_image() and impeller_texture() directly uses snapshot_delegate and calls texture->GetTextureImage(). I think this is only allowed on the raster thread. So skia_image() and impeller_texture() can only be called from the raster thread. Is this a problem? I think skia_image() and impeller_texture() do currently get called from the UI thread.

I don't think those methods are called on the UI thread. The way it works is CanvasImage::CreateFromTexture is called from the ui thread. That creates the DlImageTextureRegistry, then on the raster thread it is picked up and from there skia_image() and impeller_texture() are called.

fml::TaskRunnerAffineWeakPtr<SnapshotDelegate> has an assertion in it that it is only ever called from the raster thread. That may make it a bit more clear.

The other DlImage subclasses seem to have this whole ImageWrapper pattern which contains their usage of snapshot_delegate to be within a raster thread task runner. The resulting SkImage/impeller::Texture gets cached so it can be returned in calls to skia_image() and impeller_texture() from the UI thread. But DlImageTextureRegistry doesn't do this, and uses snapshot_delegate and calls texture->GetTextureImage() directly from skia_image() and impeller_texture().

It doesn't use the delegate until the image is read from the raster thread as noted above. The closest analog to this would be DlDeferredImageGPUImpeller. If you look at the skia version, ImageWrapper is vestigial for skia. It's for getting notifications when the app loses and gains the gpu. I think you are right that the way it is written right now will not work for skia, because when the gpu is lost and gained; the image textures will become invalidated. I think we should probably just disable this feature for skia.

I don't think those methods are called on the UI thread.

They're called for things like DlImage::Equals

CanvasImage::colorSpace():

and other places like this too. I'm not completely confident on what threads may end up calling these, but I think these might be called from the UI thread.

They're called for things like DlImage::Equals

Hmm yea. That's also the case for DlDeferredImageGPUImpeller though.

CanvasImage::colorSpace()
  DlDeferredImageGPUImpeller::impeller_texture()
    DlDeferredImageGPUImpeller::ImageWrapper::texture()

There is an assert in DlDeferredImageGPUImpeller::ImageWrapper::texture() that makes sure it's run on the raster thread.

Right. From my previous comment: "The other DlImage subclasses seem to have this whole ImageWrapper pattern which contains their usage of snapshot_delegate to be within a raster thread task runner". When a DlDeferredImageGPUImpeller is created, it creates a DlDeferredImageGPUImpeller::ImageWrapper, which starts a task on the raster thread:

The raster thread task copies the pixels into wrapper->texture_, where it is saved to be returned by impeller_texture(). So I think the UI-thread-unsafe part (copying the data into wrapper->texture_) happens on the raster thread, but the results (wrapper->texture_) is safe to be returned on the UI thread by impeller_texture().

The raster thread task copies the pixels into wrapper->texture_, where it is saved to be returned by impeller_texture(). So I think the UI-thread-unsafe part (copying the data into wrapper->texture_) happens on the raster thread, but the results (wrapper->texture_) is safe to be returned on the UI thread by impeller_texture().

It's not safe or correct though. There is a race where one thread is potentially reading the texture variable while another is writing it. There is also a window where you ask for the texture on the ui thread before the raster thread is created it.

We can mimic that behavior though by firing off a task for the raster thread though. I can make it as safe as the deferred images.

It's not safe or correct though. There is a race where one thread is potentially reading the texture variable while another is writing it. There is also a window where you ask for the texture on the ui thread before the raster thread is created it.

The design of Picture.toImageSync/DeferredImage is safe. But it's a compromise that has timing-sensitive behavior that applications need to be aware of.

The DeferredImage name references how the implementation does not have access to a texture until the raster thread renders the display list. Until then, APIs like Image.colorSpace that access the texture on the UI thread will fall back to a default value (for example, https://github.com/flutter/flutter/blob/master/engine/src/flutter/lib/ui/painting/image.cc#L53)

The value returned by Image.colorSpace may change after the texture is rendered and the API starts returning attributes of the real texture instead of the fallback value.

There probably does need to be some additional synchronization around the reading/writing of the shared_ptr<Texture> in the ImageWrapper that is not currently there.

The raster_task_runner_->RunsTasksOnCurrentThread() assert in DlDeferredImageGPUImpeller::ImageWrapper::texture() was not originally there and was added recently in feaec27.

Do you recall why you added that assert? I don't think it should be there - it will trigger if an app reads colorSpace from an Image immediately after creating it with Picture.toImageSync.

Do you recall why you added that assert? I don't think it should be there - it will trigger if an app reads colorSpace from an Image immediately after creating it with Picture.toImageSync.

I don't recall. It may have been inferred from the lack of thread-safety between the task setting the variable and the function reading it.

@b-luk I removed the skia implementation for now so we can avoid looking at what happens when we lose access to the GPU. Although, I think we could implement that easily and we can do that as a followup if we want.

I've also matched the semantics of deferred dl image and I made it safe by using atomics with the texture pointer.

I'm going to cancel this PR. I have had some conversations about this feature offline and I think we could potentially come up with a technical solution, however if you look at the PR, it requires a non-trivial addition to the engine.

The payoff isn't that big either for the effort or the maintenance cost. There is a clear workaround for this feature, which is to use backdrop filter's on Texture widgets. On opengles we can't even get a full optimization where we remove the copy because of limitations in external textures. This PR today implements this feature but it has no performance advantage for vulkan or metal, that would require more investment.

This feature just started to creep deeper and deeper. Having a design doc beforehand would have been helpful, but knowing the time required to invest in this, it probably wouldn't have been worth it.

Would it be worth it to automatically use the work around (can we detect the situation)

We can't detect the situation because you are not able to express it with the current API. Maybe with a small tweak to the API we could implement it as the workaround under the covers.

So, for example, if we added Benson's idea, FragmentShader.setTextureSampler(int textureId), under the covers we could implement it as an image filter on a subpass, which is the same way the workaround works under the hood.

Workaround today:

ImageFiltered(
  imageFilter: ImageFilter.shader(fragmentShader),
  child: Texture(textureId: yourTextureId),
)

With the hypothetical change:

class MyTexturePainter extends CustomPainter {
  @override
  void paint(Canvas canvas, Size size) {
    shader.setTextureSampler(0, yourTextureId);
    final paint = Paint()..shader = shader;
    canvas.drawRect(Offset.zero & size, paint);
  }

Maybe we can come back to this in the future. I'm ready to move on for now. Thanks for the suggestion.