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

Fix issue #1044: crash on big size images bug #1055

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KOISHI-CHEN
Copy link

@KOISHI-CHEN KOISHI-CHEN commented Apr 22, 2022

Description

  • The solved Problem:
    App crashes while adding big size images

  • The way I solve it:
    Originally, when reading large images the program will decode it and display it in the format of bitmap which causes a memory overflow, especially png images. So to avoid load so large images, I handle it by using some android api and picasso it

  • The other issue may produce:
    I consider that it will resize all images, so it will also resize the large image which is not cause memory overflow and have a high resolution. Users may not want to resize them.

  • Test environment:
    I have test it on my real cell phone. The phone model is Mi11. Android version is 11. ANd MIUI is MIUI 12.5..15.0(RKBCNXM)
    Resolution is 3200x1440. 8G RAM. The screen measures 6.81 inches
    And I also test on the virtual machine which is pixel 2 api 30 and Pixel 4 API 30. provided by android studio

Fixes #1044

Type of change

Just put an x in the [] which are valid.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • ./gradlew assembleDebug assembleRelease
  • ./gradlew checkstyle

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@codeclimate
Copy link

codeclimate bot commented Apr 22, 2022

Code Climate has analyzed commit 3feea74 and detected 0 issues on this pull request.

View more on Code Climate.

@KOISHI-CHEN KOISHI-CHEN force-pushed the 1044-fix-CrashOnBigSizeImages-bug branch from 3feea74 to 86a8439 Compare April 22, 2022 12:52
@KOISHI-CHEN
Copy link
Author

@codegsaini I consider that I have fixed the bug. The code that related to this bug is about two places.

One is when using Picasso to load a png image in app/src/main/java/swati4star/createpdf/adapter/RearrangeImagesAdapter.java and app/src/main/java/swati4star/createpdf/adapter/PreviewAdapter.java. It should resize the images to avoid using out memory.

And the second is that when loading images in app/src/main/java/swati4star/createpdf/activity/ImageEditor.java. It should restrict the size of images and scale it down.

Thanks to @codegsaini for giving me a method to solve it. And at the same time, I am sorry for giving the wrong places that bug happen.

Hope you can accept the pull request.

@codegsaini
Copy link
Contributor

Thanks for giving me a method to solve it.

@KOISHI-CHEN You're Welcome! and congratulations for your first contribution in this repo. Your commit may have solved the issue but a good contribution is not only in resolving the issue but also to make a proper documentation about it.

There are some points at which I suggest you to have a look -

  • Write proper description so that anyone can understand what was the problem and how you resolved that.
  • The description should be able to provide all answers which can come in the anyone's mind like -
    • What was the problem.
    • How you solved it.
    • Whether the solution produce another issue.
    • In which device(s), this solution is testes.
    • etc.

This is only a suggestion, hope you understood.

That being said, I would like to make it clear that - As a contributor, I can only provide/submit review for your PR(Pull Request).
Only the owner can merge/accept any PR.

Happy coding ;)

Copy link
Contributor

@codegsaini codegsaini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picasso library already have this functionality to scale down bitmap if it exceed threshold limit.
Instead of using explicit scaling down we can make use of Picasso inbuilt method.

   Picasso.with(mContext)
           .load(fileLocation)
           .resize(780, 1200)
           .onlyScaleDown()
           .into(imageView);

Here, .onlyScaleDown() method tells Picasso to only scale down the bitmap if it exceeds thresold limit (which is 780x1200 here).

@KOISHI-CHEN KOISHI-CHEN force-pushed the 1044-fix-CrashOnBigSizeImages-bug branch 3 times, most recently from 645ac63 to fc389a4 Compare April 23, 2022 08:35
PR update

PR update2

PR update3

PR update4

change some lines
@KOISHI-CHEN KOISHI-CHEN force-pushed the 1044-fix-CrashOnBigSizeImages-bug branch from fc389a4 to 5631ff0 Compare April 23, 2022 08:36
@KOISHI-CHEN
Copy link
Author

@codegsaini Thanks for your advice. I have corrected the problem you mentioned above

@KOISHI-CHEN KOISHI-CHEN changed the title 1044 fix crash on big size images bug Fix issue #1044: crash on big size images bug Apr 23, 2022
@@ -153,7 +154,7 @@ private void changeAndShowImageCount(int count) {

mCurrentImage = count % mDisplaySize;
photoEditorView.getSource()
.setImageBitmap(BitmapFactory.decodeFile(mImagePaths.get(mCurrentImage)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious why we can't use Picasso to show image in photoEditorView. Please give picasso a try...

Also at line 102!

Doing so will save us from writing explicit custom method for decoding bitmap.

@codegsaini
Copy link
Contributor

@KOISHI-CHEN Issue #1044 and #1059 are same issue but have different solutions through Pull Request #1055 and #1060.

Two different PR for same issue can be described as below -

PR #1055

In this PR scale down feature of Picasso used to display small/scaled version of bitmap in targeted Imageview.

PR #1060

It compress the actual image itself.


I am excited to know -

PROs and CONs of showing scaled down version in Imageview but leaving the actual image as it is.
PROs and CONs of compressing the actual image.
PROs and CONs of both solution if combined.

@KOISHI-CHEN
Copy link
Author

For the problem about Picasso

@codegsaini Hi! I have tried to use Picasso to load the image. But actually the photoEditor which is PhotoEditorView class is a encapsulated component from another library ja.burhanrashid52.photoeditor. I try to get the imgaeview inside the component but also cause the error

Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'android.graphics.Bitmap android.graphics.drawable.BitmapDrawable.getBitmap()' on a null object reference

It seems like it happens in a asynchronous call and I haven't find the reasons.
Maybe these two library is incompatible

For the second question that related about PR 1055 and PR 1060.

I have browse the code in PR 1060

  • PROs and CONs of showing scaled down version in Imageview but leaving the actual image as it is.
    For this question, I consider that I have to reserve the original pictures, because the final purpose for the functions of my part is to make a better performance in "convert images to pdf". Users may revise the pictures many times or they just don't want to change the original pictures.

  • PROs and CONs of compressing the actual image.
    I think that scaled down will keep the original apearance of that image. What I mean is it may cause the least distortion.
    But for compressing on bitmap, Maybe it is just the same as scale down?

  • PROs and CONs of both solution if combined.
    Well, In my opinion some part of our code are similar but the function is different. What I need to is to load a resource to some images view. But what He need to do is to create a proper pdf. I think we may separate these two methods in order to make it better in furture maintenance
    But combined two solution may reduce the amount of code.

And I also find that using Picasso may better than using the method from android document.
Actually, the method from android document solve the problem But everytime when read a large image the computer will get stuck for seconds like lag when playing online game. Forgive me, I don't know how to describe it in English.

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.

App crashes while adding big size images
2 participants