Handle transparncy correctly for watermark #11060
Conversation
magento-team
force-pushed
the
2.2-develop
branch
from
0c0393d to
e442de5
Compare
The watermark functionality uses imagecopymerge for copying the watermark into the image, this function loses the alpha information. Therefor use imagecopy for 100% opacity and use imagefilter in other cases. Resolves: magento#10661
elzekool
force-pushed
the
bugfix/handle-alpha-transparency-correctly-for-watermark
branch
from
9f1fe15 to
92f24ec
Compare
| @@ -465,7 +465,7 @@ public function watermark($imagePath, $positionX = 0, $positionY = 0, $opacity = | |||
| } elseif ($this->getWatermarkPosition() == self::POSITION_CENTER) { | |||
| $positionX = $this->_imageSrcWidth / 2 - imagesx($watermark) / 2; | |||
| $positionY = $this->_imageSrcHeight / 2 - imagesy($watermark) / 2; | |||
| imagecopymerge( | |||
| $this->imagecopymergeWithAlphaFix( | |||
There was a problem hiding this comment.
I believe the proper name for this function should be something like copyImage as we are not even using imagecopymerge anymore.
| * | ||
| * @return bool | ||
| */ | ||
| private function imagecopymergeWithAlphaFix( |
There was a problem hiding this comment.
Please just mention in PHPDoc of this method that signature matches http://php.net/manual/en/function.imagecopymerge.php function but name arguments properly: $destinationImage, $sourceImage, $destinationX, etc.
|
|
||
| $sizeX = imagesx($src_im); | ||
| $sizeY = imagesy($src_im); | ||
| if (false === $sizeX || false === $sizeY) { |
There was a problem hiding this comment.
Please convert all Yoda-style conditions to normal ones.
Is checking for false really necessary here - do we need to continue execution flow when $sizeX === 0?
|
Thanks, fix looks quite good in general, especially that you changed all the places. Probably we should even add Would be really nice to cover fix with integration test using mentioned image. Please tell us if you need any assistance with that. |
|
Have checked manually this PR on my env - works perfectly, thanx. Does image magick also requires changes? |
|
Hi @elzekool, are you going to continue work on this PR? |
|
@Koc no as problematic function As you already tested this PR, maybe you will be interested in fixing CR notes so that it can be merged? You just need to checkout it locally as described in https://help.github.com/articles/checking-out-pull-requests-locally/, commit your changes on top and create a new PR. With merge of new PR they both will be marked as Merged. |
magento-engcom-team
added
2.2.x
bugfix
Reproduced on 2.1.x
|
Closing due to inactivity. Feel free to reach me out anytime later if you wish to continue work on pull request and it will be reopened. |
|
Hi, sorry for the late response. Been realy busy, want to pick it up again |
…alpha-transparency-correctly-for-watermark
When adding a watermark that already has an alpha channel this was reset because the overal opacity was set for the image. Instead we should multiply the original alpha with the requested opacity
Added integration tests for handling alpha transparency in watermarks. Renamed the current test for watermarks as it only tests if the watermark is correctly places
|
@elzekool no problem, please go ahead 👍 |
magento-engcom-team
added
the
Reproduced on 2.3.x
Handle alpha transparency correctly for 24-bit PNG image
Description
The watermark functionality uses imagecopymerge for copying the
watermark into the image, this function loses the alpha information.
Therefor use imagecopy for 100% opacity and use imagefilter in other
cases.
Fixed Issues (if relevant)
Manual testing scenarios
Contribution checklist