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

Why not have centerInside()? #591

Closed
start141 opened this issue Aug 20, 2015 · 30 comments
Closed

Why not have centerInside()? #591

start141 opened this issue Aug 20, 2015 · 30 comments
Milestone

Comments

@start141
Copy link
Contributor

Sometimes we need to load image for center inside, but I have not found this method!

Can add method for center inside?

@TWiStErRob
Copy link
Collaborator

I think such a transformation would go against the resource sensitive nature of Glide.

.centerCrop() and .fitCenter() are loading the source image at a specified size of the target (usually an ImageView) downsampling and/or cropping to that size. If you think about what would .centerInside() do as a Transformation you will quickly see that it would need to create a bigger transparent image and draw the smaller Bitmap inside, or in other words pad the small image with transparent borders to fit in the view. This is a waste of memory.

However you CAN load images as centerInside in a traditional sense:

  • know that the image you're loading is smaller than the view
    larger images may be loaded by .asBitmap().atMost() if I remember correctly so they're downsampled just like with fitCenter.
  • set android:scaleType="centerInside"
  • add .dontTransform() to your load line
  • make sure you don't call .centerCrop() or .fitCenter(); or make sure you call them before .dontTransform(). The last call wins.

Let me know what you think about the above, I may be wrong.

@start141
Copy link
Contributor Author

I think you misunderstood what I said centerInside().

centerInside not need to create transparent image.

for example

I have a image, the size is 100x100px.
I also have a ImageView size set 300x300px.
now I use glide to load this image show on the imageView,
apparently glide will zoom in the image to 300x300px.

I think sometimes it don's necessary. because it would need more memory, and any scenes would not need zoom in the image.

my mean about centerInside(),you can see the picasso's method centerInside().

thanks for you reply!

@TWiStErRob
Copy link
Collaborator

Heh, that's right fitCenter could waste memory as well for small images and big views :)

Try the step by step I wrote.

@start141
Copy link
Contributor Author

Sorry! I don't understand Try the step by step I wrote. meaning.

@TWiStErRob
Copy link
Collaborator

Follow the bullet points at "you CAN load images as centerInside", the most important ones are 2nd and 3rd.

@start141
Copy link
Contributor Author

Thanks for your suggestion.
But some scenarios may not need zoom in images.
like image viewer:
I want to show any image, and the imageView's width and height is match_parent.
(if image size larger then imageView size, use like fitCenter, else keep the image original size)

In this time, glide can't do this. it will fit imageView.

I holp you can reconsider my suggestion. thanks again!

@TWiStErRob
Copy link
Collaborator

Glide can't do this. it will fit imageView.

dontTransform turns this automatic fitting off and lets the image view display the image as is.

if image size larger then imageView size, use like fitCenter, else keep the image original size

This is the definition of centerInside you set on the ImageView in the XML.

I'll check this later, there may be missing piece if it doesn't work when you tried it.

@start141
Copy link
Contributor Author

You can run a picasso demo, it hava centerInside() method, sometimes useful.

@sjudd
Copy link
Collaborator

sjudd commented Aug 21, 2015

Looking over this again, I think fitCenter() and centerInside() are identical?

The ScaleType definition of CENTER_INSIDE is:

Scale the image uniformly (maintain the image's aspect ratio) so that both dimensions (width and height) of the image will be equal to or less than the corresponding dimension of the view (minus padding).

The ScaleType definition of FIT_CENTER redirect's to Matrix.CENTER:

Compute a scale that will maintain the original src aspect ratio, but will also ensure that src fits entirely inside dst. At least one axis (X or Y) will fit exactly. The result is centered inside dst.

@start141
Copy link
Contributor Author

Yes!
So, we also need centerInside(), but glide didn't have. it is useful sometimes, include less memory.

@TWiStErRob
Copy link
Collaborator

@sjudd no, it's not the same. The big differnce comes when you have a drawable that's smaller the the ImageView dimensions. fitCenter will explode it and make it blurry, while centerInside will display it with transparent padding, original size. Compare "will be equal to or less than" and "ensure that src fits entirely inside".

Lookey here: http://bon-app-etit.blogspot.hu/2014/01/imageview-scaletypes.html It's sad that this is the 5th article Google gave me and all the others fail to demonstrate any difference between most of the scaleTypes.

@sjudd
Copy link
Collaborator

sjudd commented Aug 21, 2015

Ahh I see what you're saying, thanks for the clarification. Makes sense, we should add centerInside(). A pull request here would be welcome, it's probably very similar to fitCenter().

@TWiStErRob
Copy link
Collaborator

Ah so to do any of the other fitStart and others it all needs to be done via a new Transformation and a new method on builder and changing the switch in into()?

@sjudd
Copy link
Collaborator

sjudd commented Sep 14, 2015

Yes I think so. It may not need an actually new Transformation class, it may be possible to add parameters to constructors, but it should probably have a new method on the builder.

@LorneLaliberte
Copy link

Note: using dontTransform() along with android:scaleType="centerInside" does not seem to make any difference.

Switching from Picasso to Glide results in a different size when loading a 64x64 PNG image into an ImageView with height 150dp and width match_parent -- in Glide the image is much smaller than using Picasso or setImageResource() directly.

Adding dontTransform() to the load line had no visible effect on the size at all in 3.7.0.

@TWiStErRob
Copy link
Collaborator

@LorneLaliberte There may be some DPI in play there, try to load with each of those and then check the Bitmap size or Drawable intrinsic size for clues. ...or take a screenshot and do some measurements :) You can also try playing around with .asBitmap().asIs() and friends (see Downsampler).

@LorneLaliberte
Copy link

Hmmm...I don't understand how there could be any DPI differences with the same resource in the same target ImageView on the same device. The difference in visible size is quite dramatic (it's about 1/4 what it should be).

Using asBitmap().asIs() and asBitmap().asIs().dontTransform() made no difference either, the image in Glide is tiny compared to loading the exact same image with Picasso or setImageResource().

@TWiStErRob
Copy link
Collaborator

Hmm, I assumed (based on setImageResource) that you're loading from res/drawable or res/drawable-*dpi, are you? If so, which one?

@LorneLaliberte
Copy link

In this particular case it's in res/drawable-hdpi (and no alternative res folders).

However it shouldn't matter where it is being loaded from if it's loading from the same place in both tests. Using Picasso the size matches setImageResource(), using Glide it ends up smaller. It's either applying an additional transformation somewhere, or it's scaling the resource incorrectly vs. the default Android behaviour for android:scaleType="centerInside".

Edit: or somehow avoiding the standard resource transformations and not applying an equivalent transform.

@TWiStErRob
Copy link
Collaborator

Check the Drawable that is set into the ImageView in each case, that or the Bitmap may have some DPI settings that differ. I think Glide doesn't care where the image is coming from (StreamLocalUriFetcher is called with android.resource:// for R.drawable.x), if the Android Resources select that single HDPI image when the content resolution happens, then Glide will open it as a stream and load it as usual (just like if it was HTTP or in the assets folder), the difference here is that setImageResource is likely doing some more magic to upscape the image dimensions so it looks the same size on your screen and it is likely that Picasso is just calling the same method if you load a resource URI.

I was curious and checked and indeed Picasso calls BitmapFactory.decodeResourceStream (through BitmapFactory.decodeResource) in ResourceRequestHandler; and ImageView.setImageResource reaches the same point through Context/Resources.getDrawableResources.loadDrawableForCookieDrawable.createFromResourceStream and there's a lot of density related stuff going on on these decode paths. The big difference is that ContentResolver just opens a raw stream (literally, see openInputStream), so it discards any density info from the device or the drawable folder's qualifiers.

Based on this if you create all the scaled versions correctly you should end up with the same results with all three types of loading.

Alternatively you're free to replace this default loading style inside Glide with one that loads the Drawable as is through Resources or BitmapFactory.

@LorneLaliberte
Copy link

I think Glide doesn't care where the image is coming from (StreamLocalUriFetcher is called with android.resource:// for R.drawable.x)

I'm actually using an android.resource// URI in my tests (for both Glide and Picasso), but I also tested using a resource ID as well in case that was implemented differently, and the results were the same for both.

So to make Glide match the standard behaviour for density-specific resources I would need to apply a custom transformation based on the device density. Which isn't really worth it; might as well just use setImageResource() for resources instead in that case. (Or use Picasso instead since it already does that.)

It probably doesn't make a lot of sense to load density-specific resources if you're going to transform them further anyway; you'd be better off putting them in drawable-nodpi etc..

In my case this was just a test where an existing resource was used for convenience, and the difference in scaling behaviour was unexpected because I assumed Glide would also take the density into account for compatibility reasons.

@TWiStErRob
Copy link
Collaborator

Glide transforms the int ID into an android.resource Uri in ResourceLoader and then uses the ContentResolver mentioned above, that's why you saw the same behavior. There would be a few classes (no transformation, but loader/fetcher/decoder; I've never done this so not sure exactly which) needed to achieve what you want and your Glide load line would probably look horrendous; this will get better in v4 (resulting in a single extra line).

I don't see any reason (curious: what is yours?) why you would want to load them this way (other than maybe a custom transformation applied), the Resources already has built-in caches for Drawables, and Glide doesn't support XML Drawables yet anyway (#350). I'm not sure what's the history behind this type of loading (ignoring density), @sjudd may shed some light on it, but likely it wasn't worth it (why duplicate setImageResource's logic).

... thinking a little more, actually it may be possible to use a transformation/custom target that doesn't create a new Bitmap just sets the density to the correct value, if it can be set post-creation.

@LorneLaliberte
Copy link

I don't see any reason (curious: what is yours?) why you would want to load them this way (other than maybe a custom transformation applied), the Resources already has built-in caches for Drawables,

In this specific case I was using an existing resource in my test purely for convenience (it was already there).

The original motive is to use a common data binding attribute for images that might be remote or might be a resource, and applying custom transformations, so basically just to simplify the binding adapter implementation. (In our case drawable resources are supplied by a third party, although in production they would be processed for sanity checks first.)

The same can be done with multiple attributes, but I was exploring using a common attribute (hence the resource in URI form).

If I was starting from scratch I may not have noticed any difference in behaviour, but since I was testing by modifying an existing project the difference between Picasso and Glide was immediately obvious.

and Glide doesn't support XML Drawables yet anyway

Ah, that's very good to know.

... thinking a little more, actually it may be possible to use a transformation/custom target that doesn't create a new Bitmap just sets the density to the correct value, if it can be set post-creation.

Bitmap.setDensity() would lead me to believe so. :) The density is really just a hint for the draw operation -- there's nothing in the bitmap itself that is affected by it, the pixels are just a data array. A true bitmap really has no density, just pixels; the only place where density matters is when the pixels are mapped onto a target. The Bitmap object just lets you associate it with a density for scaling purposes, but it has absolutely no effect until the bitmap is drawn onto a canvas somewhere.

@TWiStErRob
Copy link
Collaborator

Ah, that's very good to know.

You would've found it out eventually ;)

Bitmap.setDensity() ...

Yep, I remembered something like this, the questions is: Is this the only difference between a Glide and Picasso/setImageResource loaded Bitmap/BitmapDrawable or there's more to it?

@TWiStErRob
Copy link
Collaborator

I think I figured out why load(int) doesn't support DPI: because it is probably not meant for loading R.drawable, it was likely meant for R.raw, the ability to decode any resource type as an Image via BitmapFactory is just a side effect.

@rpn2142
Copy link

rpn2142 commented Aug 14, 2016

This would be really helpful. fit().centerInside() or fit().centerCrop() is an important feature

@TWiStErRob
Copy link
Collaborator

@rpn2142 switch to Glide v4 and enjoy, this is closed as fixed by #727. If you feel like it, you can backport it to 3.8.0 and create a PR, or just create your own transformation.

@rpn2142
Copy link

rpn2142 commented Aug 15, 2016

@TWiStErRob I switched to 4.0 snapshot. For some reason, I get a compile error - "placeholder cannot resolve method 'placeholder(int)'
Is this a change in 4.0?

@TWiStErRob
Copy link
Collaborator

@rpn2142 Major version change means breaking changes... check the apply method and RequestOptions class.

@rpn2142
Copy link

rpn2142 commented Aug 15, 2016

@TWiStErRob Thanks. got it working.

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

5 participants