Setting to major because this is a major performance issue.

The whole idea of transformDimensions is to be able to calculate the size of an image *without* having to access it.

\Drupal\crop\Plugin\ImageEffect\CropEffect::transformDimensions() creates an image toolkit object with the *only* purpose of reading out the source again.

I'm also not sure if it's really valid to store $this->crop, as I could imagine that the same instance could be used for different images, but not sure about that.

CommentFileSizeAuthor
#2 crop-effect-avoid-io-2868638-2.patch725 bytesberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new crop-effect-avoid-io-2868638-2.patch725 bytes

This, basically, actually results in less code even. I think it is unlikely that this is called multiple times for the same image in the same request, so the optimization to not load multiple times probably isn't worth it but I have no numbers on that.

woprrr’s picture

woprrr commented
Status: Needs review » Reviewed & tested by the community

Hi @berdir,

This work like a charm :) ! This is important to avoid all uneeded complexity.

I have tested all usecases and works well (Image Widget Crop / Crop API manually / Focal point).

(This work perfectly into 1.x and 2.x branches).

  • woprrr committed 427045e on 8.x-1.x authored by Berdir 
    Issue #2868638 by Berdir, woprrr: CropEffect::transformDimensions()...

  • woprrr committed 435fdaf on 8.x-2.x authored by Berdir 
    Issue #2868638 by Berdir, woprrr: CropEffect::transformDimensions()...
woprrr’s picture

woprrr commented
Status: Reviewed & tested by the community » Fixed

That can be very informative to have more feedback about gain of performance. In all cases everything works well with that patch.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.