Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Downscale pasted PNG images based on metadata #29123

Merged
merged 25 commits into from
Feb 19, 2024
Merged

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Feb 10, 2024

Some images like MacOS screenshots contain pHYs data which we can use to downscale uploaded images so they render in the same dppx ratio in which they were taken.

Before:

image

After:

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 10, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 10, 2024
@silverwind silverwind added the type/enhancement An improvement of existing functionality label Feb 10, 2024
web_src/js/features/comp/ImagePaste.js Show resolved Hide resolved
web_src/js/utils/image.js Outdated Show resolved Hide resolved
web_src/js/utils/image.js Outdated Show resolved Hide resolved
@silverwind silverwind changed the title WIP: Downscale images based on png pHYs data WIP: Downscale images based on png metadata Feb 10, 2024
@silverwind
Copy link
Member Author

Dependency is removed 🎉.

I'm surprised how little code actually came out in the end. ~18 lines replacing over 100 lines from the dependency.

@silverwind silverwind changed the title WIP: Downscale images based on png metadata Downscale pasted images based on png metadata Feb 11, 2024
@silverwind
Copy link
Member Author

This is ready. I checked the dropzone-attached images, they always render at 120x120, so downscaling them does not really make sense.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 11, 2024
@silverwind
Copy link
Member Author

Various refactors done, still ready for review.

export async function pngInfo(blob) {
let width = 0;
let dppx = 1;
if (blob.type !== 'image/png') return {width, dppx};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nit, TBH, I never saw a function returns a valid "struct" when it can't parse something, the literal meaning of "{width: 0}" for a non-png image seems confusing.

Maybe it should throw an exception or return null?

Copy link
Member Author

@silverwind silverwind Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is something I thought about changing as well. Previously it was called imageInfo and in such a case it made sense to fall back to default values for non-png. I will at least move the type check out of the function, not sure about throwing, thought.

Copy link
Member Author

@silverwind silverwind Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it back to imageInfo as I could see it being extended with other types like webp oder jpg later.

I disagree about returning null or throwing, it would complicate the call site of the function unecessary, and right now because we always return a object the call site can stay compact and tidy.

Also I think it's good to have a static return structure which will make it a breeze to convert into Typescript later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree about returning null or throwing, it would complicate the call site of the function unecessary, and right now because we always return a object the call site can stay compact and tidy.

Well, I have never seen a widely-used framework does so. (not a blocker, just my thought)

Copy link
Member Author

@silverwind silverwind Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One example would be various React APIs where it's commonplace to return objects that are meant to be destructured. For example from https://docs.dndkit.com/api-documentation/draggable/usedraggable:

const {attributes, listeners, setNodeRef, transform} = useDraggable();

The object is just there to represent multiple named return values. Such functions should of course never return non-objects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's different. I guess useDraggable never parses something, and it never fails.

Copy link
Member Author

@silverwind silverwind Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These use* functions can also fail, but the point is they would throw instead of returning a non-object value (which would lead to a destructuring error at the caller).

I checked what is needed to return null in our case and its at least +6 lines for no real benefit.

@pull-request-size pull-request-size bot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 15, 2024
@pull-request-size pull-request-size bot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 15, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 16, 2024
@silverwind silverwind requested a review from delvh February 16, 2024 21:26
@delvh delvh changed the title Downscale pasted images based on png metadata Downscale pasted PNG images based on metadata Feb 18, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 18, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 19, 2024
@lunny lunny added this to the 1.22.0 milestone Feb 19, 2024
@lunny lunny enabled auto-merge (squash) February 19, 2024 02:19
@lunny lunny merged commit 5e72526 into go-gitea:main Feb 19, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 19, 2024
@silverwind silverwind deleted the imgscale branch February 19, 2024 08:33
silverwind added a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
Some images like MacOS screenshots contain
[pHYs](http://www.libpng.org/pub/png/book/chapter11.html#png.ch11.div.8)
data which we can use to downscale uploaded images so they render in the
same dppx ratio in which they were taken.

Before:

<img width="584" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/50979e3a-5d5a-40dc-a0a4-36eb6e28f14a">

After:

<img width="329" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/0690902a-f2fe-4c6b-97b3-6fdd67c21bad">
silverwind added a commit to silverwind/gitea that referenced this pull request Feb 21, 2024
Some images like MacOS screenshots contain
[pHYs](http://www.libpng.org/pub/png/book/chapter11.html#png.ch11.div.8)
data which we can use to downscale uploaded images so they render in the
same dppx ratio in which they were taken.

Before:

<img width="584" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/50979e3a-5d5a-40dc-a0a4-36eb6e28f14a">

After:

<img width="329" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/0690902a-f2fe-4c6b-97b3-6fdd67c21bad">
silverwind added a commit to silverwind/gitea that referenced this pull request Feb 22, 2024
Some images like MacOS screenshots contain
[pHYs](http://www.libpng.org/pub/png/book/chapter11.html#png.ch11.div.8)
data which we can use to downscale uploaded images so they render in the
same dppx ratio in which they were taken.

Before:

<img width="584" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/50979e3a-5d5a-40dc-a0a4-36eb6e28f14a">

After:

<img width="329" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/0690902a-f2fe-4c6b-97b3-6fdd67c21bad">
silverwind added a commit to silverwind/gitea that referenced this pull request Feb 22, 2024
Some images like MacOS screenshots contain
[pHYs](http://www.libpng.org/pub/png/book/chapter11.html#png.ch11.div.8)
data which we can use to downscale uploaded images so they render in the
same dppx ratio in which they were taken.

Before:

<img width="584" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/50979e3a-5d5a-40dc-a0a4-36eb6e28f14a">

After:

<img width="329" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/0690902a-f2fe-4c6b-97b3-6fdd67c21bad">
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024
Some images like MacOS screenshots contain
[pHYs](http://www.libpng.org/pub/png/book/chapter11.html#png.ch11.div.8)
data which we can use to downscale uploaded images so they render in the
same dppx ratio in which they were taken.

Before:

<img width="584" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/50979e3a-5d5a-40dc-a0a4-36eb6e28f14a">

After:

<img width="329" alt="image"
src="https://github.com/go-gitea/gitea/assets/115237/0690902a-f2fe-4c6b-97b3-6fdd67c21bad">

(cherry picked from commit 5e72526)
Copy link

Automatically locked because of our CONTRIBUTING guidelines

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants