Add more options to image resizing

[twodoorcoupe]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:

Problem

• JIRA ticket (optional): PICARD-XXX

As suggested by zas, image resizing could do with allowing the user to:

1. Ignore either width or height
2. Scaling up images as well

Solution

Here's the new options ui:

Unfortunately, I think this complicates the code quite a bit, due to all the different combinations of options. So I'm still trying to figure out if it can be improved on that front.

Also, I'm still working on creating the unit tests for each of those combinations.

Another doubt I have: for scaling up, I assumed the image should keep the aspect ratio with respect to the largest target dimension. As opposed to scaling down, for which we keep the aspect ratio with respect to the smallest target dimension.

Action

Additional actions required:

• Update Picard documentation (please include a reference to this PR)
• Other (please specify below)

[rdswift]

Actually, for scaling down or up isn't there also the option to center crop?

Perhaps the options for each (tag and file) should be:

• target width (pixels)
• target height (pixels)
• scale down (checkbox)
• scale up (checkbox)
• image format (radio buttons):
  • stretch/shrink to dimensions
  • crop scaled overflow
  • keep aspect ratio

Some examples of cropping and keeping the aspect ratio with target dimensions of 1000x1000px:

Type	Original	Final	Notes
crop	1200x1000	1000x1000	100 px removed from left and right sides
crop	2400x2000	1000x1000	image first scaled to 1200x1000 and then 100 px removed from left and right sides
crop	600x500	1000x1000	image first scaled to 1200x1000 and then 100 px removed from left and right sides
crop	1000x1200	1000x1000	100 px removed from top and bottom
crop	2000x2400	1000x1000	image first scaled to 1000x1200 and then 100 px removed from top and bottom
crop	500x600	1000x1000	image first scaled to 1000x1200 and then 100 px removed from top and bottom
keep	1200x1000	1000x833	image scaled to match maximum dimension (width)
keep	500x400	1000x800	image scaled to match maximum dimension (height)
keep	1000x1200	833x1000	image scaled to match maximum dimension (height)
keep	400x500	800x1000	image scaled to match maximum dimension (width)

[twodoorcoupe]

Thank you rdswift for the detailed breakdown!

Actually, for scaling down or up isn't there also the option to center crop?

There is not one out of the box when scaling, but there is a separate method to crop, so I can just use that one instead. I should be able to make cropping a separate ImageProcessor that runs after resizing.

[zas]

There's an issue with UI: Width & Height checkboxes are checked while the spinner is disabled on first load.

Unchecking/checking those once fixes it.

[twodoorcoupe]

I guess having both the options to scale up and down creates some chances for ambiguity?

A bit of an edge case, but for example with these options:

If we have a 1000 x 1000 image, should it be scaled down to 800 x 800 or up to 1200 x 1200? For now I gave priority to scaling down.

Since rdswift suggested to also add the options to stretch and to square crop, should I add them in this pr?

[rdswift]

If we have a 1000 x 1000 image, should it be scaled down to 800 x 800 or up to 1200 x 1200? For now I gave priority to scaling down.

Wow, that's a good question. Personally, I think it might depend on the image format option selected. If it's "stretch/shrink", then I think the final size would be 1200x800 (and the image deformed). It it's "crop", then the final size would be 1200x800 (scaled up to 1200x1200 then crop 200px off of the top and bottom). If it's "keep" then the final size would be 800x800 (scaled to make the the largest image that does not exceed either of the target dimensions).

Note that in both the "crop" and "keep" options, you really need to compare the horizontal and vertical dimensions of the scaled image (keeping the original image aspect ratio) against the target dimensions to determine how much the source image is scaled. If "crop", then the image is scaled initially so that one target dimension can be exceeded with the other dimension at the target size, and then crop the overflow. If "keep", then the image is scaled such that at least one of the target dimensions is achieved, but neither target dimension is exceeded.

The process (which applies to only the "crop" and "keep" options) would be something like:

1. Calculate the scaled image vertical size (keeping the original image aspect ratio) to match the horizontal target size (new_v = target_h / source_h * source_v)
2. If "crop":
   • If new_v < target_v then scaling factor is target_v / source_v, which matches the vertical target dimension and produces horizontal overflow to crop.
   • If new_v > target_v then scaling factor is target_h / source_h, which matches the horizontal target dimension and produces vertical overflow to crop.
   • If new_v = target_v then either target_v / source_v or target_h / source_h can be used as the scaling factor because they should be identical and match both the horizontal and vertical target dimensions.
3. If "keep":
   • If new_v < target_v then scaling factor is target_h / source_h, which matches the horizontal target dimension and produces no vertical overflow.
   • If new_v > target_v then scaling factor is target_v / source_v, which matches the vertical target dimension and produces no horizontal overflow.
   • If new_v = target_v then either target_v / source_v or target_h / source_h can be used as the scaling factor because they should be identical and match both the horizontal and vertical target dimensions.

EDIT: In short, for the 'stretch/shrink' and 'crop' cases, the output image should always match the target dimensions. Of course this also depends on whether the appropriate "scale up" or "scale down" option is enabled. For example, if the scaling factor is > 1 the image should only be scaled up if the "scale up" option is enabled. Similarly, if the scaling factor < 1 the image should only be scaled down if the "scale down" option is enabled. Even if an image is not scaled, cropping may still be applicable if one of the source image dimensions exceeds the corresponding target dimension.

[twodoorcoupe]

Thank you again rdswift, this was incredibly helpful!

If "keep", then the image is scaled such that at least one of the target dimensions is achieved, but neither target dimension is exceeded.

This is exactly what I couldn't understand at first. Now it's like you described.

For example, if the scaling factor is > 1 the image should only be scaled up if the "scale up" option is enabled. Similarly, if the scaling factor < 1 the image should only be scaled down if the "scale down" option is enabled.

This makes a lot of sense, but it gets complicated when you have an image with one dimension above the target and the other below, so one scaling factor is above 1 and the other below (like the example above).

---

I added options for cropping and stretching, now the options look like this:

I'm still not sure about the names of those radio buttons, or if I should add tooltips.

[zas]

if I should add tooltips.

Adding a tooltip with a short example might help.

[rdswift]

This makes a lot of sense, but it gets complicated when you have an image with one dimension above the target and the other below, so one scaling factor is above 1 and the other below (like the example above).

Not if you follow the process I outlined above to calculate the scaling factor. In that process there is only one scaling factor produced.

On another note, if you wanted to simplify things a bit you could leave out the "stretch/shrink" option (even though it's probably the easiest to code). I may be wrong, but I can't see too many people (if any) using that because it intentionally distorts the image. That would leave two scaling options -- "fit" and "fill", defined as:

• fit: Scale the source image so that it fits within the target dimensions. One of the final image dimensions may be less than the target dimension if the source image and target dimensions have different aspect ratios.

• fill: Scale the source image so that it completely fills the target dimensions in both directions. If the source image and target dimensions have different aspect ratios, then there will be overflow in one direction which will be (center) cropped.

[rdswift]

I'm still not sure about the names of those radio buttons, or if I should add tooltips.

I agree with @zas that tooltips with explanation or example would be beneficial. As for names, perhaps "fit" and "fill" with the tooltips containing the descriptions I provided above?

[twodoorcoupe]

Not if you follow the process I outlined above to calculate the scaling factor. In that process there is only one scaling factor produced.

Sorry, at first I could not get that to work with the options to either ignore width or height.

Now I went back and used your process for when both dimensions are considered. Otherwise, it considers each dimension's scale factor independently. (For stretch it always considers both dimensions independently)

Consider that I use the scale factor only for deciding which images should be resized and which should not. For the resizing itself, I use the QImage functions that handle different aspect ratio modes automatically.

As for names, perhaps "fit" and "fill" with the tooltips containing the descriptions I provided above?

These sound perfect, thank you!

Since the tooltip text items are repeated for different widgets, perhaps they could be saved as constants/variables and applied as required in this file rather than the options_cover_processing.ui file.

Makes sense, I just had to make it rich text, otherwise it wouldn't word wrap for whatever reason.

A final question, should I keep or remove the option to stretch images? It doesn't complicate the code by that much.

[rdswift]

Sorry, at first I could not get that to work with the options to either ignore width or height.

Now I went back and used your process for when both dimensions are considered. Otherwise, it considers each dimension's scale factor independently. (For stretch it always considers both dimensions independently)

Okay, I guess I wasn't paying attention because I missed that the target sizes might only be selected for one dimension. Sorry about that. I'm not sure I understand the reason for this or the use case, but having one of the dimensions optional obviously makes my suggested process for calculating scaling factor inappropriate in that case.

On that note, the fill processing option should only be available when both target dimensions are specified.

[phw]

Do we really want to have the option to scale up? What is the use case here?

[rdswift]

A final question, should I keep or remove the option to stretch images? It doesn't complicate the code by that much.

I have no objection to keeping it, although I expect it won't be used (but I've been wrong many times when I make this kind of assumption). Note that this option should also only be available if the target size is specified in both dimensions.

What you may want to consider is to automatically change the scale processing radio button to "fit" if one of the target dimensions is not specified, and disable the scaling entirely if neither target dimension is specified (or disallow both target dimensions from being deselected if scaling is selected).

[rdswift]

Do we really want to have the option to scale up? What is the use case here?

I can't think of a case where anyone would want to do that, but too often I've been surprised by what some users request (and expect). 😜

[twodoorcoupe]

Fixing color here isn't a good idea, it might cause issues depending on themes.

Right I'm sorry about that. I removed that and added a line break between the description and the example of each tooltip.

I needed to make the tooltip's text rich text somehow, or else it refuses to word wrap and would just look wonky.

On that note, the fill processing option should only be available when both target dimensions are specified.

Note that this option should also only be available if the target size is specified in both dimensions.

For cropping and stretching, if a dimension is disabled, its target value becomes the downloaded image's dimension. So the ignored dimension doesn't change when processed.

I understand that it's a combination of options that it's most likely never going to be used, but I'm not sure if it makes sense to outright not allow it?

Do we really want to have the option to scale up? What is the use case here?

Someone might want all their cover art images to be a certain size no matter what, even if only smaller images are available for a particular release, at the cost of losing quality.

I was asked to add these options to improve on image resizing in #2510, but let me know what needs to be kept or removed, after all you know Picard's users better than I do 😄

[twodoorcoupe]

From i18n point of view, instead of silently replacing newlines by HTML
it would be much better to make it explicit that's HTML-like rich text for translators.

Ah sorry, I assumed the opposite. Thank you for your patience zas!

[zas]

LGTM

[rdswift]

For cropping and stretching, if a dimension is disabled, its target value becomes the downloaded image's dimension. So the ignored dimension doesn't change when processed.

Is it the same for the "fit" option, where if a dimension is disabled it will default to the source image dimension in that direction? In that case, an image would never be scaled up, which is not what I would expect. For example, if the source image was 700x700 and only the horizontal target dimension is specified as 1000 (and scaling up was enabled), I would expect the final image to be 1000x1000.

If the "crop" and "stretch" options will default a missing target dimension to be the same as the source image dimension in that direction, but in the case of the "fit" option the missing option defaults to an infinitely high number (it is ignored), then our UI is inconsistent and could easily lead to user confusion.

In any event, whatever is decided needs to be made very clear to the user, because as it stands the behavior willl either be inconsistent between the processing options or it will be unintuitive. Perhaps there needs to be some text added to the UI, something like: "Note that if a target dimension is not specified for the 'crop' and 'fill' options, it defaults to the source image dimension in that direction, but in the case of the 'fit' option it defaults to an infintely large number."

[Aerozol]

Interesting discussion... It's too much for me to unpack fully, but my thoughts from the screenshots:

• Initially I thought the resize options could be renamed to 'max-width' and 'max-height', which would clarify things (this is what Adobe uses for similar UI)

The three options added later are very cool, but makes it more complex. I can think of a few options:

• Reduce it to two optional checkboxes, Stretch to fit + Crop to fit. Less technically detailed, but imo easier for the layperson to parse.
  
• Split out the options, which allows for more complexity to be added to specific options later, as well as de-cluttering the interface (but might be more work for you to implement?)
  

For reference, the Adobe Lightroom export UI:


Note that they only have a 'don't enlarge' button - I suppose scaling down is taken as implied, if this option is picked. Perhaps we should also remove the 'scale up' + 'scale down' options, and just have 'scale up' as an option at the bottom.

I'm aware I'm coming in late, so take or leave this input as wanted/needed.

[phw]

This works well and the code looks good.

My only concerns is the complexity, and specifically the UI complexity. I'm not convinced that the "Scale down" and also the "Stretch" option are actually useful. But if someone wants the options in I'll also no argue against.

But I also think my concerns can be addresses in a future iteration with some rework of the UI. Thanks @Aerozol to the suggestions.

Regardless I think we can get this merged now, as this is definitely another step forward. @twodoorcoupe Could you rebase this branch against master? Then the CI runs will also succeed again.

[twodoorcoupe]

For example, if the source image was 700x700 and only the horizontal target dimension is specified as 1000 (and scaling up was enabled), I would expect the final image to be 1000x1000.

Yes, this is the case.

In any event, whatever is decided needs to be made very clear to the user, because as it stands the behavior willl either be inconsistent between the processing options or it will be unintuitive.

I'll try to come up with something that does not complicate the UI even more 😅

---

Thank you @Aerozol for your valuable suggestions!

I definitely like the idea of moving the resize mode selection to a drop down list, and to have a "don't enlarge" check box at the bottom.

Like outsidecontext suggested, I will work on these changes in a separate pr. I was thinking it's best to wait until I've added the options to convert image formats, so as to have the whole options page before reworking it.

Regardless I think we can get this merged now, as this is definitely another step forward.

Ok, thank you!

[phw]

I was thinking it's best to wait until I've added the options to convert image formats, so as to have the whole options page before reworking it.

Yes, that makes a lot of sense

[zas]

LGTM