Division by zero

[seltix5]

Hello,
For some images i'm getting the error "Division by zero" in xymak-smartcrop/smartcrop.php:449.

example image : https://ufile.io/764g9 ( 501x376 )

$dst_w = 300;
$dst_h = 225;

$smartcrop = new \xymak\image\smartcrop($file_img_resource, ['width' => $dst_w, 'height' => $dst_h]);
$res = $smartcrop->analyse();
$smartcrop->crop($res['topCrop']['x'], $res['topCrop']['y'], $res['topCrop']['width'], $res['topCrop']['height']);
RETURN $smartcrop->get()

[johnabela]

@seltix5

You did not include the code that you are using, so you are forcing us to take a totally random guess on what it is you are trying to do. If my suggested solutions below do not fix your issue, going to need you to include some actual code that you are trying to implement.

But, if I had to take a wild guess, I would predict that your set height/image is greater than the height and/or width of the original image that you are using. That has a tendency to throw that "Division by zero" error, along with a few other things, sigh.

Sadly the author of this small script just did not really mature things, nor has seemed to want to resolve a few issues already brought up.

But here is an easy way to see/check the issue I mentioned above:

Inside of the public function canvasImageScale() add the following code:

    //
    if ( $imageOriginalHeight >= $this->options ['height'] ) {
        die('smartcrop: your set image height is greater than the original image height.');
    }
    if ( $imageOriginalWidth >= $this->options ['width'] ) {
        die('smartcrop: your set image width is greater than the original image width.');
    }
    //

place that after the line:

$imageOriginalHeight = imagesy ( $this->oImg );

That should at least output a slightly better/helpful error message for you to solve the image size issue.

I would also highly recommend making this change from using ceil() to using floor() as it can help solve some other issues that might show up in your using of this script.

And, if you plan on using the output() function, I would recommend replacing the entire thing with this:

public function output() {
        //
        $image_mime = image_type_to_mime_type(exif_imagetype($this->inputImage));
        //
        if ($image_mime === 'image/jpeg') {
            header("Content-Type: image/jpeg");
            imagejpeg($this->oImg);
        }
        elseif ($image_mime === 'image/png') {
            header("Content-Type: image/png");
            imagepng($this->oImg);
        }
        elseif ($image_mime === 'image/gif') {
            header("Content-Type: image/gif");
            imagegif($this->oImg);
        }
        //
    }

Again, the author of smartcrop.php has not done a very good job on maturing this script. The code above will at least attempt to properly output the image to the correct mime-type, unlike the hard-coded .jpg that the author used. @xymak needs to invest a good deal of effort into maturing this script to handle mime types other than just image/jpeg.

A good example of this is that the image that you have in your example janterivarvd20x92.jpg is not a image/jpg file, but rather a image/png' file that is saved as a .jpg file, which is just not good.

Likewise, the scripts author needs to put into a next version of the script the ability to handle imagecolortransparent so that images such as the one that you have (when properly set to a .png) can support a transparent background.

There are a couple other possible suggestions to resolve this issue, but as I said at the beginning, your lack of posting any code that you are using makes it so that all I/we can do at this point in time is take random guesses at trying to solve your issue. Hopefully something I have already shared solves it, but if not, well, you know what the next step is 😉

[seltix5]

hello @johnabela
Sorry I forgot the code, already update the original post.
In my case the original image is not smaller.
For the change from "ceil() to floor()" i already did it since it was my own sugestion :)
In this case the problem is in the analyse() method, the output method changes could not help.

About the image, I did notice it was a png and not a jpg, but because the script uses imagecreatefromstring ( file_get_contents ( $image ) ) and I update the code to handle png images I do not think the problem is there.
You just need to add

imagealphablending($oCanvas, false);
imagesavealpha($oCanvas, true);

on canvasImageResample() and crop() after the imagecreatetruecolor() call.

[johnabela]

Opps on not remembering your usersname and the ceil<->floor issue 🤦‍♂️

Yeah, the whole thing about the output and mime-type was just me throwing out a suggestion for overall script ability, really did not have anything to do with the 'Division by zero' issue.

So when I was bug hunting this about 6 hours ago, here is how I rewrote the function for a unit test:

public function saturation($r, $g, $b) {

    $maximum = max( $r / 255, $g / 255, $b / 255);
    $minimum = min( $r / 255, $g / 255, $b / 255);

    if ($maximum === $minimum) {
        return 0.1; // notice the change from 0 to 0.1
    }

    if ( abs(($maximum + $minimum)) > 0.1 ) {
        //
        print abs(abs($maximum - $minimum) / abs(2 - $maximum - $minimum));
        die(' :: 001');
        //
    }
    else {
        print abs( abs($maximum - $minimum) / abs($maximum + $minimum));
        die(' :: 002');
    }

}

My effort was to try to prevent an actual mathematical division by zero by changing all of the 0 (zeros) to 0.1 (zero.one) and adding the abs() to prevent negative values, which I have seen come up on a couple of images over the last year.

So when I change the function to the above, for your image I get:

0.058823529411765 :: 002

Could you update your saturation() function to the same as I have above, for a brief test, and see if you get the same number on your end?

Also, could you give this small change a try...

Inside of the saturationDetect() function could you change the return 0; to return 0.1; and see if that has any effect. Trying to negate the "by zero" issue... if we can get it to use "0.1" in the equation, that should (??) maybe resolve this bug.

Some stupid math issue that is using a zero, or a negative number, somewhere, eh!

[johnabela]

btw, here is the exact code that I am using for your image, and it is working, ref the attached image.

$file = 'janterivarvd20x92.jpg'; // 501 x 376
//
$smartcrop = new xymak\image\smartcrop($file,['width' => 300,'height' => 225]);
$res = $smartcrop->analyse();
//
if ( empty($res['topCrop']) ) {
    //
    die('Bummer, we could not make any smartcrop from this image.');
    //
}
$smartcrop->crop($res['topCrop']['x'], $res['topCrop']['y'], $res['topCrop']['width'], $res['topCrop']['height']);
$smartcrop->output();

When using your image, the only time I am able to reproduce your 'Divided by zero' error is when I set the height to something higher than the size of your original file.

[seltix5]

Hello,
Ok after further testing the error is related with the PNG format.
Because I updated the canvasImageResample() method with the code as I said before the "oImg" internal variable must have some diferent values because of the transparent attribute.

The division by zero only occurs once, with this values :

r : 382
g : 128
b : 130

[seltix5]

so,
I just add this code :

if ($l > 0.5) {
    if ((2 - $maximum - $minumum) == 0) {
        RETURN 0;
    }
} else {
    if (($maximum + $minumum) == 0) {
        RETURN 0;
    }
}

in the saturation() method before the return to just prevent any division by zero and return 0 instead.
I'm not sure if it will cause any problems in the "smartcrop" calculations but I did one test or two and I think it is working as usual

should I create a pr?

[johnabela]

should I create a pr?

Certainly worth a try submitting a PR/Commit and seeing if @xymak accepts it.

Maybe also:

• Have your ceil() <-> floor() changes
• My mime_type update in the output() function
• Your png update/fix
• The saturation() updates (btw, should probably use === and not == for the zero/int if() checks).
• Might as well throw in the $imageOriginalHeight/$imageOriginalWidth checks that I mentioned, just to help throw error messages if anybody tries to set the output image to be larger than original.

In regards to the changes you made to the canvasImageOpen() function, we should probably get that entire function rewritten to have some file exist checking taking place, here is what I have for mine, just some standard file exists detection, before we go and try to do the file_get_contents call.

public function canvasImageOpen($image) {

	if (\strlen($image) <= PHP_MAXPATHLEN && \file_exists($image)) {

	    $this->oImg = imagecreatefromstring(file_get_contents($image)); // guess this needs to get changed to whatever code you changed to fix the png issue(?)
	    return $this;

	}
	return null;
}

And here is my image size check code:

    if ( $imageOriginalHeight > $this->options ['height'] ) {
        die('smartcrop: your set image height is greater than the original image height.');
    }
    if ( $imageOriginalWidth > $this->options ['width'] ) {
        die('smartcrop: your set image width is greater than the original image width.');
    }

And here is my output() code:

/**
 * Output a image
 */
public function output() {
    //
    $image_mime = image_type_to_mime_type(exif_imagetype($this->inputImage));
    //
    if ($image_mime === 'image/jpeg') {
        //
        header("Content-Type: image/jpeg");
        imagejpeg($this->oImg);
        //
    }
    elseif ($image_mime === 'image/png') {
        //
        header("Content-Type: image/png");
        imagepng($this->oImg);
        //
    }
    elseif ($image_mime === 'image/gif') {
        //
        header("Content-Type: image/gif");
        imagegif($this->oImg);
        //
    }
    //
}
//

Might as well give it all one big PR/Commit at once, and just see if he accepts it or not. If nothing else, we can use it to point others too if others have issues in the future, eh.

[seltix5]

hi,
I just create the PR
#4
with a small diference, your image with/height check with original width/height, I put it inside the debug variable because I dont think by default the code should die just because of that.
I have my own backoffice and I dont want the frameword to die just because of that. For debug it may be usefull but for staging the check probably should be done before the smarcrop request or develop smarcrop class more to handle error feedback.
In my case I even want the crop to be done anyway even if the original is smaller. I have a webstore in my framework and the design was created to work with specific sizes images in some cases, if the user just does not upload big enought images it is not my problem and all should work noramlly.

[seltix5]

PS : the comparison in saturation() method must be with == and not with === because 0 !== 0.0 and thus causing false negatives.