This is my first attempt at submitting a patch, so please go easy on me. I have noticed many posts from people using imagecache that would like to avoid having black backgrounds on cropped images. One solution is to allow the scaling function to upscale images, but in my situation, I would just rather have the crop action not perform the crop at all if the image is smaller than the size to be cropped too.
The drawback here is that you can't enforce a Standard size for images across the board - but what can you do when your users upload low resolution images? I say you should just let them be low resolution.
Here is what the patch does, it is pretty simple addition on line 199:
case 'crop':
// Patch to ensure images dont get cropped to a larger size if they are smaller than the crop size, otherwise they will get a black background
if ($action['data']['width'] > $size[0]) {
$action['data']['width'] = $size[0];
$action['data']['xoffset'] = 0;
}
if ($action['data']['height'] > $size[1]) {
$action['data']['height'] = $size[1];
$action['data']['yoffset'] = 0;
}
....
Comment | File | Size | Author |
---|---|---|---|
#40 | drupal-allow-upcropping-199957-40.patch | 6.46 KB | tstaylor7 |
#40 | interdiff.txt | 923 bytes | tstaylor7 |
#38 | drupal-allow-upcropping-199957-38.patch | 6.41 KB | tstaylor7 |
#38 | interdiff.txt | 762 bytes | tstaylor7 |
#37 | drupal-allow-upcropping-199957-37.patch | 5.67 KB | tstaylor7 |
Comments
Comment #1
FiReaNGeL CreditAttribution: FiReaNGeL commentedWouldn't it be better to patch image.inc itself instead of imagecache? I'll test this as soon as I get home as I've been experiencing this problem (black background) and found no acceptable solution thus far.
Comment #2
FiReaNGeL CreditAttribution: FiReaNGeL commentedTested the patch, confirms that it works. However, in times where we absolutely want the image to have the specified height / width, this solution doesn't work - I think it's a minor point.
Comment #3
wilco CreditAttribution: wilco commentedI agreed so much with the idea of putting a fix into image.inc that I did so.
The following patch adds some extra conditions to both image_gd_crop() and image_crop(). Hopefully, as I have not tested it with Imagick, it should work with both image kits.
This is not necessarily an imagecache patch, but since this issue already exists here, why not add it to the discussion.
Please review and tell me what you all think.
Caviat Emptor: you must provide scale or resize preset to imagecache to see this puppy work. Recall, it is important to have both Width and Height settings set, otherwise Image.inc will through an error.
Happy Drupalling!
Comment #4
dopry CreditAttribution: dopry commentedI'm moving this to a core issue since the patch is for image.inc.
Comment #5
sunCoding style needs work. See http://drupal.org/coding-standards
Comment #6
kevcol CreditAttribution: kevcol commentedDang, I couldn't get this patch to work for me -- is it not for Drupal 6?
Anyone have any suggestions on fixes? Imagecache Actions would extremely well for my need, but this black background is making it impossible.
Comment #7
wuf31 CreditAttribution: wuf31 commentedsubscribe
Comment #8
klucid CreditAttribution: klucid commentedHave you guys fixed this yet? I've successfully gotten it to work with this: http://drupal.org/project/imagecache_actions
Just pay careful attention to the versions of imageapi and imagecache_actions that are compatible with each other. The modules seem to be conflicted a bit due to imageapi revisions.
Comment #9
ltwinner CreditAttribution: ltwinner commentedI also would like imagecache not to perform the crop if the width and/or height of the image is smaller than the size to be cropped to. I think it looks much much better to just let users have non standard images sizes than to force an upscale of their image and put big black backgrounds around them. Basically, in my case, I only want the width to be cropped if it goes over 200px and the height to be cropped if it goes over 400px. So can this be done with a custom action? Here is a pic of one of my user's profile image -
http://drupal.org/files/issues/imagecache_problem.jpg
The image is 60x60 and because it is cropped to 200x400 it has the black background. So could a custom action be created that would only crop the image if it's width/height was larger than the specified dimensions?
Comment #10
ltwinner CreditAttribution: ltwinner commentedFigured it out with a custom action. Here's what I did in case someone else has the same problem -
1) First I use a scale action to scale the image to 200px. No upscaling so if the width is less than that the picture will remain unchanged.
2) Then I added a custom action. As the image is never going to be over 200px because of step 1, only the height needs to be cropped.
3) The $width field in imageapi_image_crop should just be set to the value $width which is the width of your existing image, which will either be 200px (max value) or smaller.
That will let you have smaller images without having black borders around them while still allowing you to crop larger images.
Comment #11
drewish CreditAttribution: drewish commentedComment #12
dunx CreditAttribution: dunx commentedI did it a bit differently. I need standard-sized logos 120x90.
So first I scale 120 width, then scale 90 height.
I used the imagecache_actions module noted and enabled the canvas sub-module.
Then I created a white 120x90 image and saved it under .../sites/default/files/logo_bg.jpg and added a final action to "Underlay" that image.
No PHP required. Bootiful.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedI vote for custom background color settings per cropping action, and a general setting, for those cases where the canvas is enlarged. Default setting set to white background. With Drupal's already available color picker. Come on, make it user friendly damnit.
Comment #14
drewish CreditAttribution: drewish commented#437702: Separate background color and transparency options, add them to crops that enlarge the canvas had some changes to give the crop a background color.
Comment #15
afoster CreditAttribution: afoster commentedI found a solution to a similar to dunx but with imagefield crop. We needed to have a client upload logos to their portfolio. The whole process to be "photoshop free" but the problem was some source logos had backgrounds and some didn't. Our page required fixed size logos at the end of the day because we has some jquery fading roll-over animation with the logos.
We needed those logos to:
-Crop with imagefield
-Scale to fit the container (180x140)
-Center Vertically & Horizontally with a White Background
This how I did it.
1. Attached imagefield crop to the content type and the following settings:
- The resolution to crop the image onto: 0
- Enforce box crop box ratio (false)
(So it crops the image as the user needs, but does no scaling/resizing)
2. Created the Imagecache Preset
- Action #1: Scale 180x140 (upscaling allowed) << Note this would work without upscaling enabled but would make very tiny logos.
- Action #2: Define Canvas 180x140 (center, center) #ffffff under image
This uses
http://drupal.org/project/imagecache_actions
http://drupal.org/project/imagefield_crop
Now anyone who can draw a box around their logo can properly crop their logo on the site.
Comment #16
scor CreditAttribution: scor commentedbesides coding standards, #3 will need to be rerolled from Drupal root, see instructions at http://drupal.org/patch/create
Comment #17
jonahan CreditAttribution: jonahan commentedAwesome - thank you. And thanks to Imagecache Actions! This let me make all thumbnails the same size and avoid the nasty black border.
Comment #18
datarazor CreditAttribution: datarazor commentedHello, working on D6.20 with image 6.x-1.0-alpha6, imagecache 6.x-2.0-beta10
Patch in #3 doesn't work [half it fails] and the patch in #1 also fails.
Is it not going to work with these release versions? The site needs these versions for other reasons...
Seems to me like not having a black backgroun, or this conditional-scale should be a very common feature. Any solutions to get this to work? Or how the patch needs to be changed to run?
Thanks!
Comment #19
datarazor CreditAttribution: datarazor commentedBTW here is a good alternative crop which is, as the title suggests, much smarter than the default crop:
http://drupal.org/project/smartcrop
Doesn't add black space around my images anymore, phew!
BTW who would ever want black around an image? doesn't make any sense to me. Not many websites have black backgrounds...?
Comment #20
Darinka CreditAttribution: Darinka commentedThank you, smart crop totally did it for me!
Comment #21
RaulMuroc CreditAttribution: RaulMuroc commentedFor me it's the same. I get Black background color with smart crop.
I am using it on Views slideshow gallerific + image smart crop.
Don't know what else try to solve it as well as get centered images, they are all at the top left position by default.
Comment #22
RaulMuroc CreditAttribution: RaulMuroc commentedComment #23
bforchhammer CreditAttribution: bforchhammer commentedNote: issue #204497: Make the background fill color configurable where image styles result in blank space tries to solve the underlying problem with the background color always being black... The original problem seems to be the same as described here, so maybe this should actually be marked as a duplicate?
Comment #24
RaulMuroc CreditAttribution: RaulMuroc commentedI think yes!
Watching there. ;) thanks!
Comment #25
gabrielu CreditAttribution: gabrielu commentedThis is no real use, actually it behaves worst than normal Crop. I think there should normally be a solution in core.
Gabriel
Comment #26
gravit CreditAttribution: gravit commentedI just was checking my dashboard and found it Interesting that this issue is still popping around -
I would like to note to other developers that in the real world of image editing there are zero instances where "crop" actually makes an image bigger.
I think if an end user wants to enforce a size - they can simply create a "scale up" rule before the "scale and crop" if they feel that user uploaded images might be too small to crop to an exact size. Or in my case - if the image is too small to begin with, then leave it at the original size.
Comment #27
NewZeal CreditAttribution: NewZeal commentedOne way to solve this problem is by using imagecache actions and adding an underlay which consists of a white image whose dimensions fit the preset exactly. Then make sure that you set final dimensions to background so that your background image fully covers the default black background.
Comment #28
tstaylor7 CreditAttribution: tstaylor7 commentedI'm reopening this issue, as the it is related to but different from #204497: Make the background fill color configurable where image styles result in blank space.
I adapted c4rl's and greggles's patches from #367484: Add option to stop up sizing during crops ("Allow upcrop") for Drupal 7.
Note that I reduced the options to a single "Allow Upcropping" checkbox, rather than one each for width and height, to make the code more consistent with the "Allow Upscaling" feature (as consistent as I could, anyway, given that some things are already handled differently between the two effects).
I'm guessing this should be an 8.x issue, at this point, but am uncertain, so I'm leaving it as-is.
Warning: This will set "Allow Upcropping" off by default (again to be consistent with "Allow Upscaling"), so existing crop effects will need to be updated to maintain their current functionality. An upgrade path will need to be created to prevent this.
Comment #30
tstaylor7 CreditAttribution: tstaylor7 commentedI forgot to set a sane default value for the new key. This has been fixed.
If this has any hope of being used, I'll setup some tests and an upgrade path for it.
The warning about existing crop effects above in #28 still applies.
Comment #31
tstaylor7 CreditAttribution: tstaylor7 commentedWhile the image is generated with the appropriate dimensions, the themed markup is output with the wrong dimensions. It still displays correctly (at least in Firefox 21.0b2), but this is obviously not ideal, even if it displays correctly in all other browsers (which I'm sure it doesn't).
I'll post a new patch when this is fixed.
Comment #32
SchwebDesign CreditAttribution: SchwebDesign commentedAwesome. I needed this today and implemented patch in #30 and it works great but watching this thread for fix mentioned above.
Comment #33
marcingy CreditAttribution: marcingy commentedFeatures go into latest version first
Comment #34
tstaylor7 CreditAttribution: tstaylor7 commentedI fixed the issue with themed output mentioned in #31. The warning about existing crop effects in #28 still applies.
This is still for Drupal 7, I'll try to get it updated to Drupal 8 next.
Comment #35
tstaylor7 CreditAttribution: tstaylor7 commentedHere's the 8.x patch ported from #34.
Comment #37
tstaylor7 CreditAttribution: tstaylor7 commentedThis should fix the tests. Also, I just noticed I was using the issue number from the other thread in the patch names, this has been corrected. I'll look into making an upgrade path for existing crop effects next.
Comment #38
tstaylor7 CreditAttribution: tstaylor7 commentedI added an upgrade path to maintain the current functionality of crop effects in existing image styles.
Comment #40
tstaylor7 CreditAttribution: tstaylor7 commentedI removed the entity hook from the upgrade path. config() may need to be changed later: #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage, but image_update_8000() uses it as well.
All that remains, it appears, are the tests. Other than updating all the 'data' arrays with the new variable and adding a dimension test for when upcropping is disabled, are there any other tests that should be added or updated?
Comment #41
tstaylor7 CreditAttribution: tstaylor7 commentedComment #43
chx CreditAttribution: chx commentedI will try to look into this during the weekend.
Comment #44
claudiu.cristeaI think that, even this is NOT a duplicate, it still have to be merged with #204497: Make the background fill color configurable where image styles result in blank space.
The reason is that #204497: Make the background fill color configurable where image styles result in blank space is introducing color for additional created areas (like
image_rotate
is doing currently). But according to this issue those additional areas will be created only if "Allow Upcropping" is on.Think we should unify the issues.
Comment #45
tim.plunkettThese should be kept separate.
Comment #46
chx CreditAttribution: chx commentedComment #47
MarkusDBX CreditAttribution: MarkusDBX commentedIs this still an open issue being worked at, or is there a solution in another module or different way of doing it? Curious to know if I should try contributing to make it happen.
Comment #60
candelas CreditAttribution: candelas as a volunteer commentedGood ideas are good ideas.
I would love to have this one for Drupal 11 and 10.
Do you know if any module has implemented? I don't find any.