Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improper handling of minor RGB pixel formats in saving #162

Open
kimikage opened this issue Apr 19, 2024 · 2 comments
Open

Improper handling of minor RGB pixel formats in saving #162

kimikage opened this issue Apr 19, 2024 · 2 comments
Labels
bug Something isn't working help wanted Extra attention is needed
Milestone

Comments

@kimikage
Copy link
Contributor

Perhaps some of the following are known problems, but are not shown in the documentation or (broken) tests.

using ColorTypes
using ColorTypes.FixedPointNumbers
using TiffImages

TiffImages.save("rgb_n0f8.tif", fill(RGB{N0f8}(1, 0.4, 0.0), 10, 200)) # ok

TiffImages.save("rgb_n0f16.tif", fill(RGB{N0f16}(1, 0.4, 0.0), 10, 200)) # ok

TiffImages.save("rgb_f32.tif", fill(RGB{Float32}(1, 0.4, 0.0), 10, 200)) # ok 

TiffImages.save("rgb24.tif", fill(RGB24(1, 0.4, 0.0), 10, 200)) # broken

TiffImages.save("argb32.tif", fill(ARGB32(1, 0.4, 0.0, 0.6), 10, 200)) # blueish

TiffImages.save("bgr_n0f8.tif", fill(BGR{N0f8}(1, 0.4, 0.0), 10, 200)) # blueish

TiffImages.save("rgb_q7f8.tif", fill(RGB{Q7f8}(1, 0.4, 0.0), 10, 200)) # broken

struct AlwaysOrange <: AbstractRGB{N0f8} end
ColorTypes.red(::AlwaysOrange) = 1N0f8
ColorTypes.green(::AlwaysOrange) = 0.4N0f8
ColorTypes.blue(::AlwaysOrange) = 0N0f8

TiffImages.save("orange.tif", fill(AlwaysOrange(), 10, 200)) # broken

tiffimages

The AlwaysOrange example is an extreme case, but lazy formats are somewhat of a practical problem.
I am working on supporting the RGBE format used in HDR images. RGBE itself is not officially supported by TIFF, but it would be better to store RGBE32{T} as RGB{T}.
https://kimikage.github.io/HDRColorTypes.jl/dev/#RGBE32-and-XYZE32

Of course, I don't think it is necessary to support every format.
In general, for color types where the specific storage format is unknown, it might be better to convert to a known color type.
At least, it is better to show an error or warning before generating a broken file.

@tlnagy tlnagy added bug Something isn't working help wanted Extra attention is needed labels Apr 19, 2024
@tlnagy
Copy link
Owner

tlnagy commented Apr 19, 2024

Thanks for the report @kimikage. I agree, we should definitely not silently fail. Our writing support is definitely much more primitive than our reading support and we don't have enough safety checks for mapping Julian types to TIFF compatible types. This is something we should rectify before v1.0.

Our writing strategy at the moment is quite primitive, we simply write each pixel to disk following the Julia structure of the data. E.g. I'm not surprised that RGB24 fails because we write it is a UInt32 to disk, but likely tell TIFF that it's a RGB{N0f8} on read leading to incorrect indexing. The proper solution is to have logic to map the complex Julian types to TIFF compatible structures. I'm actually not sure if TIFF has any structure analogous to the Julia RGB24 (correct me if I'm wrong here) and we should throw an error.

EDIT: I'm unlikely to have the bandwidth to fix this till the fall but I'm happy to review a PR.

@tlnagy tlnagy added this to the v1.0 milestone Apr 19, 2024
@kimikage
Copy link
Contributor Author

Unfortunately, I do not have sufficient bandwidth either, but would like to address this issue in several steps.

One point I'm wondering, is it a bug fix (patch) or a breaking change, to show error or warning messages?
Perhaps PR #159 is the priority at this time.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants