Skip to content

CenterCrop is not always working  #613

Closed
@yrizk

Description

@yrizk

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

Activity

yrizk

yrizk commented on Sep 9, 2015

@yrizk
Author

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

TWiStErRob commented on Sep 9, 2015

@TWiStErRob
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

yrizk commented on Sep 9, 2015

@yrizk
Author

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

TWiStErRob commented on Sep 11, 2015

@TWiStErRob
Collaborator

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

TWiStErRob

TWiStErRob commented on Sep 11, 2015

@TWiStErRob
Collaborator

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

yrizk

yrizk commented on Sep 11, 2015

@yrizk
Author

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

gintechsystems commented on Sep 28, 2015

@gintechsystems

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

gintechsystems commented on Sep 28, 2015

@gintechsystems

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

TWiStErRob commented on Sep 30, 2015

@TWiStErRob
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

TWiStErRob commented on Sep 30, 2015

@TWiStErRob
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

gintechsystems commented on Sep 30, 2015

@gintechsystems

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

TWiStErRob

TWiStErRob commented on Sep 30, 2015

@TWiStErRob
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

gintechsystems commented on Sep 30, 2015

@gintechsystems

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

TWiStErRob commented on Sep 30, 2015

@TWiStErRob
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

gintechsystems commented on Sep 30, 2015

@gintechsystems

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.

26 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @SarvagyaVaish@sjudd@lingyfh@TWiStErRob@yrizk

        Issue actions

          CenterCrop is not always working · Issue #613 · bumptech/glide