Skip to content

Expose attention center when doing attention-based cropping. #3164

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
Merged
merged 4 commits into from
Nov 28, 2022
Merged

Expose attention center when doing attention-based cropping. #3164

merged 4 commits into from
Nov 28, 2022

Conversation

ejoebstl
Copy link
Contributor

Hola,

Thanks for your work on libvips.

I'm currently trying to expose the region of interest when using smart/attention cropping. In libvips, only the crop offset is exposed, but having access to the center would be quite interesting.

However, I'm failing to implement this correctly. One of the unit tests crashes with the following backtrace:

#0  0x00007ffff775acc7 in __strchr_avx2 () at /lib64/libc.so.6
#1  0x00007ffff7f71623 in g_param_spec_pool_lookup () at /lib64/libgobject-2.0.so.0
#2  0x00007ffff7f68dbc in g_object_class_find_property () at /lib64/libgobject-2.0.so.0
#3  0x00007ffff7c003a5 in vips_object_get_argument
    (object=0x5004a0, name=0xffffffffffffffff <error: Cannot access memory at address 0xffffffffffffffff>, pspec=0x7ffff4b34fe8, argument_class=0x7ffff4b34fe0, argument_instance=0x7ffff4b34fd8) at ../libvips/iofuncs/object.c:773
#4  0x00007ffff7c04937 in vips_object_set_valist (object=0x5004a0, ap=0x7ffff4b35068)
    at ../libvips/iofuncs/object.c:2426
#5  0x00007ffff7c1c825 in vips_call_required_optional
    (operation=0x7ffff4b350d0, required=0x7ffff4b35100, optional=0x7ffff4b35210)
    at ../libvips/iofuncs/operation.c:905
#6  0x00007ffff7c1c99e in vips_call_by_name
    (operation_name=0x7ffff7c86052 "extract_area", option_string=0x0, required=0x7ffff4b35100, optional=0x7ffff4b35210) at ../libvips/iofuncs/operation.c:954
#7  0x00007ffff7c1d078 in vips_call_split (operation_name=0x7ffff7c86052 "extract_area", optional=0x7ffff4b35210)
    at ../libvips/iofuncs/operation.c:1058
#8  0x00007ffff7b96287 in vips_extract_area
    (in=0x50f960, out=0x7fffec00edf0, left=0, top=0, width=290, height=442) at ../libvips/conversion/extract.c:285
#9  0x00007ffff7a67085 in read_jpeg_image (jpeg=0x7fffec00be40, out=0x50f640)
    at ../libvips/foreign/jpeg2vips.c:925
#10 0x00007ffff7a67272 in vips__jpeg_read (jpeg=0x7fffec00be40, out=0x50f640, header_only=0)
    at ../libvips/foreign/jpeg2vips.c:989
#11 0x00007ffff7a6731a in vips__jpeg_read_source
    (source=0x50e480, out=0x50f640, header_only=0, shrink=1, fail_on=VIPS_FAIL_ON_NONE, autorotate=0, unlimited=0)
    at ../libvips/foreign/jpeg2vips.c:1014

I think it's a problem with how I implemented the additional arguments, but I fail to see the error.

Any guidance would appreciated - happy to finish this PR.

@jcupitt
Copy link
Member

jcupitt commented Nov 17, 2022

Hi @ejoebstl,

This looks interesting, but it's a huge API change. Could you explain the effect you are trying to achieve? Perhaps there's a less intrusive way of doing this.

Maybe just adding x_pos and y_pos as optional output arguments of the smartcrop operation would be enough?

@ejoebstl
Copy link
Contributor Author

ejoebstl commented Nov 17, 2022 •
edited
Loading

Hi,

Thank you for getting back.

I want to expose the center of attention computed by vips_smartcrop_attention via the API. I'm using vips via sharp to create thumbnails for a responsive website. The idea is to position thumbnails according to device size. And for this I'd use the center of attention provided by libvips.

I agree that adding x_pos and y_pos to the VImage class is a large change. Could you point me in the right direction on how to implement optional output arguments?

For this PR I followed what was done with Xoffset and Yoffset. The arguments are essentially added to the image after cropping.

@jcupitt
Copy link
Member

jcupitt commented Nov 17, 2022

max has optional output arguments for the position of the maximum:

https://github.com/libvips/libvips/blob/master/libvips/arithmetic/max.c#L444-L456

After computing the values in _build, set them with g_object_set():

https://github.com/libvips/libvips/blob/master/libvips/arithmetic/max.c#L232-L242

And then callers should be able to read them out.

@ejoebstl
Copy link
Contributor Author

Hello @jcupitt - thank you for the guidance. This is way less intrusive now.

@ejoebstl ejoebstl changed the title [WIP/Help Needed] Expose attention center when doing attention-based cropping. Expose attention center when doing attention-based cropping. Nov 26, 2022
@ejoebstl ejoebstl mentioned this pull request Nov 26, 2022
Merged
@ejoebstl
Copy link
Contributor Author

Added unit test, this should be good for merging.

I did not find the documentation in the package, please feel free to point me in the right direction and I can update that as well.

@jcupitt
Copy link
Member

jcupitt commented Nov 26, 2022

Ah nice, yes, that's much neater.

The docs are in the comment block near the end:

https://github.com/libvips/libvips/blob/master/libvips/conversion/smartcrop.c#L445-L468

Just add your two new optional output params, plus a paragraph of text below saying what they are for.

Add a line to libvips/ChangeLog as well and credit yourself, and a date and one-line summary to the top of smartcrop.c.

@ejoebstl
Copy link
Contributor Author

Done, hope that's good!

@jcupitt
Copy link
Member

jcupitt commented Nov 27, 2022

Great! Just two more tiny things ...

Thank you for doing this work, @ejoebstl, it'll be in 8.14, due RSN.

@jcupitt jcupitt mentioned this pull request Nov 27, 2022
Merged
@ejoebstl
Copy link
Contributor Author

Done 🙌

@jcupitt jcupitt merged commit 25444cd into libvips:master Nov 28, 2022
@jcupitt
Copy link
Member

jcupitt commented Nov 28, 2022

Great! Thanks again for this useful thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcupitt jcupitt jcupitt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ejoebstl @jcupitt