Skip to content

Conversation

@rayguo17
Copy link
Contributor

@rayguo17 rayguo17 commented Jan 5, 2026

In c898457, we change to encoder in jpeg_encoder instead of our own implementation, this PR add more ExtendedColorType that is supported in jpeg_encoder crate, but is previously not supported by our own implementation.

let color = jpeg_encoder::ColorType::Rgb;
encode_jpeg(color)
}
ExtendedColorType::Rgba8 => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The JPEG encoder should not claim to support color types with alpha channels and then silently drop them.

See the note here: https://docs.rs/image/0.25.9/image/enum.DynamicImage.html#color-conversion

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 look into the DynamicImage::write_to function, seems like it would not try to drop the Alpha before creating JpegEncoder, see:

pub(crate) fn encoder_for_format<'a, W: Write + Seek>(
format: ImageFormat,
buffered_write: &'a mut W,
) -> ImageResult<Box<dyn ImageEncoderBoxed + 'a>> {
Ok(match format {
#[cfg(feature = "png")]
ImageFormat::Png => Box::new(png::PngEncoder::new(buffered_write)),
#[cfg(feature = "jpeg")]
ImageFormat::Jpeg => Box::new(jpeg::JpegEncoder::new(buffered_write)),

perhaps the claim in https://docs.rs/image/0.25.9/image/enum.DynamicImage.html#color-conversion is a bit outdated?

The JPEG encoder should not claim to support color types with alpha channels and then silently drop them.

In this case, are there any recommended way to encode Jpeg format while having rgba8 raw data? previously, there was jpeg_encoder::encode_image function which support doing that before this commit: c898457#diff-4bfeb443c862627de67d89c87d6e2f988bd087923eb53382c00dcb4869030a51L511

Copy link
Member

Choose a reason for hiding this comment

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

Conversion happens later, by probing the encoder in make_compatible_img and only for DynamicImage where silent degredation is the 'expected' behavior (by some experience reports). It does not happen if you try the raw interface that will attempt to stay true the underlying capabilities.

Copy link
Contributor Author

@rayguo17 rayguo17 Jan 10, 2026

Choose a reason for hiding this comment

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

I see, I will close this PR, since this is not the intended behaviour for the jpeg_encoder, but note that the make_compatible_img may have some performance overhead, it will iterate through the old dyn_image to create the new dyn_image, and eventually iterating through the new dyn_image to encode the image. Is it possible that we can introduce the use of GenericImageView when encoding jpeg with image format that required some transformation?

@rayguo17 rayguo17 requested a review from fintelia January 10, 2026 14:41
@rayguo17 rayguo17 closed this Jan 10, 2026
@rayguo17
Copy link
Contributor Author

Closing as not intended.

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.

3 participants