Page MenuHomePhabricator

Review and deploy VipsScaler extension
Closed, ResolvedPublic

Description

Author: Bryan.TongMinh

Description:
Review and deploy the VipsScaler extension on Wikimedia so that large PNGs can be properly scaled. The VipsScaler supports conditional scaling, so it is possible to configure it to scale only images that match a certain condition (i.e. area > 12.5M)

Such a condition would look like:

$wgVipsConditions[] = array( 'mimeType' => 'image/png', 'minArea' => 12.5e6 );

Note that a the development version 7.25 needs to be installed on the image scalers because older versions do not handle certain PNGs properly.

Unfortunately there are a lot of dependent phase3 revisions in BitmapHandler, so this can probably not be done until deployment of 1.18.


Version: unspecified
Severity: enhancement
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=9497

Details

Reference
bz28135

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 21 2014, 11:25 PM
bzimport set Reference to bz28135.
bzimport created this task.Mar 20 2011, 3:49 PM

I added some initial comments to bug 25990.

Outstanding issues:

  • $wgMaxImageArea should only be used for scalers which decompress the source image. This is essential if VIPS deployment is going to allow larger PNGs to be uploaded. Needs a little bit of refactoring.
  • The "comment" parameter in $scalerParams is not used, so that feature would disappear entirely. Maybe shell out to exiv2 to add comments. $wgExiv2Command is available in core and is used by PagedTiffHandler already.
  • A testing feature of some sort would be nice, to allow for some sort of beta testing.
  • Consider having PagedTiffHandler call VipsScaler to do the scaling, to reduce code duplication and to get sharpening support in PagedTiffHandler.

The testing feature I'm imagining would have a special page, say Special:VipsTest, which allows you to specify an image name and a destination width. The image would be scaled with VIPS and saved to a temporary extension-specific directory. Then both the ImageMagick and VIPS thumbnails would be displayed to the user by overlaying them with CSS position:absolute, with the visible image toggled by JavaScript to allow for easy visual comparison.

Bryan.TongMinh wrote:

To make the test feature without hacking into the 404-handler I would need to stream the vips thumbnail through the Special page. Is vips available on all mediawiki servers or only on the image scalers?

Bryan.TongMinh wrote:

(In reply to comment #2)

  • The "comment" parameter in $scalerParams is not used, so that feature would

disappear entirely. Maybe shell out to exiv2 to add comments. $wgExiv2Command
is available in core and is used by PagedTiffHandler already.

I've looked into this, but I can't find out what IM does with the comment parameter. I used "exiv2 print" to print all exif information from a Commons thumbnail, but it does not list any exif data.

(In reply to comment #4)

To make the test feature without hacking into the 404-handler I would need to
stream the vips thumbnail through the Special page. Is vips available on all
mediawiki servers or only on the image scalers?

It looks like VIPS will be just on the scalers for now. I think the easiest way to do it is to hit the image scalers using Http::request() with proxy = rendering.svc.pmtpa.wmnet in $options. The URL could just be a secret subpage of the special page, which would cause a thumbnail to be generated with specified parameters.

(In reply to comment #5)

I've looked into this, but I can't find out what IM does with the comment
parameter. I used "exiv2 print" to print all exif information from a Commons
thumbnail, but it does not list any exif data.

For a JPEG try exiv2 pr -p c <filename>. It's the JPEG image comment rather than the EXIF data. To set it use exiv2 mo -c.

Bryan.TongMinh wrote:

(In reply to comment #2)

Outstanding issues:

  • $wgMaxImageArea should only be used for scalers which decompress the source

image. This is essential if VIPS deployment is going to allow larger PNGs to be
uploaded. Needs a little bit of refactoring.

r99911

  • The "comment" parameter in $scalerParams is not used, so that feature would

disappear entirely. Maybe shell out to exiv2 to add comments. $wgExiv2Command
is available in core and is used by PagedTiffHandler already.

r99912

  • Consider having PagedTiffHandler call VipsScaler to do the scaling, to reduce

code duplication and to get sharpening support in PagedTiffHandler.

Probably not going to have time for this the coming weeks.

(In reply to comment #6)

(In reply to comment #4)

To make the test feature without hacking into the 404-handler I would need to
stream the vips thumbnail through the Special page. Is vips available on all
mediawiki servers or only on the image scalers?

It looks like VIPS will be just on the scalers for now. I think the easiest way
to do it is to hit the image scalers using Http::request() with proxy =
rendering.svc.pmtpa.wmnet in $options. The URL could just be a secret subpage
of the special page, which would cause a thumbnail to be generated with
specified parameters.

If we need to fetch it over HTTP from the scalers, can't the browser just request it directly?

(In reply to comment #7)

If we need to fetch it over HTTP from the scalers, can't the browser just
request it directly?

There's no way for the browser to request that a special page request should be handled by the image scalers. They are on an internal network (10.0.0.0/8), they have no public IP. Squid routes thumb.php requests through to the image scalers, and we could add an extra URL regex to it to allow some particular special page request to be routed in the same way, but that seems like the wrong way to handle a one-off test page.

Bryan.TongMinh wrote:

I started on the VipsTest special page some days ago, but I'm not sure if I have time this weekend to finish this.

In any case we could already enable VipsScaler for large PNGs; those can't be compared to ImageMagick thumbnails anyway. I believe that there are no outstanding issues for this.

Config:

$wgVipsOptions = array(
array(

'conditions' => array(
 'mimeType' => 'image/png',
 'minArea' => 1.25e7,
)

)
);

FYI, I have created the VipsScaler component in Bugzilla.

Bryan.TongMinh wrote:

(In reply to comment #10)

FYI, I have created the VipsScaler component in Bugzilla.

Can you add me as default CC?

hashar added a comment.Nov 3 2011, 6:12 PM

(In reply to comment #11)

Can you add me as default CC?

Already did :-) I have added myself in as well.

Bryan.TongMinh wrote:

The JavaScript magic requested in comment #3 should be mostly done now.

TheDJ added a comment.Dec 4 2011, 1:04 PM

question, what does vips do with metadata ?

Bryan.TongMinh wrote:

(In reply to comment #15)

question, what does vips do with metadata ?

Nothing.

TheDJ added a comment.Dec 7 2011, 10:47 AM

Which m(In reply to comment #16)

(In reply to comment #15)

question, what does vips do with metadata ?

Nothing.

Does that mean it will strip any existing metadata ? or will it preserve ? I see it does apparently support the comment field, which we use to reference from thumb to the original image page ?

Bryan.TongMinh wrote:

(In reply to comment #17)

Which m(In reply to comment #16)

(In reply to comment #15)

question, what does vips do with metadata ?

Nothing.

Does that mean it will strip any existing metadata ? or will it preserve ? I
see it does apparently support the comment field, which we use to reference
from thumb to the original image page ?

It is purely a scaler, all the metadata stuff will still be handled by BitmapHandler.

What's the status of this bug?

Bryan.TongMinh wrote:

(In reply to comment #20)

What's the status of this bug?

We're waiting for somebody to invent a machine that creates free-time out of air for me.

On a serious note, the last problem we encountered iirc, was that reading PNG images is a WIO operation instead of PIO. I believe that this was fixed in some of the later releases of VIPS, but somebody should benchmark it to check whether that is the case.

And, yet again, Wikipedia decides not to do anything to fix pre-made solutions. Really, I do think Wikimedia should hire coders if they can't get by. This is basic competence-to-run-a-website stuff.

Adam: Please refrain from Bugzilla comments that have nothing specifically to do with the issue that you comment on. Feel free to discuss project prioritities, WMF behavior and high-level problems on https://meta.wikimedia.org/wiki/Wikimedia_Forum . Thanks.

(In reply to comment #22)

This is basic competence-to-run-a-website stuff.

So basic that you're incapable of resolving this bug yourself. :-)

Bryan is apparently the only person interested in this bug/extension and he's a volunteer with limited free time currently. Until you can find another volunteer (or organization!) to pick up maintenance of this extension, it's going to sit, as it isn't a priority for the Wikimedia Foundation.

Bryan.TongMinh wrote:

Also, I should note the incredible constructive feedback on this bug...

Just for your information, I can spend my free time fixing bugs for people that are aggressive and impolite, or I can spend my free time on doing things that does not involve dealing with these kind of people. I have decided already some time ago that it is going to be the latter. So, if you wonder why your comments are being ignored and bugs you report are not fixed, it might not be coincidence.

content hidden as private in Bugzilla

If you seriously think that 7 years waiting for something that's a basic part of my work on Wikipedia to be fixed so that it works isn't going to result in frustration, then I don't know what planet you live on.

I have never complained that you worked on it. But if you can't do it in about 7 years, with two or three solutions proposed then never fnished up, then clearly this isn't something that can be done by volunteers, and Wikipedia needs to just put funds towards a coder. Hiring a part-time coder would likely be a tiny fraction of the fundraising that gets splashed all over this site, and would mean thatt things didn't stay broken for years.

Seriously, after over half a decade, expect complaints.

brylie wrote:

Brian, it seems like you need more time. What are some blocking issues preventing this bug from getting resolved?

Adam, does your frustration stem from your need to be heard? Would timeliness or resolution help you to feel at ease?

I appreciate both of your honesty and cooperation, working together on this issue.

sumanah wrote:

I've updated https://www.mediawiki.org/wiki/VipsScaler so it links to the test page, bugs that this might fix, wikitech-l communications, etc.

People in bug 9497 (thumbnails of large PNGs aren't being generated, affecting maybe 1600 PNGs on Commons?) have said that VipsScaler is the real solution, but a commenter recently wrote of VipsScaler: "I think it can be considered dead now, implementing pngscale as an extension (like wikidiff2) or whatever seems definitely the way to go." So if you think VipsScaler is or isn't dead, you may want to mention that in bug 9497 so we have a better idea of how to move forward on that.

greg added a comment.Jun 24 2013, 7:51 PM

Jan/Tim/Reedy:

Right now VipsScaler (through the wmgUseVipsTest option) is only enabled on Mediawiki.org, test, and test2.

From my understanding, VipsScaler is ready for deployment across all wikis, yes?

Performance Review: done (by Tim I assume?)
Security Review: any substantial needs here?
Design Review: not needed, from my understanding.

Help clarifying this, please.

(In reply to comment #30)

From my understanding, VipsScaler is ready for deployment across all wikis,
yes?

Read comment 21.

It would be helpful to figure out exactly which version of VIPS you want to deploy. (Or rather, which version is installed on the relevant image scalers.)

Comment 9 suggests there wouldn't be much issue enabling this for now and improving it later. My guess is that this could be enabled on nearly every wiki except en.wikipedia.org and commons.wikimedia.org and would be fine. But this is a largely uneducated guess.

jgerber wrote:

(In reply to comment #30)

Jan/Tim/Reedy:
Right now VipsScaler (through the wmgUseVipsTest option) is only enabled on
Mediawiki.org, test, and test2.
From my understanding, VipsScaler is ready for deployment across all wikis,
yes?

new vips packages are available on apt.wikimedia.org and installed on the scalers for some time and the extension should be ready for deployment on more wikis.

greg added a comment.Jun 25 2013, 4:01 PM

(In reply to comment #32)

(In reply to comment #30)

Jan/Tim/Reedy:
Right now VipsScaler (through the wmgUseVipsTest option) is only enabled on
Mediawiki.org, test, and test2.
From my understanding, VipsScaler is ready for deployment across all wikis,
yes?

new vips packages are available on apt.wikimedia.org and installed on the
scalers for some time and the extension should be ready for deployment on
more
wikis.

Thanks much Jan, that's what I was thinking (that the image scalers already had the package, it just needed to be used).

Jan: There's only one question left that I have before we push it out everywhere: The wmgUseVipsTest setting variable implies, well, that it is only for a test instance.

I haven't looked at the code in any detail, but does this do anything that we don't want for our production wikis going forward? If not and it is just fine to deploy Vips via this config setting, then let's do that.

Can we rename the config variable, too? It might confuse third-parties :-)

Reedy added a comment.Jun 25 2013, 4:12 PM

reedy@mw1153:~$ dpkg -l | grep -i vips
ii libvips-doc 7.32.3-1wmf1 image processing system good for very large images (doc)
ii libvips-tools 7.32.3-1wmf1 image processing system good for very large images (tools)
ii libvips15 7.26.3-1build1 image processing system good for very large images
ii libvips31 7.32.3-1wmf1 image processing system good for very large images
reedy@mw1153:~$ apt-get install libvips31 -s

NOTE: This is only a simulation! apt-get needs root privileges for real execution. Keep also in mind that locking is deactivated, so don't depend on the relevance to the real current situation! Reading package lists... Done Building dependency tree Reading state information... Done libvips31 is already the newest version. libvips31 set to manually installed. The following packages were automatically installed and are no longer required: libpgm-5.1-0 libzmq3 Use 'apt-get autoremove' to remove them. 0 upgraded, 0 newly installed, 0 to remove and 47 not upgraded. reedy@mw1153:~$

Is there a newer version still?

Reedy added a comment.Jun 25 2013, 4:16 PM

(In reply to comment #33)

Jan: There's only one question left that I have before we push it out
everywhere: The wmgUseVipsTest setting variable implies, well, that it is
only
for a test instance.
I haven't looked at the code in any detail, but does this do anything that we
don't want for our production wikis going forward? If not and it is just fine
to deploy Vips via this config setting, then let's do that.
Can we rename the config variable, too? It might confuse third-parties :-)

It's only a configuration variable used by WMF, so we can do what we want. Dropping the "Test" part probably makes sense though

jgerber wrote:

@greg yes that variable should be renamed to $wmgUseVips (https://gerrit.wikimedia.org/r/70439)

@sam 7.32.3 is the latest version in debian and 7.32.3-1wmf1 is a backport of that (http://packages.debian.org/experimental/libvips31)

greg added a comment.Jul 12 2013, 9:48 PM

Update on the bug: We plan to push this out next Thursday (July 18th) pending the completion of bug 51149.

sumanah wrote:

Update: launch delayed pending the completion of bug 51370.

greg added a comment.Jul 25 2013, 6:41 PM

Deployed! Sorry about the delay!

TheDJ added a comment.Aug 9 2014, 1:21 PM

Bug 69336

It seems we never added the 'set comment' option for the configuration ?