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

Jump to bottom

Fix none of the embedded image except Rgb8 display properly #368

Merged
merged 5 commits into from
Dec 30, 2024
Merged

Fix none of the embedded image except Rgb8 display properly #368

merged 5 commits into from
Dec 30, 2024

Conversation

lanyeeee
Copy link
Contributor

Bug Fixes

  • Fixed incorrect color type detection for JPEG images
  • Corrected BitsPerComponent calculation for different color types
  • Corrected ColorSpace mapping for different color types
  • Corrected image data handling in PDF content stream

Testing

Add a comprehensive test that:

  • Creates PDF with images of different color types
  • Verifies support for various image formats
  • Generates visual output for manual verification

Notes

  • 32-bit floating point formats (Rgb32F, Rgba32F) are currently not supported

Related Issues

@lanyeeee
Fix incorrect color type detection for JPEG images
ff8b8d7 
Previously assumed all JPEG images were RGB8, but they can also be L8(grayscale)
Add get_dimensions_and_color_type function to detect actual color type without decoding

(cherry picked from commit 25b030f)
@lanyeeee
Fix BitsPerComponent calculation and improper ColorSpace mapping
3873a04 
- Fix incorrect BitsPerComponent calculation that was dividing bits_per_pixel by 3
- Properly handle ColorSpace for different image types:
  * Use correct bit depth (8 or 16) based on color type
  * Set proper ColorSpace for RGBA images to DeviceRGB
  * Add proper error handling for unsupported color types (Rgb32F, Rgba32F)
  * Remove incorrect DeviceN color space usage

(cherry picked from commit 46d71d7)
@lanyeeee
Fix incorrect image data handling in PDF content stream
ce1d9a3 
Before this fix, raw image data was incorrectly used directly as PDF content. Now properly:
- Handle alpha channel removal for LA8/RGBA8/LA16/RGBA16 formats
- Convert 16-bit pixel data to big-endian bytes
- Keep L8/RGB8 data unchanged as they can be used directly

(cherry picked from commit 3eeef56)
@lanyeeee
Refactor and optimize image processing logic in xobject.rs
0196cfc 
(cherry picked from commit 34222ee)
@lanyeeee
Add test for supported color types in PDF image embedding
3ef44e2 
This commit adds a comprehensive test that:
- Creates PDF with images of different color types
- Verifies support for various image formats
- Generates visual output for manual verification

(cherry picked from commit a375ab7)
@Heinenen
Copy link
Collaborator

I haven't run the tests but the code looks good.
The only thing that confused me a bit is, why Rgb32F isn't supported. Is that a limitation of the PDF spec or of the image crate, or something else?

@lanyeeee
Copy link
Contributor Author

I cant find a suitable ColorSpace in the PDF specification that supports floating-point.

I did experiment with using DeviceRGB by converting RgbF32 and RgbaF32 to Rgb16. While this works and the images appear normal in my testing, there are several concerns:

  • Using DeviceRGB for f32 images may not be technically correct as it doesn't properly handle the full floating point range
  • Professional HDR/f32 images would likely show significant color distortion

If you are comfortable with these trade-offs, I can implement it. However, I wanted to be transparent about these potential issues rather than implementing a solution that might produce incorrect results for proper f32/HDR images.

@J-F-Liu J-F-Liu merged commit 8819727 into J-F-Liu:main Dec 30, 2024
8 checks passed
@Heinenen
Copy link
Collaborator

Thanks for the clarification. I think it's definitely better to keep it as is then to produce invalid PDFs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lanyeeee @Heinenen @J-F-Liu