fix: Crash on iOS devices with half image cropping #1582
Conversation
…ith a custom size. Coordinates are also hardcoded
github-actions
bot
added
the
🤳🥫 Scan
Codecov Report
@@ Coverage Diff @@
## develop #1582 +/- ##
==========================================
- Coverage 8.86% 8.61% -0.26%
==========================================
Files 161 161
Lines 6623 6816 +193
==========================================
Hits 587 587
- Misses 6036 6229 +193
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hi @g123k!
I have minor remarks about the way you handle abstraction, but I could survive without fixes.
My main concern is that the whole point of those classes is to get better performance by copying less data, e.g. half less data for the top half.
What I understand in your iOS code is that
- you still copy the whole thing (therefore no expected performance gain)
- while pretending the width and the height are smaller than the full width and height (= what you copied), which may cause incoherences between what you copied and
getSize. - and for that you copy/paste existing code from
CameraImageFullGetter
If your intention is for the iOS not to crash, you can simply call CameraImageFullGetter just in the iOS case. But copying the whole data while pretending it's less data sounds confusing and a source of errors.
Am I right?
| late int _width; | ||
| late int _height; | ||
|
|
||
| void _computeCropParameters() { | ||
| if (orientation == 0) { | ||
| _width = _computeWidth(); | ||
| _height = _computeHeight(); | ||
| } else if (orientation == 90) { | ||
| _width = _computeWidth(); | ||
| _height = _computeHeight(); | ||
| } else { | ||
| throw Exception('Orientation $orientation not dealt with for the moment'); | ||
| } | ||
| } | ||
|
|
||
| @override | ||
| Size getSize() => Size( | ||
| _width.toDouble(), | ||
| _height.toDouble(), | ||
| ); |
There was a problem hiding this comment.
All that could be in the super class CameraImageCropper.
| _height.toDouble(), | ||
| ); | ||
|
|
||
| // Same implementation as [CameraImageFullGetter] |
There was a problem hiding this comment.
I'm a bit surprised: what's the added value if you copy the whole set of planes?
We would be better off with CameraImageFullGetter, wouldn't we?
There was a problem hiding this comment.
Basically it's a CameraImageFullGetter with a custom size.
Since this class still an "ImageCropper", using a mixin may be a solution
There was a problem hiding this comment.
Basically it's a CameraImageFullGetter with a custom size.
That's what I read, but I have serious doubts about copying a full size while saying it's half size. It's not worth the complication anyway as there's no performance gain.
Since this class still an "ImageCropper", using a mixin may be a solution
I'm not familiar with mixin, but from what I read that's too complex for what we want. Assuming your idea of "full copy but half getSize" is correct, you could just extend CameraImageFullGetter and override getSize.
In the end we don't need a CameraImageCropper anyway, we need an AbstractCameraImageGetter.
| import 'package:flutter/material.dart'; | ||
| import 'package:google_ml_barcode_scanner/google_ml_barcode_scanner.dart'; | ||
| import 'package:smooth_app/pages/scan/abstract_camera_image_getter.dart'; | ||
| import 'package:typed_data/typed_buffers.dart'; | ||
|
|
||
| /// Camera Image Cropper, in order to limit the barcode scan computations. | ||
| /// | ||
| /// Note: on iOS, we can only crop the size, as the coordinates are hardcoded. | ||
| /// [width01] and [height01] are thus ignored on this platform. |
There was a problem hiding this comment.
You mean "[left01], [top01]"?
|
Ok, so there are 2 things in this PR: focus and cropping. |
|
Ok, will do that! |
|
Now that we have a fork of the library, I will work on the iOS code directly to support the cropping feature |
On iOS, the native code assumes coordinates start at 0,0.
The only crop we can provide is based on the size.
This PR splits
CameraImageCropperinto two implementations:Will fix #1581