Commit
- Loading branch information
There are no files selected for viewing
23 comments
on commit 94881ba
There was a problem hiding this comment.
GOTCHA! GOTCHA! GOTCHA!
It took me hours to get there!
The \! is wrong and should be ! instead. On my WIN cmd console it works even without.
Why was that introduced here?
This is related to the very hard to debug, squared non thumb resizing issue, in http://board.s9y.org/viewtopic.php?f=10&t=20083, which is partly an old bug by imageselectorplus thumbnail resizing, in cases where isp option 1 and 2 were set to a squared size, like 250 for both. And the other part is this.
Can I fix this for us all and remove the backslash? Or even both?
There was a problem hiding this comment.
Why was that introduced here?
If I read the convert docs correctly, the exlamation point is meant to force the image geometry (which is in the convert command before it), e.g. 400x200!. WIthout the exclamation point, the geometry values will be interpreted as max values to maintain the aspect ratio of the image.
I assume the backslash is meant to mask the ! (because it's a special character in PHP?). However, I'm really just guessing here.
There was a problem hiding this comment.
Yes I assume this too, but it doesn't work and prevents the correct execution, say, just takes the original image size as serendipity_escapeshellarg($newSize).
So if that backslash removement doesn't harm elsewhere, I would like to remove it very soon!
There was a problem hiding this comment.
The reason for this parameter is mentioned here...
http://board.s9y.org/viewtopic.php?f=3&t=19427&p=10435848
There was a problem hiding this comment.
So we should keep the ! parameter but get rid of the backslash. I can't really state if the backslash is necessary here, so you PHP folks figure that out, please. :-)
There was a problem hiding this comment.
@bauigel
Can you confirm this is working well without the backslash too, please?!
There was a problem hiding this comment.
The ! is a special character on bash (unix) shells and needs escaping, please check if this works in those kind of environments?
(http://unix.stackexchange.com/questions/19252/why-does-the-exclamation-mark-sometimes-upset-bash first comment explains what ! means in bash when not escaped)
There was a problem hiding this comment.
Well, I can't really test this heavingly remote. I need helpers!
So let me state this. On Win OS, the backslash prevents the bang to be correctly parsed. The "interactive shell" - I assume - uses the history size in this case as fallback, width & height, which is the original uploaded image size. This is, while the uploading image creation procedure works as follow: upload and thumb original image with original thumb settings (400 on 2.0) and in a second run only change this thumb to the new size, defined by a plugin (in my case ISP for example).
Now, we have this, from the IM usage docs:
Use > to change the dimensions of the image only if its width or height exceeds the geometry specification. For example, if you specify '640x480>' and the image size is 256x256, the image size does not change. However, if the image is 512x512 or 1024x1024, it is resized to 480x480.
And we have
Append an exclamation point to the geometry to force the image size to exactly the size you specify.
which is not quite the same as
The ! character invokes bash's history substitution.
in Garvins quoted page comment.
I assume this is the real cause and bottom of all.
For IM and the command line the bang is a differently used command.
Our script now handles this in two different ways.
if (!$force_resize && serendipity_ini_bool(ini_get('safe_mode')) === false) {
$newSize .= '>'; // Tell imagemagick to not enlarge small images, only works if safe_mode is off (safe_mode turns > in to \>)
}
$cmd = escapeshellcmd($serendipity['convert'] . ' ' . $serendipity['imagemagick_thumb_parameters']) . ' -antialias -resize ' . serendipity_escapeshellarg($newSize) . '\! ' . serendipity_escapeshellarg($infile) .' '. serendipity_escapeshellarg($outfile);which looks like
/usr/bin/convert -antialias -resize "300x300>"\! "/path/to/blog/uploads/squared-testimage.jpg" "/path/to/blog/uploads/squared-testimage.serendipityThumb.jpg"on command line.
I would recommend to use it like this, as this appends it to IM only and does not need a backslash (at least here):
if (!$force_resize && serendipity_ini_bool(ini_get('safe_mode')) === false) {
$newSize .= '>!'; // Tell imagemagick to not enlarge small images but force to newSize others, only works if safe_mode is off (safe_mode adds a backslash)
}
$cmd = escapeshellcmd($serendipity['convert'] . ' ' . $serendipity['imagemagick_thumb_parameters']) . ' -antialias -resize ' . serendipity_escapeshellarg($newSize) . ' ' . serendipity_escapeshellarg($infile) .' '. serendipity_escapeshellarg($outfile);or maybe even like this
if (serendipity_ini_bool(ini_get('safe_mode')) === false) {
if (!$force_resize) {
$newSize .= '>'; // Tell imagemagick to not enlarge small images, only works if safe_mode is off (safe_mode turns > in to \>)
}
$newSize .= '!'; // force the image size to exactly the size specified
}
$cmd = escapeshellcmd($serendipity['convert'] . ' ' . $serendipity['imagemagick_thumb_parameters']) . ' -antialias -resize ' . serendipity_escapeshellarg($newSize) . ' ' . serendipity_escapeshellarg($infile) .' '. serendipity_escapeshellarg($outfile);Here (http://www.imagemagick.org/Usage/resize/#noaspect) it is explained even better:
Ignore Aspect Ratio ('!' flag)
If you want you can force "-resize" to ignore the aspect ratio and distort the image so it always generates an image exactly the size specified. This is done by adding the character '!' to the size. Unfortunately this character is also sometimes used for special purposes by various UNIX command line shells. So you may have to escape the character somehow to preserve it.
For the last sentence, one could add:
(if not put into the quotes around the new resize specification.)
So one for the last, I even think changing command order to !> is even better.
What do you think?
If any needs or wants to help testing, in special on this rounding portrait images, please take this as a playground copy command, eg
/usr/bin/convert -antialias -resize "300x300>!" "/path/to/blog/uploads/squared-testimage.jpg" "/path/to/blog/uploads/squared-testimage.serendipityThumb.jpg"
There was a problem hiding this comment.
I think
if (!$force_resize && serendipity_ini_bool(ini_get('safe_mode')) === false) {
$newSize .= '>'; // Tell imagemagick to not enlarge small images, only works if safe_mode is off (safe_mode turns > in to \>)
}
$cmd = escapeshellcmd($serendipity['convert'] . ' ' . $serendipity['imagemagick_thumb_parameters']) . ' -antialias -resize "' . serendipity_escapeshellarg($newSize) . '!" ' . serendipity_escapeshellarg($infile) .' '. serendipity_escapeshellarg($outfile);
should be best and prevent unix problems with !.
There was a problem hiding this comment.
Looks funny -antialias -resize ""300x300>"!", but it works for WIN OS.
Shall I commit that?
There was a problem hiding this comment.
Just as an addition, with ISP option max-width and max-height set to 300 each,
S9y 1.7, which hasn't have the bang, sizes an image from - Orig.: 100x60, to Thumb: 100x60, 1.26kb
while 2.0 now does upsize the thumb - Orig.: 100x60 Thumb.: 300x180, 1,26 KB
There was a problem hiding this comment.
For the latter issue we could add to line 661
if (is_array($size)) {
+ if ($fdim[0] > $size['width'] && $fdim[1] > $size['height']) {
$r = $size;
+ } else {
+ return array(0,0);
+ }
} else { ...which is a "via plugin" reaction to the bang addition.
So what about the commit?
There was a problem hiding this comment.
It should actually look like this:
-antialias -resize "300x300>!"
no double quotes...
HOWEVER your second comment strikes me as odd. Why do we allow to distort image resizements?! Shouldn't the aspect ration be left? What was the reasoning behind changing this?
Sorry I'm out of touch right now, but this should be taken care of
There was a problem hiding this comment.
As for the “distorted” aspect ratio, see (at least I think that's it) this forum post. Keeping the aspect ratio exactly can apparently result in two original images of the same size to have thumbnails with different sizes. Although the thumbnail size only varies by a pixel, that can be an issue in “image rows” in the frontend. The patch for this is 14 months old.
What worries me more is that we should never scale thumbnails (or any pixel-based image, for that matter) up. If an original image is smaller than the thumbnail size in s9y, the thumbnail should be the size of the original image.
There was a problem hiding this comment.
Looks funny -antialias -resize ""300x300>"!", but it works for WIN OS.
This was catched via echo $cmd.
Being double quoted now is the result of $newSize coming in as a string - single quoted, eg '222x222' ($newSize = $r['width'] . 'x' . $r['height'];var_export($newSize);) and runs through serendipity_escapeshellarg -> escapeshellarg methods, which outputs ""222x222>"!"
If it works everywhere like that, this funny double quote is ok for me though. And it nicely does that for all kind of sized images, in special in combination with that > command. Except for one single case. the latterly found issue with Miniatures.
For the second, please read my last comment again. It does not touch/change/distort aspect ratio; it just resizes a very small uploaded image to the $newSize set Thumb preview, respecting the > command, by plugin ISP max options. And this because of the bang. It does not make real sense to me, to inflate miniatures for the thumb. Therefore the check.
What worries me more is that we should never scale thumbnails (or any pixel-based image, for that matter) up. If an original image is smaller than the thumbnail size in s9y, the thumbnail should be the size of the original image.
This is what I meant and why I proposed the checking patch.
There was a problem hiding this comment.
I think there is a bug in the logic, insofar as an image that is only bigger in one dimension should still get a thumbnail. Otherwise, I think you should just commit the check and we try it out :)
Edit: Imho, the ! vs ! has to be tested on Linux, it worked for 14 month - but if it is properly in quotes it should be alright.
There was a problem hiding this comment.
No, you did not catch my means. It is not a bug in the logic and not preventing thumbnails in any case.
The method serendipity_makeThumbnail() is processed twice, once for the real core set thumbnail, second for any plugin adding new thumb sizes, like the fixed ISP 0.45 (which had a bug before and added nothing to $newSize). It now resizes, via bang, inflating the thumb in Miniature cases, which should prevent my patch only.
There was a problem hiding this comment.
Might be. I still think that code is wrong when only one dimension is bigger, but we can sure try it out.
There was a problem hiding this comment.
Do you mean this case?
ISP: 222:415
Orig.: 900x411
Thumb.: 400x183
Then the thumb will just return array(0,0) (see patch) for the second run and the thumb is the one from cores first run.
There was a problem hiding this comment.
👍
There was a problem hiding this comment.
See my commit ddd3b91, which avoids the double quoted $cmd and should do on (unix) shells.
There is just a decision for future behaviour left:
This bang forcing is new to 2.0 using ImageMagick.
The behaviour is now, that all sizes to come in as squared (*) are forced to this format, which squeezes or bloats landscape/portrait images. This behaviour is different to S9y systems below 2.0 Series.
I am easily able to avoids this in functions_images.inc by doing something like
if ($size['width'] == $size['height']) $squared = true;
if (!$squared) $newSize .= '!';Now, shall we break previous behaviour or take this non-forcing route?
(*) eg, ISP set to 300x300, Orig.: 1732x1238, previous S9y Thumb: 300x214, now Thumb: 300x300
There was a problem hiding this comment.
Regarding the second run resizing bang issue with ISP, I found this out so far.
The use of the bang makes exact sizes as defined, without minding calculating and rounding differences after comma. This "be more accurate than totally exact" behaviour, including the bang, is needed for floating image galleries only.
Now, I think we want to keep this, since it makes life a little bit easier for our users, even if adding or loosing a single image pixel for the generated thumbs.
The problem is not in the first run of the serendipity_makeThumbnail() method. It is with ISP having set the MAX-values. My further tests showed, that It is definitely sure, that the ISPs second run needs an exception for the bang, while else to many image deformations can happen. Having set this if (!$serendipity['imagemagick_nobang']) $newSize .= '!';, it just resizes by correct total ratio, which is like the previous Serendipity Series imageThumb behaviour.
We would need to make sure the people know, that the image thumb creation without ISP slightly changes the ratio and same with ISP set to 0 x 0. But having the ISP MAX values set, the returned resized imageThumbs are like they were without the pre-2.0 bang forcement. To say in short: No good for floating galleries! (Edit: Well, I checked this too, it still works floated good! Scaled right by landscape!)
I think any other complicated exception createment would just be too much.
I am going to submit this to both, if no one really protests with exact test cases or pointing to a logical error.
Btw: Are there any other plugins around changing thumb size values?
There was a problem hiding this comment.
bang resizing finished with 27c5b2d and s9y/additional_plugins@3e63648
Duplicate
$cmd =?