Skip to content

Conversation

@Socolin
Copy link
Contributor

@Socolin Socolin commented Dec 5, 2025

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

I took a look at the code to see how to implements #3025 and I ends up writting a bit of code, so I'm sharing (There is not test yet, I'm not sure this is the right way to do this), if you are interested in this, let me know and I can continue the PR and add some tests

First I tried to do it the same way it's done in the JpegDecoder, when the pixel data is read. But this seems to be impossible with the PngDecoder since the Chunk containing the ICC profile might be after the image data (It was on my tests images)
Since the decoder is processing a stream (and the stream might not be rewindable) I abandon this idea (the only solution I see would be to store the bytes in memory to be able to find the profile first, but I think this would badly impact the performance / memory usage)

So I ends up doing the conversion at the end of the PngDecoder.Decode once all the chunk have been read and the image is already process. I'm using TPixel.ToScaledVector4() and TPixel.FromScaledVector4() to get the value directly as float for the profile conversion, and to avoid loosing precision if the image is 16 bit depth and TPixel is Rgba64

Let me know if this is the correct approach or if I missed something (all comments are welcome)

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

I think this has to be the correct approach given the non-standard location of the profiles. We just need to fix up how we do it.

=> (this.scanline, this.previousScanline) = (this.previousScanline, this.scanline);

// FIXME: Maybe this could be a .Mutate(x => x.ApplyIccProfile(destinationProfile)) ? Nothing related to png here
private static void ApplyIccProfile<TPixel>(Image<TPixel> image, IccProfile sourceProfile, IccProfile destinationProfile)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could move this to an extension method for the converter itself?

We'd have to rename it also to include the name RgbaCompatible since it would not work for other color spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think too, and from what I saw, Webp will need the same method. Should we add TFrom and TTo as generic parameter ?

Copy link
Member

Choose a reason for hiding this comment

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

No generic parameters should be used since it must be compatible with Rgb to work since we're converting from/to the rgba representative Vector4 - ICC profiles are very particular about this. Rgb and Grayscale both use Rgb profiles but others do not.

WebP shouldn't need the same method since the specification requires ICC to be before Color information and WebP has very strict conformance.

https://developers.google.com/speed/webp/docs/riff_container#color_profile

However, we could use this as a stopgap to a more optimized solution there.

I eventually might add a more optimized version for PNG for specification conformant images and use this as a fallback for those that don't.

The iCCP chunk is supposed to be before iDAT

https://www.w3.org/TR/png/#5ChunkOrdering

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 kind of like the possibility to apply the ICC profile during decoding if available and use this as a fallback. If I find the time I'll take a look at this.

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 started adding the conversion during the decode process, it's done for now in ProcessInterlacedRgbaScanline.

If you have some time, I'll appreciate some feedback on this before I continue the other one. Basically I'm duplicating the code and add a if (iccProfile != null) / else and adjust the code with the iccProfile. Is that correct ?

@JimBobSquarePants
Copy link
Member

@Socolin I'll have another look at this soon. I've been wiped out with the flu for the last 10 days so have not been able to do anything. I'll likely pull down your fork and make some initial code changes to add the appropriate extension method to ColorProfileConverter to allow both processes.

@JimBobSquarePants
Copy link
Member

@Socolin I have re-implemented the per-image version of the operation in ImageDecoderCore.

I also took a closer look at the per-scanline variants, and I am not convinced they are a net win in this case. The color conversion must occur after any blending, which already forces a separation between decoding and conversion. That means we do not get to fuse the work into a single pass anyway, and we explicitly want to avoid per-pixel conversion because the ICC path is computationally expensive and not structured for that granularity.

The per-image approach has a practical advantage here: the conversion is applied over the entire pixel buffer using ProcessPixelRowsAsVector4, which internally partitions the work into n = Environment.ProcessorCount parallel chunks. Each chunk processes multiple rows, amortizing allocator usage, reducing per-row setup overhead, and allowing the ICC conversion to run over contiguous spans with better cache locality. In contrast, a per-scanline API would serialize more of that overhead. Even if scanlines were scheduled in parallel, each invocation would still pay the fixed costs of buffer setup, conversion dispatch, and synchronization at a much finer granularity, which tends to dominate the actual math.

So while a per-scanline API looks attractive on the surface, in practice it would be slower for this workload. The per-image conversion aligns better with how the ICC code wants to operate, and it lets us fully exploit coarse-grained parallelism rather than fragmenting the work into many small units with higher overhead.

@JimBobSquarePants JimBobSquarePants marked this pull request as ready for review January 12, 2026 11:16
Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

I'm happy with this now. Thanks @Socolin for your help!!

@JimBobSquarePants JimBobSquarePants linked an issue Jan 12, 2026 that may be closed by this pull request
4 tasks
@JimBobSquarePants JimBobSquarePants merged commit 3027000 into SixLabors:main Jan 12, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ColorProfileHandling.Convert is not converting PNG files

2 participants