Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(android): replace image path usage with image uris #906

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

breautek
Copy link
Contributor

@breautek breautek commented Oct 25, 2024

Platforms affected

Android

Motivation and Context

Note this contains #902 will rebase after merge.

Raw system file paths are dangerous to use nowadays since they are often restricted, especially when dealing with external filesystems or filesystems that might not even be on-device (e.g. Google Drive).

Therefore all usages of imageFilePath was removed and replaced with imageUri.

Code that used imageFilePath generally now accepts a Uri instead or an InputStream.

Description

readData / getPageSize

Utility methods to read data into memory. getPageSize is an optimization to read a page of memory, which is usually 4kb but Android 15 devices may be shipped with 16kb page size devices.

There is a lot of disk usage when processing images which might be necessary back in the day when RAM is limited but modern devices have plenty of RAM to deal with image content. It's far more efficient to read the data into memory once and provide several instances of in-memory data streams instead of constantly reading data from disk. (See notes on getScaledAndRotatedBitmap for more details)

ExifHelper

A new method exposed to instantiate via InputStream.

getScaledAndRotatedBitmap

No longer accepts a file path. Instead it accepts the raw binary data. It no longer opens or manages it's own stream.

It will create ByteArrayInputStream which is a stream that doesn't require to be closed, as it operates on in-memory data. It doesn't need to manage kernel objects like file descriptors.

Older code used to create several read streams and read the input source from disk several times. It also created temporary files which was deemed a requirement when dealing with "off-device" files such as Google Drive. There is some truth to this, but temporary files isn't necessary. Data can be read from in-memory instead saving the need to constantly read and write from disk. Content Resolver APIs can also obtain some data like mime type.

Therefore lots of stream management code was removed, which allowed us to remove a lot of try/catches as well. Using the exif changes, we use a ByteArrayInputStream instead virtually everywheres fileStream was previously used.

Note that this is used when sourcing an image from a gallery, it has an unrelated issue that is out of scope of this PR that causes it to not use the result of the modifications and instead returns the original image anyway.

Sourcing an image from camera and applying modifications via targetWidth or targetHeight does work as expected.

on save instance / on restore instance

the image file path key and serialization/restoration were removed.

processResultFromCamera

sourcePath was replaced with InputStream input.

croppedFilePath is still used and is wrapped by a FileInputStream. Refactoring this file path is out of scope of this particular PR, which is only applicable if allowEdit is enabled.

Otherwise imageUri is used and resolved by the content resolver.

input is then asserted for non null, and throws IOException otherwise.

The input is then read into memory and stored in sourceData which is fed into getScaledAndRotatedBitmap. See notes above.

Other Changes

I realise there's a ton of other "format" changes, which
Android Studio IDE is responsible for and I do plan on cleaning them up in another PR.

Testing

Manual testing on API 28 (for non-scoped access framework) & 34 (for scoped access framework).
Paramedic tests passes.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@breautek breautek added this to the 8.0.0 milestone Oct 25, 2024
@breautek breautek force-pushed the refactor/image-file-paths branch from 42babc5 to 7be7422 Compare October 25, 2024 21:39
@breautek breautek force-pushed the refactor/image-file-paths branch from 7be7422 to 314928e Compare October 26, 2024 04:01
@breautek breautek marked this pull request as ready for review October 26, 2024 04:03
@breautek breautek requested a review from erisu October 27, 2024 11:35
@breautek breautek merged commit 0d86764 into apache:master Oct 28, 2024
14 of 15 checks passed
@breautek breautek deleted the refactor/image-file-paths branch October 28, 2024 15:35
Exigerr pushed a commit to PebblePad/cordova-plugin-camera that referenced this pull request Jan 21, 2025
… / audio) files from the gallery picker.

Changes from apache#906 resulted in all gallery picker results being read in full into a byte array. This works fine when dealing with images, however when larger files (video / audio) are selected it will nearly always result in an out of memory error (reading too much data in one go). With videos no transformation is required the URI, the URI simply need to be returned.
Exigerr pushed a commit to PebblePad/cordova-plugin-camera that referenced this pull request Jan 21, 2025
…) files from the gallery picker.

Changes from apache#906 resulted in all gallery picker results being read in full into a byte array. This works fine when dealing with images, however when larger files (videos for example) are selected it will nearly always result in an out of memory error (reading too much data in one go). With videos no transformation is required, the URI simply need to be returned.
Exigerr pushed a commit to PebblePad/cordova-plugin-camera that referenced this pull request Jan 21, 2025
…large media (video) files from the gallery picker.

Changes from apache#906 resulted in all gallery picker results being read in full into a byte array. This works fine when dealing with images, however when larger files (videos for example) are selected it will nearly always result in an out of memory error (reading too much data in one go). With videos no transformation is required, the URI simply need to be returned.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants