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

Default height to VIPS_MAX_COORD for vips_thumbnail #1639

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Default height to VIPS_MAX_COORD for vips_thumbnail #1639

wants to merge 1 commit into from

Conversation

kleisauke
Copy link
Member

This to prevent reduction in the vertical axis when the height is omitted. Marked this PR as draft since it changes the behavior of vips_thumbnail into a resize based on a specific axis (which is more common, I think) instead than a resize based on a square bounding box.

This PR resolves (partially) #709, but a better way to address that issue is to make width an optional parameter. This allows users to do this, for example:

// Resize to a width of 200 pixels, no matter what the height of the input image is
if( vips_thumbnail( filename, &image,
	"width", 200,
	NULL ) )
	return( -1 );

// Resize to a height of 200 pixels, no matter what the width of the input image is
if( vips_thumbnail( filename, &image,
	"height", 200, 
	NULL ) )
	return( -1 );

// Identity transform
if( vips_thumbnail( filename, &image, NULL ) )
	return( -1 );

Instead of the current behavior:

// Resize to a width of 200 pixels, no matter what the height of the input image is
if( vips_thumbnail( filename, &image, 200,
	"height", VIPS_MAX_COORD,
	NULL ) )
	return( -1 );

// Resize to a height of 200 pixels, no matter what the width of the input image is
if( vips_thumbnail( filename, &image, VIPS_MAX_COORD,
	"height", 200,
	NULL ) )
	return( -1 );

// Identity transform
if( vips_thumbnail( filename, &image, VIPS_MAX_COORD,
	"height", VIPS_MAX_COORD, 
	"size", VIPS_SIZE_DOWN,
	NULL ) )
	return( -1 );

This PR resolves kleisauke/net-vips#71 when merged.

@kleisauke
Default height to VIPS_MAX_COORD for vips_thumbnail
755ecee 
This to prevent reduction in the vertical axis when the height is omitted.

See: kleisauke/net-vips#71.
@jcupitt
Copy link
Member

jcupitt commented May 3, 2020

I'm not sure, this seems like quite a large behaviour change :-( It's likely to break a lot of downstream code.

How about adding a new operation with the new behaviour? thumbnail_axis perhaps?

@kleisauke
Copy link
Member Author

I'm also a little worried about this behavior/compatibility change. I created this PR mainly for reference purposes. I'll add the VIPS_MAX_COORD constant to NetVips instead.

A new thumbnail_axis function might be a little odd if it only sets the thumbnail->height here

libvips/libvips/resample/thumbnail.c

Lines 563 to 564 in 54bfa23

if( !vips_object_argument_isset( object, "height" ) )
thumbnail->height = thumbnail->width;

to VIPS_MAX_COORD instead. A new declaration which makes the width and height optional, for e.g.:

int
vips_thumbnail_axis( const char *filename, VipsImage **out, ... )

Is better, but having 2 functions that can do exactly the same thing is a bit weird.

Anyways, feel free to label this PR as snoozing. 😄

@jcupitt
Copy link
Member

jcupitt commented May 4, 2020

Yes, it would be odd :( Really we'd want to replace all the vips_thumbnail*() functions with your proposed new API, but we'd need to do it under a new name to avoid breakage.

vips_thumbnail2()? vips_load_and_resize()? They all seem ugly :(

@kleisauke kleisauke mentioned this pull request Jul 8, 2020
Open
@kaspergrubbe kaspergrubbe mentioned this pull request Aug 17, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Thumbnail width for portrait images
2 participants
@kleisauke @jcupitt