Posted by dokumori on October 15, 2011 at 3:24am
12 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | image system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | dokumori |
| Status: | needs review |
| Issue tags: | image module, Image system, needs backport to D7 |
Issue Summary
Image Toolkit gets the JPEG quality setting from the variables just when the image is saved and there are no hooks to alter this.
The attached patch does the following to make it possible:
- adds drupal_alter() to image_gd_save() to allow other modules like Image Quality module to alter the image quality
- make image_toolkit_invoke() pass $style to custom functions
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| image-override_quality.patch | 3.91 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image-override_quality_0.patch. Unable to apply patch. See the log in the details link for more information. | View details | Re-test |
Comments
#1
Motivation:
For high-traffic websites, it is important to be able to provide quality images while reducing the amount of data transferred by reducing JPEG images' qualty where appropriate. Perhaps the attached patch (and the module that utilises it) is not the best way of executing the idea, but considering the number of high-profile websites running on Drupal, it's a feature that we should consider adding.
#2
#3
This is exactly what i wanted to do before starting the search to check if anybody already thought about that.
A good example/usecase for that are overview pages like this: http://www.texturecase.com/texturen/holz
As far as i can see, adding that to 7.x would not break api (for other modules that use this api). But i am not so advanced with architectural stuff. So whats the opinion of the core developers?
manually applied it and also tested your image quality module. works fine here.
#4
ok this patch breaks, that the correct quality setting is used, from the toolkit page where you set jpeg quality
it changes image.gd.inc line 272
//$success = $function($image->resource, $destination, variable_get('image_jpeg_quality', 75));$quality = variable_get('imageapi_jpeg_quality', 75);
seems this uses an old d6 variable from the imageapi.
correct:
$quality = variable_get('image_jpeg_quality', 75);i fixed the above patch and attached it.
#5
Nope, that's a non-starter.
The image toolkit must not have any knowledge of the Image module and its image styles. The image toolkit API only cares for 1) pluggable toolkits, and 2) performing actions on an image resource.
#6
Let's actually go one step further and not only allow to override it, but also allow to adjust the quality in an Image style.
The latter is only a few lines of code, so I don't see any reason for why that shouldn't be directly in core.
#7
The last submitted patch, drupal8.image-quality-effect.6.patch, failed testing.
#8
@sun: thanks so much for your review, as well as your time and effort in moving it forward.
The patch looks great and makes so much more sense to do it this way rather than what I initially proposed. One very minor improvement I can think of is regarding the quality range - the starting value should be 1 rather than 0, as it should have some kind of quality.
Attaching a re-rolled patch with with the minor change.
#9
#10
Image quality can also be used for PNG, but should then be split in the quality/10 for compression and quality % 10 for filter.
#11
@fietserwin
Thanks for your comment. I'm working on this now.
> quality % 10 for filter
Can you elaborate on this? I was looking at imagepng() and available filters are:
PNG_FILTER_NONEPNG_FILTER_SUB
PNG_FILTER_UP
PNG_FILTER_AVG
PNG_FILTER_PAETH
PNG_ALL_FILTERS
... and it's a bitmask field, so it would not be in %. Or am I not understanding you correctly?
#12
I based my comment on http://www.imagemagick.org/Usage/formats/#png_quality, but I see it is specified different in GD. So if this is going to be cross toolkit, we need to come up with a UI that hides the differences. In my opinion this should be an effect that can be added anywhere in the list to a style and that then overrides the default setting. See e.g. the change image format effect of imagecache_actions that also contains a quality parameter (which is used as is, thus no good UI).
#13
@fietserwin: Thank you for the info. I looked at the ImageMagick documentation and learned the offered options are similar, but not the same so unifying the UI seems rather tricky:
GD
ImageMagick
Having said, at least in D7, ImageMagick module is required to use ImageMagick. So I believe it should not be handled by core, but it would be the contrib module's responsibility to override the form etc. to realise this feature. What do you think?
#14
By the way, as you can see in the amended title, I do think core should handle GD2-based PNG compression. I'll provide an updated patch.
#15
re #13: The need to override forms is tricky. Currently, you are able to change toolkit without having to change the parameters of existing image style effects. So the effects should be geared towards what is possible with images and image formats, not how a specific toolkit implements it.
My idea is that with the default settings (admin/config/media/image-toolkit) we get:
- 1 field for the default jpeg quality (currently existing, 0-100%)
- 1 field for the default png compression (0-9, default 6)
- 1 field for the default png filter (none, sub, up, avg, Paeth, adaptive/all, single valued drop-down value, thus no bitmask, default adaptive)
- The code should not use the GD constants, these may be not available...it may, of course. use the same values so that no lookup conversion is needed for the GD tookit.
The new to create quality effect should offer the same fields.
#16
@fietserwin: Thank you for your suggestions and apologies for the long absence. The PNG compression and filter feature have been built.
Regarding the filter:
- I agree I should not use the GD constants so I used numeric values that are compatible with the bitmasks.
- Since multiple filters can be applied to a given image, I made it a select box rather than a dropdown.
- Also I did not label the highest value option 'all/adaptive', but just 'all'. This is because the option 'adaptive' is only available if you have ImageMagick installed. It is a contrib module and although it is just a label, the core should have no knowledge of specific contrib modules.
Regarding the compression:
After seeing no changes in the image quality while changing the value of $quality parameter in imagepng(), I've RTFMed and learned the value is indeed for compression and not quality (http://www.php.net/manual/en/function.imagepng.php#106093 ). So I will relabel this soon and not worry about converting the compression rate to quality.
Toolkit:
While I wrote this patch, it made me wonder whether these two fields should be included in the GD toolkit. If we are able to set the default JPEG quality in the toolkit admin page, why not PNG settings?
#17
(changing the status)
#18
The last submitted patch, drupal8-image_quality-1310452-16.patch, failed testing.
#19
+++ b/core/modules/image/image.admin.incundefined@@ -453,6 +453,32 @@ function image_effect_scale_validate($element, &$form_state) {
+ if ($value == 8 && $count > 1 || $value == 248 && $count > 1) {
You better add parenthese to express what you mean. I think the expressions is wrong. It can also be simplified, as the science of logic teaches us that (a and c) or (b and c) <=> (a or b) and c
+++ b/core/modules/image/image.admin.incundefined@@ -453,6 +453,32 @@ function image_effect_scale_validate($element, &$form_state) {
+ form_error($element, t("The options 'None' or 'All' can only be applied on its own", array('!name' => $element['#title'])));
For words that need to be translated anyway, I prefer to pass them in as a param. It is better to keep them equal.
+++ b/core/modules/image/image.admin.incundefined@@ -584,6 +610,97 @@ function image_rotate_form($data) {
+ // To make it consistent with the JPEG quality option, this dropdown is for
+ // setting the quality rather than the compression level. The scale of
+ // 1 (lowest) - 10 (highest) is used.
+ $form['png']['compression'] = array(
+ '#type' => 'select',
+ '#title' => t('Quality'),
+ '#default_value' => isset($data['png']['compression']) ? $data['png']['compression'] : 4,
+ '#required' => TRUE,
+ '#options' => $image_quality_options['compression'],
+ '#multiple' => FALSE,
+ '#description' => t('Define the image quality for PNG manipulations. Ranges from 1 to 10. Higher values mean better image quality but bigger files.'),
+ );
This is thus wrong: higher compression means same quality (png is lossless), lower file size but increased generation time.
+++ b/core/modules/image/image.admin.incundefined@@ -849,3 +966,39 @@ function theme_image_rotate_summary($variables) {
+ $summary = t('JPEG: ') . $data['quality'] . '%; ';
JPEG (and further on PNG) are probably not translated in and by themselves. What may be translated though is the word order, so I think that the whole sentence should be 1 translatable string (with a numeric parameter).
In French e.g, you normally add a space before a colon.
+++ b/core/modules/image/image.effects.incundefined@@ -312,3 +320,28 @@ function image_rotate_dimensions(array &$dimensions, array $data) {
+ */
+function image_quality_effect(&$image, $data) {
+ $image->quality = $data['quality'];
+ $image->filter = $data['png']['filter'];
+ $image->png_quality = $data['png']['compression'];
+ return TRUE;
+}
function image_quality_effect contains tabs instead of spaces as do the changes in function image_gd_save.
That's it for now. I didn't look at it as a whole yet, just function by function. Nor about if and how to add tests.
#20
Hello,
is there a patch for Drupal 7? Patchs in this post are for Drupal 8 and 6.
Thank you.
#21
@fietserwin: Thank you for the review. All makes sense. I've made changes as suggested and also made some small changes. My editor is misbehaving (or misconfigured) and I'm having issues with the soft tab settings (i.e. tabs->spaces). I'll remember to double-check before submitting new patches...
@kumkum29: if all goes well, it may be backported to D7, I believe. No patches for D7 at the moment, I'm afraid.
#22
The last submitted patch, drupal8-image_quality-1310452-21.patch, failed testing.
#23
This one passed all the tests locally
#24
The last submitted patch, drupal8-image_quality-1310452-23.patch.patch, failed testing.
#25
#23: drupal8-image_quality-1310452-23.patch.patch queued for re-testing.
#26
The last submitted patch, drupal8-image_quality-1310452-23.patch.patch, failed testing.
#27
For the record:
The patch #23 passes the tests locally, but fails on qa.drupal.org. The test in question is Drupal\system\Tests\Upgrade\BareMinimalUpgradePathTest
I overheard chx and beejeebus talking about this issue, which is quite relevant to this one
https://drupal.org/node/1893800
I'll try retesting some more and see if it passes.
#28
#23: drupal8-image_quality-1310452-23.patch.patch queued for re-testing.