Fix image scaling to return the originals when no scaling is required

[datakurre]

Plone REST API use cases require cacheable unique URL also for the original version of the image 1).
Until now, this has caused some performance penalty, created unnecessary blob and actually delivered lower quality copy of the original. Now, if the scaling parameters would result to exact size with the original, with now other parameters (e.g. change in format), no scaling will happen, but the scale URL will return the original blob.

1. plone/plone.restapi@dd0ca2542ddc4ef868b78d89789711777ff94076&

[datakurre]

@jenkins-plone-org please run jobs

[datakurre]

@jenkins-plone-org please run jobs

[erral]

This is jot only a plone.restapi use-case but a general one, when one wants to create a full cacheable URL of the original image instead of using the @@images/image like url.

[datakurre]

@jenkins-plone-org please run jobs

[datakurre]

It seems this breaks plone.restapi tests, which do actually test that the REST API original download url is not the original, but a scale 🤔

[datakurre]

@jenkins-plone-org please run jobs

[datakurre]

@jenkins-plone-org please run jobs

[tisto]

@erral would you mind having a look at this failing test?

https://jenkins.plone.org/job/pull-request-5.2/1208/testReport/junit/plone.restapi.tests.test_dxfield_serializer/TestDexterityFieldSerializing/test_namedblobimage_field_serialization_returns_dict/

I suspect this if/else stmt is causing this:

https://github.com/plone/plone.restapi/blob/eba24db81509ea977406e416efda8cf07296e0e7/src/plone/restapi/tests/test_dxfield_serializer.py#L27

[erral]

Will do.

[erral]

This PR needs a slight change in plone.restapi too.

Here: https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/tests/test_dxfield_serializer.py#L346

The point is that now the original image URL is not a "scale" but the original image, so it is not converted to PNG or JPEG depending on Plone version.

With the previous code we will have always a recreated scale so the URL of the item was also changed. Now, in the linked code lines, we need to distinguish between the original URL which will be ending in gif because the tested image is a gif, and the scale URLs which will be PNG or JPEG.

[bastei]

With this change the original image is used with all its metadata and with the original compression, which could easily be twice the size of a scaled representation of the same dimensions. Is that really intended?

[datakurre]

@bastei Shouldn't original be original? Also I assume that current implementation might convert optimized JPEG2000 into older JPEG version, probably increasing the size even dimensions stay and compression is added. 🤔

If it is really intended that the "original" means compressed version of the original just with matching dimensions. Then this pull could be dismissed.

[tisto]

@erral thanks for the clarification. I actually kind of got that point. Though, I don't fully understand the test and how it works for the different configurations. I usually try really hard to keep those if/else statements out of tests because of this. When reading the test I have to first check what GIF_SCALE_FORMAT is. Then I have to find the if/else and this is where Iost track. Why did that change in Plone 5.1? So my recommendation would be that we try to refactor the plone.restapi test to work without those indirections and the if/else statements if that's possible. Since you wrote the test code if I am not mistaken I was hoping you could help us with that. :)

[bastei]

@datakurre Well, I'm not sure if I have an overview of all the use cases here, but isn't it that the original image would be submitted only because a scale of the same dimensions (possibly by accident) is requested? So not the original image is requested, but only a (web-optimized) image representation with the dimensions of the original image.

As to JPEG2000: There is nearly no support in browsers for that compression, so not converting it would be a real problem. I think providing modern compressions is out of scope here. It involves providing the same scale in different compression formats, but again: Not necessarily the one of the original. In my opinion, providing an ideal web-optimized representation of an image should be the responsibility of Plone, not the editor.

[erral]

@tisto Yes, it's not easy to follow. At some point in Plone 5.1 the images scales started being created as PNG images instead of JPEG as previously. According to plone.scale's changelog this was changed in plone.scale 2.0. Vanilla Plone 5 uses plone.scale 1.4.2 and Vanilla Plone 5.1 uses plone.scale 3.0.

So what this test tries to do is to compare the URLs of the generated scales. We are using a GIF image as a test image in plone.restapi. As the URLs differ depending on the Plone version (ones are whatever.jpg and other whatever.png), I needed that GIF_SCALE_FORMAT to identify the extension.

I could write the test in some other way, for instance instead of checking the full URL, check that it contains the UUID of the generated scale, or something like that.

Anyway we need a new branch in plone.restapi so we can test it against the changes of this PR. I can work on it, no problem.

[datakurre]

@bastei @erral @tisto About this pull removing a feature of "compressed scale with original dimensions". Well, my understanding is that the current "compressed original" image scale was unintentional side-effect of generating unique URL for the original (for caching). So, the original scale was supposed to be the original image, not compressed scale. (This is also how the "original" size has traditionally worked in Plone.)

But if also "compressed image with original dimensions" is now a use case, that is new to Plone and should be implemented as a new dynamically calculated scale for plone.scale, maybe. So that also in plone.restapi represetation https://plonerestapi.readthedocs.io/en/latest/plone-content.html#image it would be a "scale" not "download". "download" IMO should be the original, not compressed one.

[sneridagh]

@datakurre I think our original assumptions are correct, and the ones who implemented that confirmed it. It's a side effect. It should be the original, otherwise you don't have a way using shorthands to get it. I would stick to the original plan.

[erral]

Yes, I understand the same.

[tisto]

@datakurre couldn't agree more. We introduced a bug in plone.restapi and now we are fixing that. Easy as this. :)

[bastei]

@datakurre Sorry, maybe I was unclear. "Download" should be the original image, of course! What I meant was actually a "scale". If I load a scale that has by chance the same dimensions as the original image, I don't get a scale (in terms of compressed image) anymore, but the original image. Or am I wrong?

[datakurre]

@bastei Thanks for correcting! That's been taken account with not parameters in https://github.com/plone/plone.namedfile/pull/92/files#diff-03f29bbadcdf640341c9061de5a07dccR221 So, when you explicitly give quality as a scaling parameter, the compressed scale is being created as before.

And that's been also tested in https://github.com/plone/plone.namedfile/pull/92/files#diff-c69449fd7e2eea6ccbfd529cae4b114dR98

[erral]

I have fixed the tests on plone.restapi side ti adapt to these changes.

[bastei]

@datakurre Thanks for the explanation. The distinction between "download" and "scale" is not really straightforward from my point of view, but I understand the intention.
So the assumption is, that in every case of a generation of a scale, arguments are set that don't correspond to a formal parameter (e.g. quality). I have to admit that I have difficulties to identify the points where this is the case at all, in particular not for the generation of scales in plone.restapi. If you don't mind I would be happy if you could elaborate on how that works together so I can expand my narrow picture of Plone. :)

[sneridagh]

@jenkins-plone-org please run jobs

[tisto]

Ok. I guess I have to set up buildout coredev locally with p.restapi and p.namedfile checkout and then merge both PRs together...will look into it...

[datakurre]

I recall that Jenkins UI allows to pass list of PRs for tests.

[erral]

Yes, that's what I did, and Jenkins manages to inform both PRs that the tests are green or they failed.

[tisto]

@erral do you have a link to that Jenkins job?

[tisto]

https://jenkins.plone.org/job/pull-request-5.2/1243/
https://jenkins.plone.org/job/pull-request-5.2-3.6/1201/
https://jenkins.plone.org/job/pull-request-5.2-3.7/1180/
https://jenkins.plone.org/job/pull-request-5.2-3.8/43/

[tisto]

@jensens do you want to have a look at this PR as well before we merge?

[tisto]

Jenkins is green and we have three approvals. Will merge now. Then we can test it in the p.restapi PR...