Page MenuHomePhabricator

Increase the maximum size eligible for image transforms configurable from 4MB->16MB
ClosedPublic

Authored by nipunn on Nov 20 2015, 5:12 AM.

Details

Summary

Also increase the timeout for the external process to complete the transform.

Test Plan

Careful inspection

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nipunn updated this revision to Diff 35143.Nov 20 2015, 5:12 AM
nipunn retitled this revision from to RFC: Make the maximum size eligible for image transforms configurable.
nipunn updated this object.
nipunn edited the test plan for this revision. (Show Details)
nipunn added a reviewer: epriestley.
nipunn updated this revision to Diff 35144.Nov 20 2015, 5:20 AM
nipunn edited edge metadata.

Running unit tests

chad added a subscriber: chad.Nov 20 2015, 5:45 AM

Why not just increase the base to 8MB? Can you give some insight into the problems your seeing on your install?

Increasing seems reasonable to me too. The problem I see is a 4.1MB gif which doesn't get thumbnailed for the pinboard view. It's not a major issue, but it seemed one that was avoidable.

I think as a practical matter to make larger image transforms work consistently the timeout will also need to be bumped. We patch that locally because otherwise Business Critical Memes are inoperable, but as a DDoS vector it's iffy for public installs to have a large timeout. I'm not sure if large files are also a problem for public installs.. T5258 has some related discussion.

A one line patch isn't a big deal, but it would be mildly nice not to have it. Maybe we could have one config -- memes.max-awesomness -- that goes from 0-11 and scales both values? ๐Ÿ˜Ž

epriestley requested changes to this revision.Nov 23 2015, 3:20 PM
epriestley edited edge metadata.

I don't want to add an option, see T8227.

I'll accept a change of the constant 4 to 16, raising the hardcoded limit to 16MB. You should also change the hard-coded 10 second timeout to a larger timeout (like 60).

If you have GIFs larger than 16MB, contribute a transcoding / wemb / dedicated meme engine patch instead. The advance of hardware has failed outpace the advance of GIF sizes.

This may not actually fix the problem you're encountering, per T5258.

src/applications/files/transform/PhabricatorFileImageTransform.php
122โ€“124

Change this to 60.

This revision now requires changes to proceed.Nov 23 2015, 3:20 PM
nipunn updated this revision to Diff 35245.Nov 25 2015, 8:51 AM
nipunn edited edge metadata.

Only change the constants. No extra option

nipunn retitled this revision from RFC: Make the maximum size eligible for image transforms configurable to Increase the maximum size eligible for image transforms configurable from 4MB->16MB.Nov 25 2015, 8:53 AM
nipunn updated this object.
nipunn edited the test plan for this revision. (Show Details)
nipunn edited edge metadata.
nipunn updated this revision to Diff 35246.Nov 25 2015, 9:24 AM

Fix comment from 10->60

epriestley accepted this revision.Nov 25 2015, 2:42 PM
epriestley edited edge metadata.

Thanks!

src/applications/files/transform/PhabricatorFileImageTransform.php
122โ€“123

Sometimes I'll sneakly rewrite these as "more than a short time resizing..." or similar to avoid the sort of soft DRY issue, but then I keep writing the constants anyway.

This revision is now accepted and ready to land.Nov 25 2015, 2:42 PM
This revision was automatically updated to reflect the committed changes.