[framework] load files through ImmutableBuffer.fromFilePath (if exact file type)#112892
[framework] load files through ImmutableBuffer.fromFilePath (if exact file type)#112892cbracken merged 7 commits intoflutter:masterfrom
Conversation
flutter-dashboard
bot
added
the
framework
|
Even when we fix the linked issue, we won't be able to open files greater than INT_MAX on windows- it will just fail with an exception instead of crashing. This change is required to be able to load images this big at all |
There was a problem hiding this comment.
This is different than the behavior of readAsBytes(), and so may have different performance.
There was a problem hiding this comment.
Unfortunately, this approach might have two additional copies since readInto doesn't work exactly as advertised. See the allocation of an intermediate buffer in the the implementation here https://github.com/dart-lang/sdk/blob/main/runtime/bin/file.cc#L249
There was a problem hiding this comment.
Yeah, there is really no winning here 😆 . Something that returns already sized chunks then requires us to write them into a new buffer anyway.
There was a problem hiding this comment.
That is, I don't see a way to do this using the dart:io APIs that doesn't involve two copies. Maybe I should apply this strategy only after a certain file size threshold?
There was a problem hiding this comment.
Do we have an issue filed for this?
jonahwilliams
force-pushed
the
fix_file_image_win
branch
from
129af0f to
207b743
Compare
|
Maybe lets just see if we can land flutter/engine#36623 first. Then use that new API instead, which doesn't have the problem dart:io does |
jonahwilliams
changed the title
zanderso
left a comment
zanderso
left a comment
There was a problem hiding this comment.
lgtm but wondering if it's easy to expand test coverage.
| final Uint8List bytes = await file.readAsBytes(); | ||
| if (bytes.lengthInBytes == 0) { | ||
| final int lengthInBytes = file.lengthSync(); | ||
| if (lengthInBytes == 0) { |
There was a problem hiding this comment.
Is there test coverage of the == 0 case?
There was a problem hiding this comment.
Based on the test failure title, it appears that one of the failures is from such a case:
03:52 +6199 ~13 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/painting/image_provider_test.dart: File image with empty file throws expected error and evicts from cache [E]
Expected: <true>
Actual: <false>
package:test_api expect
package:flutter_test/src/widget_tester.dart 460:16 expect
test/painting/image_provider_test.dart 77:5 main.<fn>
There was a problem hiding this comment.
Yeah, we have a test that covers this. Interestingly if fails if I use the sync length method: #113044
|
|
||
| if (decode != null) { | ||
| return decode(await ui.ImmutableBuffer.fromUint8List(bytes)); | ||
| if (file.runtimeType == File) { |
There was a problem hiding this comment.
Is it possible to have test coverage of the != File case?
There was a problem hiding this comment.
Lots of the existing tests use a MemoryFileSystem entity, so they would break without this change
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Overall lgtm for the approach; looks like there's a test failure related to empty files that needs dealing with.
jonahwilliams
added
the
autosubmit
|
The next magic trick should be |
…if exact file type) (flutter/flutter#112892)
…if exact file type) (flutter/flutter#112892)
4 participants
Update FileImage to use the new ImmutableBuffer.fromFilePath constructor to avoid loading bytes through the Dart heap. As a side effect, tis fixes #112881 which prevents dart:io from opening a file larger than INT_MAX bytes on windows.
For files that are not exactly a dart:io file, we still use the old method of calling loadBytes() in order to not make this a breaking change.