Page MenuHomePhabricator
Differential D14528

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

Authored by nipunn on Nov 20 2015, 5:12 AM.
Tags
None
Referenced Files
F14057000: D14528.diff
Sat, Nov 16, 11:57 PM
F14043009: D14528.id.diff
Tue, Nov 12, 7:17 AM
F14039745: D14528.id35246.diff
Mon, Nov 11, 6:15 AM
F14029470: D14528.id35248.diff
Fri, Nov 8, 8:44 PM
F14029468: D14528.id35245.diff
Fri, Nov 8, 8:44 PM
F14029466: D14528.id35144.diff
Fri, Nov 8, 8:44 PM
F14029464: D14528.id35143.diff
Fri, Nov 8, 8:44 PM
F14029463: D14528.id.diff
Fri, Nov 8, 8:44 PM
View All Files
Subscribers
cburroughs
chad
joshuaspence
Korvin

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Commits
Restricted Diffusion Commit
rPc4ea1e6e21d1: Increase the maximum size eligible for image transforms configurable from 4MB…
Summary

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

Test Plan

Careful inspection

Diff Detail

Repository
rP Phabricator
Branch
param
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 9054
Build 10652: arc lint + arc unit

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.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptNov 20 2015, 5:12 AM
Herald added a subscriber: Korvin. · View Herald Transcript
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?

chad added a comment.Nov 20 2015, 5:46 AM

See T8227: Why not just add an option/setting/preference? for more detail.

nipunn added a comment.Nov 20 2015, 6:43 AM

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.

cburroughs added a subscriber: cburroughs.Nov 20 2015, 2:46 PM

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? 😎

joshuaspence added a subscriber: joshuaspence.Nov 22 2015, 2:39 AM
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
Closed by commit rPc4ea1e6e21d1: Increase the maximum size eligible for image transforms configurable from 4MB… (authored by nipunn, committed by epriestley). · Explain WhyNov 25 2015, 2:42 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
applications/
files/
transform/
4 lines

Diff 35245

View Options

src/applications/files/transform/PhabricatorFileImageTransform.php

Loading...
Phabricator · Built by Phacility