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

DynamicImage::ImageRgb16 copy_from doesn't preserve all the color data #2407

Open
AugLuk opened this issue Jan 23, 2025 · 1 comment
Open

Comments

@AugLuk
Copy link

AugLuk commented Jan 23, 2025

"image" version: 0.25.5
rustc version: 1.84.0
target: stable-aarch64-apple-darwin
profile: release

Expected

When copying sub-images between 2 DynamicImage::ImageRgb16 images, the all color data should be preserved

Actual behaviour

Only u8 worth of data (256 values) per pixel's color channel is transferred
This is because get_pixel that is used inside copy_from always returns Rgba<u8>

Reproduction steps

Hopefully it's self explanatory

Possible fix implementation direction

For my project I'm using this crude image_copy_from function I wrote myself
It doesn't work with all DynamicImage variants, but I only needed ImageRgb16
It could be expanded to all variants using a macro
Consider that:

  • Flat samples might hold u8, u16 or f32, depending on the variant
  • Different variants would have different pixel_channels values

Also, it might be more performant than the original copy_from, but I haven't tested it

Code:

use image::DynamicImage::ImageRgb16;

pub fn image_copy_from(
  target: &mut image::DynamicImage,
  source: &image::DynamicImage,
  x: usize,
  y: usize,
) {
  let target_width = target.width() as usize;
  let target_height = target.height() as usize;
  let source_width = source.width() as usize;
  let source_height = source.height() as usize;

  let source_x_max = if x < target_width {
    source_width.min(target_width - x)
  } else {
    0
  };
  let source_y_max = if y < target_height {
    source_height.min(target_height - y)
  } else {
    0
  };

  match (target, source) {
    (ImageRgb16(target), ImageRgb16(source)) => {
      let pixel_channels = 3;

      let source_fs = source.as_flat_samples();
      let mut target_fs = target.as_flat_samples_mut();
      
      for source_y in 0..source_y_max {
        let first_source_index = source_y * source_width * pixel_channels;
        let first_target_index = ((y + source_y) * target_width + x) * pixel_channels;

        let last_source_index = first_source_index + source_x_max * pixel_channels;
        let last_target_index = first_target_index + source_x_max * pixel_channels;

        let source_slice = &source_fs.as_slice()[first_source_index..last_source_index];
        let target_slice = &mut target_fs.as_mut_slice()[first_target_index..last_target_index];

        target_slice.copy_from_slice(source_slice);
      }
    }
    _ => unimplemented!(),
  }
}
@fintelia
Copy link
Contributor

Unfortunately, I think this is another consequence of DynamicImage implementing the GenericImage trait. See also: #2237, #2274, and #2344

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

No branches or pull requests

2 participants