[Impeller] App crashes on wide-gamut indexed PNG decompression in Skia with wide-gamut enabled. #133013
Closed
2 tasks done
Assignees
Labels
a: images
Loading, displaying, rendering images
c: fatal crash
Crashes that terminate the process
dependency: skia
Skia team may need to help us
e: impeller
Impeller rendering backend issues and features requests
engine
flutter/engine repository. See also e: labels.
found in release: 3.13
Found to occur in 3.13
found in release: 3.14
Found to occur in 3.14
has reproducible steps
The issue has been confirmed reproducible and is ready to work on
P1
High-priority issues at the top of the work list
platform-ios
iOS applications specifically
r: fixed
Issue is closed as already fixed in a newer version
team-engine
Owned by Engine team
triaged-engine
Triaged by Engine team
waiting for PR to land (fixed)
A fix is in flight
Comments
nerder
changed the title
|
Hi @nerder. Thanks for filing this. I'm unable to reproduce this on the latest
Can you provide more information on the reproduction steps and the device the crash occurs on? stable, master flutter doctor -v
|
dam-ease
added
the
waiting for customer response
|
Hey @dam-ease, I was only able to reproduce it on a real device (Iphone 11), but since this was shipped in production we have logs from a variety of devices (ie: iPhone 14) Since this is a memory issue I think it might be unlikely to be reproduced on the simulator, but I haven't tried tbh. |
github-actions
bot
removed
the
waiting for customer response
|
Hi @nerder. Thanks for your response. I tested with a real device and I am able to reproduce this issue on the latest Code Sample
import 'package:flutter/material.dart';
void main() {
runApp(const MyApp());
}
class MyApp extends StatelessWidget {
const MyApp({super.key});
@override
Widget build(BuildContext context) {
return MaterialApp(
title: 'Flutter Demo',
theme: ThemeData(
colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
useMaterial3: true,
),
home: const MyHomePage(title: 'Flutter Demo Home Page'),
);
}
}
class MyHomePage extends StatefulWidget {
const MyHomePage({super.key, required this.title});
final String title;
@override
State<MyHomePage> createState() => _MyHomePageState();
}
class _MyHomePageState extends State<MyHomePage> {
@override
Widget build(BuildContext context) {
return Scaffold(
body: Container(
decoration: BoxDecoration(
image: DecorationImage(
image: AssetImage(
"assets/images/image_crash.png",
),
fit: BoxFit.cover,
)),
child: Container(),
),
);
}
}
Log
stable, master flutter doctor -v
|
dam-ease
added
platform-ios
|
Is it possible to increase priority on this issue? It can be dangerous since it doesn't normally occur on simulator, leading developers to believe they aren't effected. @dam-ease |
|
Hi @hyped0001. The issue needs to be identified by the domain expert and the priority label follows. See https://github.com/flutter/flutter/wiki/Issue-hygiene#priorities for more information on this. |
chinmaygarde
removed
the
e: impeller
|
@chinmaygarde According to the repro above this is specific to impeller; is there a reason you removed the impeller label? (Sending back to engine queue for triage, as is standard for impeller issues.) |
stuartmorgan
added
team-engine
|
The crash is in I do think this is an engine bug. Just not an Impeller one. |
|
confirming it's reproduced in https://github.com/nerder/ios_crash_asset_image on the real iPhone with both Flutter 3.13.0 and 3.13.1 though fine on 3.10.6 and 3.10.7 |
jason-simmons
added a commit
to jason-simmons/flutter_engine
that referenced
this issue
Aug 31, 2023
Skia's PNG codec does not support wide gamut output formats for some images (such as palette-based images) See flutter/flutter#133013
|
This started when wide gamut support was enabled by default for iOS apps using Impeller.
But Skia's PNG codec is unable to decode the image into that format because this PNG asset uses a palette. Specifically, For now, apps can work around this by disabling wide gamut using this entry in Within the Flutter engine, we could work around this by disabling wide gamut when decoding PNG images. I've prototyped this at flutter/engine#45357 |
This sounds like an oversight in the Skia decoder that should be addressed. PNG files can definitely contain wide gamut content. So throwing them out would be throwing out the baby with the bath water. We had to implement that pixel format in Skia for this feature so we should fix it. |
danagbemava-nc
changed the title
|
Updated title as this is not specific to asset images. See #134063 |
2 tasks
chinmaygarde
changed the title
chinmaygarde
added
e: impeller
chinmaygarde
changed the title
chinmaygarde
changed the title
|
The compression side crash with wide-gamut is here #133942. I'll add a point of discussion for this in the weekly to figure out hot-fixes and such. |
2 tasks
gaaclarke
changed the title
gaaclarke
changed the title
|
I'll update the title one more time in the past 24 hours =). @chinmaygarde fyi this only effects decoding wide-gamut images PNG files that use indexed colors |
|
Is this fix gonna be in 3.13.4? |
|
After taking into account the severity, workaround, and hot-fix window, the consensus was that this will not be cherry-picked into 3.13. |
gaaclarke
added a commit
to flutter/engine
that referenced
this issue
Sep 13, 2023
…ix for Skia. (#45399) fixes flutter/flutter#133013 depends on skia fix: https://skia-review.googlesource.com/c/skia/+/751696 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
danagbemava-nc
added
the
r: fixed
gaaclarke
added a commit
to gaaclarke/engine
that referenced
this issue
Sep 20, 2023
…ix for Skia. (flutter#45399) fixes flutter/flutter#133013 depends on skia fix: https://skia-review.googlesource.com/c/skia/+/751696 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
This was referenced Sep 20, 2023
|
It was not included in 3.13.4 and 3.13.5 as well, when can we expect this to be released? |
|
What are some temporary ways to solve it？ |
|
The update is out on stable now so flutter upgrade should be good enough :) |
|
nice, flutter 3.13.6 is working fine |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Is there an existing issue for this?
Steps to reproduce
Simply running an app that contains an
AssetImagecontaining a large image (~1MB) causes the app to crash in IOS.As per my logs, this is happening starting from v3.13.0.
Expected results
The app not to crash
Actual results
The app crashes
Code sample
I've reproduced the bug in this small repository: https://github.com/nerder/ios_crash_asset_image
NOTE: I've also added a compressed version of the same image which instead will work. I couldn't manage to find the exact number of bytes that an image needs to be to generate the crash.
Logs
Logs
Flutter Doctor output
Doctor output
The text was updated successfully, but these errors were encountered: