Update Image_lib.php #3106

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

Only wm_x_transp and wm_y_transp greater than 0,function "imagecolortransparent" with execution.
Because in wm_type = overlay, wm_x_transp = 0, wm_y_transp = 0, the image have partially transparent.

@linauror linauror Update Image_lib.php
Only wm_x_transp and wm_y_transp greater than 0,function "imagecolortransparent" with execution.
Because in wm_type = overlay, wm_x_transp = 0, wm_y_transp = 0, the image have partially transparent.
d327717
Contributor

narfbg commented Jun 16, 2014

I'm sorry, but this description is barely understandable. Can you elaborate please?

See the following section.

$imga
a

$imgb
b

php code
d

public function index()
{   
    $this->load->library('image_lib');
    $config['image_library'] = 'gd2';
    $config['source_image'] = FCPATH . 'a.jpg';
    $config['wm_type'] = 'overlay';
    $config['wm_overlay_path'] = FCPATH . 'b.jpg';
    $config['wm_vrt_alignment'] = 'top';
    $config['wm_hor_alignment'] = 'left';
    $config['wm_vrt_offset'] = 0;
    $config['wm_hor_offset'] = 0;
    $config['wm_opacity'] = 100;
    $config['quality'] = 100;
    $config['wm_x_transp'] = 0;
    $config['wm_y_transp'] = 0;
    $config['dynamic_output'] = TRUE;
    $this->image_lib->initialize($config);
    $this->image_lib->watermark();     
}

result
c

If i set $config['wm_x_transp'] = 0 and $config['wm_y_transp'] = 0, because i dont want it be translucent, but it still translucent.

Contributor

narfbg commented Jun 16, 2014

I still don't get what you mean, but regardless - what if I want to specify the 0x0 pixcell via those settings?

I'm sorry,it's beacuse the function "imagecolorat".
imagecolorat($img, 0, 0) === imagecolorat($img, 1, 1).
it's get 1x1 pixcell color when i want get 0x0 pixcell color.

Contributor

GDmac commented Jun 26, 2014

ok, i tested a bit, and get what is happening. The overlay_watermark() method _always_ uses transparancy. Either it uses the alpha info from the image or it will pick the color at the specified wm_x_transp / wm_y_transp coordinate.

You want to just overlay the image, so, you can:

  • Extend the library to MY_Image_lib.php and create a overlay_image() method and use that.
  • Use imagecopymerge() in the controller/model instead of using the image lib.
  • Best: Create a pull request to add a overlay_image() method :-)
Contributor

GDmac commented Jun 26, 2014

Ok, i did find an issue with image_lib. When supplying a 8-bit png or a gif, the transparancy info from the image itself is never used, you always have to specify the coordinates (not want).

  • The 8-bit png is img_type 3, but the image is not 24-bit so the rgba check returns alpha=0
  • The GIF is img_type 1 (so, no alphablend), and rgba 24-bit check returns alpha=0

I made a quick testversion where alpha is simply always assumed for gif/png
if ($wm_img_type === 3 || $wm_img_type === 1 && function_exists('imagealphablending'))...
This brings up a backwards compatibility issue, which i think might not be a problem. where i would like your thoughts please.

  • In the current library you always have to specify the coordinates of the pixel to use for the transparancy color. In my opinion this was never very useful, especially since png/gif (can) have transparancy info in the image itself.
  • If 100% backwards compatibility is very important and a must, then i propose a new config setting wm_transparant = true/false (default true). When set to true (default) the old behaviour is used, when set to false, no transparancy _settings_ will be used, the image will be straight overlaid, using only transparancy info inside the image (if present).

thoughts?

Contributor

narfbg commented Jun 27, 2014

Image manipulation is far from my area of expertise and I don't get anything of what the OP is saying, so I've basically given up on this one. If you feel something should be changed - go for it, and of course - be descriptive. :)

Edit:

Forgot about the BC question ... a new setting, as long as it's not a mandatory choice, is always better than a BC break.

Contributor

GDmac commented Jun 27, 2014

I think the image lib needs more work than i can handle at the moment. e.g. rotate image looses transparancy on a 24bit PNG. And the whole lib feels a bit awkward when you want to rotate() and resize() because for every action you have to clear() and initialize(), and every time the image is read and written from/to file.

@linauror with a small change your fix is the quickest solution. (comment in code)

@GDmac GDmac commented on an outdated diff Jun 27, 2014

system/libraries/Image_lib.php
@@ -1194,7 +1194,7 @@ public function overlay_watermark()
else
{
// set our RGB value from above to be transparent and merge the images with the specified opacity
- imagecolortransparent($wm_img, imagecolorat($wm_img, $this->wm_x_transp, $this->wm_y_transp));
+ $this->wm_x_transp && $this->wm_y_transp && imagecolortransparent($wm_img, imagecolorat($wm_img, $this->wm_x_transp, $this->wm_y_transp));
@GDmac

GDmac Jun 27, 2014

Contributor

If you change the test to FALSE than you can have a simple way to ignore transparancy.

if($this->wm_x_transp !== FALSE) imagecolortransparent($wm_img, imagecolorat($wm_img, $this->wm_x_transp, $this->wm_y_transp));
Contributor

narfbg commented Jun 27, 2014

I'm not accepting this pull request, in its current form, even with your suggested change. For one it doesn't match the styleguide, and as I already said - I've got no clue what this is about.

Contributor

GDmac commented Jun 27, 2014

It is about not being able to overlay an image without using transparancy.
my solution is to be able to just set wm_x_transp to false and then
the image_lib won't do imagecolortransparent().

Might as well close this issue then.

Contributor

narfbg commented Jun 27, 2014

Well, I said "in its current form" ... it might be improved.

Contributor

ivantcholakov commented Jun 28, 2014

The properties $wm_x_transp and $wm_y_transp are not described well at the beginning of the library, this causes difficulty for understanding and should be corrected:

    /**
     * Default transparency for watermark
     *
     * @var int
     */
    public $wm_x_transp     = 4;

    /**
     * Default transparency for watermark
     *
     * @var int
     */
    public $wm_y_transp     = 4;

They are the coordinates of the watermark's pixel, which color is to be set as transparent within the watermark before merging with the source image. This is my interpretation.

I agree with GDmac that: if transparency feature within the watermark is to be disabled (before merging with the source image), then FALSE value for $this->wm_x_transp may be used. Also, if you wish, $this->wm_y_transp could be made sensitive to FALSE in the same way.

Contributor

GDmac commented Jun 29, 2014

Also the opacity setting doesn't work, because of a bug in PHP/GD (alpha images don't retain alpha info when copymerged with opacity). The work-around is quite simple, by first merging the watermark image with the background image to retain alpha, and then copymerge that resulting image with the background again with opacity value. (see imagecopymerge_alpha() function in the comments on the PHP page)

More important, as mentioned, merging 8bit PNG or GIF and keeping transparancy info can't be done. With this library, you must specify a pixel-coordinate to pick a color that will be used as transparancy-color. Also, you loose transparancy when rotating a image. I wonder who actually uses the lib beyond simple thumbnail generation or a resize...

TL:RD;
In my opinion, the current image library is broken beyond repair.
Any fix to the lib (transparancy, rotation) will break backward functionality,
Any functionality change (method chaining and save method) breaks backwards compatibility.

Thoughts?

Contributor

ivantcholakov commented Jun 29, 2014

So, let us not touch anything now?

Contributor

GDmac commented Jun 29, 2014

Well, the original PR could be changed a bit to accept FALSE for wm_x_trans or wm_y_transp.
For anything else we'ld need green light to rethink and redo the image library.

@ivantcholakov ivantcholakov added a commit to ivantcholakov/starter-public-edition-3 that referenced this pull request Jun 30, 2014

@ivantcholakov ivantcholakov Image_lib: A possible patch for CodeIgniter, see bcit-ci/CodeIgniter#… 13d3ec7
Contributor

ivantcholakov commented Jun 30, 2014

Within a project of mine I made the possible patch on this topic, see the reference/link above. I tested how watermarking works with the provided images a.jpg and b.jpg. As a result of this patch, the image b.jpg covers the background image a.jpg entirely (which is the desired result). Here is code for testing:

<?php defined('BASEPATH') OR exit('No direct script access allowed');

class Img_test extends CI_Controller
{

    public function __construct()
    {
        parent::__construct();
    }

    public function index()
    {
        $this->load->library('image_lib');

        $config['image_library'] = 'gd2';
        $config['source_image'] = FCPATH.'a.jpg';
        $config['wm_type'] = 'overlay';
        $config['wm_overlay_path'] = FCPATH.'b.jpg';
        $config['wm_vrt_alignment'] = 'top';
        $config['wm_hor_alignment'] = 'left';
        $config['wm_vrt_offset'] = 0;
        $config['wm_hor_offset'] = 0;
        $config['wm_opacity'] = 100;
        $config['quality'] = 100;
        $config['wm_x_transp'] = FALSE;
        $config['dynamic_output'] = TRUE;

        $this->image_lib->initialize($config);
        $this->image_lib->watermark();     

        exit;
    }

}

@linauror If you like this patch, you are to update your pull request, the contribution is yours.

@ivantcholakov ivantcholakov added a commit to ivantcholakov/starter-public-edition-3 that referenced this pull request Jun 30, 2014

@ivantcholakov ivantcholakov Image_lib: A minor correction to the previously proposed patch for Co…
…deIgniter, see bcit-ci/CodeIgniter#3106
00a7797
Contributor

ivantcholakov commented Jun 30, 2014

I fixed a typo with the second commit.

Contributor

narfbg commented Jun 30, 2014

It is about not being able to overlay an image without using transparancy.

Technically, that wouldn't be watermarking and the PR is about overlay_watermark().

The properties $wm_x_transp and $wm_y_transp are not described well at the beginning of the library, this causes difficulty for understanding and should be corrected:

Agreed.

In my opinion, the current image library is broken beyond repair.

I don't think it's broken, nor beyond repair, but I do agree we've hit its limits and a rewrite would be necessary to allow for new, better functionalities.

Any fix to the lib (transparancy, rotation) will break backward functionality,

If something's broken, fixing it isn't a BC break, it's a bugfix.

Any functionality change (method chaining and save method) breaks backwards compatibility.

With the current design, method chaining doesn't make sense and we're not aiming for it.

Well, the original PR could be changed a bit to accept FALSE for wm_x_trans or wm_y_transp.

Sure, if it makes sense - go for it.

For anything else we'ld need green light to rethink and redo the image library.

Not happening for 3.0, or in other words - you've got yellow lights. ;)

Contributor

GDmac commented Jun 30, 2014

@ivantcholakov 👍 for the patch. @linauror you picking this up for PR?

With yellow light i do see some possibilities, but not something i can pickup soon.

  • needfix: preserve alpha information for watermark with GIF and 8bit PNG
  • needfix: opacity setting is currently not working for 24bit alpha channel images
  • needfix: preserve alpha information when rotate image
Contributor

ivantcholakov commented Jun 7, 2016

a - a.jpg
b - b.jpg

The images from the original report have been lost. I am attaching two of them again for easing reproduction and testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment