ImageOptimizer

[Toxaris]

Originally Posted by Doitsu

Can you please check the version number of jpegtran? Toxaris's plugin uses version 9, which uses a different syntax than the previous version which accepts only one file name parameter.

The old version has the following syntax:

usage: jpegtran [switches] [inputfile]

however, the new version uses the following syntax:

usage: jpegtran [switches] [inputfile] [outputfile]

If you use the new syntax with the old version, it enters an endless loop, which causes the plugin to hang.

AFAIK, libjpeg-turbo is a fork of jpegtran that uses the old syntax. I.e. you can't use it to optimize .jpeg files under Linux.

Actually, this is not the syntax I use. I specify the switch outfile and that should work anyway. However, I will probably use the alternative that DiapDialer mentioned to make it more flexible.

[eschwartz]

Originally Posted by Doitsu

Can you please check the version number of jpegtran? Toxaris's plugin uses version 9, which uses a different syntax than the previous version which accepts only one file name parameter.

Looks to me like it uses the old syntax, actually.

    elif (filename.lower().endswith('.jpg') or filename.lower().endswith('.jpeg'))    :
        # get correct arguments
        if iswindows:
            exe_path = os.path.join(SCRIPT_DIR,'win', 'jpegtran.exe')
        elif isosx:
            exe_path = os.path.join(SCRIPT_DIR,'osx','jpegtran')
        else:
            exe_path = 'jpegtran'
                
        args = [exe_path]
        args.append("-optimize")
        args.append("-progressive")
        args.append("-copy")
        args.append("none")
        args.append("-outfile")
        args.append(filename)
        args.append(filename)

The outfile is passed via -outfile, not as the last argument... exactly as both libjpeg-turbo and mozjpeg expect.
In fact, you can use libjpeg's version 9 executable with either syntax. Which is I guess why Toxaris used the old syntax.

The old version has the following syntax:

usage: jpegtran [switches] [inputfile]

however, the new version uses the following syntax:

usage: jpegtran [switches] [inputfile] [outputfile]

If you use the new syntax with the old version, it enters an endless loop, which causes the plugin to hang.

AFAIK, libjpeg-turbo is a fork of jpegtran that uses the old syntax. I.e. you can't use it to optimize .jpeg files under Linux.

What -- because it uses the old syntax, this file optimizer doesn't work on linux? Or the plugin cannot use it?

Because I am pretty sure both are untrue.

Also, on my machine, using the libjpeg version 9 syntax with libjpeg-turbo (or mozjpeg) does exactly what I expect -- it immediately returns a failure:

jpegtran: only one input file
usage...

Originally Posted by DiapDealer

That way the user can just modify the json prefs file to 1) point to a newer version of jpegtran (which I'm betting calibre's is); or 2) use wine to run the included Windows version.

calibre uses jpegtran from the latest version of mozjpeg, rather than using libjpeg-turbo (the latest version of which is on roger64's machine).
mozjpeg and libjpeg-turbo are both equally NOT libjpeg.




....




I suppose the real question is, why are you using subprocess.call(list(args), shell=True)

subprocess.call(args) works fine without any other changes.
(Don't use a shell. Minor nit: don't convert a list into a list)

[Toxaris]

Updated version which includes a preference file to chose another version of jpegtran and the usage of wine.

[Doitsu]

Originally Posted by eschwartz

calibre uses jpegtran from the latest version of mozjpeg, rather than using libjpeg-turbo (the latest version of which is on roger64's machine).
mozjpeg and libjpeg-turbo are both equally NOT libjpeg.

It appears that I was mistaken. When the jpeg optimization didn't work on my machine I checked the jpegtran version number and found out that it was jpegtran 8d, but the Windows version bundled with the plugin is 9a.

I then executed the same command line that the plugin uses:

jpegtran -optimize -progressive -copy none -outfile /tmp/tmp2RD8h1/image.JPG /tmp/tmp2RD8h1/image.JPG

and noticed that the execution entered an endless loop. Since the plugin didn't work on roger64's Arch Linux machine and he said that he's installed libjpeg-turbo, I erroneusly assumed that libjpeg-turbo might have the same problem as my jpegtran 8d version.

Did you install the ImageOptimizer plugin on your machine and does it work with libjpeg-turbo?

[eschwartz]

Again, it works fine once I fixed the plugin.

The problem is that subprocess.Popen expects either:

1. a list, and shell=False (shell defaults to false)
2. a string, and shell=True

Passing a list to the shell means it tries to execute "jpegtran" and pass the additional arguments as arguments to the shell itself, not jpegtran.

And yes, when you run

jpegtran

it hangs.

Why on earth does it specifically need a shell? The plugin isn't using shell builtins, pipes, variable expansion, tilde expansion, or shell globbing. Plus it is now vulnerable to shell injection (not that this is the easiest way to mess with someone's computer and make them run malicious commands, but still, it's the theory of the thing...)

[Doitsu]

Originally Posted by eschwartz

Again, it works fine once I fixed the plugin.

The problem is that subprocess.Popen expects either:

1. a list, and shell=False (shell defaults to false)
2. a string, and shell=True

To be honest, I can't follow you. Can you please post the lines that you've changed?

[eschwartz]

def OptiWrapper(*args):
    # execute optimizer program
    # list(args) is a no-op, since it is already a list...
    ret = call(list(args), shell=True)
    return ret

This is BAD.

args is a list, and shell=True

These conditions cannot both be true.


Either use:

def OptiWrapper(*args):
    # execute optimizer program
    ret = call(args)
    return ret

or

def OptiWrapper(*args):
    # execute optimizer program
    # this actually requires a lot more checking of proper shell
    # quoting/escaping than I display here
    ret = call(' '.join(args), shell=True)
    return ret

See the python docs: https://docs.python.org/3/library/su...en-constructor

On POSIX with shell=True, the shell defaults to /bin/sh. If args is a string, the string specifies the command to execute through the shell. This means that the string must be formatted exactly as it would be when typed at the shell prompt. This includes, for example, quoting or backslash escaping filenames with spaces in them. If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself. That is to say, Popen does the equivalent of:

Popen(['/bin/sh', '-c', args[0], args[1], ...])

EDIT: useless nit #2
Why define a special function as a wrapper for subprocess.call, when it is used only once and adds no particular value other than turning on the shell and doing list --> list conversion?
It was staring me in the face while looking at the source of the hanging...

[Toxaris]

The shell=True is indeed a mistake which was entered during the testing process. I apparently forgot to remove it. On Windows it does not cause an issue...

A small update where this is corrected.

The reason why I used a sub for the wrapper is simple. It is easier for updates if this is already split. I agree that it seems silly for one command only, but that is the current situation.