Repository navigation
[Impeller] when decoding image on iOS, block on command buffer scheduling and log gpu errors. - #169378
Conversation
| << "GPU Error submitting image decoding command buffer."; | ||
| } | ||
| }, | ||
| /*block_on_schedule=*/true) |
There was a problem hiding this comment.
This is going to be inefficient, needlessly synchronizing the cpu and the gpu in most cases.
I think the better way to implement this would be for ImageDecoderImpeller to maintain a receipt from submitting the command buffer. It will add a fml::SyncSwitch::Observer to the fml::SyncSwitch, when that notifies the ImageDecoderImpeller that we are losing access to the GPU it uses the receipt to get access to the buffer and call waitUntilScheduled.
There was a problem hiding this comment.
that is not what block_on_scheduled does. in fact, scheduling has nothing to do with gpu usage at all (that would be waitUntilCompleted). All it blocks on is the queue acknowledging the buffer, which is not synchronous unlike GLES/Vulkan.
There was a problem hiding this comment.
There is an asynchronous operation happening which waitUntilScheduled is causing the calling thread to block on. The approach I'm proposing will only block the thread in the case of applicationDidEnterBackground: which is my understanding of what Apple is recommending.
There was a problem hiding this comment.
yes, but blocking a background worker for a ms or two as part of an operation (image decoding) that regularly takes 100s of ms hardly seems like a problem
There was a problem hiding this comment.
Maybe not. I'm not quite sure why we would prefer it though when we know of an alternative. Plus the alternative doesn't make everyone that submits a command buffer learn what that flag means when it's only relevant on one platform, in one usage.
There was a problem hiding this comment.
Plus we're dealing with a bit of a black box with the driver. I'd say it's preferred we follow Apple's recommendation.
There was a problem hiding this comment.
...and I'll add that we don't know if it fixes it for real or not. If we want to engineer a better solution if it works, I'm down for that. But if we end up back at square one because it doesn't fix it I plan to revert this.
There was a problem hiding this comment.
From offline chat: Calling waitForScheduled when we don't have access to the gpu will cause deadlock, but this is happening only inside of a syncswitch block to guarantee we have the GPU.
| << "GPU Error submitting image decoding command buffer."; | ||
| } | ||
| }, | ||
| /*block_on_schedule=*/true) |
There was a problem hiding this comment.
Is this only going to be an issue for the decoder? Seems like command buffers submitted on the raster thread could run into the same problem too, no?
There was a problem hiding this comment.
never gotten a report of that. Perhaps if it did happen it isn't noticable that the encoding fails since by the time the app is actually foregrounded another frame will have rendered. Whereas if the upload fails for image decoding that failure sticks around
gaaclarke
left a comment
There was a problem hiding this comment.
Okay, I'm fine going ahead with this to test out once we've cleaned it up. Add documentation and file an tech debt issue for the extra thread blocking we could remove.
| virtual std::shared_ptr<BlitPass> OnCreateBlitPass() = 0; | ||
|
|
||
| [[nodiscard]] virtual bool OnSubmitCommands(CompletionCallback callback) = 0; | ||
| [[nodiscard]] virtual bool OnSubmitCommands(bool block_on_schedule, |
There was a problem hiding this comment.
Add docstring for block_on_schedule. (just link to SubmitCommands)
| /// @param[in] callback The completion callback. | ||
| /// | ||
| [[nodiscard]] bool SubmitCommands(const CompletionCallback& callback); | ||
| [[nodiscard]] bool SubmitCommands(bool block_on_schedule, |
There was a problem hiding this comment.
Add docstring to block_on_schedule.
|
Re-opening this PR as the other fix didn't fix it |
Sounds good. I was/am fine with trying this out after the above feedback is addressed. |
|
I added the doc comment |
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…ling and log gpu errors. (flutter#169378) Attempted fix for flutter#166668 . Plan is to ask for testing once rolled into g3 since we weren't given a repro. Explaination: we can only submit command buffers to the metal command queue when the device is in the foreground. push notifications temporarily wake up the app and can trigger images to begin decoding. This decoding can fail, possibly due to the app re-backgrounding. In theory, the sync switch should prevent this from happening - but because we don't actually block on the scheduling of the command buffer there is technically a race that can happen. Add the ability to block submission on scheduling for metal. note that other graphics APIs do not have a distinction for commited vs scheduled and so it no-ops for them. See https://developer.apple.com/documentation/metal/preparing-your-metal-app-to-run-in-the-background > When UIKit calls your app delegate’s [applicationDidEnterBackground(_:)](https://developer.apple.com/documentation/UIKit/UIApplicationDelegate/applicationDidEnterBackground(_:)) method, make sure Metal has scheduled all command buffers you’ve already committed before your app returns control to the system. ... Finish encoding commands to render the frame and commit the command buffer, then call [waitUntilScheduled()](https://developer.apple.com/documentation/metal/mtlcommandbuffer/waituntilscheduled()).
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
Includes: * flutter/flutter#169596 * flutter/flutter#169378 Fix image decode errors on iOS that could occur if a push notification triggered image decoding while the app is backgrounded.
…ling and log gpu errors. (flutter#169378) Attempted fix for flutter#166668 . Plan is to ask for testing once rolled into g3 since we weren't given a repro. Explaination: we can only submit command buffers to the metal command queue when the device is in the foreground. push notifications temporarily wake up the app and can trigger images to begin decoding. This decoding can fail, possibly due to the app re-backgrounding. In theory, the sync switch should prevent this from happening - but because we don't actually block on the scheduling of the command buffer there is technically a race that can happen. Add the ability to block submission on scheduling for metal. note that other graphics APIs do not have a distinction for commited vs scheduled and so it no-ops for them. See https://developer.apple.com/documentation/metal/preparing-your-metal-app-to-run-in-the-background > When UIKit calls your app delegate’s [applicationDidEnterBackground(_:)](https://developer.apple.com/documentation/UIKit/UIApplicationDelegate/applicationDidEnterBackground(_:)) method, make sure Metal has scheduled all command buffers you’ve already committed before your app returns control to the system. ... Finish encoding commands to render the frame and commit the command buffer, then call [waitUntilScheduled()](https://developer.apple.com/documentation/metal/mtlcommandbuffer/waituntilscheduled()).
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…r scheduling and log gpu errors. (flutter/flutter#169378)
…#190445) Fixes [flutter#170599](flutter#170599). Metal image uploads currently wait synchronously for every upload command buffer to reach the scheduled state. That wait was added to protect an iOS backgrounding race, but paying it after every foreground upload unnecessarily blocks image decoding and can delay images becoming ready. This PR moves that synchronization to the lifecycle boundary where it is required: - foreground image uploads commit their Metal command buffer, register a scheduled-or-terminal receipt, and return without waiting; - when GPU access changes to disabled, `ContextMTL` synchronously drains any still-pending image-upload receipts before the transition returns; - receipt registration remains inside the enabled side of the GPU `SyncSwitch`, so a concurrent disable transition cannot miss an upload that was just committed; - existing GLES tracking-fence and `glFlush`/`glFinish` behavior is unchanged. This is a deeper, correctness-preserving follow-up to my deliberately oversimplified first experiment, [flutter#181601](flutter#181601). That PR demonstrated the performance opportunity by only changing `block_on_schedule` to `false`, but it did not replace the iOS lifecycle barrier and did not include the required tests. This PR implements the receipt-based design proposed during the original review and verifies it with deterministic concurrency tests and repeated physical-device measurements. Related lifecycle work includes the original magenta-image report [flutter#166668](flutter#166668), the scheduling wait in [flutter#169378](flutter#169378), the deferred-task race fix in [flutter#169596](flutter#169596), their stable cherry-pick [flutter#170846](flutter#170846), and the separate initial GPU-state fix [flutter#171785](flutter#171785). Older Impeller synchronization discussions [flutter#120399](flutter#120399), [flutter#120406](flutter#120406), and [flutter#131490](flutter#131490) are background context only and are not closed by this PR. ## Why was every upload waiting? The original report, [flutter#166668](flutter#166668), described images becoming pink/magenta after an iOS application was awakened in the background and later foregrounded. [flutter#169378](flutter#169378) addressed a narrow commit-versus-background transition race: 1. A notification can wake an application while it is still in the background. 2. Image decoding starts while the engine still considers GPU access available. 3. The decoder commits a Metal upload command buffer. 4. The application can re-enter the background before Metal acknowledges the command buffer as scheduled. 5. The upload can then fail under iOS background GPU restrictions, leaving a persistent magenta image. The fix submitted image uploads with `block_on_schedule=true`. Because the upload runs inside the shared side of the GPU `SyncSwitch`, waiting before that handler returns prevents the switch's exclusive disable transition from passing until the upload is scheduled. That is correct, but expensive: every normal upload performs the native `[MTLCommandBuffer waitUntilScheduled]`, even when no lifecycle transition is happening. This cost was called out in the original review. In particular: - [the review proposed retaining a submission receipt and waiting only when GPU access is lost](flutter#169378 (comment)); - [a follow-up clarified that ordinary asynchronous scheduling should only block during `applicationDidEnterBackground:`](flutter#169378 (comment)); - [the deadlock discussion warned against waiting after GPU access is already unavailable](flutter#169378 (comment)). The tech-debt issue [flutter#170599](flutter#170599) was then filed specifically to remove this extra foreground thread blocking. This PR implements that deferred design rather than removing synchronization. ## Relationship to the previous attempt [flutter#181601](flutter#181601) only disabled the per-upload wait. It found a very large reduction in the pathological `UploadTextureToPrivate` span, proving that the effective Metal wait was in the submission path. However, the review correctly identified two blockers: - [removing the wait risked reintroducing the magenta-image lifecycle race](flutter#181601 (comment)); - [the change needed significant deterministic test coverage](flutter#181601 (review)). It was [closed because there was no follow-up at that time](flutter#181601 (comment)). [An automated review also suggested removing a later generic `command_buffer->WaitUntilScheduled()` call](flutter#181601 (comment)). That suggestion does not account for backend behavior: Metal's generic `WaitUntilScheduled()` and `WaitUntilCompleted()` hooks are no-ops in this path, while the same block has real `glFlush`/`glFinish` behavior on GLES. This PR leaves that backend-specific visibility behavior intact. ## Correctness invariant When a transition to GPU-disabled returns, every tracked image-upload Metal command buffer committed while the GPU `SyncSwitch` was enabled must have reached either: - the scheduled state; or - a terminal completed/failed state from which it cannot later begin GPU work. At the same time, ordinary foreground image uploads must not synchronously wait for scheduling or completion. ## How the new ordering works - **Foreground:** while holding the enabled/shared `SyncSwitch` lock, install the Metal handlers, commit the upload, register its receipt, and return. - **GPU disable:** the exclusive switch transition first waits for those shared handlers to finish, then synchronously drains every pending receipt before returning. The `SyncSwitch` lock ordering is the proof that registration cannot be missed: `SetSwitch(true)` cannot acquire its unique lock until an upload's shared handler has committed and registered its receipt. Observer notification is synchronous, so the disable transition cannot return before those receipts are safe. Receipt waiting happens outside both the switch lock and tracker mutex. ## Implementation - `CommandBufferSchedulingReceipt` provides a thread-safe, idempotent scheduled-or-terminal signal. Scheduling, completion, and failure all wake waiters, and callbacks run outside its mutex. - `CommandBufferMTL` installs scheduled and completed handlers before `commit` and returns the receipt without waiting or creating a retain cycle. - `PendingImageUploadScheduleTracker` tracks every outstanding upload receipt, handles callback/registration races, uses weak callback state, and waits only after releasing its mutex. - `ContextMTL` drains the tracker when GPU access is disabled while preserving the existing deferred-task behavior when it is enabled again. - `SubmitWithReceipt` replaces the generic `block_on_schedule` Boolean. Metal returns a real receipt; Vulkan and generic fallbacks preserve their existing submission paths; GLES fence and flush/finish behavior is unchanged. - Focused traces cover foreground submission, lifecycle drains, pending counts, and individual waits. ## Positive impact This changes where synchronization is paid, rather than weakening it: - image-decode workers return after commit and receipt registration instead of stalling on Metal scheduling for every image; - batches become ready sooner, with the largest gains for generated pixels, camera buffers, and other upload-heavy sources; - ordinary JPEG and disk-cached image paths also show repeatable end-to-end gains; - the rare GPU-disable transition waits only for image uploads that are still unscheduled, and only until scheduled/terminal—not until GPU completion; - the original backgrounding guarantee is retained and GLES behavior is unchanged. There is no Dart or public Flutter framework API change. ## Performance All measurements below used profile builds with Impeller on the same wired physical iPhone 14 Pro Max running iOS 26.5. Lower is better. ### Existing `large_image_changer` macrobenchmark: 30-run A/B/A The repository macrobenchmark was run 10 times with the baseline, 10 times with this implementation, and 10 more times after returning to the baseline. The same trace surrounded the submission block in both variants. | Metric | Combined baseline | This PR | Change | |---|---:|---:|---:| | Foreground submit p50 | 2.098 ms | 0.024 ms | **−98.86%** | | Foreground submit p99 | 2.855 ms | 0.056 ms | **−98.04%** | | Complete `UploadTextureToPrivate` p50 | 2.654 ms | 0.559 ms | **−78.94%** | | Complete `UploadTextureToPrivate` p99 | 4.355 ms | 0.925 ms | **−78.76%** | The direct trace confirms that the native scheduling wait leaves the foreground critical path. ### Image-upload stress suite: repeated A/B/A A standalone A/B/A suite used three runs per engine and 120 image uploads per run. End-to-end values are run medians; native p50 values pool 360 events. | Workload | Median time until 120 images are ready | Synchronous `UploadTextureToPrivate` p50 | |---|---:|---:| | Bundled JPEG + `ResizeImage` | 768.692 → 726.815 ms (**−5.4%**) | 505.5 → 60.0 µs (**−88.1%**) | | Preseeded `CachedNetworkImageProvider` + `ResizeImage` | 847.037 → 799.973 ms (**−5.6%**) | 477.0 → 56.0 µs (**−88.3%**) | | Raw 512×512 RGBA + resize | 111.705 → 70.902 ms (**−36.5%**) | 547.5 → 141.0 µs (**−74.2%**) | Every changed run was faster than every baseline run. The raw-pixel result is an upload-heavy upper bound; the 5.4–5.6% JPEG results are more representative. Native percentages measure removed synchronous CPU blocking, not faster GPU copy throughput. ## Standalone benchmark reproduction This self-contained sample reproduces the upload-heavy raw-pixel workload. Run it in profile mode against baseline and PR engines on the same iOS device and compare `elapsed_micros` plus the `UploadTextureToPrivate` trace spans. <details> <summary>Expand standalone Flutter benchmark source and commands</summary> Create a project with `flutter create --platforms=ios impeller_upload_benchmark`, then replace/add the following files. ### `pubspec.yaml` ```yaml name: impeller_upload_benchmark publish_to: none environment: sdk: '>=3.4.0 <4.0.0' dependencies: flutter: sdk: flutter dev_dependencies: flutter_driver: sdk: flutter flutter_test: sdk: flutter test: any flutter: uses-material-design: true ``` ### `lib/main.dart` ```dart import 'dart:convert'; import 'dart:ui' as ui; import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; final GlobalKey<BenchmarkPageState> benchmarkKey = GlobalKey<BenchmarkPageState>(); void main() { WidgetsFlutterBinding.ensureInitialized(); runApp(const BenchmarkApp()); } Future<String> handleDriverMessage(String? message) async { switch (message) { case 'warmup': await benchmarkKey.currentState!.warmup(); return 'ready'; case 'run': return jsonEncode(await benchmarkKey.currentState!.runBenchmark()); default: throw UnsupportedError('Unsupported driver message: $message'); } } class BenchmarkApp extends StatelessWidget { const BenchmarkApp({super.key}); @OverRide Widget build(BuildContext context) { return MaterialApp( debugShowCheckedModeBanner: false, home: Scaffold( body: SafeArea(child: BenchmarkPage(key: benchmarkKey)), ), ); } } class BenchmarkPage extends StatefulWidget { const BenchmarkPage({super.key}); @OverRide State<BenchmarkPage> createState() => BenchmarkPageState(); } class BenchmarkPageState extends State<BenchmarkPage> { static const int imageCount = 120; static const int sourceWidth = 512; static const int sourceHeight = 512; static const int targetWidth = 860; static const int targetHeight = 1864; late final Uint8List _pixels; late List<ImageProvider<Object>> _providers; int _generation = 0; @OverRide void initState() { super.initState(); _pixels = _makePixels(); _providers = _makeProviders(); } Uint8List _makePixels() { final Uint8List pixels = Uint8List(sourceWidth * sourceHeight * 4); for (var offset = 0; offset < pixels.length; offset += 4) { final int pixel = offset ~/ 4; pixels[offset] = pixel & 0xFF; pixels[offset + 1] = (pixel >> 8) & 0xFF; pixels[offset + 2] = 0x80; pixels[offset + 3] = 0xFF; } return pixels; } List<ImageProvider<Object>> _makeProviders() { return List<ImageProvider<Object>>.generate( imageCount, (int index) => RawPixelsImageProvider( id: _generation * imageCount + index, pixels: _pixels, width: sourceWidth, height: sourceHeight, targetWidth: targetWidth, targetHeight: targetHeight, ), growable: false, ); } Future<void> _reset() async { PaintingBinding.instance.imageCache ..clear() ..clearLiveImages(); _generation += 1; _providers = _makeProviders(); setState(() {}); await WidgetsBinding.instance.endOfFrame; } Future<void> warmup() async { for (final ImageProvider<Object> provider in _providers.take(40)) { await precacheImage(provider, context); } await _reset(); await Future<void>.delayed(const Duration(milliseconds: 500)); } Future<Map<String, Object>> runBenchmark() async { await _reset(); if (!mounted) { throw StateError('Benchmark page was disposed'); } final Stopwatch stopwatch = Stopwatch()..start(); for (final ImageProvider<Object> provider in _providers) { await precacheImage(provider, context); } stopwatch.stop(); return <String, Object>{ 'image_count': imageCount, 'elapsed_micros': stopwatch.elapsedMicroseconds, }; } @OverRide Widget build(BuildContext context) { return Center( key: const ValueKey<String>('benchmark-ready'), child: Text('Ready (generation $_generation)'), ); } } @immutable class RawPixelsImageProvider extends ImageProvider<RawPixelsImageProvider> { const RawPixelsImageProvider({ required this.id, required this.pixels, required this.width, required this.height, required this.targetWidth, required this.targetHeight, }); final int id; final Uint8List pixels; final int width; final int height; final int targetWidth; final int targetHeight; @OverRide Future<RawPixelsImageProvider> obtainKey(ImageConfiguration configuration) { return SynchronousFuture<RawPixelsImageProvider>(this); } @OverRide ImageStreamCompleter loadImage( RawPixelsImageProvider key, ImageDecoderCallback decode, ) { return MultiFrameImageStreamCompleter( codec: _loadCodec(), scale: 1, debugLabel: 'RawPixelsImageProvider($id)', ); } Future<ui.Codec> _loadCodec() async { final ui.ImmutableBuffer buffer = await ui.ImmutableBuffer.fromUint8List( pixels, ); final ui.ImageDescriptor descriptor = ui.ImageDescriptor.raw( buffer, width: width, height: height, pixelFormat: ui.PixelFormat.rgba8888, ); try { return await descriptor.instantiateCodec( targetWidth: targetWidth, targetHeight: targetHeight, ); } finally { descriptor.dispose(); buffer.dispose(); } } } ``` ### `test_driver/app.dart` ```dart import 'package:flutter_driver/driver_extension.dart'; import 'package:impeller_upload_benchmark/main.dart' as app; void main() { enableFlutterDriverExtension(handler: app.handleDriverMessage); app.main(); } ``` ### `test_driver/benchmark_test.dart` ```dart import 'dart:convert'; import 'dart:io'; import 'package:flutter_driver/flutter_driver.dart'; import 'package:test/test.dart' hide TypeMatcher, isInstanceOf; Future<void> main() async { late FlutterDriver driver; setUpAll(() async { driver = await FlutterDriver.connect(); await driver.waitUntilFirstFrameRasterized(); await driver.waitFor(find.byValueKey('benchmark-ready')); }); tearDownAll(() async { await driver.close(); }); test('measure raw image upload scheduling', () async { expect(await driver.requestData('warmup'), 'ready'); await driver.forceGC(); late String response; final Timeline timeline = await driver.traceAction(() async { response = await driver.requestData( 'run', timeout: const Duration(minutes: 5), ); }, streams: const <TimelineStream>[TimelineStream.embedder]); final Map<String, dynamic> result = jsonDecode(response) as Map<String, dynamic>; expect(result['image_count'], 120); expect(result['elapsed_micros'], greaterThan(0)); await Directory('build').create(recursive: true); await File('build/raw_upload_result.json') .writeAsString(const JsonEncoder.withIndent(' ').convert(result)); await TimelineSummary.summarize(timeline) .writeTimelineToFile('raw_upload', pretty: true, includeSummary: false); // Also keep the value visible in CI/terminal logs. // ignore: avoid_print print(response); }, timeout: Timeout.none); } ``` ### Run against each local engine ```sh flutter pub get flutter drive \ --profile \ -d <device-id> \ --local-engine-src-path=<flutter-checkout>/engine/src \ --local-engine=<baseline-ios-profile-output-name> \ --local-engine-host=<host-profile-output-name> \ -t test_driver/app.dart \ --driver test_driver/benchmark_test.dart # Repeat with only --local-engine changed to the PR engine output. ``` Each run writes: ```text build/raw_upload_result.json build/raw_upload.timeline.json ``` Use at least three runs per engine and return to the baseline after the changed phase. In the timeline, pair `B`/`E` events named `UploadTextureToPrivate` by PID and thread ID before calculating percentiles. </details> ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. --------- Co-authored-by: gaaclarke <[email protected]>
Attempted fix for #166668 . Plan is to ask for testing once rolled into g3 since we weren't given a repro.
Explaination: we can only submit command buffers to the metal command queue when the device is in the foreground. push notifications temporarily wake up the app and can trigger images to begin decoding. This decoding can fail, possibly due to the app re-backgrounding. In theory, the sync switch should prevent this from happening - but because we don't actually block on the scheduling of the command buffer there is technically a race that can happen.
Add the ability to block submission on scheduling for metal. note that other graphics APIs do not have a distinction for commited vs scheduled and so it no-ops for them.
See https://developer.apple.com/documentation/metal/preparing-your-metal-app-to-run-in-the-background