Problem/Motivation

This should be more or less straight port of similar effect from D7 version of ImageCache actions module. Additionally it should be build for ImageMagick, too.

It should allow us to adjust the contrast of an image in the range -100% to +100%.

Proposed resolution

Provide patch and test coverage.

Comments

hctom created an issue. See original summary.

hctom’s picture

Assigned: hctom » Unassigned
Status: Fixed » Needs review
Parent issue: #2651216: Add "Brightness" image effect »
StatusFileSize
new12.25 KB

And here is the patch... cross your fingers that the color values in the test are valid ;)

Status: Needs review » Needs work

The last submitted patch, 2: add_contrast_image-2659676-2.patch, failed testing.

mondrake’s picture

+++ b/README.md
@@ -28,6 +28,7 @@ Effect name      | Description
+Contrast         | Supports changing contrast settings of an image. Also supports negative values.              | X          | X                   |

Let's keep alphabetical sort here. Then it should be one line below.

+++ b/src/Plugin/ImageEffect/ContrastImageEffect.php
@@ -0,0 +1,79 @@
+      '#field_prefix' => ' ' . t('±'),
+      '#field_suffix' => ' ' . t('%'),

$this->t()... actually this is wrong in the brightness effect too.

+++ b/src/Plugin/ImageToolkit/Operation/imagemagick/Contrast.php
@@ -0,0 +1,39 @@
+      $this->getToolkit()->addArgument('-brightness-contrast ' . $this->getToolkit()->escapeShellArg('0x' . $arguments['level']));

'0x' - Maybe this is expecting an hex level?

hctom’s picture

Status: Needs work » Needs review
StatusFileSize
new12.98 KB
new2.71 KB

Here is a new patch with the following additional changes:

* Alphabetically sorted list of effects in README.md
* Changed t() to $this->t() (also for Brightness effect - I know this is out of scope of this issue, but let's get that fixed fast, hehe)

@mondrake: The '0x' you mentioned is required for the correct value of the ImageMagick command, as it represents the brightness change (0 = none). You can find more information at: http://www.imagemagick.org/script/command-line-options.php#brightness-co...

Unfortunately the test will fail again, because of the wrong test color results and I am working on that. But unfortunately I can't get my test to work on my local machine, bacause of strange errors thrown in the ImageEffectsTestBase class. So perhaps there will be another ticket fixing this before I can examine my tests locally ;)

Fail      Completion ImageEffectsContr   30 Drupal\image_effects\Tests\ImageEff
    The test did not complete due to a fatal error.
Fail      Other      ImageEffectsTestB   44 Drupal\image_effects\Tests\ImageEff
    Unable to install modules image, image_effects, simpletest due to missing
    modules image_effects.
Fail      Role       ImageEffectsTestB   63 Drupal\image_effects\Tests\ImageEff
    Invalid permission administer image styles.
Exception Recoverabl WebTestBase.php    600 Drupal\simpletest\WebTestBase->drup
    Argument 1 passed to Drupal\simpletest\WebTestBase::drupalLogin() must
    implement interface Drupal\Core\Session\AccountInterface, boolean given,
    called in ImageEffectsTestBase.php
    on line 64 and definedDrupal\simpletest\WebTestBase->drupalLogin()
    Drupal\image_effects\Tests\ImageEffectsTestBase->setUp()
    Drupal\image_effects\Tests\ImageEffectsContrastTest->setUp()
    Drupal\simpletest\TestBase->run(Array)
    simpletest_script_run_one_test('82',
    'Drupal\image_effects\Tests\ImageEffectsContrastTest')
hctom’s picture

Whoops, I hid the wrong file!

mondrake’s picture

1.

+++ b/src/Plugin/ImageEffect/ContrastImageEffect.php
@@ -0,0 +1,79 @@
+      '#field_prefix' => ' ' . $this->t('±'),
+      '#field_suffix' => ' ' . $this->t('%'),

is there a reason for the blank space before $this-t ?

2.

The '0x' you mentioned is required for the correct value of the ImageMagick command, as it represents the brightness change (0 = none).

Thank you, clear.

3. Just in case you find issues with color results being different in GD and ImageMagick, also consider reviewing the patch in #2651960: GD watermark operation loses watermark image alpha , that one has a method to check colors 'closeness', not just absolute equality

EDIT - sorry, xpost, it looks like the patch files should be uploaded again :(

hctom’s picture

Issue summary: View changes

Fixed typo in issue description!

hctom’s picture

StatusFileSize
new12.96 KB
new2.69 KB

Here are the updated patches (unfortunately still with failing test, but I will have a look at that now) for reference.

@mondrake: I really don't know why there were these whitespaces ;) I removed them in both contrast and image effect now.

Status: Needs review » Needs work

The last submitted patch, 9: add_contrast_image-2659676-9.patch, failed testing.

hctom’s picture

Status: Needs work » Needs review
StatusFileSize
new12.96 KB
new1.4 KB

Just came back from the new Star Wars movie and I hope the force is strong with me right now ;) So let's give it another try!!!

Here is a patch with adjusted test color values. And for anybody interested in the local test problems mentioned in #5: It is not possible to test a module installed in an install profile folder ;) So it has to reside in docroot/modules otherwise it won't be found, because simpletest uses the testing profile during test runs.

hctom’s picture

StatusFileSize
new13.58 KB
new2.54 KB

So... and finally here is another new patch with adjusted test color values for ImageMagick toolkit.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Tested manually and automatic tests for ImageMagick pass locally. The color differences between GD and ImageMagick may be addressed by using the 'colorsAreClose' method being introduced in #2651960: GD watermark operation loses watermark image alpha , but that could be a follow-up.

RTBC

hctom’s picture

Cool, thanx! ;)

I guess the different color values need to be addressed as is, because both toolkits handle contrast changes completely different. ImageMagick even has another contrast method -sigmoidal-contrast (http://www.imagemagick.org/script/command-line-options.php#sigmoidal-con...), which increases the contrast without saturating highlights or shadows.

mondrake’s picture

Yep, we are warning that different toolkits may give different results on the same effect. Here it would be rather a possibility to 'lax' the color equality check in the tests, and avoid hardcoding results by toolkit. Let's discuss in a follow-up if that makes sense.

slashrsm’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

  • slashrsm committed 21fd34f on authored by hctom
    Issue #2659676 by hctom, mondrake: Add "Contrast" image effect
    

Status: Fixed » Closed (fixed)

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