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

CenterCrop is not always working #613

Closed
yrizk opened this issue Sep 8, 2015 · 27 comments
Closed

CenterCrop is not always working #613

yrizk opened this issue Sep 8, 2015 · 27 comments

Comments

@yrizk
Copy link

yrizk commented Sep 8, 2015

Glide Version/Integration library (if any): 3.6.0
Device/Android Version: any
Issue details/Repro steps/Use case background:

How to repo:
I have a demo app with this bug here: https://github.com/yrizk/glide-centercrop-bug

but essentially...
no scale type declared in the xml
centerCrop() when loading image into an imageview.
make sure the app is cleared from memory

The image will start w/o the center crop, then if you scroll past or above it, then sometimes it comes back with the correct scale type.

on a separate note, the library is really good! thank you guys for building and supporting this

@yrizk
Copy link
Author

yrizk commented Sep 9, 2015

I found a hack that fixes this issue:

Glide.with(itemView.getContext()).load(url).asBitmap().centerCrop().diskCacheStrategy(DiskCacheStrategy.ALL).into(new BitmapImageViewTarget(imageView) {
            @Override
            public void onResourceReady(Bitmap bitmap, GlideAnimation anim) {
                super.onResourceReady(bitmap, anim);
                Glide.with(itemView.getContext()).load(url).centerCrop().into(imageView);
            }
        });

EDIT: this fix prevents cardview:cardPreventCornerOverlap="false" from working correctly , and I'm almost certain that since its fixed with a bit of off-screen scrolling, the issue is related to a recycler view.

@TWiStErRob
Copy link
Collaborator

Didn't run your code, but one thing worth trying is changing that wrap_content to match_parent or 0dp on the custom ImageView.

Also try it without the custom ImageView altogether, to see if Glide is compatible with your ImageView. In case it works with stock ImageView you could try to generalize the aspect ratio thing into a custom layout (ViewGroup) and have a fill-fill ImageView inside that.

@yrizk
Copy link
Author

yrizk commented Sep 9, 2015

hello @TWiStErRob thanks for the reply. Actually the code in my project is using a std ImageView, I accidentally added a custom imageview to that sample app . Also I have given the image a set height (150dp) and width (300dp) . Just update the github project to reflect these things.

@TWiStErRob
Copy link
Collaborator

Is it possible that the images you load (imgurImage.getLink()) are smaller than that so there's nothing to crop?

@TWiStErRob
Copy link
Collaborator

I'm sorry for guessing, I don't have computer access to compile and debug atm.

@yrizk
Copy link
Author

yrizk commented Sep 11, 2015

I don't thikn so but lets see. the imageview itself is set to 300 * 150 dp , the image that's being loaded is 615 * 425 px (maybe not on the imgur link, but it is with the real data source). On a htc one m8 the width is 1080 pixels with a densityDPI of 480 which means the the ImageView is actually
900 px * 450 px ?? that's definetly not right, cz the device itself is 1080 px wide, and there grid is 2 columns wide, which is impossible if each is 900 px wide. But that's what the math says...

anyways, if that there was the case, then why is it applying it after the ImageView comes back in the viewport ?
I don't know if this is related, but you guys mention in the wiki that you use Android's CENTER_CROP for your centerCrop() if you apply them side you'll see that Android's ImageView.ScaleType.CENTER_CROP is actually cropping around a slightly different part of the image. Perhaps the CenterCrop algorithm itself isn't deterministic, and you guys are both doing the same thing.

Hope this is helpful. I think if you follow the repo steps you'll see what I mean.

@gintechsystems
Copy link

This also happens to me. I will go into my activity and the images will load. If I hit back and then go back into the activity again, they scale correctly the second time around? Also @yrizk I tried your hack and it seems to work but then it no longer animates.

EDIT:

Moving super.onResourceReady after setting my imagedrawable keeps the animation.

@gintechsystems
Copy link

Also from playing around if I set my src and have it set to centerCrop on the imageview, it seems glide will display the new image the same exact size on the initial load, if you go leave and go back so it displays it again, it displays correctly. Maybe layout issue of some sort or how the image takes cropping?

EDIT:

One new note, if you set the imageview resource on SetResource that is when it happens, it seems to work fine using SetResourceReady.

                           @Override
                            protected void setResource(GlideDrawable resource) {
                            }

                            @Override
                            public void onResourceReady(GlideDrawable resource, GlideAnimation<? super GlideDrawable> glideAnimation) {
                           view.setImageDrawable(resource);                                   
                           super.onResourceReady(resource, glideAnimation);

                            }

@TWiStErRob
Copy link
Collaborator

Sooo, I ran your example app. Sorry for the delay, I was AFK. Here's my investigation:

First run: buggy, symptoms are: first screen of images load correctly, along with partially visible list items. The next row below that doesn't seem to apply centerCrop, but rather fitCenter, neither the next one, but after that centerCrop is correctly applied again, implication: the problem has to do with recycling the item views.

The layout and is too complex to debug, simplify and gather more info:

  • remove TextView from item layout: still buggy
  • .dontAnimate(): still buggy, faster repro
    implication: not related to .crossFade() or TransitionDrawable
  • .diskCacheStrategy(ALL): still buggy, faster repro, no network, even with resized images
  • .skipMemoryCache(true): still buggy. I expected this to always repoduce the issue, even on second scrolling over the buggy images, but it behaved the same.
    implication: not related to disk or memory cache hit
  • .centerCrop() -> scaleType="centerCrop": AOK
    implication: I'm confused!?
  • StaggeredGridLayoutManager->LinearLayoutManager: AOK
    StaggeredGridLayoutManager->GridLayoutManager: still buggy
    implication: probably related to two columns
  • RelativeLayout->FrameLayout: AOK
    RelativeLayout->LinearLayout: AOK
    implication: it's a layout sizing issue

What's up with that .centerCrop() -> scaleType="centerCrop" fixing the issue?
The issue manifestation looks like fitCenter. It's hard to imagine Glide would fail to apply an explicitly asked for .centerCrop(). Moving the explicit cropping into XML, makes it implicit and also overrides the default fitCenter. implication: the Bitmap must be cropped and then shown with fit when the issue is present.

Test image: http://i.imgur.com/jMvECu2.jpg, original size: 604x453
Add .listener() to log (only excerpt shown here):

http://i.imgur.com/G5CSRw1.png ImageView{528e56cc 0,0-476,450} onResourceReady(false, true) bitmap: 476, 450 // first screen loaded
http://i.imgur.com/jMvECu2.jpg ImageView{528a4510 0,0-476,450} onResourceReady(false, true) bitmap: 900, 450 // first load: buggy image
http://i.imgur.com/jMvECu2.jpg ImageView{528a5b38 0,0-476,450} onResourceReady(false, true) bitmap: 476, 450 // later load: AOK image

So, indeed the bitmap is loaded with bigger than the view size, fitting applies.

Why is Glide loading a 900x450 image into a clearly 476x450 size ImageView?
I'm using a Galaxy S5 emulator with FullHD display, 480dpi -> xxhdpi/3x. Screen width: 1080
Let's debug ViewTarget.SizeDeterminer#getSize: when populating the ViewHolder I set the tag:

imageView.setTag(R.id.image, imgurImage.getLink());

this helps to identify which hits to the getSize method I need. Next, add a conditional breakpoint in getSize:

"http://i.imgur.com/jMvECu2.jpg".equals(view.getTag(com.amg.android.projectt.R.id.image))

App startup: no hits as expected, scroll down, breakpoint hit. currentWidth=900, hmm, @yrizk was confused about the item size too. Let's see where it comes from:

view.getWidth(): 0
view.getLayoutParams().width: 900

Aah, makes sense, since the XML says layout_width="300dp". But then why did it work with Frame and Linear layouts? They're much simpler than RelativeLayout and they probably ask for how big they can be, before asking their children to measure themselves.

TL;DR and summary
Setting the layout width to 300dp (900px) for a 2-column grid on a 1080px screen confuses Glide, because it tries to read the sizing information from the laid out image and then the layout params, if neither is available it waits for a layout, see ViewTarget.SizeDeterminer#getViewWidthOrParam. Since the hardcoded value is there, it just loads the image 900px wide. But the grid sizing constrians the view sizes to 476px, so ImageView has to display a larger image in a smaller space with the default fitCenter, that's why it looks like centerCrop wasn't applied.

Then that item scrolls off screen and gets reused. When scrolled back the same image is bound to a recycled item which means it was already laid out and now view.getWidth() will give the right size. The grid items are identical even with staggered grid layout. Their width doesn't change even when the TextView contains more text. Now the SizeDeterminer figures 476px wide image is needed and crop can properly do it's work which is then shown in a perfectly sized ImageView, so the fitCenter's effect is not visible.

Solutions

  • The simplest is to not use hardcoded values, so Glide waits for a proper layout:

    <CardView
        ...
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        >
        <RelativeLayout
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            >
            <ImageView
                ...
                android:layout_width="match_parent" <!-- this makes Glide wait -->
                android:layout_height="150dp"
                />
            ...
        </RelativeLayout>
    </CardView>
  • moving .centerCrop() -> scaleType="centerCrop" is not a good solution, because it still makes Glide load 900px images. The original is cropped to 900px by Glide and then cropped by the ImageView to the real size. It's double work and even more, because when scrolled out and back even the disk cache misses, because the second time the original needs to be cropped to the right size by Glide, resulting in double network requests and double caching for the same image in RESULT cache.

  • using another layout may be ok, but it still hides the problem like above.

I still suggest to move centerCrop() into XML, because it's a view thing, not logic:

    <ImageView
        ...
        android:layout_width="match_parent" <!-- this makes Glide wait -->
        android:layout_height="150dp"
        android:scaleType="centerCrop"
        />

it also simplifies the loading to:

Glide.with(itemView.getContext()).load(imgurImage.getLink()).into(imageView);

and all the benefits of resource directories are unleashed, for example you can use fitCenter and different size on tablet sized screens, all just defined in XML.

@TWiStErRob
Copy link
Collaborator

@gintechsystems

set to centerCrop on the imageview, it seems glide will display the new image the same exact size on the initial load

This is the second solution I described, it just hides the issue more, please review your layout and try to make it more dynamic.

@gintechsystems
Copy link

Thanks twister. So for now just do these workarounds, will this possibly be fixed in the future?

@TWiStErRob
Copy link
Collaborator

It cannot be fixed as I see it. Reading the LayoutParams.width is a really good way to speed up image loading, in most cases, and only when the layout agrees. Try to replace the width to match_parent, which is not a workaround, it is the correct way to allow Glide bring it's best performance; In a way Glide forces you to make better layouts, because it may misbehave if you half-ass it. Double-loading and custom target hacks will just bite you later.

@gintechsystems
Copy link

Thanks, understood. I currently use match_parent but I also use match_parent for my height because I want aspect ratio and I measure on imageview which creates a perfect square. Not sure if this is bad practice though.

@TWiStErRob
Copy link
Collaborator

I use a simple layout around the ImageView to make it square: https://gist.github.com/TWiStErRob/f8190f1f55ec1e7777fc. It plays real nice with Glide so far.

@gintechsystems
Copy link

Thanks, that is how mine is setup and is working great with glide. Was just checking to make sure what I was doing was ok.

@TWiStErRob
Copy link
Collaborator

This also looks like a duplicate of #291.

@gintechsystems
Copy link

Yes I agree.

@yrizk
Copy link
Author

yrizk commented Oct 1, 2015

@TWiStErRob thank you so much for that very detailed investigation. Seriously, I'm impressed. That 900px certainly makes more sense light of what you just said. I understand that waiting the layout traversal so that dimensions are finalized removes the confusion, but I'm still confused as to why the fitCenter as opposed to centerCrop . if Glide mistook the ImageView to be 900px wide instead of what it was allowed to be (476px) , isn't that still a larger image loading into a smaller view? Shouldn't a scaling type still be a valid operation? does it somehow lose its scaling type because the bitmap itself was resized from 900px to 476px?

@TWiStErRob
Copy link
Collaborator

thank you so much for that very detailed investigation.

That's what you get for providing a sample project... ;)

@yrizk the centerCrop disappears because the upscaled centerCropped image is fitCentered (even if you say .centerCrop() the default on the ImageView is still fitCenter) into the small ImageView:

  1. Original size: 604x453 (upper left)
  2. View size in layout: 900x450 (blue), which is upscaled (humula emulates centerCrop)
  3. Which is then fitted into 476x450 (red): notice the height is already good, which means it has to fit the width meaning it has to resize 900 to 476 keepig aspect
  4. The result is a 476x238 image displayed in a 476x450 sized box (red), which means top and bottom padding.

When you have match_parent, Glide receives 475x450 resulting in the perfect size (green) where it also doesn't matter which scaleType is used, since none of them can alter the image in a way that will distort it.

stuff

Feel free to ask more if this doesn't clear it up!

@yrizk
Copy link
Author

yrizk commented Oct 1, 2015

yes I understand exactly what you mean now. and wow you keep raising the bar!! Seriously thank you again this is all very illuminating. I'd buy you a drink , but I just looked into int'l fund transfer and its discouraging. if you've had success with paypal lmk :) Glide takes the cake for best support in my book

@lingyfh
Copy link

lingyfh commented Sep 19, 2017

when I change placeholder image size, it's ok. OMG!!!!!!

@SarvagyaVaish
Copy link

My ImageView has a layout_width="0dp" since i am using a layout_weight="2". Do you have any suggestions for when I cannot use match_parent?

@gintechsystems
Copy link

@SarvagyaVaish What happens when you don't use match_parent. can you show us the issue from a screenshot?

@SarvagyaVaish
Copy link

After further testing I found out that the problem was with setting a placeholder image. I was setting a square placeholder image. Once Glide fetched the actual rectangular image it replaced the placeholder with it, but ended up forcing it to be square.
For now my workaround is to use a color as a placeholder instead of the square png placeholder. If you know of a way to get glide to respect the aspect ratio of the image it is fetching, please let me know. Also, if you want I can create a sample app that demonstrates this oddity.

Thanks!

@lucazin
Copy link

lucazin commented Apr 26, 2019

I still suggest to move centerCrop() into XML, because it's a view thing, not logic:

<ImageView
    ...
    android:layout_width="match_parent" <!-- this makes Glide wait -->
    android:layout_height="150dp"
    android:scaleType="centerCrop"
    />

it also simplifies the loading to:

Glide.with(itemView.getContext()).load(imgurImage.getLink()).into(imageView);

this fix my problem !

@Mkurbanov
Copy link

Mkurbanov commented Dec 21, 2020

i just removed placheldor from glide and it's worked for me :)

Hocuri added a commit to deltachat/deltachat-android that referenced this issue Mar 4, 2021
fix  #1797

Apparently, we ran into a bug similar to the one described in
bumptech/glide#613 (comment) as
we cropped twice (in addition to the cropping I removed, we also set
`android:scaleType="centerCrop"` in `thumbnail_view.xml`)
Hocuri added a commit to deltachat/deltachat-android that referenced this issue Mar 5, 2021
fix  #1797

Apparently, we ran into a bug similar to the one described in
bumptech/glide#613 (comment) as
we cropped twice (in addition to the cropping I removed, we also set
`android:scaleType="centerCrop"` in `thumbnail_view.xml`)
@madisadyk
Copy link

this bug is still in version 4.13.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants