ISSUE-680: proper /MAX handling under pre-cropped and un-cropped scenarios #709
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is as small as i could make the code work. Basically i don't use the Image Size (source) directly, i request from the Operation List a "size so far". And from there on Max/Scaling works on that new Size (if there is any. E.g FULL will give me same as the Source Size/no cropping). This also requires that the scaled Size is based on that, since (another side effect of the previous code) Crop Operations in excess (e.g requesting more pixels than present, which is IIIF valid) would end with a scaling that would not respect the original cropping)
This method requires pre calculated region width/height, given that we don't parse Operations (or have access to operations, just the URI and some expectations). Basically if set `uri` to
/iiif/{version}/{image-60-by-60}/0,0,50,50/max/color.png
The function requires this invocation
`testRegionToMaxPixels(uri,60,60,50,50,1000)`
Just for clarity. I added both pct and pixel based ones. This covers regions that are in area less than MAX_PIXELS and also those that exceed them
|
I'm getting Tests failing here (not related to this pull? -- yes, related to this pull).
Fixing this |
…exception If the arguments passed for crop are not the right ones. So we have to be more selective (so much for short/clean code), just pick the Crop operation (if any), validate (so we can throw a catchable exception later on) and then request the resulting size only for that one. Don't love reusing requestedSize (bc of read-ability) but better than creating yet another Objects
|
FYI: This assumes there is max. a single Crop (or extending class) operation in the Operation List. I don't know if somehow one could via a normal processing chain add more than one at all? but maybe (via code?) someone would? |
jcoyne
approved these changes
Nov 6, 2024
glenrobson
approved these changes
Nov 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Solves #680
I tried to be as minimalistic (code-wise) as possible (after some iterations that had nothing of minimal).
HOW?
Basically now, before processing
MAXat::constrainSizeToMaxPixels, we request from the Operation List the actual "So far what size would this image result in?", and use thatDimensionas base for all calculations. This way if aREGION/CROP(if any) results in less pixel area thanMAX_PIXELS,MAXhas no effect, but if not, scaling is done proportional to the REGION requested (including ifREGION\CROPexceeded the actual source dimensions .. which is IIIF valid), and will then constrain the final dimensions to fit into a MAX_PIXELS sized region.As always, test code is > than the actual fix :)
Note: according to me this works (tested with #680 image too). Test pass. But would be nice though if someone had the time to give this a human test.