search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.53k stars 2.88k forks source link

[HOLD for payment 2023-02-20] [$2000] Android - Attachment- User seen frame when attaching picture #14448

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

  1. Launch the app
  2. Log i with expensifail account
  3. Go to any chat
  4. Tap on plus button
  5. Select Add Attachment
  6. Select Take a photo
  7. Take a photo and send it

Expected Result:

Picture should be displayed in full size

Actual Result:

User seen frame when attaching picture

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.2.57.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/213818645-e69d3738-60ae-44ac-9864-51d54cd680ab.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ddb836bc6b1f8022
  • Upwork Job ID: 1618698186309328896
  • Last Price Increase: 2023-02-08
dylanexpensify commented 1 year ago

reviewing now

dylanexpensify commented 1 year ago

Using browserstack I believe I could create (hard to take photo though on the app)

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01ddb836bc6b1f8022

melvin-bot[bot] commented 1 year ago

Current assignee @dylanexpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @mountiny (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

alexxxwork commented 1 year ago

RCA:

the issue comes from wrong image size calculation in https://github.com/Expensify/App/blob/142fbba48c2d5b17c809eff8f85cfc3611dee47b/src/components/ThumbnailImage.js#L55 Vertical images doesn't get rotated on Android phones - they are still horizontal images with exif orientation flag set. So RNImage.getSize returns width > height for vertical images and doesn't know anything about rotation

Proposal:

We could:

  1. Use react-native-exif to get image orientation from thumbnail
  2. It could be a backend problem - to rotate images with rotation flag in exif and make them 'ordinary' - this is the most right way to do this imho.
eh2077 commented 1 year ago

Proposal

RCA

This issue comes from react-native-fast-image library. In android platform we can read rotation information of image through ExifInterface otherwise the image width and height maybe exchanged. We can fix this by improving the patch patches/react-native-fast-image+8.6.3.patch by

  1. changing the input type of BitmapSizeDecoder from InputSteam to File
  2. adding RequestOptions to let Glide skipMemoryCache

FastImageViewWithUrl.java

requestManager
                .as(Size.class)
                .apply(new RequestOptions()
                        .skipMemoryCache(true)
                        .diskCacheStrategy(DiskCacheStrategy.DATA))
                .load(imageSource == null ? null : imageSource.getSourceForLoad())
                .into(new SimpleTarget<Size>() {
                    @Override
                    public void onResourceReady(@NonNull Size resource, @Nullable Transition<? super Size> transition) {
                        WritableMap resourceData = new WritableNativeMap();
                        resourceData.putInt("width", resource.width);
                        resourceData.putInt("height", resource.height);
                        eventEmitter.receiveEvent(viewId,
                            "onFastImageLoad",
                            resourceData
                        );
                    }
                });

BitmapSizeDecoder.java

public Resource<BitmapFactory.Options> decode(@NonNull File source, int width, int height, @NonNull Options options) throws IOException {
        BitmapFactory.Options bitmapOptions = new BitmapFactory.Options();
        bitmapOptions.inJustDecodeBounds = true;
        BitmapFactory.decodeFile(source.getAbsolutePath(), bitmapOptions);
        int orientation = Integer.parseInt(new ExifInterface(source.getAbsolutePath()).getAttribute(ExifInterface.TAG_ORIENTATION));
        if (orientation == ExifInterface.ORIENTATION_ROTATE_90 || orientation == ExifInterface.ORIENTATION_ROTATE_270) {
            int outHeight = bitmapOptions.outHeight;
            int outWidth = bitmapOptions.outWidth;
            bitmapOptions.outHeight = outWidth;
            bitmapOptions.outWidth = outHeight;
        }
        return new SimpleResource(bitmapOptions);
    }

If you want to verify the solution, you can just apply the following diff to patches/react-native-fast-image+8.6.3.patch and rebuild the Android App

Click me ```diff diff --git a/patches/react-native-fast-image+8.6.3.patch b/patches/react-native-fast-image+8.6.3.patch index fc7e59c17c..b07bbc1d74 100644 --- a/patches/react-native-fast-image+8.6.3.patch +++ b/patches/react-native-fast-image+8.6.3.patch @@ -1,12 +1,13 @@ diff --git a/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/BitmapSizeDecoder.java b/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/BitmapSizeDecoder.java new file mode 100644 -index 0000000..03ad017 +index 0000000..5614e4f --- /dev/null +++ b/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/BitmapSizeDecoder.java -@@ -0,0 +1,31 @@ +@@ -0,0 +1,39 @@ +package com.dylanvann.fastimage; + +import android.graphics.BitmapFactory; ++import android.media.ExifInterface; + +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; @@ -16,22 +17,29 @@ index 0000000..03ad017 +import com.bumptech.glide.load.engine.Resource; +import com.bumptech.glide.load.resource.SimpleResource; + ++import java.io.File; +import java.io.IOException; -+import java.io.InputStream; + -+public class BitmapSizeDecoder implements ResourceDecoder { ++public class BitmapSizeDecoder implements ResourceDecoder { + + @Override -+ public boolean handles(@NonNull InputStream source, @NonNull Options options) throws IOException { ++ public boolean handles(@NonNull File source, @NonNull Options options) throws IOException { + return true; + } + + @Nullable + @Override -+ public Resource decode(@NonNull InputStream source, int width, int height, @NonNull Options options) throws IOException { ++ public Resource decode(@NonNull File source, int width, int height, @NonNull Options options) throws IOException { + BitmapFactory.Options bitmapOptions = new BitmapFactory.Options(); + bitmapOptions.inJustDecodeBounds = true; -+ BitmapFactory.decodeStream(source, null, bitmapOptions); ++ BitmapFactory.decodeFile(source.getAbsolutePath(), bitmapOptions); ++ int orientation = Integer.parseInt(new ExifInterface(source.getAbsolutePath()).getAttribute(ExifInterface.TAG_ORIENTATION)); ++ if (orientation == ExifInterface.ORIENTATION_ROTATE_90 || orientation == ExifInterface.ORIENTATION_ROTATE_270) { ++ int outHeight = bitmapOptions.outHeight; ++ int outWidth = bitmapOptions.outWidth; ++ bitmapOptions.outHeight = outWidth; ++ bitmapOptions.outWidth = outHeight; ++ } + return new SimpleResource(bitmapOptions); + } +} @@ -67,7 +75,7 @@ index 0000000..7d208d1 +} \ No newline at end of file diff --git a/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/FastImageOkHttpProgressGlideModule.java b/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/FastImageOkHttpProgressGlideModule.java -index 811292a..f60b87c 100644 +index 811292a..dcfb413 100644 --- a/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/FastImageOkHttpProgressGlideModule.java +++ b/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/FastImageOkHttpProgressGlideModule.java @@ -2,6 +2,7 @@ package com.dylanvann.fastimage; @@ -78,12 +86,20 @@ index 811292a..f60b87c 100644 import com.bumptech.glide.Glide; import com.bumptech.glide.Registry; -@@ -47,6 +48,9 @@ public class FastImageOkHttpProgressGlideModule extends LibraryGlideModule { +@@ -11,6 +12,7 @@ import com.bumptech.glide.load.model.GlideUrl; + import com.bumptech.glide.module.LibraryGlideModule; + import com.facebook.react.modules.network.OkHttpClientProvider; + ++import java.io.File; + import java.io.IOException; + import java.io.InputStream; + import java.util.HashMap; +@@ -47,6 +49,9 @@ public class FastImageOkHttpProgressGlideModule extends LibraryGlideModule { .build(); OkHttpUrlLoader.Factory factory = new OkHttpUrlLoader.Factory(client); registry.replace(GlideUrl.class, InputStream.class, factory); + // Decoder + Transcoder pair for InputStream -> Size -+ registry.prepend(InputStream.class, BitmapFactory.Options.class, new BitmapSizeDecoder()); ++ registry.prepend(File.class, BitmapFactory.Options.class, new BitmapSizeDecoder()); + registry.register(BitmapFactory.Options.class, Size.class, new BitmapSizeTranscoder()); } @@ -115,7 +131,7 @@ index dbeb813..bf8f21c 100644 return false; } diff --git a/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/FastImageViewWithUrl.java b/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/FastImageViewWithUrl.java -index 34fcf89..1339f5c 100644 +index 34fcf89..8f6e741 100644 --- a/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/FastImageViewWithUrl.java +++ b/node_modules/react-native-fast-image/android/src/main/java/com/dylanvann/fastimage/FastImageViewWithUrl.java @@ -2,6 +2,7 @@ package com.dylanvann.fastimage; @@ -126,7 +142,7 @@ index 34fcf89..1339f5c 100644 import android.annotation.SuppressLint; import android.content.Context; import android.graphics.drawable.Drawable; -@@ -9,16 +10,24 @@ import android.graphics.drawable.Drawable; +@@ -9,16 +10,26 @@ import android.graphics.drawable.Drawable; import androidx.annotation.Nullable; import androidx.appcompat.widget.AppCompatImageView; @@ -134,10 +150,12 @@ index 34fcf89..1339f5c 100644 import com.bumptech.glide.RequestBuilder; import com.bumptech.glide.RequestManager; +import com.bumptech.glide.load.DataSource; ++import com.bumptech.glide.load.engine.DiskCacheStrategy; +import com.bumptech.glide.load.engine.GlideException; import com.bumptech.glide.load.model.GlideUrl; import com.bumptech.glide.request.Request; +import com.bumptech.glide.request.RequestListener; ++import com.bumptech.glide.request.RequestOptions; +import com.bumptech.glide.request.target.SimpleTarget; +import com.bumptech.glide.request.target.Target; +import com.bumptech.glide.request.transition.Transition; @@ -151,7 +169,7 @@ index 34fcf89..1339f5c 100644 import java.util.ArrayList; import java.util.Collections; import java.util.List; -@@ -124,9 +133,34 @@ class FastImageViewWithUrl extends AppCompatImageView { +@@ -124,9 +135,34 @@ class FastImageViewWithUrl extends AppCompatImageView { RCTEventEmitter eventEmitter = context.getJSModule(RCTEventEmitter.class); int viewId = this.getId(); @@ -189,7 +207,7 @@ index 34fcf89..1339f5c 100644 } if (requestManager != null) { -@@ -148,6 +182,25 @@ class FastImageViewWithUrl extends AppCompatImageView { +@@ -148,6 +184,28 @@ class FastImageViewWithUrl extends AppCompatImageView { builder.listener(new FastImageRequestListener(key)); builder.into(this); @@ -199,7 +217,10 @@ index 34fcf89..1339f5c 100644 + int viewId = this.getId(); + requestManager + .as(Size.class) -+ .load(imageSource == null ? null : imageSource.getSourceForLoad()) ++ .apply(new RequestOptions() ++ .skipMemoryCache(true) ++ .diskCacheStrategy(DiskCacheStrategy.DATA)) ++ .load(imageSource == null ? null : imageSource.getUri()) + .into(new SimpleTarget() { + @Override + public void onResourceReady(@NonNull Size resource, @Nullable Transition transition) { ```
alexxxwork commented 1 year ago
  1. changing the input type of BitmapSizeDecoder from InputSteam to File

What's the point of using File instead of input stream? ExifInterface works with InputStream as well: https://android.googlesource.com/platform/frameworks/base/+/master/media/java/android/media/ExifInterface.java#1590

eh2077 commented 1 year ago
  1. changing the input type of BitmapSizeDecoder from InputSteam to File

What's the point of using File instead of input stream? ExifInterface works with InputStream as well: https://android.googlesource.com/platform/frameworks/base/+/master/media/java/android/media/ExifInterface.java#1590

Hey @alexxxwork , thanks for looking into it.

It's mainly because the File constructor API has wider sdk version support than the newer InputStream constructor. You can check the API level and the minSdkVersion property defined in build.gradle file of related projects. You can also get hints from an IDE, like Android Studio.

mountiny commented 1 year ago

Not overdue, waiting to review proposals cc @thesahindia

thesahindia commented 1 year ago

I am not familiar with the code here, so I think we should assign a new C+.

0xmiros commented 1 year ago

I can review proposals today

melvin-bot[bot] commented 1 year ago

📣 @0xmiroslav You have been assigned to this job by @mountiny! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

ShekovNikita commented 1 year ago

If XML is used, then one or two lines in the View is sufficient: scaleType="select the desired type" adjustViewBounds="true"

s77rt commented 1 year ago

"Help Wanted" label is missing.

0xmiros commented 1 year ago

@mountiny can you please add Help Wanted label back that was removed by Melvin?

mountiny commented 1 year ago

Added it back, thanks!

alexxxwork commented 1 year ago
  1. changing the input type of BitmapSizeDecoder from InputSteam to File

What's the point of using File instead of input stream? ExifInterface works with InputStream as well: https://android.googlesource.com/platform/frameworks/base/+/master/media/java/android/media/ExifInterface.java#1590

Hey @alexxxwork , thanks for looking into it.

It's mainly because the File constructor API has wider sdk version support than the newer InputStream constructor. You can check the API level and the minSdkVersion property defined in build.gradle file of related projects. You can also get hints from an IDE, like Android Studio.

As far as I can see InputStream work from API 24 which is Android 7.0 https://stackoverflow.com/questions/12944123/reading-android-jpeg-exif-metadata-from-picture-callback

Also, sadly I can't make your patch work :( Maybe I'm doing something wrong? In the way of how patch-package works you should first correct the module in node_modules and then make a patch by runnning npx patch-package react-native-fast-image

mountiny commented 1 year ago

@0xmiroslav Any thoughts on the proposals?

0xmiros commented 1 year ago

@alexxxwork

It could be a backend problem - to rotate images with rotation flag in exif and make them 'ordinary' - this is the most right way to do this imho.

This is not backend issue but android native only issue

Use react-native-exif to get image orientation from thumbnail

Can you elaborate more detail where to integrate in our code? And this should be fixed in native level, not importing another RN library.

@eh2077

This issue comes from react-native-fast-image library.

This is correct but it's not an upstream issue. It's technically a regression from https://github.com/Expensify/App/pull/13304 which customised library code using patch-package.

It's mainly because the File constructor API has wider sdk version support than the newer InputStream constructor. You can check the API level and the minSdkVersion property defined in build.gradle file of related projects. You can also get hints from an IDE, like Android Studio.

Can you share link of min sdk version supported for InputStream constructor?

eh2077 commented 1 year ago

Hi @0xmiroslav

This issue comes from react-native-fast-image library.

This is correct but it's not an upstream issue. It's technically a regression from #13304 which customised library code using patch-package.

Yes, I agreed. It was introduced by the previous react-native-fast-image library based PR that improves image loading performance.

It's mainly because the File constructor API has wider sdk version support than the newer InputStream constructor. You can check the API level and the minSdkVersion property defined in build.gradle file of related projects. You can also get hints from an IDE, like Android Studio.

Can you share link of min sdk version supported for InputStream constructor?

The InputStream constructor is added in API level 24. See https://developer.android.com/reference/android/media/ExifInterface#ExifInterface(java.io.InputStream)

I also tested the InputStream API, however it doesn’t work on my emulator.

If you would like to verify the solution, you can use the following file to replace patches/react-native-fast-image+8.6.3.patch

react-native-fast-image+8.6.3.patch

And then run 

rm -rf node_modules/react-native-fast-image && npm install && npm run android
0xmiros commented 1 year ago

@eh2077 can you please explain why this code was needed?

-+                .load(imageSource == null ? null : imageSource.getSourceForLoad())
++                .apply(new RequestOptions()
++                        .skipMemoryCache(true)
++                        .diskCacheStrategy(DiskCacheStrategy.DATA))
++                .load(imageSource == null ? null : imageSource.getUri())

What happens if image is just base64 data or content uri starting with content:// scheme?

alexxxwork commented 1 year ago

This is not backend issue but android native only issue

I mean that the issue comes from image handling on android platform - picture doesn't get rotated and the issue is in react native component - it returns width and height regardless rotation attribute. It could be fixed on backend by rotation of uploaded pictures using exif rotation tag

The second option is https://github.com/eh2077 proposal to fix RN component but it seems that it requires to fallback to File instead of InputStream

alexxxwork commented 1 year ago

I also tested the InputStream API, however it doesn’t work on my emulator.

Also tried to apply your patch with InputStream on API 33 (hardware) - doesn't work :(

0xmiros commented 1 year ago

@alexxxwork as I commented earlier, the issue came from customisation of react-native-fast-image library. If we use react-native Image component instead of FastImage, width/height is returned correctly. I verified this.

alexxxwork commented 1 year ago

react-native-fast-image+8.6.3.patch

Tested it - it works on API 33 hardware 👍

alexxxwork commented 1 year ago

Can you elaborate more detail where to integrate in our code? And this should be fixed in native level, not importing another RN library.

I should say that @eh2077 approach is better. Hadn't realized that react-native-fast-image isn't a part of RN and already has a patch in the project.

alexxxwork commented 1 year ago

If we use react-native Image component instead of FastImage, width/height is returned correctly. I verified this.

RN Image native module is written in C++ and already uses orientation of images https://github.com/facebook/react-native/blob/37171ec78f377fbae89ce43010f9cf69c1e60fbc/Libraries/Image/RCTImageLoader.mm#L1040 React-native-fast-image uses different native implementation in Java, and needs to be patched to use orientation

eh2077 commented 1 year ago

@eh2077 can you please explain why this code was needed?

-+                .load(imageSource == null ? null : imageSource.getSourceForLoad())
++                .apply(new RequestOptions()
++                        .skipMemoryCache(true)
++                        .diskCacheStrategy(DiskCacheStrategy.DATA))
++                .load(imageSource == null ? null : imageSource.getUri())

Because we need to skip the in-memory cache to read dimension orientation data from file.

What happens if image is just base64 data or content uri starting with content:// scheme?

I found that the image uploaded to the App will be saved as an attachment which is a link. Please correct me if I'm missing something.

I understand that by filtering out no file resources will make them unable to fire the onLoad event. I'm trying to make a new proposal to avoid downgrading to the File decoder

alexxxwork commented 1 year ago

I understand that by filtering out no file resources will make them unable to fire the onLoad event. I'm trying to make a new proposal to avoid downgrading to the File decoder

ExifInterface doesn't work with remote InputStreams, as written here: https://android-developers.googleblog.com/2016/12/introducing-the-exifinterface-support-library.html?m=1

Note: ExifInterface will not work with remote InputStreams, such as those returned from a [HttpURLConnection]
It is strongly recommended to only use them with content:// or file:// URIs.

UPDATE: This info if obsolete

alexxxwork commented 1 year ago

I understand that by filtering out no file resources will make them unable to fire the onLoad event. I'm trying to make a new proposal to avoid downgrading to the File decoder

Maybe we could use Glide for it? https://bumptech.github.io/glide/javadocs/410/com/bumptech/glide/load/ImageHeaderParser.html

https://github.com/bumptech/glide/blob/c283043e722466142d026609ab2599ece6eac247/library/src/main/java/com/bumptech/glide/load/resource/bitmap/DefaultImageHeaderParser.java#L217

alexxxwork commented 1 year ago

Here's working patch using InputStream and Glide to get orientation (based on @eh2077 proposal) react-native-fast-image+8.6.3.patch

    public Resource<BitmapFactory.Options> decode(@NonNull InputStream source, int width, int height, @NonNull Options options) throws IOException {
        BitmapFactory.Options bitmapOptions = new BitmapFactory.Options();
        bitmapOptions.inJustDecodeBounds = true;
        BitmapFactory.decodeStream(source, null, bitmapOptions);
        source.reset();
        int orientation = new DefaultImageHeaderParser().getOrientation(source, Glide.get(this.context).getArrayPool());
        if (orientation == ExifInterface.ORIENTATION_ROTATE_90 || orientation == ExifInterface.ORIENTATION_ROTATE_270 ) {
            int outHeight = bitmapOptions.outHeight;
            int outWidth = bitmapOptions.outWidth;
            bitmapOptions.outHeight = outWidth;
            bitmapOptions.outWidth = outHeight;
        }
        return new SimpleResource(bitmapOptions);
    }
eh2077 commented 1 year ago

Here's working patch using InputStream and Glide to get orientation (based on @eh2077 proposal) react-native-fast-image+8.6.3.patch

    public Resource<BitmapFactory.Options> decode(@NonNull InputStream source, int width, int height, @NonNull Options options) throws IOException {
        BitmapFactory.Options bitmapOptions = new BitmapFactory.Options();
        bitmapOptions.inJustDecodeBounds = true;
        BitmapFactory.decodeStream(source, null, bitmapOptions);
        source.reset();
        int orientation = new DefaultImageHeaderParser().getOrientation(source, Glide.get(this.context).getArrayPool());
        if (orientation == ExifInterface.ORIENTATION_ROTATE_90 || orientation == ExifInterface.ORIENTATION_ROTATE_270 ) {
            int outHeight = bitmapOptions.outHeight;
            int outWidth = bitmapOptions.outWidth;
            bitmapOptions.outHeight = outWidth;
            bitmapOptions.outWidth = outHeight;
        }
        return new SimpleResource(bitmapOptions);
    }

@alexxxwork

Just tested it and it works perfectly on my emulator. Great improvement!

eh2077 commented 1 year ago

Here's working patch using InputStream and Glide to get orientation (based on @eh2077 proposal) react-native-fast-image+8.6.3.patch

    public Resource<BitmapFactory.Options> decode(@NonNull InputStream source, int width, int height, @NonNull Options options) throws IOException {
        BitmapFactory.Options bitmapOptions = new BitmapFactory.Options();
        bitmapOptions.inJustDecodeBounds = true;
        BitmapFactory.decodeStream(source, null, bitmapOptions);
        source.reset();
        int orientation = new DefaultImageHeaderParser().getOrientation(source, Glide.get(this.context).getArrayPool());
        if (orientation == ExifInterface.ORIENTATION_ROTATE_90 || orientation == ExifInterface.ORIENTATION_ROTATE_270 ) {
            int outHeight = bitmapOptions.outHeight;
            int outWidth = bitmapOptions.outWidth;
            bitmapOptions.outHeight = outWidth;
            bitmapOptions.outWidth = outHeight;
        }
        return new SimpleResource(bitmapOptions);
    }

Below is a trivial improvement to catch potential exceptions

    // method of BitmapSizeDecoder.java
    public Resource<BitmapFactory.Options> decode(@NonNull InputStream source, int width, int height, @NonNull Options options) throws IOException {
        BitmapFactory.Options bitmapOptions = new BitmapFactory.Options();
        bitmapOptions.inJustDecodeBounds = true;
        BitmapFactory.decodeStream(source, null, bitmapOptions);
        try {
            source.reset();
            int orientation = new DefaultImageHeaderParser().getOrientation(source, Glide.get(this.context).getArrayPool());
            if (orientation == ExifInterface.ORIENTATION_ROTATE_90 || orientation == ExifInterface.ORIENTATION_ROTATE_270 ) {
                int outHeight = bitmapOptions.outHeight;
                int outWidth = bitmapOptions.outWidth;
                bitmapOptions.outHeight = outWidth;
                bitmapOptions.outWidth = outHeight;
            }
        } catch (Exception ig) {
            // ignored
        }
        return new SimpleResource(bitmapOptions);
    }
dylanexpensify commented 1 year ago

@0xmiroslav can we please get some reviews going today?

mountiny commented 1 year ago

I am reverting the price back to $1000 since we have active proposal discussion here.

melvin-bot[bot] commented 1 year ago

Upwork job price has been updated to $1000

0xmiros commented 1 year ago

If we can get orientation from InputStream supporting our minSdkVersion (21), I prefer this approach. So @alexxxwork's proposal makes sense, but I have some concerns.

So it doesn't recommend to swap width/height here.

alexxxwork commented 1 year ago
  • can you please explain what source.reset() is for and why not working without it?
  • Lint says like this:

So it doesn't recommend to swap width/height here.

  • Your approach to get orientation doesn't seem ideal. Isn't there any better approach to get orientation without using Glide function?
  1. Documentation says that this func leaves stream position where it was after reading https://developer.android.com/reference/android/graphics/BitmapFactory#decodeStream(java.io.InputStream) so we need to rewind the stream to read exif (I tried and it doesn't work without stream rewinding)
  2. Yeah this don't look very nice. Lint says that we probably typo width and height but we don't :) In FastImageOkHttpProgressGlideModule we use decoder and transcoder to get dimensions of the image and put them in our Size class. Decoder generates BitmapFactory.Options object with width and height of the image but this class doesn't have any specific properties of image orientation or exif data. It is used only to transfer these values to transcoder and output to Size class so I don't see any big problem in property reassignment but we can use another approach:
    • Make a custom class extends BitmapFactory.Options with additional orientation property
    • Use this class in decoder and transcoder to get orientation property and transfer to transcoder
    • Swap width and height in BitmapSizeTranscoder But we'll result in a similar operation like: 
      if (tilted) {
size.width = bitmap.outHeight;
size.height = bitmap.outWidth;
} else {
size.width = bitmap.outWidth;
size.height = bitmap.outHeight;
}

      What do you feel about it? Should we add an additional class?

  3. What side effects do you see in using Glide function to get exif? This method only parses image header for exif values (source)
0xmiros commented 1 year ago

@alexxxwork

  1. makes sense
  2. we don't need additional class. it's better to swap directly like in your proposal
  3. we can use ExifInterface from native sdk
alexxxwork commented 1 year ago

3. we can use ExifInterface from native sdk

It doesn't work with remote streams, I wrote earlier about it https://github.com/Expensify/App/issues/14448#issuecomment-1416199819

0xmiros commented 1 year ago
  1. we can use ExifInterface from native sdk

It doesn't work with remote streams, I wrote earlier about it #14448 (comment)

It's very old article 7 years ago. can you check any side effects of this class from androidx library which is recent? It seems also compatible with lower sdk versions.

import androidx.exifinterface.media.ExifInterface;
alexxxwork commented 1 year ago

Proposal

It's very old article 7 years ago. can you check any side effects of this class from androidx library which is recent? It seems also compatible with lower sdk versions.

🤦 Actually both libraries work. You just need to source.reset() after calling decodeStream

    public Resource<BitmapFactory.Options> decode(@NonNull InputStream source, int width, int height, @NonNull Options options) throws IOException {
        BitmapFactory.Options bitmapOptions = new BitmapFactory.Options();
        bitmapOptions.inJustDecodeBounds = true;
        BitmapFactory.decodeStream(source, null, bitmapOptions);
        source.reset();
        int orientation = Integer.parseInt(new ExifInterface(source).getAttribute(ExifInterface.TAG_ORIENTATION));
        if (orientation == ExifInterface.ORIENTATION_ROTATE_90 || orientation == ExifInterface.ORIENTATION_ROTATE_270 ) {
            int tmpHeight = bitmapOptions.outHeight;
            int tmpWidth = bitmapOptions.outWidth;
            bitmapOptions.outHeight = tmpWidth;
            bitmapOptions.outWidth = tmpHeight;
        }
        return new SimpleResource(bitmapOptions);
    }
alexxxwork commented 1 year ago

Here's the patch file react-native-fast-image+8.6.3.patch

0xmiros commented 1 year ago

@alexxxwork ok, so code seems settled. can you please test all cases to confirm it doesn't cause any regression (i.e. right image rotated 90%, horizontal image in vertical frame, etc)?

(test both horizontal and vertical images)

alexxxwork commented 1 year ago

Ok, tested it

alexxxwork commented 1 year ago

@alexxxwork ok, so code seems settled.

@0xmiroslav, could you please tell me the next step. Should I make a PR now?

0xmiros commented 1 year ago

@alexxxwork no, I will test and update soon

mountiny commented 1 year ago

thanks guys, keep it going, this is not an easy issue at all