This is a sub-patch of #371374: Add ImageCache UI Core. In order to operate on images multiple times (such as crop, scale, then desaturate) without quality loss, we need to pass images by their raw GD (or other library) resources rather than re-opening the same image repeatedly, which causes wasted processing and loss of quality when using JPEG images.

I'm currently working on the patch unit tests, I'll post when they're finished.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

subscribing

tstoeckler’s picture

Issue tags: +ImageCacheInCore
quicksketch’s picture

FileSize
22.22 KB

Here's a patch that is just the ImageAPI portion. I'm still working on the tests.

quicksketch’s picture

Status: Active » Needs work
FileSize
315.35 KB
24.15 KB
14.39 KB
183 bytes
46.88 KB

Hurray! A completed patch.

This version includes extensive image handling tests (see screenshot), and handles just about every situation. There is one single exception that GIF images when desaturated loose transparency. Considering there are no options for imagefilter for GD, there's nothing we can do, other than build our own pixel-by-pixel desaturate. It's a minor problem in my opinion.

This patch needs 3 additional testing images that are created specially for testing colors and cropping. Each image is tested by checking height, width, and the colors in each of the corners (so we can identify rotates and croppings).

I'm marking as CNW, since I'm guessing testing bot can't handle the addition of new images through CVS. Maybe we could get these images added to the /modules/simpletest/files directory before the rest of the patch? We'll eventually need them regardless of the final code.

drewish’s picture

quicksketch, looks like you forgot to the tests into the patch.

quicksketch’s picture

FileSize
33.32 KB

Well! Now I'm just silly, I didn't include the tests! Here they are all wrapped together.

quicksketch’s picture

FileSize
1.27 KB

I was curious why my PNG file was so big. Looks like I left in the Fireworks meta data. This version of the png is identical in appearance but much smaller.

webchick’s picture

Status: Needs work » Needs review

Marking back to needs review so testing bot can have a crack at it.

drewish’s picture

Assigned: quicksketch » drewish
Status: Needs review » Needs work
FileSize
34.06 KB

In file_validate_image_resolution() i think we need to move

           // Clear the cached filesize and refresh the image information.
           clearstatcache();

into image_close().

The chmod there also reminded me that we should probably get #203204: Uploaded files have the permissions set to 600 into core soon.

Fixed up some of the docs. There were @see tags referencing image_load() rather than image_open(). Fixed the case on some true and falses.

I moved some stuff around to minimize the number of diffs and help spot the changes between the old code and new. so the open and close functions are at the bottom of the file. Doing so let me catch a lot of reversions in comments. I think it make sense logically to have them at the top but i'd really like to move that in a separate patch.

I can't seem to run the tests so I'm going to post this then re-install and keep working on it.

drewish’s picture

FileSize
35.84 KB

In image.gd.inc I removed the & by reference passing of $image since it's an object and in PHP5 always passed by reference.

Lots more docs cleanups.

quicksketch’s picture

Webchick: Testing bot will inevitably fail because our sample images can't be added through a patch. Could we get image-3.png, image-4.gif, and image-5.jpg added to simpletest (we'll need them eventually no matter the final version of this patch) so testing bot can handle it?

quicksketch’s picture

FileSize
34.43 KB

The logic in image_gd_rotate() seemed a little heavy, and now that I understand GIF images a bit better I can make a better patch that avoids an unnecessary image_gd_create_tmp. I updated the docs inside image_gd_rotate() to describe the process more clearly. Also a few tiny PHPdoc whitespace fixes.

webchick’s picture

Per #11, I committed the three images as image-test.png/gif/jpg. While it's a bit premature, since we don't know if this patch will make it into core yet:

a) I'm hopefully optimistic that it will :)
b) Even if we don't put image API in core, we could still use these files to write tests against our existing image API.

Patch will need a re-roll to take these new files into account.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
34.44 KB

Nice! Here's a rerolled patch using the new file names. Go to it testing bot!

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
34.44 KB

Webchick and I both managed to get 0 fails locally. Maybe we'll try the "upload the same patch again" trick to see if test bot likes it more the second time.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review

I suggest you add type hints to the function parameters, i.e. function image_gd_desaturate(stdClass $image) { (see #318016: Type hint array parameters).

In particular, when the signature of an existing function is changing, type hints will make it easier to upgrade existing modules.

drewish’s picture

Status: Needs review » Needs work

Running this locally I'm getting the following failures in the user module:

Upload failed.	Other	user.test	584	UserPictureTestCase->testWithGDinvalidSize()	
File size cited as reason for failure.	Other	user.test	586	UserPictureTestCase->testWithGDinvalidSize()	
File was not uploaded.	Other	user.test	589	UserPictureTestCase->testWithGDinvalidSize()

I'd agree with c960657 that class type hints would be good. What do we think about adding an Image class type?

drewish’s picture

Status: Needs work » Needs review
FileSize
35.96 KB

added stdClass type hints. also fixed the user upload bug.

renamed the methods in ImageTestCase to use camelCase rather than underscores.

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

Status: Needs work » Needs review
FileSize
36.21 KB

chx provided a review on irc where he suggested:
- streamlining image_toolkit_invoke()
- having image_scale() drop it's last three lines that duplicate image_resize() and just call image_resize() directly.
- renaming $image->res to ->resource.
- renaming $open_func and $close_funct to loose the abbreviation.

drewish’s picture

For testing purposed I rebuilt PHP without GD and got the same 3 failures. So it looks like the testbot doesn't have GD compiled in.

quicksketch’s picture

drewish, your three errors are because you didn't update your patch to use the new file names "image-test.jpg/png/gif". If you look at the test results for #16, we get 95 passes but just one (mysterious) fail.

Status: Needs review » Needs work

The last submitted patch failed testing.

tstoeckler’s picture

Attached patch fixes #24.
Go get it, testbot!

EDIT: Crossposted with drewish...

drewish’s picture

Status: Needs work » Needs review
FileSize
38.77 KB

corrected the filenames as quicksketch suggested but the tests still fail without GD so we should make the manipulation tests GD dependent.

i spent some time trying to have this code behave sanely when there's no toolkit. i modifed image_gd_info() to only return gd when image_gd_check_settings() is true. and the gd settings form now handles the case of 0 toolkits. I also cleaned up image_get_toolkit() to make a best effort to return a valid toolkit.

quicksketch’s picture

FileSize
37.77 KB

This version removes the $toolkit parameter from image_toolkit_invoke. Instead we check $image->toolkit if necessary to use a different image toolkit (this can be set by using image_open($filepath, $toolkit)).

Drewish put in an exception to check if GD was available to make the tests pass in #27. Unfortunately what this means is that testing bot doesn't have GD installed, at least on some slaves. Some testing servers must have GD though since #16 passed most of the image tests except for one.

So to keep testing bot from unexpectedly passing when GD isn't available, I've put in a $this->fail() into the Image tests if GD is missing entirely.

drewish’s picture

Quicksketch, it was actually my intent for it to pass if GD isn't installed because the tests should pass on all sites. The user pictures are conditional for the same reason. If I don't want GD and want to use an alternate tool kit I shouldn't be penalized. That said the testbots should have GD as a requirement.

quicksketch’s picture

FileSize
37.72 KB

Okay that's fair enough. The skip is back in this one. However, I can't figure out why test bot isn't running the "Image API" tests at all. In all the above patches that have passed, the "Image API" category is completely missing. I'd think it'd show up even if the internal tests were being skipped.

drewish’s picture

Then I think we might need to do a pass and report that GD tests were skipped because it wasn't installed. I'd noticed that the tests didn't stay selected when 0 assertions were made so it probably folds up empty testcases.

quicksketch’s picture

FileSize
37.84 KB

Adding a $this->pass() for systems that don't have GD to notify that the tests were not run.

quicksketch’s picture

Well, I don't know what the problem is. Testing bot cannot see our new tests. None of the new tests were run in #27, #28, #30 or #32.

quicksketch’s picture

Let me just say, boombatower is my hero for fixing our testing problems. All tests are now run and pass successfully.

boombatower’s picture

NOTE: seems to just have been issue with #12...so I disabled it.

drewish’s picture

Status: Needs review » Needs work
FileSize
52.98 KB

finished cleaning up the changes to image_toolkit_invoke() since we document that we take image objects from image_open() it's safe to assume that the $image->toolkit will always be set. i separated the $image and $params parameters because i thought it looked cleaner. the one trade off what that we couldn't use the invoke on the settings form. oh well.

did some refactoring on the tests. ImageToolkitTestCase is there for testing image.inc and ImageToolkitGdTestCase tests GD specific stuff.

stole some code from the file_test.module and created image_test.module that implements a toolkit. filled out the basics of code to test image.inc's functions. i left some TODOs for other things to flush out, one of the tests is broken but i can't seem to figure out why. i'm sure it'll be obvious to someone who hasn't been staring at this.

in the process of building a toolkit i realized that the data returned by image_TOOLKITNAME_info() and hook_image_toolkits() overlapped enough that it made sense to merge the two. In the process i added an 'unavailable' flag to warn that there's a problem and remove the toolkit from the list returned by image_get_available_toolkits().

one other thing that i'm too tired to figure out the right way to fix: image_scale_and_crop() and image_scale() both seem to have functionality for filling in the height and width from the aspect ratio but they use different code. also they don't have this behaviour documented on the @params.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
51.91 KB

Here's a reroll that tidies up all the changes that drewish made in #37.

i left some TODOs for other things to flush out, one of the tests is broken but i can't seem to figure out why. i'm sure it'll be obvious to someone who hasn't been staring at this.

Fixed all the TODOs. The broken test was just a problem that a variable_get() was called with the wrong string.

in the process of building a toolkit i realized that the data returned by image_TOOLKITNAME_info() and hook_image_toolkits() overlapped enough that it made sense to merge the two.

Awesome. I removed the image_gd_info() function since we don't need it any more.

In the process i added an 'unavailable' flag to warn that there's a problem and remove the toolkit from the list returned by image_get_available_toolkits().

I changed the name of this to "available" and made it required, since it's more clear than trying to specify something like "unavailable == FALSE".

All tests are now 100% passing and working. Andrew if you can take another look we can finish this one off.

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

The testing bot seems to be missing GD. The tests pass locally.

drewish’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
51.8 KB

Fixed a couple of small docs issues I noticed giving this a final once over.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

drewish’s picture

Status: Needs work » Needs review
FileSize
51.79 KB

dries didn't like the open/close so i renamed it to load/save.

Status: Needs review » Needs work

The last submitted patch failed testing.

boombatower’s picture

Aren't there other tests that require GD?

drewish’s picture

boombatower, there are other tests that use GD, but i don't think they fail if GD is not available.

c960657’s picture

I can reproduce one of the failures with PHP 5.2.0 on Debian Etch. The problem is in image.test line 142:

    $this->assertEqual($calls['crop'][0][1], 8, t('X was computed and passed correctly'));

On my box, $calls['crop'][0][1] is 7.5.

I cannot complete the tests due to this error:

Fatal error: Call to undefined function imagerotate() in /home/chsc/www/drupal7/modules/system/image.gd.inc on line 147

According to the PHP manual, “This function is only available if PHP is compiled with the bundled version of the GD library.” I have no idea why this is.

Dries’s picture

The lack of imagerotate seems to be specific to php5-gd on Debian. Details available at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=298061.

Dries’s picture

It sounds reasonable to do a "function exists" in the tests? We'd still be exercising the tests on all other platforms. If we do, make sure to add a code comment as well as a TODO to remind ourselves to remove this hack once this has been fixed in Debian.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
46.87 KB

Why not split out imagerotate into it's own patch? I didn't realize we were going to have differences in GD installations. It's not really part of this "task" anyway since our current implementation doesn't have rotate. Save some kittens right? Here's a patch without rotate.

Dries’s picture

Status: Needs review » Fixed

Looks good. Committed to CVS HEAD. Thanks.

quicksketch’s picture

Woohooooooo! Image rotate: #396074: Add Rotate To Image Actions.

c960657’s picture

imagefilter() is also not available on Debian, so one test is now failing.

alexanderpas’s picture

Category: task » bug
Priority: Normal » Critical
Status: Fixed » Active

Confirming #52: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=321237

note: also true for ubuntu, and others using the non-bundled GD

drewish’s picture

quicksketch... i'm really not sure why you split the rotation out of the patch. our current implementation definitely has rotation: http://api.drupal.org/api/function/image_rotate/6

in the imageapi dopry had worked around debian's bugs by adding a "software" rotation written in PHP.

i think we should just use drupal_function_exists() to test for those functions and report a failure if they don't exist. that would allow contrib to implement those functions if PHP does not.

quicksketch’s picture

FileSize
12.28 KB

Alright, now I've went and caused a ruckus. Drewish is right, we do currently have image_rotate(). So now we've got all the image functions converted to ImageAPI except a single function. Doh.

So our real problem here is that our new implementation of image_gd_rotate() dropped the function_exists() call. Whereas the current implementation still has this.

So this patch moves image_rotate() over to ImageAPI, and adds in two checks: one for imagerotate() and one for imagefilter(), since neither of these functions are available if using non-bundled versions of GD. I used drupal_function_exists() for the checks like drewish suggested, so any ambitious developers can hand-code these functions for GD if their installations are missing them.

quicksketch’s picture

Status: Active » Needs review
FileSize
11.01 KB

A little bit of redundant code slipped into #55.

drewish’s picture

Category: bug » task
Priority: Critical » Normal

Since I'm still viewing this as cleanup on the initial patch I added a bunch of @see tags to link from the toolkit interface functions to the GD implementations since it's usefully to be able to see the code executed.

I also fixed the docs on image_gd_rotate() to match up with image_rotate() and provide more info on the direction of rotation and color formats.

Other than those changes I'd say this code looks great. Once we get these in we can get back to #371374: Add ImageCache UI Core.

drewish’s picture

Category: task » bug
Priority: Normal » Critical
FileSize
12.86 KB

screwed up the status and forgot the patch.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

Looks pristine to me. All GD tests running and passing for servers with the bundled GD2.

c960657 and Alexander Pas, any lingering problems?

drewish’s picture

FileSize
51.79 KB

noticed one more, tiny thing when started a patch to backport this to imageapi. while examining the diffs i just noticed that image_resize() still has a $toolkit parameter on it. dropped that but left everything else unchanged.

drewish’s picture

FileSize
13.03 KB

whoops, old version of the patch.

c960657’s picture

Debian Etch is happy :-) 100 passes, 0 fails, and 0 exceptions

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

quicksketch’s picture

Even more awesome than the original! Thanks again!

drewish’s picture

Status: Fixed » Needs review
FileSize
692 bytes

one more tiny fix that i found when updating the imageapi's imagemagick toolkit to use the new style of toolkits.

c960657’s picture

Looks good to me.

drewish’s picture

Status: Needs review » Reviewed & tested by the community

I'm assuming that c960657 just forgot to change the status ;)

webchick’s picture

Status: Reviewed & tested by the community » Needs review

That's a typo that ought to be showing flaming red errors in our tests and obviously isn't. How hard would it be to add a test for this to cover the gap?

drewish’s picture

webchick, i don't really remember what i had to do to trigger it originally but it was something to do with having multiple toolkits installed and then disabling one. i'm not sure how to do that easily.

webchick’s picture

Status: Needs review » Fixed

Ok, cool. Let's keep our eyes open for holes in our testing suite wrt image handling.

I went to commit this but discovered I accidentally committed it when I rolled back another patch. Oops. :\ Well anyway, fixed. :)

Status: Fixed » Closed (fixed)
Issue tags: -ImageCacheInCore

Automatically closed -- issue fixed for 2 weeks with no activity.