buzz wrote:

I have just seen that in defaultsettings it refers to using customconvert command for example with graphicsmagick. However, this doesn't make it as easy as not everyone will know about the various workaround in imagemagick (such as white background for older browsers on transparent images), that would also apply to graphicsmagick.

buzz wrote:

patch to implement a graphicsmagick option

Here is my patch against 1.14.0 that implements a new variable $wgUseGraphicsMagick.
It uses the config values from the imagemagick config like

$wgUseGraphicsMagick = true;
$wgImageMagickConvertCommand = "/usr/bin/gm";

This is just one way to implement it, and I'm not suggesting it is the best way.
it is more useful than using the custom command as it handles the differing compression for png/jpeg quality as well as other workarounds for animated gifs etc.

attachment diff.txt ignored as obsolete

buzz wrote:

add graphicsmagick option

I had accidentally replaced thumbnail with resize for imagemagick. oops

attachment diff.txt ignored as obsolete

Patches should be against trunk (most recent development version). 1.14.0 is based on r45489, while trunk is at r51464 (1.16alpha). You patch does not apply in a clean way. Please provide an updated patch.

buzz wrote:

Sorry. I didn't consider that it would be accepted anyway, but for reference so get any feedback on what would be the "right" way to implement this (and to get comments on whether it should be the default)

buzz wrote:

add graphicsmagick option

Updated patch to apply against trunk

attachment diff.txt ignored as obsolete

Thanks. The patch applies cleanly now. Added keyword 'need-review'.

buzz wrote:

add option for graphicsmagick support

cleanup. was missing a tab

Attached:

Hm, it'd probably be wise to break up the thumbnailing code to make it easier to extend. We've got a loooot of things in this big giant if/switch block. :)

Bryan.TongMinh wrote:

(In reply to comment #9)

Hm, it'd probably be wise to break up the thumbnailing code to make it easier
to extend. We've got a loooot of things in this big giant if/switch block. :)

Do you suggest not applying this patch until somebody rewrites that code or apply it and hope somebody will clean it up someday?

buzz wrote:

I'm still happy to rework the patch. just got a notification as this bug was tweaked and reverted perhaps by accident, but I think this is still a good issue. for my and my wiki personally, the switch was a significant performance increase.

Bryan.TongMinh wrote:

(In reply to comment #10)

(In reply to comment #9)

Hm, it'd probably be wise to break up the thumbnailing code to make it easier
to extend. We've got a loooot of things in this big giant if/switch block. :)

Do you suggest not applying this patch until somebody rewrites that code or
apply it and hope somebody will clean it up someday?

Note, did this some months ago. So the patch won't apply without significant rework effort.

sumanah wrote:

CC'ing a few people to ask whether this question/issue something we should follow up on.

(In reply to comment #13)

CC'ing a few people to ask whether this question/issue something we should
follow up on.

You mean supporting graphikmagic? I would call it a low priority issue. If someone wants to write support/adapt the patch on this bug, by all means, but we already support several thumbnailers, so not something to spend large effort on, unless someone is specifically independently interested

buzz wrote:

it's very similar to imagemagick so I don't think it should be a massive issue to integrate it. I already made a patch before (here) which I realise is no longer good with the current codebase. I may well update it, but then, it would be good to know if it would be accepted so I'm not wasting any time. It's worth noting, when using graphicsmagick before, thumbnailing was significantly faster on my hardware at the time, however this was some time ago.

I think it would be worth changing the focus of any patch to the definition of a global with a string input rather than multiple booleans, similar to the way we handle SVG conversion at the moment. Then, $wgUseImageMagick could be deprecated and the whole thing would be more future proof.

If that were the case, I can't see any reason not to merge a patch that added support for an alternative renderer but didn't even seek to make it the default.

Krinkle: is the plan to use testing to make sure this definitely is a perf benefit?

@Jarry1250 As @Gilles has already shown in the comment before you, Yes we're doing our own comparisons and benchmarks to see what the benefits are of GM for Wikimedia. And unless there are reasons not to, we'll gradually start using it where it makes sense.

In T21073#1556829, @Krinkle wrote:

@Jarry1250 As @Gilles has already shown in the comment before you, Yes we're doing our own comparisons and benchmarks to see what the benefits are of GM for Wikimedia. ...

Yup (my previous msg was a little more brief than I intended, sorry). Gilles' linked task, T107885, describes a test run on two images (a large PNG and a large JPG). I don't think it would be too tricky to run against a representative sample of hundreds or low thousands of PNGs to improve confidence in the general result. Of course one could do so in the wild by implementing the change and watching the stats, but --

In T21073#1556829, @Krinkle wrote:

...And unless there are reasons not to, we'll gradually start using it where it makes sense.

`-- per my comment above, I think some work would be necessary to rationalise the way shelled-out-to image handlers are defined in the config before one could usefully implement the distinction between JPG and PNG handling Gilles' research implies. At the moment we have a confusing mixture of boolean switches and command line strings for general images and SVGs, which IMHO isn't particularly scalable. So I think it might be better to do some large scale PNG testing before investing time in changing the config structure.

I'd argue that the messy way the code is organized is actually an argument for cleaning things up regardless and having a saner per-format logic, and that time is better invested doing so than setting up a larger scale test. The worst that can happen is that GM doesn't perform better and we've made our code more flexible and future-proof.

As you said, we can "do it live" to figure out if it performs better. It's the only way to be really sure, in fact, because regardless of how cleverly you pick your test images, or how large the sample is, it will always be an approximation of reality, and the real deal is what matters.

Most of the work required here is code cleanup and reorganization. If our code was better organized, testing GraphicsMagick would only amount to switching from a "convert" command to "gm convert" with exactly the same parameters. Unfortunately, as you know, we're far from that.

Testing new scaler software for a particular format in our code should be easier. Then we wouldn't ask ourselves about creating synthetic tests, we'd just switch some wikis to the new software and measure the impact.

Most of the work required here is code cleanup and reorganization. If our code was better organized, testing GraphicsMagick would only amount to switching from a "convert" command to "gm convert" with exactly the same parameters. Unfortunately, as you know, we're far from that.

Perhaps I missed something - I agree that the scalar code is spaghetti, but if GM is compatible with all IM switches, why can't we just do that?

In T21073#1574459, @Bawolff wrote:

Most of the work required here is code cleanup and reorganization. If our code was better organized, testing GraphicsMagick would only amount to switching from a "convert" command to "gm convert" with exactly the same parameters. Unfortunately, as you know, we're far from that.

Perhaps I missed something - I agree that the scalar code is spaghetti, but if GM is compatible with all IM switches, why can't we just do that?

Because it only seems to benefit PNGs. At equal performance, I'd rather stick with IM, which is likely to have better support and faster bugfixes, for JPGs. Currently switching the IM command to GM would affect all formats, afaik.

Oh I got you. I was thinking as a drop in replacement for im over everything.

We could do that as a temporary test, like running GM on Commons for a day. It would confirm on a large scale whether it's indeed not better than IM for JPGs and better than IM for PNGs. If what I've seen is confirmed, though, we're back to square 1 with the same amount of work to do.

Well there is a hook one could use to handle it all as an extension... but it would be nice to eventually reduce that mess instead of just keep piling on alternate paths.

Is GraphicsMagick supported? This task indicates that it is not, but this mediawiki.org page indicates it is.

There's no test coverage for it, I don't think there's any guarantee that everything will work with it. Mediawiki.org states that you can use it, but offers no guarantees about what will work. GM tried to be command-compatible with IM but there can be some syntax differences. I think I recall that it worked by just pointing to the GM commands in the mediawiki configuration, but I didn't try all the thumbnailing options and scenarios.