Hello,

I'm working on a project in which JPEG 2000 images are stored as Image Fields, and I'd like to be able to use the core image thumbnailing. However, these thumbnail images need to be in some format actually understood by browsers, i.e. not JPEG 2000, so the format needs to be changed when saving the thumbnail images. At the moment, I'm not concerned with renaming the thumbnail images, just altering their format.

The ImageCache Actions module has a submodule called coloractions which currently contains an effect to alter the format of an image, but it needs a change to the ImageMagick module to work. The attached patch implements these changes, and is working for me, and does not seem to break other effects. The coloraction_convert effect claims that this must be the final effect in the list of effects in a style for it to work, but this patch makes it possible for the effect to be anywhere in the list.

Note that this patch relies on the assumption that there are no other elements in $args (that is, no $image->ops) that end in a colon. My reading of the documentation for my version of the 'convert' command suggests that there aren't any options that end in a colon, so I'm guessing this assumption is safe.

Adding key-value pairs to $image->ops instead of just pushing them onto the array seems to be against conventions, but if it's OK, I'd prefer to do this by changing the image_imagemagick_convert_image function in Imagecache Actions to add a special 'output_format' key to $image->ops with a value of the file format to convert into, then looking for that key in _imagemagick_convert. If that's acceptable, let me know and I'll alter this patch and submit a patch to ImageCache Actions to cover that side.

The same effect could also be done using the drupal_alter hooks in image_imagemagick_save or _imagemagick_convert if the values in $context that are possibly changed were used later in those functions. If that's preferred, let me know and I'll write a patch to do that.

Any input on how to handle this better would be greatly appreciated!

Thanks to fietserwin for the original implementation of this patch #1046084: imagemagick toolkit: allow imagecache_action: convert format, and thanks for the ImageMagick module!

Comments

sun’s picture

Status: Active » Needs work

A magic colon is not really clean. Do you have any other approaches or ideas?

I can only guess we need to use a non-standard ->ops value or extension.

+++ b/imagemagick.module
@@ -372,6 +372,15 @@ function _imagemagick_convert($source, $dest, $args) {
   drupal_alter('imagemagick_arguments', $args, $context);

I also wonder whether such kind of an adjustment shouldn't be done via hook_imagemagick_arguments_alter() ?

gdl’s picture

> A magic colon is not really clean. Do you have any other approaches or ideas?

I certainly agree that this is not a pretty implementation, but unfortunately, I don't know that there are many clean ways to get image format conversion available, and useful (to me, at least!).

At the risk of being pedantic, the imagemagick 'convert' command-line tool has two mechanisms for converting between formats[1]. One is to use a different extension for the output file name, e.g. "convert something.jpg something.png", the other to use an image format name followed by a colon in front of the output file name, e.g. "convert something.jpg png:something.jpg" to create a png image that happens to have the file extension ".jpg". There does not appear to be a "-convert mime-type" output option like there are for the majority of other image manipulations.

My reading of the Image module, particularly regarding caching, suggests that it relies on the assumption that cached images will have the same filename and extension as the original, so using the first method described above and altering the extension in the destination would convert the images from one type to another. But it would mean giving up some part of the Image module's functionality, particularly the part I'm most interested in! So I'd prefer not to implment it that way.

If the main objection is that the patch relies on the colon being in a particular place in the $operation string, instead of using a "format:" option, I could use a specific key->value pair in the $ops array along the lines of "output_format" => 'format' and alter the destination based on that particular key. But that would break the apparent assumption that the $ops array is a numerically indexed array.

For the drupal_alter hooks to be useful for image format conversion via either method described above, the _imagemagick_convert (or possibly image_imagemagick_save) functions would still need to be patched to use the possibly altered value of $context->destination instead of $destination after the drupal_alter calls.

In any event, I can't see a way to alter image formats using the "convert" command without making changes in the ImageMagick module itself. After writing all this out, my preferred way to do this is to patch image_imagemagick_save to use $context->destination instead of $destination after the drupal_alter, and whatever module implements the "convert format" effect could then implement hook_imagemagick_save to alter the destination and $image->ops array. That makes the changes to the ImageMagick module itself as small as possible. If that sounds good, let me know, and I'll write a patch.

I can think of more convoluted ways as well, if this one isn't ugly enough! :)

Cheers,
-G

[1] To definitely be pedantic, here's the relevant section of the "convert" version 6.6.0-4 manpage from my Debian Squeeze machine:

"By default, the image format of `file' is determined by its magic number. To specify a particular image format, precede the filename with an image format name and a colon (i.e. ps:image) or specify the image type as the filename suffix (i.e. image.ps). Specify 'file' as '-' for standard input or output."

sun’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

For the drupal_alter hooks to be useful for image format conversion via either method described above, the _imagemagick_convert (or possibly image_imagemagick_save) functions would still need to be patched to use the possibly altered value of $context->destination instead of $destination after the drupal_alter calls.

I was under the impression that this is already possible and especially the 'destination' context variable would be used.

Attached patch allows that. However, we need to resolve the @todo I've added.

sun’s picture

+++ b/imagemagick.module
@@ -359,28 +359,35 @@ function image_imagemagick_get_info(stdClass $image) {
+    'destination_original' => $destination_original,
...
-  return file_exists($dest);
+  // @todo Fails if $destination is altered into e.g. png:myimage.jpg :(
+  return file_exists($destination);

We could test for $destination_original instead. However, that would prevent people from using ImageMagick as a pure API module.

An alternative would be to add yet another 'destination_filename' variable to $context that would allow developers to change 'destination', but leave 'destination_filename' intact, or in case they are purposively changing the filename, they'd also have to change 'destination_filename' to make the file_exists() test pass.

gdl’s picture

The additional destination_filename variable you describe would work for me.

Alternately, something more specific to image conversion would be a 'destination_format' variable in $context which, if it exists, could be prepended with a colon to $destination in the escapeshellarg() call.

Best,
-G

sun’s picture

If there is only the [extension]:[filename] syntax for the format conversion (without further edge-cases), then a 'destination_format' or just simply 'format' would work. This would have to be documented. Do you have ImageMagick API docs pointers for this?

gdl’s picture

> Do you have ImageMagick API docs pointers for this?

I'm not sure.

I'm aware of the imagemagick.api.php file, where I think the alter hooks are supposed to be documented, but I'm not familiar with any documentation specific to writing ImageMagic specific API, uh, documentation. Pointers would be appreciated!

Thanks,
-G

sun’s picture

Sorry, I meant in the official vendor API docs of ImageMagick. I didn't know of the extension: prefix for format conversion.

gdl’s picture

I believe the manpage I quoted earlier is official documentation. At least, it doesn't have Debian's boilerplate about the manpage being created specifically for Debian because the upstream didn't have one. That said, the man page also points to what appears to be the official documentation for the convert command, which says nothing about the "{format}:" syntax for image format conversion. I did manage to track down a mention of this syntax on the "Usage Examples" page, which appears to be as good as it gets.

sun’s picture

Status: Needs review » Needs work

In that case, I'd say let's go with a new 'destination_format' parameter in $context, defaulting to an empty string, and if filled in by an alter hook with an extension, the extension, plus a colon, is prefixed to the $destination variable.

That way, an alter hook should be able to mimic and use the entire image save possibilities of ImageMagick by adjusting $context, even including the "ext:-" variant.

The code as well as the alter hook API phpDoc should contain pointers to http://www.imagemagick.org/Usage/files/#save

gdl’s picture

Here's a first pass at implementing this as described in #10, based on the the patch in #3. It is working for my case, converting JPEG-2000 images to other formats without renaming them for use as thumbnails, and appears to work in styles of four or five image effects, but I didn't test it much more than that. I ran the tests for the Image module with this patch applied, but there were errors. Running those tests without this patch applied gave the same test failures, so that doesn't appear to mean much.

In order to work around the @todo in #3, I added a function to prepend the format to the destination inside imagemagick.module itself. I'm curious if it might not be better to make the implementations of hook_imagemagick_arguments responsible for doing the prepending of the image format to the destination and returning the "format:destination_path" as the 'destination_format' of $context so a simpler test that doesn't muck with the $destination could be used within ImageMagick.

Or, to frame it as a more general question, should the implementation of a hook should be responsible for making sure altered values are sane before returning them, or is the module that invokes the hook responsible? That is, multiple implementations of hook_imagemagick_arguments might leave the 'destination_format' in an invalid state, say, with multiple image formats prepended to it. Should there be a test in ImageMagick for the validity of the destination, or should it be up to each implementation of a hook to be sure the destination it returns is valid?

Thanks for all your help and direction!
-G

sun’s picture

wow! Amazing!!! This patch is of excellent quality! :)

Seriously, where have you been all the time? I'd totally like to see more patches from you! :)

I'm curious if it might not be better to make the implementations of hook_imagemagick_arguments responsible for doing the prepending of the image format to the destination and returning the "format:destination_path" as the 'destination_format' of $context so a simpler test that doesn't muck with the $destination could be used within ImageMagick.

That wouldn't be a good idea. If you happen to know hook_form_alter() in D6, and if you ever tried to add a CSS class to a form element, or even better, if you ever tried to adjust the query string in $form_state['redirect'], then you basically know the problem: You've no effin' idea of whether the value is already in the format you're trying to alter it to, or whether a previous alter hook changed it into the format already.

In other words: If alter hook #1 changes 'destination' into "png:file.jpg", then alter hook #2 would change it to "jpg:png:file.jpg", unless you force all alter hooks to implement whack code to check what's actually contained in 'destination' first.

Or, to frame it as a more general question, should the implementation of a hook should be responsible for making sure altered values are sane before returning them, or is the module that invokes the hook responsible? [...] Should there be a test in ImageMagick for the validity of the destination, or should it be up to each implementation of a hook to be sure the destination it returns is valid?

That's a slightly different question than the above. In almost all cases, the common design pattern in Drupal is that alter hooks (actually any kind of function or callback) is expected to return valid data. If we'd introduce validation of returned values, then Drupal's codebase would likely be twice as large as it is today.

There are only very few exceptions to this in Drupal core, where super-essential data is affected. Don't have examples right now. But in general: No, the code that implements a hook is responsible for returning valid data.

So...

given your excellent patch, I only have very minor remarks:

+++ b/imagemagick.api.php
@@ -2,6 +2,44 @@
+function hook_imagemagick_load_alter(stdClass $image) {
...
+function hook_imagemagick_save_alter(stdClass $image, $context = array()) {

Yay! Thanks for those! Didn't know that the .api.php was still empty ;)

+++ b/imagemagick.api.php
@@ -2,6 +2,44 @@
+ *   - source_original: A string file URI of the original image.
+ *   - destination_original: A string file URI of the derivative image.
+ *   - source: A string filesystem path of the original image.
+ *   - destination: A string filesystem path of the derivative image.
+ *   - destination_format: A string format of the derivative image.

+++ b/imagemagick.module
@@ -490,6 +499,29 @@ function _imagemagick_convert_exec($command_args, &$output = NULL, &$error = NUL
+ * Destination paths must be prepended with a format in order for 'convert'
+ * to write the destination file in a different format than the source file.
+ *
+ * @see http://www.imagemagick.org/script/command-line-processing.php#output
+ * @see http://www.imagemagick.org/Usage/files/#save

For better readability, the context parameters should be ordered as in the actual PHP code; i.e., "source*" and "destination*" grouped together.

Also, the essential pointers and documentation about the 'destination_format' context parameter should be contained in the phpDoc of the alter hook. Developers expect to find this info in the API documentation in .api.php, not somewhere deeply hidden in the .module file ;)

+++ b/imagemagick.api.php
@@ -2,6 +2,44 @@
\ No newline at end of file

This shouldn't happen. ;)

+++ b/imagemagick.module
@@ -359,28 +359,37 @@ function image_imagemagick_get_info(stdClass $image) {
+  $command = escapeshellarg($source) . ' ' . implode(' ', $args) . ' ' . escapeshellarg(_imagemagick_destination($destination, $context['destination_format']));

@@ -490,6 +499,29 @@ function _imagemagick_convert_exec($command_args, &$output = NULL, &$error = NUL
+function _imagemagick_destination($destination, $format = NULL) {

Don't think we need a separate helper function for that. Let's do this directly within _imagemagick_convert().

gdl’s picture

wow! Amazing!!! This patch is of excellent quality! :)

Seriously, where have you been all the time? I'd totally like to see more patches from you! :)

Thanks! I try to write the kind of patches I'd want to get! And thanks for your help in making it a good patch!

Yay! Thanks for those! Didn't know that the .api.php was still empty ;)

You're quite welcome!

Also, the essential pointers and documentation about the 'destination_format' context parameter should be contained in the phpDoc of the alter hook. Developers expect to find this info in the API documentation in .api.php, not somewhere deeply hidden in the .module file ;)

I moved all of the documentation pointers to the end of the function documentation header, as suggested in the Doxygen docs. I also added these to the hooks group.

This shouldn't happen. ;)

D'oh. I know better than that.

Don't think we need a separate helper function for that. Let's do this directly within _imagemagick_convert().

Since we rely on $destination being a real path at the end of the _imagemagick_convert(), I don't want to muck with it and then have to unmuck with it just before the return, hence the helper function. Here's another attempt, but I'm not sure if it's overkill since we're assuming that $destination_format is valid when it comes out of drupal_alter, or if it's cool to repurpose the $destination_format variable like I'm doing.

sun’s picture

Title: Enable support for existing coloractions_convert_image effect » Add support for changing destination image format
Status: Needs work » Fixed
FileSize
4.5 KB

Thanks for reporting, reviewing, and testing! Committed attached patch. (a couple of adjustments)

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

gdl’s picture

Great! Thanks for all your feedback and help!

mgmhunt’s picture

Priority: Normal » Minor

Hi, I'm gathering from the above 'fixed' that we can now determine the output format (and file extension?) of an image processed with ImageMagick. How do I implement this? I'm using the module Raw ImageMagick to insert command options and currently trying "-format png" to convert JPG to PNG (so I can use transparency on uploaded images). How can I get this to work? The outputted files are still JPG and have no transparency. Thanks.

I'm using the latest Dev 7 version.. or will this only be 'officially' released?

sun’s picture

Priority: Minor » Normal

@mgmhunt: Is it possible that you're mistaken about the purpose of the -format option? According to the details, it's supposed to be used with identify, not convert.

mgmhunt’s picture

Yep very possible. The convert documentation states "The convert command recognizes these options. "... and then lists "-format string output formatted image characteristics" as an option. Which then leads to "%o output filename" or "%e filename extension or suffix". Which appears to only allow the printing of these attributes.

The ImageMagick documentation gives the example of "convert rose.jpg rose.png" as the method to convert the image format. But how can I do this with the Raw ImageMagick module? ie as a command line. As it seems to indicate ImageMagick uses the extension of the output file name to determine this. (which the module specifies not to do "Do not add an input or output file.")

sun’s picture

+++ b/imagemagick.module
@@ -359,28 +359,47 @@ function image_imagemagick_get_info(stdClass $image) {

@see http://www.imagemagick.org/script/command-line-processing.php#output
@see hook_imagemagick_arguments_alter()

mgmhunt’s picture

Thanks for the pointer. I'm still not sure what I need to type into the Raw ImageMagick module to effect the variables at "$destination_format .= ':' . $destination;". As this is called through multiple image styles, it must concat the arguments of all effects and then send them to this function. Would an argument of *.png in the Raw IM module coming last be OK? How do I reference the $destination_format via the effectmodule? Or is this outside the scope of the project and I should either override this programmatically or go to the Raw IM module forum?

sun’s picture

Image style effects cannot access or alter the command arguments directly. They need to implement hook_imagemagick_arguments_alter() and (smartly) figure out whether they need to adjust the 'destination_format' in the alter hook.

Ideally, the IM Raw module project maintainers should be made aware of this issue to implement the proper logic into that module. So, yes, ideally post to the IM Raw module's issue queue (but search for existing issues first).

mgmhunt’s picture

Thanks for your time. I'll progress this one on the other side! Cheers.

Addendum:

Have found a bit more info here http://drupal.org/node/628146#comment-4740296 "Some versions of Firefox and Safari do not view images whose format has been converted with the "Change File Format" action properly." and here http://drupal.org/node/219261#comment-4583488 "This is something that's come up in the im_raw issue a couple of times too, people not understanding why they can't 'change' format - they most likely have, but didn't see a changed file-extension."

So I guess there are issue with the Image Cache area...which is core in Drupal 7. Couldn't see anything in Drupal 7 image issue queue. http://drupal.org/project/issues/drupal?text=&status=Open&priorities=All...

gdl’s picture

As an update for those looking for an implementation of image format changing, I submitted a patch to the Color Actions module since there was an existing stub function for it there: #1235966: Implement image_imagemagick_convert_image

Status: Fixed » Closed (fixed)

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

kumkum29’s picture

Hello Sun,

i have installed the dev version of Imagemagick. I am looking for a solution to convert my files in .jpg format.
Apparently, i need to create a hook in my template.php.

I have created this hook :
function hook_imagemagick_arguments_alter($args, $context = array()) {
$context['jpg'];
}
or
function hook_imagemagick_arguments_alter($args, $context = array()) {
$context['.jpg'];
}

But, it's not functionnal.
Can you give an example of a hook with jpg format ?

Thanks a lot.