microsoft / PowerToys

Windows system utilities to maximize productivity
MIT License
111.68k stars 6.57k forks source link

Image Resizer always ignores images orientation with images #17202

Open gersteba opened 2 years ago

gersteba commented 2 years ago

Microsoft PowerToys version

"0.56.2"

Running as admin

Area(s) with issue?

Image Resizer

Steps to reproduce

✔️ Expected Behavior

The resized image should have 384px width and 768px height.

❌ Actual Behavior

The image still has 600px width and 1200px height. The behavior is independent of setting 'Ignore image orientation' - this setting seems to be ignored.

Other Software

No response

jaimecbernardo commented 2 years ago

I've tried replicating this, without success. With these options: image

It resized a 600x1200 image to 384x768.

Could you give it another try? Perhaps send the picture inside a zip file so we can test it? /needinfo

gersteba commented 2 years ago

Tried it also on another computer, no success. Here is the original file: 702022159_univ_lsr_xl.zip

jaimecbernardo commented 2 years ago

I've been able to replicate with the photo you've uploaded. Thank you.

This seems to be caused by the metadata in the photo https://docs.microsoft.com/en-us/windows/win32/properties/props-system-photo-orientation

/app1/{ushort=0}/{ushort=274} | 8

So the photo has metadata indicating that it is rotated 90 degrees. This makes it so that the Encoder reports to us that the photo actually is 1200x600. If I resize it to 768x1200 it then as the expected behavior. The 'Ignore image orientation' doesn't actually refer to this metadata orientation, but rather the orientation of the number of pixels that's reported to Image Resizer.

I wonder what the actual behavior should be here. Perhaps it makes sense that selecting Ignore Orientation would ignore the metadata orientation while resizing as well.

jaimecbernardo commented 2 years ago

This is fixable if we exchange width or height when we detect the orientation, but this might break expected behavior from before. For example, we can add some code right after https://github.com/microsoft/PowerToys/blob/b3a0bf79199f94e8494cdd5b59d8566d50116b63/src/modules/imageresizer/ui/Models/ResizeOperation.cs#L159-L161

Adding this makes it so this is fixed:

            var metadata = (BitmapMetadata)source.Metadata;
            ushort orientation = (ushort)metadata.GetQuery("System.Photo.Orientation");
            if (orientation == 8 // rotated 90 degrees
                || orientation == 6 // rotated 270 degrees
                )
            {
                var temp = width;
                width = height;
                height = temp;
            }

This might however break behavior for people that expect photos that are rotated to behave as they are right now. @CleanCodeDeveloper , I think you used Image Resizer more frequently. What are your thoughts here in terms of expected behavior?

jaimecbernardo commented 2 years ago

Another way around this would be to add a setting in settings to make this operation and call it something like "Ignore Orientation Metadata", with the description "If the image is rotated, use the file width/height for resizing instead of the orientation width/height."

CleanCodeDeveloper commented 2 years ago

This topic pops up from time to time. Most recent discussion was #14904. Takeaway is that the checkbox "Ignore the orientation of pictures" only makes sense if you use absolute values and resize more than one image.

I always wanted to asked @niels9001 if he could apply his UX Magic onto the ImageResizer UI. I can offer to do the the code changes according to some mockups.

htcfreek commented 2 years ago

Another way around this would be to add a setting in settings to make this operation and call it something like "Ignore Orientation Metadata", with the description "If the image is rotated, use the file width/height for resizing instead of the orientation width/height."

Feels like souch a setting should be in the resize dialog to be more flexible.

niels9001 commented 2 years ago

@jaimecbernardo What if we add a TeachingTip/Dialog when checking the 'Ignore..' CheckBox showing the metadata property qnd explaining that there might be some issues?

@CleanCodeDeveloper The latest concept (using UWP) can be found here: https://github.com/microsoft/PowerToys/issues/1053

I think the longer term plan is to port ImageResizer to WinUI 3 (esp. since the WindowsAppsSDK 1.0.1 that was released last week supports multiwindow). I'm happy to collaborate on that if that is something you'd like to pursue 😁👍

CleanCodeDeveloper commented 2 years ago

@niels9001 wow that's great! Didn't know that we already have mockups. What needs to be done that we can start working on this?

niels9001 commented 2 years ago

@CleanCodeDeveloper We'd need to port ImageResizer from WPF to WinUI 3. We can then re-use the XAML from the mockup project. I don't believe this is on the short term planning of the PowerToys team however - but if we move we could tackle this issue as well.

CleanCodeDeveloper commented 2 years ago

@niels9001: I found that WinUI3 was really rough when I tried it 2-3 month ago. Do you feel that Windows App SDK 1.0.x is ready or shall we wait for Windows App SDK 1.1?

jaimecbernardo commented 2 years ago

I feel like this should be a broader settings option, since what we're really doing is selecting what Image Resizer should consider as the actual size of the image, the size that is reported by the image file, taking into account that it might be rotated, or the size that is seen by the user in the OS?

Jay-o-Way commented 2 years ago

Is this issue about unexpected behavior related to rotating, or Winui3? For the first: to me it's simple. Users are reporting something isn't working because the do not see what they expect. Image Resizer must account for the metadata and make sure the visible result matches expectations. Please, no more settings.

CleanCodeDeveloper commented 2 years ago

Is this issue about unexpected behavior related to rotating, or Winui3?

It is about unexpected behavior not WinUi3. I asked Niels if he could something to improve the UX. He let me know that there are already mockups.

For the first: to me it's simple. Users are reporting something isn't working because the do not see what they expect.

I think the option is useful but should be named (and maybe explained) differently.

Please, no more settings.

I agree. I would add that the "Ignore the orientation of pictures" should only be visible in cases where it makes sense (absolute values, more than one image).