ENH Improve performance of InterventionBackend::getImageResource() #409
Conversation
|
The lines above say:
I assume that’s why we write a local file and only use streams as a fallback - we need exif data to correct orientation. Would that be broken by this change? |
Ah good point. Isn't EXIF data from streams supported from PHP 7.2 now? https://bugs.php.net/bug.php?id=65187 related: @dhensby looks like he was looking into using local paths where available... #34 (comment) |
|
@christopherdarling that would be good as presumably it would save an unnecessary copy into the temp directory. I wonder if we could somehow grab the underlying Flysystem adapter and check |
|
This needs linting. So is this speed improvement only for when the cropped images need to be generated and otherwise not a problem? If so, I feel like it's of limited importance, but if we can improve it easily we should. If exif data can be read from streams in 7.2 that's good, but we still support lower PHP versions and I think that we need to make sure that those don't see a regression. |
I don't think so. I think it's also an improvement for when a dimension (width/height) is needed which relies on getImageResource() also.
Looking into this today, hopefully. |
|
I've just sent up an alternative version which maintains the use of streams, no temp files in any case and extracts exif data and handles rotation (I think for iPhones). Some quick stats from a local dev site I'm working on at the mo'. No Intervention_Manipulations cache & Resampling 5 images 5 times (total 25 resampled images)
No Intervention_Manipulations cache & Resampling 1 image 5 times (total 5 resampled images)
No Intervention_Manipulations cache & Resampling files already generated - 1 image 5 resampled versions
So some quite considerable improvements in most cases Note: I haven't done any testing with any other asset stores like S3, just the default. |
|
@christopherdarling So that’s failing on PHP 7.1 (as you pointed out,
My order of preference is 3 > 1 > 4 > 2. Thoughts @silverstripe/core-team? |
|
Just had a look at our stats for PHP7.1:
|
|
Maybe, I'm misreading this, but looking at the Intervention implementation, sounds like it would make more sense to do a PR there to fix the underlying problem that forces us to create a physical file. https://github.com/Intervention/image/blob/master/src/Intervention/Image/Commands/ExifCommand.php |
|
While i agree, it looks like @dhensby was looking into this some time ago Intervention/image#745 so figured bed to resolve it here for the time being. |
|
I agree we should be fixing this in the upstream library - that's the whole point of open source; rather than implementing our own specific fix we can apply a generic fix in the upstream that helps everyone. |
| // somewhere along that process the stream changes and exif becomes | ||
| // unreadable | ||
| try { | ||
| $exif = @exif_read_data($stream); |
There was a problem hiding this comment.
We shouldn't be using @ to silence errors
| $exif = @exif_read_data($stream); | ||
|
|
||
| if (!$exif || !array_key_exists('Orientation', $exif)) { | ||
| throw new NotSupportedException('missing exif or exif[Orientation]'); |
There was a problem hiding this comment.
Why are we throwing and then catching (and ignoring) this exception?
|
Looks like @tractorcow had some ideas of how to implement this on your issue thread @dhensby. I presumed a jump from PHP 5.4 (in their composer.json) to 7.2 would have been a blocker? I won't bother implementing your feedback Daniel if the consensus is not to merge anyway? Not sure I'll be able to do the PR for the Intervention/Image repo and for the meantime I'll be running my fork, but if I do figure it out before anyone else gets to it I'll update this PR |
when using the default local filesystem
christopherdarling
force-pushed
the
patch-1
branch
from
4451900
to
521968f
Compare
|
This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now? |
|
Quite happy to do further work on the PR if it's likely to be accepted? I got the impression that it wouldn't be accepted so have since continued to use my fork, which seems to be working well on a number of sites. |
|
Looking back through the previous comments, you're right it doesn't seem likely. I'll close this one for now though it could be resurrected in the future if people feel this should be progressed further. |
|
Shame to hear that, it's a reasonable performance improvement for little
effort. Daniel previously tried to get similar to this into Intervention
and dont think he had any luck, so don't hold up much hope in it getting
fixed there...
…On Thu, 10 Sep 2020, 22:46 Steve Boyd, ***@***.***> wrote:
Closed #409 <#409>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#409 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABLO5ZKZC45UWSNCAPOAHLSFFCKRANCNFSM4OJGMFKQ>
.
|
|
From my perspective the main blocker was PHP 7.1 support. I can live with doing without the upstream patch if that seems impractical. I know that PHP 7.1 is EOL, but that occurred less than a year ago and dropping support for it will probably frustrate more people than the issue this PR resolves. InterventionBackend is designed to be a pluggable service. Perhaps the best bet is to offer this implementation and a second service and retain the previous code as LegacyInterventionBackend. If we implemented http://github.com/silverstripe/silverstripe-framework/issues/9683 then the yaml config could be smart enough to choose the legacy implementation for PHP 7.1. |
|
I still believe this needs to be fixed in the upstream library and the effort should be focused there - I've put some time into it myself, but it's a bit complicated because of the way intervention image is designed. |
when using the default local filesystem
From my tests, I'm seeing a speed improvement of 10-20% on requests that require the generation of
Intervention_Mamipulationcache files (see related merged PR #362) or image manipulations performed by GD.n.b. the 10-20% figure was based on a page using three reasonably sized original images (~3000px square), I presume the % will increase with the number of images on the page.