Skip to content

Default height to VIPS_MAX_COORD for vips_thumbnail#1639

Draft
kleisauke wants to merge 1 commit into
libvips:masterfrom
kleisauke:prevent-vertical-reduction
Draft

Default height to VIPS_MAX_COORD for vips_thumbnail#1639
kleisauke wants to merge 1 commit into
libvips:masterfrom
kleisauke:prevent-vertical-reduction

Conversation

@kleisauke

@kleisauke kleisauke commented May 3, 2020

Copy link
Copy Markdown
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

jcupitt commented May 3, 2020

Copy link
Copy Markdown
Member

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

kleisauke commented May 3, 2020

Copy link
Copy Markdown
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

jcupitt commented May 4, 2020

Copy link
Copy Markdown
Member

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
Closed
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