Skip to content

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Dec 5, 2025

fixes #178488

This doesn't implement the following. They can be implemented in later PRs.

  • a skia implementation (maybe won't implement?)
  • a web implementation
  • resizing
  • target pixel format

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@gaaclarke gaaclarke marked this pull request as draft December 5, 2025 22:11
@github-actions github-actions bot added the engine flutter/engine related. See also e: labels. label Dec 5, 2025
gemini-code-assist[bot]

This comment was marked as outdated.

@gaaclarke gaaclarke force-pushed the decodeImagePixelsSync branch from 22d04c5 to a666217 Compare December 5, 2025 23:37
@gaaclarke gaaclarke marked this pull request as ready for review December 8, 2025 19:07
@github-actions github-actions bot added the platform-web Web applications specifically label Dec 8, 2025
@gaaclarke
Copy link
Member Author

Hmm, the linux test runs successfully for me locally.

@gaaclarke
Copy link
Member Author

@flar @chinmaygarde friendly ping

Copy link
Contributor

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ThrowException not unwinding the stack is the big red flag. But past that, the rest looks good to me!

Dart_Handle raw_image_handle) {
auto* dart_state = UIDartState::Current();
if (!dart_state) {
Dart_ThrowException(tonic::ToDart("Dart state is null."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, Dart_ThrowException doesn't unwind the C++ stack. So you can't early return and reasonably expect to have RAII'ed stuff cleaned up. Instead of multiple Dart_ThrowExceptions, you could just return a StatusOr or an optional with an error message, propagate that up and just call a single Dart_ThrowException. This reason is why we have so many instances where we just log the error. I think we can do better now with absl and the new patterns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @jason-simmons who might also remember more about the stack unwinding stuff (or lack therof).

Copy link
Member Author

@gaaclarke gaaclarke Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was following the pattern in canvas.cc. I thought it was setting some state in the dart vm and not performing any jump.

example:

Copy link
Member Author

@gaaclarke gaaclarke Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, you're right. We should fix this in canvas too. Here's from the dart documentation:

 * Dart_PropagateError and Dart_ThrowException do not return.  Instead
 * they transfer control non-locally using a setjmp-like mechanism.
 * This can be inconvenient if you have resources that you need to
 * clean up before propagating the error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I split out a helper function that returns static strings as errors. I also fixed a potential problem in canvas.cc where there was a c++ struct on the stack when we threw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably remove those return statements after the throws too, those are confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should fix this in canvas too... I also fixed a potential problem in canvas.cc where there was a c++ struct on the stack when we threw...

Yeah. I am sure we have missed a couple of other spots in review. I was also considering just banning throwing of exception from native code. Instead returning an error string to Dart code that can then throw the exception.

remove those return statements after the throws too, those are confusing

Yeah. Perhaps we can decorate the call with [[noreturn]]. That way, the compiler will warn about redundant code the author didn't realize was redundant.

sk_data = SkData::MakeWithCopy(pixels.data(), pixels.num_elements());
sk_image = SkImages::RasterFromData(image_info, sk_data, row_bytes);
if (!sk_image) {
Dart_ThrowException(tonic::ToDart("Failed to create image from pixels."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing after this ThrowException will run including RAII cleanups, so the data and image above will be leaked. Hence the recommendation to only call Dart_ThrowException once.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


void PixelDeferredImageGPUImpeller::ImageWrapper::SnapshotImage(
sk_sp<SkImage> image) {
fml::TaskRunner::RunNowOrPostTask(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may only be called once right? Perhaps guard against incorrect use?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a couple of verifying questions...

@gaaclarke gaaclarke added this pull request to the merge queue Dec 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 19, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 19, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 19, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 19, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 19, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 21, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 21, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 21, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 22, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 22, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 22, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 23, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 23, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 23, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Dec 23, 2025
Roll Flutter from 6e1aa82 to d81baab (47 revisions)

flutter/flutter@6e1aa82...d81baab

2025-12-16 68449066+zijiehe-google-com@users.noreply.github.com Revert "chore: linux fuchsia tests are flaking" (flutter/flutter#179897)
2025-12-15 planetmarshall@users.noreply.github.com add default-linux-sysroot to gn frontend args (flutter/flutter#179303)
2025-12-15 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#179751)
2025-12-15 engine-flutter-autoroll@skia.org Roll Skia from 783ce2077e46 to 6903a4e65c3f (2 revisions) (flutter/flutter#179903)
2025-12-15 adilhanney@disroot.org Add `Adwaita Sans` as a font fallback on Linux (flutter/flutter#179144)
2025-12-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Unmodified android sdk bundle (#179647)" (flutter/flutter#179904)
2025-12-15 robert.ancell@canonical.com Update window settings as they change rather than the more outdated "Apply" pattern. (flutter/flutter#179861)
2025-12-15 49699333+dependabot[bot]@users.noreply.github.com Bump dessant/lock-threads from 5.0.1 to 6.0.0 in the all-github-actions group (flutter/flutter#179901)
2025-12-15 engine-flutter-autoroll@skia.org Roll Skia from 9decbd4c216b to 783ce2077e46 (2 revisions) (flutter/flutter#179896)
2025-12-15 34871572+gmackall@users.noreply.github.com Unmodified android sdk bundle (flutter/flutter#179647)
2025-12-15 mdebbar@google.com [web] Fix `resizeToAvoidBottomInset` on Android web (flutter/flutter#179581)
2025-12-15 iliyazelenkog@gmail.com Suppress deprecation warning for AChoreographer_postFrameCallback (flutter/flutter#178580)
2025-12-15 planetmarshall@users.noreply.github.com remove unnecessary virtual destructor from VertexDescriptor (flutter/flutter#178682)
2025-12-15 engine-flutter-autoroll@skia.org Roll Dart SDK from b245f6022727 to 20d114f951db (1 revision) (flutter/flutter#179893)
2025-12-15 jason-simmons@users.noreply.github.com [Impeller] Delete GLES framebuffer objects only on the thread where they were created (flutter/flutter#179768)
2025-12-15 jason-simmons@users.noreply.github.com [Impeller] Convert paths in ImpellerC command line flags to std::filesystem::path objects (flutter/flutter#179717)
2025-12-15 engine-flutter-autoroll@skia.org Roll Skia from 5e1ff611b36b to 9decbd4c216b (2 revisions) (flutter/flutter#179890)
2025-12-15 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from DtfDWAx6t_CsD2e1W... to 433KtmJvbMyaDMJvD... (flutter/flutter#179889)
2025-12-15 116356835+AbdeMohlbi@users.noreply.github.com Remove unnecessary unboxing in `FlutterLoader.java‎` (flutter/flutter#179869)
2025-12-15 engine-flutter-autoroll@skia.org Roll Skia from f04f8ca3b284 to 5e1ff611b36b (1 revision) (flutter/flutter#179882)
2025-12-15 engine-flutter-autoroll@skia.org Roll Packages from 0ac7a03 to 2cd921c (1 revision) (flutter/flutter#179885)
2025-12-15 bkonyi@google.com [ Widget Preview ] Pass DTD URI as a constant in a generated file (flutter/flutter#179821)
2025-12-15 116356835+AbdeMohlbi@users.noreply.github.com Bump minSdk to `24` in `engine` (flutter/flutter#175508)
2025-12-15 engine-flutter-autoroll@skia.org Roll Dart SDK from bca8bb37d95f to b245f6022727 (1 revision) (flutter/flutter#179879)
2025-12-15 engine-flutter-autoroll@skia.org Roll Skia from eb1347d18957 to f04f8ca3b284 (1 revision) (flutter/flutter#179876)
2025-12-15 engine-flutter-autoroll@skia.org Roll Skia from 41bcfb848b13 to eb1347d18957 (1 revision) (flutter/flutter#179871)
2025-12-15 engine-flutter-autoroll@skia.org Roll Skia from d194c3b7a9a9 to 41bcfb848b13 (6 revisions) (flutter/flutter#179866)
2025-12-14 robert.ancell@canonical.com Remove obsolete windowing channel (flutter/flutter#179718)
2025-12-14 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from SCLq31whvg8nJvuYE... to DtfDWAx6t_CsD2e1W... (flutter/flutter#179856)
2025-12-14 engine-flutter-autoroll@skia.org Roll Skia from 5d98ce0af031 to d194c3b7a9a9 (1 revision) (flutter/flutter#179854)
2025-12-13 engine-flutter-autoroll@skia.org Roll Skia from f9b242d9a365 to 5d98ce0af031 (1 revision) (flutter/flutter#179843)
2025-12-13 engine-flutter-autoroll@skia.org Roll Skia from abe90c72cf56 to f9b242d9a365 (1 revision) (flutter/flutter#179838)
2025-12-13 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from fppT9ZrwbFx7iYrIh... to SCLq31whvg8nJvuYE... (flutter/flutter#179836)
2025-12-13 engine-flutter-autoroll@skia.org Roll Dart SDK from 34654f2a3baa to bca8bb37d95f (1 revision) (flutter/flutter#179835)
2025-12-13 engine-flutter-autoroll@skia.org Roll Skia from 9e7c893bb593 to abe90c72cf56 (1 revision) (flutter/flutter#179830)
2025-12-13 engine-flutter-autoroll@skia.org Roll Dart SDK from f105c2168574 to 34654f2a3baa (1 revision) (flutter/flutter#179823)
2025-12-13 97480502+b-luk@users.noreply.github.com Add FilterQuality parameter to FragmentShader.setImageSampler (flutter/flutter#179760)
2025-12-13 evanwall@buffalo.edu Add profiling counts to pipeline uses (flutter/flutter#179374)
2025-12-13 engine-flutter-autoroll@skia.org Roll Skia from edf52cb27f29 to 9e7c893bb593 (3 revisions) (flutter/flutter#179819)
2025-12-12 jacksongardner@google.com Update Skwasm to engine style guidelines. (flutter/flutter#179756)
2025-12-12 engine-flutter-autoroll@skia.org Roll Dart SDK from 9a65db770758 to f105c2168574 (5 revisions) (flutter/flutter#179814)
2025-12-12 engine-flutter-autoroll@skia.org Roll Skia from f23b00a407a4 to edf52cb27f29 (5 revisions) (flutter/flutter#179807)
2025-12-12 97480502+b-luk@users.noreply.github.com Use depth buffer to implement geometry overdraw protection (flutter/flutter#179426)
2025-12-12 nshahan@google.com [flutter_tool] Force UTF-8 character set for dev (flutter/flutter#179419)
2025-12-12 engine-flutter-autoroll@skia.org Roll Skia from e66816c3645e to f23b00a407a4 (2 revisions) (flutter/flutter#179798)
2025-12-12 30870216+gaaclarke@users.noreply.github.com Implements decodeImageFromPixelsSync (flutter/flutter#179519)
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine flutter/engine related. See also e: labels. platform-web Web applications specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Image.decodeImageFromPixelsSync

3 participants