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.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 9, 11:35 PM
Unknown Object (File)
Wed, Mar 6, 4:47 AM
Unknown Object (File)
Wed, Mar 6, 4:47 AM
Unknown Object (File)
Wed, Mar 6, 4:47 AM
Unknown Object (File)
Wed, Mar 6, 4:47 AM
Unknown Object (File)
Wed, Mar 6, 4:46 AM
Unknown Object (File)
Wed, Mar 6, 4:46 AM
Unknown Object (File)
Feb 12 2024, 2:16 PM

Details

Summary

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

Test Plan

Careful inspection

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 edited edge metadata.

Running unit tests

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 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 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.
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.