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.

Sign up for GitHub

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

Jump to bottom

@uppy/thumbnail-generator: don't enlarge already small thumbnails #4406

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

@uppy/thumbnail-generator: don't enlarge already small thumbnails #4406

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6ff9aa
Don't enlarge thumbnail image — only resize if larger than target
arturi Apr 7, 2023
0d08610
Also add dimensions to file.meta
arturi Apr 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions packages/@uppy/thumbnail-generator/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@ export default class ThumbnailGenerator extends UIPlugin {

return Promise.all([onload, orientationPromise])
.then(([image, orientation]) => {
const originalImageDimensions = { width: image.width, height: image.height }
this.uppy.setFileMeta(file.id, { dimensions: originalImageDimensions })
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this being stored somewhere else? is thumbnail-generator the right place to add this metadata? (it doesn't have anything to do with thumbnails I think?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean, stored somewhere else? Already, you mean? Not that I know of.

No, it’s not the right place, ideally we have a separate exif plugin that does this. But:

  1. It’s available “for free” here, we already read the original image to memory, so I thought why not store useful data (some people have asked for it).
  2. Alex asks the preview to behave differently, according to it’s size — if the image is smaller than container, like a tiny emoji, we have a white background for it, for instance. Ideally we need to know the image dimensions in CSS. So this would help here.

I also understand it doesn't feel “correct” to add this here, but since I was in this territory, decided to experiment and gather feedback.

Copy link
Contributor

@mifi mifi Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean, stored somewhere else? Already, you mean? Not that I know of.

I just thought that width and height is very common metadata for an image so I thought we are already setting it somewhere else in the code.

  • It’s available “for free” here, we already read the original image to memory, so I thought why not store useful data (some people have asked for it).

Yea, but then if someone needs the dimensions, but they don't want the thumbnail plugin, then they won't get it. and if we later add this to core, then there will be two different width/height metadata, which would be confusing.

2. Alex asks the preview to behave differently, according to it’s size — if the image is smaller than container, like a tiny emoji, we have a white background for it, for instance. Ideally we need to know the image dimensions in CSS. So this would help here.

Shouldn't it instead be stored inside plugin state then, because it's a thumbnail plugin specific state?

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it instead be stored inside plugin state then, because it's a thumbnail plugin specific state?

👍


const dimensions = this.getProportionalDimensions(image, targetWidth, targetHeight, orientation.deg)
const rotatedImage = rotateImage(image, orientation)
const resizedImage = this.resizeImage(rotatedImage, dimensions.width, dimensions.height)
Expand All @@ -183,17 +186,25 @@ export default class ThumbnailGenerator extends UIPlugin {
aspect = img.height / img.width
}

let targetWidth = width
let targetHeight = height

// Thumbnail shouldn’t be enlarged / upscaled, only reduced.
// If img is already smaller than width/height, leave it as is.
if (img.width < width) targetWidth = img.width
if (img.height < height) targetHeight = img.height

if (width != null) {
return {
width,
height: Math.round(width / aspect),
width: targetWidth,
height: Math.round(targetWidth / aspect),
}
}

if (height != null) {
return {
width: Math.round(height * aspect),
height,
width: Math.round(targetHeight * aspect),
height: targetHeight,
}
}

Expand Down