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

Feature: methods for getting a pixel using signed coordinates #1851

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

Conversation

rostyq
Copy link

@rostyq rostyq commented Feb 1, 2023

This PR adds two methods, each with default implementation to GenericImageView trait: checked_get_pixel and saturating_get_pixel. Both allow to get a pixel from an image view using signed coordinates. That can be convenient for different computer vision algorithms like object detection or tracking.

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option.

- add `GenericImageView::checked_get_pixel` method
  with `i64` arguments returning `Option`
- add `GenericImageView::saturating_get_pixel` method
  with `i64` arguments returning nearest pixel in case location
  is not in bounds
@fintelia
Copy link
Contributor

fintelia commented Feb 1, 2023

This could be simplified using the max() and min() methods. For instance, x.max(0).min(width - 1) as u32

Also worth double checking that zero-sized images are either prohibited by GenericImage or properly handled by this patch

src/image.rs Outdated
Comment on lines 957 to 981
fn saturating_get_pixel(&self, x: i64, y: i64) -> Self::Pixel {
#[inline(always)]
fn convert(value: i64, bound: u32) -> u32 {
value.is_positive()
.then(|| {
match u32::try_from(value) {
Ok(value) => {
if value < bound {
value
} else {
bound - 1
}
},
Err(_) => bound,
}
}).unwrap_or(0u32)
}

unsafe {
self.unsafe_get_pixel(
convert(x, self.width()),
convert(y, self.height())
)
}
}
Copy link
Member

@HeroicKatora HeroicKatora Feb 1, 2023

Choose a reason for hiding this comment

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

Also worth double checking that zero-sized images are either prohibited by GenericImage or properly handled by this patch

They are not. Panic in those cases would be permissible behavior but maybe it's not desirable. The methodical evaluation of how we wrote the trait is, however:

Safety

  • The coordinates must be in_bounds of the image.

Note that in_bounds is both safe, and the trait itself is safe. Thus, we can't make any assumption about its implementation other than the method signature. Since we haven't called the method here either, we can not argue to have fulfilled the precondition properly. I don't think this method possible in a generic, sound manner either way.

However, we can add specialized implementations of these two new methods for ImageBuffer. Since the in_bounds method is local, we can use its implementation details and rely on them being correct.

Copy link
Member

Choose a reason for hiding this comment

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

This is similar to get_pixel_unchecked. Note that the generic implementation provided by the trait does not in fact call any unsafe method, it defaults to a purely safe fallback. This is correct and intended; since there's nothing unsafe about the trait, the implementor has no obligations to 'do the right thing'. There's no inherent property of in_bounds that the generic implementation could rely on for any effect; so calling the safe fallback is indeed the best useful implementation we can provide.

Note to self: ImageBuffer is still dubious if the user chooses a non-default container. Damn.

@strasdat
Copy link
Contributor

strasdat commented Feb 3, 2023

When designing image types (e.g. https://github.com/strasdat/Sophus/blob/23.04-beta/cpp/sophus/image/image_view.h#L125), I came to the same conclusion that signed integer pixel coordinates are beneficial. For instance, it is quite useful/common to ask whether a 3d point almost projects into a camera's field of view - hence talking about a pixel coordinate of e.g. (-5, -10) can indeed by meaningful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants