-
Notifications
You must be signed in to change notification settings - Fork 6k
Made Picture::toImage happen on the IO thread with no need for an onscreen surface. #9813
Changes from all commits
4a43442
cfa2f79
38ca94d
cebfbfd
4063f54
1cf6dd8
ca8776e
3e3e711
b3678f9
651e55c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,9 +5,11 @@ | |
#include "flutter/lib/ui/painting/picture.h" | ||
|
||
#include "flutter/fml/make_copyable.h" | ||
#include "flutter/fml/trace_event.h" | ||
#include "flutter/lib/ui/painting/canvas.h" | ||
#include "flutter/lib/ui/ui_dart_state.h" | ||
#include "third_party/skia/include/core/SkImage.h" | ||
#include "third_party/skia/include/core/SkSurface.h" | ||
#include "third_party/tonic/converter/dart_converter.h" | ||
#include "third_party/tonic/dart_args.h" | ||
#include "third_party/tonic/dart_binding_macros.h" | ||
|
@@ -16,6 +18,64 @@ | |
#include "third_party/tonic/logging/dart_invoke.h" | ||
|
||
namespace flutter { | ||
namespace { | ||
|
||
sk_sp<SkSurface> MakeSnapshotSurface(const SkISize& picture_size, | ||
fml::WeakPtr<GrContext> resource_context) { | ||
SkImageInfo image_info = SkImageInfo::MakeN32Premul( | ||
picture_size.width(), picture_size.height(), SkColorSpace::MakeSRGB()); | ||
if (resource_context) { | ||
return SkSurface::MakeRenderTarget(resource_context.get(), // context | ||
SkBudgeted::kNo, // budgeted | ||
image_info // image info | ||
); | ||
} else { | ||
return SkSurface::MakeRaster(image_info); | ||
} | ||
} | ||
|
||
/// Makes a RAM backed (Raster) image of a picture. | ||
/// @param[in] picture The picture that will get converted to an image. | ||
/// @param[in] surface The surface tha will be used to render the picture. This | ||
/// will be CPU or GPU based. | ||
/// @todo Currently this creates a RAM backed image regardless of what type of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "RAM backed" is odd terminology. Even device memory is in RAM right? Can we stick to just the device/host terminology like you have done for the traces? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO device/host is very confusing. Even yuqian asked me to add a comment to his code to clarify them. Images in RAM vs VRAM is the model I've used when dealing with the discrete GPU model. Can you tell me exactly what you want here and I'll plop it in. |
||
/// surface is used. In certain instances we may want a GPU backed image | ||
/// from a GPU surface to avoid the conversion. | ||
sk_sp<SkImage> MakeRasterSnapshot(sk_sp<SkPicture> picture, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here are some points that Chinmay and I discussed offline, and think would be nice to add to the code comment:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I'll add an issue once this lands. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to add that #2 was discussed here at length: Would it be possible to work #2 in since this section of code is related? Having a GPU-backed image and avoiding the GPU-to-CPU overhead would be beneficial. Adding @Hixie - I will soon address the concerns brought up there about increased documentation and tests in this area. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Noted. I think it's something we should do as well, but I think it should be prioritized against other issues and not serve as an impediment to this change. |
||
sk_sp<SkSurface> surface) { | ||
TRACE_EVENT0("flutter", __FUNCTION__); | ||
|
||
if (surface == nullptr || surface->getCanvas() == nullptr) { | ||
return nullptr; | ||
} | ||
|
||
surface->getCanvas()->drawPicture(picture.get()); | ||
|
||
surface->getCanvas()->flush(); | ||
|
||
// Here device could mean GPU or CPU (depending on the supplied surface) and | ||
// host means CPU; this is different from use cases like Flutter driver tests | ||
// where device means mobile devices and host means laptops/desktops. | ||
sk_sp<SkImage> device_snapshot; | ||
{ | ||
TRACE_EVENT0("flutter", "MakeDeviceSnpashot"); | ||
device_snapshot = surface->makeImageSnapshot(); | ||
} | ||
|
||
if (device_snapshot == nullptr) { | ||
return nullptr; | ||
} | ||
|
||
{ | ||
TRACE_EVENT0("flutter", "DeviceHostTransfer"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe leave a comment here like "Here device means GPU and host means CPU; this is different from use cases like Flutter driver tests where device means mobile devices and host means laptops/desktops." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
if (auto raster_image = device_snapshot->makeRasterImage()) { | ||
return raster_image; | ||
} | ||
} | ||
|
||
return nullptr; | ||
} | ||
} // namespace | ||
|
||
IMPLEMENT_WRAPPERTYPEINFO(ui, Picture); | ||
|
||
|
@@ -75,8 +135,8 @@ Dart_Handle Picture::RasterizeToImage(sk_sp<SkPicture> picture, | |
new tonic::DartPersistentValue(dart_state, raw_image_callback); | ||
auto unref_queue = dart_state->GetSkiaUnrefQueue(); | ||
auto ui_task_runner = dart_state->GetTaskRunners().GetUITaskRunner(); | ||
auto gpu_task_runner = dart_state->GetTaskRunners().GetGPUTaskRunner(); | ||
auto snapshot_delegate = dart_state->GetSnapshotDelegate(); | ||
auto io_task_runner = dart_state->GetTaskRunners().GetIOTaskRunner(); | ||
fml::WeakPtr<GrContext> resource_context = dart_state->GetResourceContext(); | ||
|
||
// We can't create an image on this task runner because we don't have a | ||
// graphics context. Even if we did, it would be slow anyway. Also, this | ||
|
@@ -111,17 +171,16 @@ Dart_Handle Picture::RasterizeToImage(sk_sp<SkPicture> picture, | |
delete image_callback; | ||
}); | ||
|
||
// Kick things off on the GPU. | ||
fml::TaskRunner::RunNowOrPostTask( | ||
gpu_task_runner, | ||
[ui_task_runner, snapshot_delegate, picture, picture_bounds, ui_task] { | ||
sk_sp<SkImage> raster_image = | ||
snapshot_delegate->MakeRasterSnapshot(picture, picture_bounds); | ||
|
||
fml::TaskRunner::RunNowOrPostTask( | ||
ui_task_runner, | ||
[ui_task, raster_image]() { ui_task(raster_image); }); | ||
}); | ||
fml::TaskRunner::RunNowOrPostTask(io_task_runner, [ui_task_runner, picture, | ||
picture_bounds, ui_task, | ||
resource_context] { | ||
sk_sp<SkSurface> surface = | ||
MakeSnapshotSurface(picture_bounds, resource_context); | ||
sk_sp<SkImage> raster_image = MakeRasterSnapshot(picture, surface); | ||
|
||
fml::TaskRunner::RunNowOrPostTask( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to revert this too. It was causing a deadlock for some tests. |
||
ui_task_runner, [ui_task, raster_image]() { ui_task(raster_image); }); | ||
}); | ||
|
||
return Dart_Null(); | ||
} | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the colorspace is nullptr, it defaults to
SRGB
. We have stuck to this default behavior elsewhere. Please do the same here as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove this change, it started causing tests to fail.