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

Simplify Pixel trait #1099

Closed
wants to merge 3 commits into from
Closed

Simplify Pixel trait #1099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f47eece
Remove Pixel::{to_bgr, to_bgra, to_rgb}
fintelia Dec 8, 2019
ae3d58e
Remove Pixel::to_luma
fintelia Dec 8, 2019
c4dc33b
Remove Pixel::invert
fintelia Dec 8, 2019
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
15 changes: 0 additions & 15 deletions src/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,24 +86,12 @@ pub trait Pixel: Copy + Clone {
/// that the slice is long enough to present panics if the pixel is used later on.
fn from_slice_mut(slice: &mut [Self::Subpixel]) -> &mut Self;

/// Convert this pixel to RGB
fn to_rgb(&self) -> Rgb<Self::Subpixel>;

/// Convert this pixel to RGB with an alpha channel
fn to_rgba(&self) -> Rgba<Self::Subpixel>;
Copy link
Member

Choose a reason for hiding this comment

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

As long as to_rgba is required on Pixel it seems to still hold that its color space must be interpreted as sRGB. But then the other methods, except invert, could also be default implemented utilizing this one. Why remove them outright instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to remove as much as I could without breaking too much other code in the crate. Perhaps it would be better to step back and consider what generic functionality we can provide on pixels without baking in assumptions about color spaces and so forth. For instance, all of the map/apply variations are rather questionable without knowing the color space

Copy link
Member

@HeroicKatora HeroicKatora Dec 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That was contrary to my thoughts, map and apply were the most reasonable in my eye. They do not themselves enforce any particular interpretation. Sure, the caller can not currently find out the color space through image but if it is known then the function can correctly interpret the channel value. And no color type has to come up with an interpretation of its own, just copy values into the function argument and back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't the channels have to be either L or RGB (plus an optional alpha) for that to work? I don't think it could work for things like HSV or xyY since different channels have different meanings but the map and apply functions would run the same closure on each?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess one view is that Pixel should just be a wrapper around a &[Subpixel] plus an associated ColorType and ColorSpace. It would then fall on the user to properly handle each possible pair they cared about. There would be convenience functions to operate on channels but they'd have no higher level understanding of what they meant.

The alternative is to discourage users from interacting with pixels that way and instead provide higher level functionality so that generic code could work while remaining oblivious to the ColorType and ColorSpace of the pixel. It isn't really obvious to me what this would look like, but seems like the preferable option if feasible

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess one view is that Pixel should just be a wrapper around a &[Subpixel] plus an associated ColorType and ColorSpace...The alternative is to discourage users from interacting with pixels that way and instead provide higher level functionality

Those may not be mutually exclusive alternatives, but levels of abstraction in the API. Pixels are just buffers with color types and spaces as you say, then higher level pixel traits provide higher level functionality where available. So if a user just needs (strawmen) Blendable, Invertible, HasChannel<Channel::Luminance> or InterpretableAs<Channel::Red, ColorSpace::sRGB>, they can construct fn<A: Invertible + HasChannel<Channel::Luminance>, B: Blendable<A> + InterpretableAs<Channel::Red, ColorSpace::sRGB>> myFancyFilter(a: &A, b: &mut B), etc.

If they need something more format-specific not captured in those higher level traits provided by image or imageproc or other crates, then they can operate on Pixels directly and match/bound the color types and spaces they need to, creating new higher level traits if they wish, e.g., trait Darken: Pixel {...} impl<P: Pixel<ColorType=RGB, ColorSpace=sRGB>> Darken for P { ... }.

So while I agree with @HeroicKatora that it's not an entirely coherent change to remove some of these conversions while leaving others, I can see the point in at least shrinking the exposed API since what's exposed is not really coherent as it is, with color spaces and types a bit abused.


/// Convert this pixel to luma
fn to_luma(&self) -> Luma<Self::Subpixel>;

/// Convert this pixel to luma with an alpha channel
fn to_luma_alpha(&self) -> LumaA<Self::Subpixel>;
Copy link
Member

Choose a reason for hiding this comment

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

Was this missed during removal of methods or is keeping this method intentional?


/// Convert this pixel to BGR
fn to_bgr(&self) -> Bgr<Self::Subpixel>;

/// Convert this pixel to BGR with an alpha channel
fn to_bgra(&self) -> Bgra<Self::Subpixel>;

/// Apply the function ```f``` to each channel of this pixel.
fn map<F>(&self, f: F) -> Self
where
Expand Down Expand Up @@ -159,9 +147,6 @@ pub trait Pixel: Copy + Clone {
where
F: FnMut(Self::Subpixel, Self::Subpixel) -> Self::Subpixel;

/// Invert this pixel
fn invert(&mut self);

/// Blend the color of a given pixel into ourself, taking into account alpha channels
fn blend(&mut self, other: &Self);
}
Expand Down
105 changes: 0 additions & 105 deletions src/color.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,36 +214,12 @@ impl<T: Primitive + 'static> Pixel for $ident<T> {
unsafe { &mut *(slice.as_ptr() as *mut $ident<T>) }
}

fn to_rgb(&self) -> Rgb<T> {
let mut pix = Rgb([Zero::zero(), Zero::zero(), Zero::zero()]);
pix.from_color(self);
pix
}

fn to_bgr(&self) -> Bgr<T> {
let mut pix = Bgr([Zero::zero(), Zero::zero(), Zero::zero()]);
pix.from_color(self);
pix
}

fn to_rgba(&self) -> Rgba<T> {
let mut pix = Rgba([Zero::zero(), Zero::zero(), Zero::zero(), Zero::zero()]);
pix.from_color(self);
pix
}

fn to_bgra(&self) -> Bgra<T> {
let mut pix = Bgra([Zero::zero(), Zero::zero(), Zero::zero(), Zero::zero()]);
pix.from_color(self);
pix
}

fn to_luma(&self) -> Luma<T> {
let mut pix = Luma([Zero::zero()]);
pix.from_color(self);
pix
}

fn to_luma_alpha(&self) -> LumaA<T> {
let mut pix = LumaA([Zero::zero(), Zero::zero()]);
pix.from_color(self);
Expand Down Expand Up @@ -292,10 +268,6 @@ impl<T: Primitive + 'static> Pixel for $ident<T> {
}
}

fn invert(&mut self) {
Invert::invert(self)
}

fn blend(&mut self, other: &$ident<T>) {
Blend::blend(self, other)
}
Expand Down Expand Up @@ -1016,83 +988,6 @@ impl<T: Primitive> Blend for Bgr<T> {
}
}


/// Invert a color
pub(crate) trait Invert {
/// Inverts a color in-place.
fn invert(&mut self);
}

impl<T: Primitive> Invert for LumaA<T> {
fn invert(&mut self) {
let l = self.0;
let max = T::max_value();

*self = LumaA([max - l[0], l[1]])
}
}

impl<T: Primitive> Invert for Luma<T> {
fn invert(&mut self) {
let l = self.0;

let max = T::max_value();
let l1 = max - l[0];

*self = Luma([l1])
}
}

impl<T: Primitive> Invert for Rgba<T> {
fn invert(&mut self) {
let rgba = self.0;

let max = T::max_value();

*self = Rgba([max - rgba[0], max - rgba[1], max - rgba[2], rgba[3]])
}
}


impl<T: Primitive> Invert for Bgra<T> {
fn invert(&mut self) {
let bgra = self.0;

let max = T::max_value();

*self = Bgra([max - bgra[2], max - bgra[1], max - bgra[0], bgra[3]])
}
}


impl<T: Primitive> Invert for Rgb<T> {
fn invert(&mut self) {
let rgb = self.0;

let max = T::max_value();

let r1 = max - rgb[0];
let g1 = max - rgb[1];
let b1 = max - rgb[2];

*self = Rgb([r1, g1, b1])
}
}

impl<T: Primitive> Invert for Bgr<T> {
fn invert(&mut self) {
let bgr = self.0;

let max = T::max_value();

let r1 = max - bgr[2];
let g1 = max - bgr[1];
let b1 = max - bgr[0];

*self = Bgr([b1, g1, r1])
}
}

#[cfg(test)]
mod tests {
use super::{Luma, LumaA, Pixel, Rgb, Rgba, Bgr, Bgra};
Expand Down
61 changes: 44 additions & 17 deletions src/dynimage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,12 @@ impl DynamicImage {
/// Invert the colors of this image.
/// This method operates inplace.
pub fn invert(&mut self) {
dynamic_map!(*self, ref mut p -> imageops::invert(p))
use traits::Primitive;
fn invert_pixel<T: Primitive>(p: &mut impl Pixel<Subpixel = T>) {
p.apply_with_alpha(|c|T::max_value() - c, |a|a);
}

dynamic_map!(*self, ref mut img -> img.pixels_mut().for_each(invert_pixel))
}

/// Resize this image using the specified filter algorithm.
Expand Down Expand Up @@ -747,32 +752,54 @@ impl GenericImage for DynamicImage {
type InnerImage = DynamicImage;

fn put_pixel(&mut self, x: u32, y: u32, pixel: color::Rgba<u8>) {
let color::Rgba([r, g, b, a]) = pixel;
let (r16, g16, b16, a16) = (r as u16 * 257, b as u16 * 257, g as u16 * 257, a as u16 * 257);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be let color::Rgba::<u16>([r16, g16, b16, a16]) = pixel.into_color(); instead so as to not duplicate color conversion logic?

Copy link
Contributor

@aschampion aschampion Dec 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Now that I think about it, all of these could be replaced with into_color, e.g.,

 DynamicImage::ImageBgra8(ref mut p) => p.put_pixel(x, y, pixel.into_color()),

For the 16bpc types you just need an extra,

let pixel16: color::Rgba<u16> = pixel.into_color();

...

DynamicImage::ImageRgb16(ref mut p) => p.put_pixel(x, y, pixel16.into_color()),

This is consistent with the two-level API described above in the other comment, with IntoColor being a high level trait expressing convertibility across color types that lets DynamicImage not have to be responsible for how these conversions are done. This means the FromColor/IntoColor implementation will be responsible for handling color space conversions once we represent them, as they should. The downside for now is still needing to special case the 16bpc types, but I expect we'll clear that up later with the Pixel types themselves.

To note this means that put_pixel could really be (not that we would want to expose this API yet, given there is still design flux):

fn<P: Pixel> put_pixel(&mut self, x: u32, y: u32, pixel: P)
    where P: IntoColor<color::Rgba<u8>> + IntoColor<color::Rgba<u16>> {
    let pixel8: color::Rgba<u8> = pixel.into_color();
    let pixel16: color::Rgba<u16> = pixel.into_color();
    ...
}

match *self {
DynamicImage::ImageLuma8(ref mut p) => p.put_pixel(x, y, pixel.to_luma()),
DynamicImage::ImageLuma8(ref mut p) => {
let color::LumaA([l, _]) = pixel.to_luma_alpha();
p.put_pixel(x, y, color::Luma([l]))
}
DynamicImage::ImageLumaA8(ref mut p) => p.put_pixel(x, y, pixel.to_luma_alpha()),
DynamicImage::ImageRgb8(ref mut p) => p.put_pixel(x, y, pixel.to_rgb()),
DynamicImage::ImageRgb8(ref mut p) => p.put_pixel(x, y, color::Rgb([r, g, b])),
DynamicImage::ImageRgba8(ref mut p) => p.put_pixel(x, y, pixel),
DynamicImage::ImageBgr8(ref mut p) => p.put_pixel(x, y, pixel.to_bgr()),
DynamicImage::ImageBgra8(ref mut p) => p.put_pixel(x, y, pixel.to_bgra()),
DynamicImage::ImageLuma16(ref mut p) => p.put_pixel(x, y, pixel.to_luma().into_color()),
DynamicImage::ImageLumaA16(ref mut p) => p.put_pixel(x, y, pixel.to_luma_alpha().into_color()),
DynamicImage::ImageRgb16(ref mut p) => p.put_pixel(x, y, pixel.to_rgb().into_color()),
DynamicImage::ImageRgba16(ref mut p) => p.put_pixel(x, y, pixel.into_color()),
DynamicImage::ImageBgr8(ref mut p) => p.put_pixel(x, y, color::Bgr([b, g, r])),
DynamicImage::ImageBgra8(ref mut p) => p.put_pixel(x, y, color::Bgra([b, g, r, a])),
DynamicImage::ImageLuma16(ref mut p) =>{
let color::LumaA([l, _]) = color::Rgb([r16, g16, b16]).to_luma_alpha();
p.put_pixel(x, y, color::Luma([l]))
}
DynamicImage::ImageLumaA16(ref mut p) =>
p.put_pixel(x, y, color::Rgba([r16, g16, b16, a16]).to_luma_alpha()),
DynamicImage::ImageRgb16(ref mut p) =>
p.put_pixel(x, y, color::Rgb([r16, g16, b16])),
DynamicImage::ImageRgba16(ref mut p) =>
p.put_pixel(x, y, color::Rgba([r16, g16, b16, a16])),
}
}
/// DEPRECATED: Use iterator `pixels_mut` to blend the pixels directly.
fn blend_pixel(&mut self, x: u32, y: u32, pixel: color::Rgba<u8>) {
let color::Rgba([r, g, b, a]) = pixel;
let (r16, g16, b16, a16) = (r as u16 * 257, b as u16 * 257, g as u16 * 257, a as u16 * 257);
match *self {
DynamicImage::ImageLuma8(ref mut p) => p.blend_pixel(x, y, pixel.to_luma()),
DynamicImage::ImageLuma8(ref mut p) => {
let color::LumaA([l, _]) = pixel.to_luma_alpha();
p.blend_pixel(x, y, color::Luma([l]))
}
DynamicImage::ImageLumaA8(ref mut p) => p.blend_pixel(x, y, pixel.to_luma_alpha()),
DynamicImage::ImageRgb8(ref mut p) => p.blend_pixel(x, y, pixel.to_rgb()),
DynamicImage::ImageRgb8(ref mut p) => p.blend_pixel(x, y, color::Rgb([r, g, b])),
DynamicImage::ImageRgba8(ref mut p) => p.blend_pixel(x, y, pixel),
DynamicImage::ImageBgr8(ref mut p) => p.blend_pixel(x, y, pixel.to_bgr()),
DynamicImage::ImageBgra8(ref mut p) => p.blend_pixel(x, y, pixel.to_bgra()),
DynamicImage::ImageLuma16(ref mut p) => p.blend_pixel(x, y, pixel.to_luma().into_color()),
DynamicImage::ImageLumaA16(ref mut p) => p.blend_pixel(x, y, pixel.to_luma_alpha().into_color()),
DynamicImage::ImageRgb16(ref mut p) => p.blend_pixel(x, y, pixel.to_rgb().into_color()),
DynamicImage::ImageRgba16(ref mut p) => p.blend_pixel(x, y, pixel.into_color()),
DynamicImage::ImageBgr8(ref mut p) => p.blend_pixel(x, y, color::Bgr([b, g, r])),
DynamicImage::ImageBgra8(ref mut p) => p.blend_pixel(x, y, color::Bgra([b, g, r, a])),
DynamicImage::ImageRgb16(ref mut p) =>
p.blend_pixel(x, y, color::Rgb([r16, g16, b16])),
DynamicImage::ImageRgba16(ref mut p) =>
p.blend_pixel(x, y, color::Rgba([r16, g16, b16, a16])),
DynamicImage::ImageLuma16(ref mut p) =>{
let color::LumaA([l, _]) = color::Rgb([r16, g16, b16]).to_luma_alpha();
p.blend_pixel(x, y, color::Luma([l]))
}
DynamicImage::ImageLumaA16(ref mut p) =>
p.blend_pixel(x, y, color::Rgba([r16, g16, b16, a16]).to_luma_alpha()),
}
}

Expand Down
23 changes: 4 additions & 19 deletions src/imageops/colorops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
//! Functions for altering and converting the color of pixelbufs

use buffer::{ImageBuffer, Pixel};
use color::{Luma, Rgba};
use image::{GenericImage, GenericImageView};
use color::{Luma, LumaA, Rgba};
use image::GenericImageView;
use math::nq;
use math::utils::clamp;
use num_traits::{Num, NumCast};
Expand All @@ -24,29 +24,14 @@ where

for y in 0..height {
for x in 0..width {
let p = image.get_pixel(x, y).to_luma();
out.put_pixel(x, y, p);
let LumaA([l, _]) = image.get_pixel(x, y).to_luma_alpha();
out.put_pixel(x, y, Luma([l]));
}
}

out
}

/// Invert each pixel within the supplied image.
/// This function operates in place.
pub fn invert<I: GenericImage>(image: &mut I) {
let (width, height) = image.dimensions();

for y in 0..height {
for x in 0..width {
let mut p = image.get_pixel(x, y);
p.invert();

image.put_pixel(x, y, p);
}
}
}

/// Adjust the contrast of the supplied image.
/// ```contrast``` is the amount to adjust the contrast by.
/// Negative values decrease the contrast and positive values increase the contrast.
Expand Down
2 changes: 1 addition & 1 deletion src/imageops/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub use self::affine::{flip_horizontal, flip_vertical, rotate180, rotate270, rot
pub use self::sample::{blur, filter3x3, resize, thumbnail, unsharpen};

/// Color operations
pub use self::colorops::{brighten, contrast, dither, grayscale, huerotate, index_colors, invert,
pub use self::colorops::{brighten, contrast, dither, grayscale, huerotate, index_colors,
BiLevel, ColorMap};

mod affine;
Expand Down