claviska / SimpleImage

A PHP class that makes working with images and GD as simple as possible.
MIT License
1.38k stars 383 forks source link

Artifacts in the image #109

Closed ognistyi closed 9 years ago

ognistyi commented 9 years ago

I use this code to resize: $image = new SimpleImage($fullName); $image->thumbnail($imagePattern['w'], $imagePattern['h'])->output(null, 70);

and the image resized with artifacts - before convertation - https://www.dropbox.com/s/a6fm4m1cfh1ms9e/before_img.jpeg?dl=0 after convertation - https://www.dropbox.com/s/hy7g77gcaerpfq4/after_img.jpeg?dl=0

ognistyi commented 9 years ago

it's affects only when output quality less than 100%

mikedidomizio commented 9 years ago

This might be just a GD problem or a problem with the image. What if you try a different image?

ognistyi commented 9 years ago

I test it on different JPEGs, locally on mac, and Ubuntu server.

claviska commented 9 years ago

I used your source image with the following code at 200x200:

$image = new SimpleImage('test.jpeg');
$image->thumbnail(200, 200)->output(null, 70);

Resulting image:

result

So, a couple theories as to why I can't reproduce this:

  1. The original image has been reprocessed by Dropbox and no longer causes the problem. This is unlikely since you said it happens with any image, unless they all originated from the same device or application. (e.g. did you run them all through some kind of compression software?)
  2. Your browser doesn't like something in the image data that GD is producing. Also unlikely if it's happening with multiple images. GD is heavily used and this probably would have been reported already.
  3. The processed images are somehow getting corrupted by something else on your system.

In any case, there's probably not much we can do about this since the processing is done by GD internally, but it couldn't hurt to identify what's happening.

Did you run the images through any type of compression software? How were they created? What browser were you using (and do you get the same result in all browsers)?

Edit: I missed your PR before. Does #111 actually fix the problem for you?

ognistyi commented 9 years ago

Yes, it is. After that changes all looks good.

Some update to be more clear:

$image->thumbnail($imagePattern['w'], $imagePattern['h'])->output(null, 70); // cause this bag (PR #111 fix this for me)
$image->thumbnail($imagePattern['w'], $imagePattern['h'])->output('jpeg', 70); // will work

It's strange you cannot reproduce this effect.. interesting

claviska commented 9 years ago

I'm reluctant to accept that PR because it forces all JPEGs to be progressive, which may not be desirable in all cases. That should be a separate argument in the save() method, not the default.

I realize that it solves your issue, but probably by coincidence. Without being able to reproduce the problem I'm not sure what the proper fix should be.

Is anyone else able to reproduce this?

If not, I'm afraid it will take some additional testing on your end (since you're the only one who can reproduce the problem) to determine the root of the issue.

ognistyi commented 9 years ago
$image = new SimpleImage("./test.jpeg");

$image->output('jpeg'); // imageinterlace() will be APPLIED (forced)
$image->output(); // imageinterlace() will not be applied

My pull request only do this same when you pass $format actually to the output method, or omit this one.

And look to the save() method, imageinterlace() will be applied for all images(with no exclusions) before save, as I can see..

claviska commented 9 years ago

You're right. I was looking at save() instead of output() and thought you had added it there as well. Since this makes the behavior consistent between the two methods, I think we can accept. However, I think this should be a separate argument that defaults to false in the future.

@nazar-pc Any thoughts on #111 before I accept?

nazar-pc commented 9 years ago

To be honest - I'd make all jpeg images progressive (looks like with this PR all cases except one will produce progressive jpeg, so why $image->output(); should produce anything different?). It barely hurts anyone (just a bit bigger file), but it is what majority of people would prefer on the web.

Alternatively (preferable case), we can add explicit control for this feature using separate method like this:

$image = new SimpleImage("./test.jpeg");
$image->output('jpeg'); // Not progressive
$image->progressive();
$image->output('jpeg'); // Progressive
$image->progressive(false);
$image->output('jpeg'); // Not progressive

Anyway we should be it consistent and predictable.

claviska commented 9 years ago

Ok, accepted #111 to make the library behave consistently. I like the idea of a progressive() method to enable/disable it.