Add opt-in iccMAX support for Hybrid Printer Profiles with Reflectance - #593
Add opt-in iccMAX support for Hybrid Printer Profiles with Reflectance#593maxderhak wants to merge 1 commit into
Conversation
|
Hi Max, First of all, thank you very much for taking the time to implement this work and submit the PR. I genuinely appreciate the effort you've put into it. It's a substantial contribution, and I understand the motivation behind it. A quick update on my side: I'm no longer at EFI and I'm now self-employed. My current focus is on building a business around Little CMS by developing commercial applications and also offering a professional edition of Little CMS. That work is already underway. The professional version is where I'm concentrating new functionality such as GPU acceleration and iccMAX support. One consequence of the rise of AI-assisted development is that I've had to rethink how I invest my development time. In the past I was happy to contribute significant new functionality directly to the open source version. Today, however, AI systems routinely ingest those algorithms and implementations without attribution, and as a result the market for commercial Little CMS plug-ins has essentially disappeared. Since the emergence of AI coding assistants, plug-in sales have dropped to virtually zero. Because of that, I'm no longer adding major new features to the open source core unless there is a compelling reason. That said, I would very much like to incorporate your contribution into the open source ecosystem if we can find the right architectural approach. For me, accepting a feature into Little CMS is never just about merging the code. It also means committing to documentation, test coverage, long-term maintenance, API stability, security review, portability across supported platforms, and all the other responsibilities that come with becoming part of the core library. One idea that I think could work very well would be to encapsulate this functionality as a plug-in rather than integrating it directly into the core. I'm perfectly willing to extend the plug-in architecture if necessary to support this cleanly. Users who need Hybrid Printer Profiles with Reflectance could simply load the plug-in, while the core library would remain unchanged, which is currently one of my main design goals. This approach would also give us flexibility to evolve the implementation independently from the core and potentially accommodate additional iccMAX-related functionality in the future without increasing the complexity of the base library. I'd be very interested in hearing your thoughts on this approach. Do you think your implementation could be structured as a plug-in, or do you see technical obstacles that would make that impractical? Thanks again for the contribution and for pushing this area forward. Best regards, Marti |
|
Hi Marti, Good to hear from you, and that you are open to moving this work forward in some fashion. I too Short answer: yes, I think it can be implemented as a plug-in, and I built a prototype to check The prototype is on a branch in my fork if you'd like to look: The plug-in registers four things, all through hooks that already exist:
Nothing re-implements 'cvst', 'clut', 'matf', 'mpet', 'curf', 'parf' or 'samf'. That was my main On your design goal of the core library remaining unchanged — I'd gently suggest that if some Concretely it comes to four places, about 200 lines as prototyped, and they fall into two quite Two are really plug-in architecture extensions — the thing you offered:
Two are genuine core changes, but small, and I think each stands on its own merits:
Separately, and independent of all of this: raising that output ceiling invalidates an overflow Two things I found by accident while prototyping, both reproducible on pristine master and unrelated One honest gap in the prototype: the test profile's curves are all singleSampledCurves, so it has no So — I'd be glad to restructure #593 along these lines. The result would be a plug-in carrying all Before I do the work, does that split sound acceptable to you — particularly the curve encoding hook Your feedback is appreciated! Your Friend... |
Implements the ICS for HybridPrinterWithReflectance, so a caller can
read and author the spectral reflectance transforms of an ICC.2 hybrid
printer profile. All of it sits behind CMS_USE_ICCMAX_SPECTRAL, which
is undefined by default; a default build is behaviourally identical to
master.
Added, all guarded:
- extendedCLUTElement ('xclt') in its four value encodings, and
singleSampledCurve ('sngf')
- the five ICC.2-only formulaCurveSegment function types, 0003h to
0007h, as parametric types 9 to 13
- the spectral PCS header fields, with accessors
- an embedded ICC.2 profile in an 'ICC5' tag
- spectralWhitePointTag ('swpt') in all three permitted encodings,
including uInt16ArrayType via a pair of accessors
- spectralViewingConditionsTag ('svcn')
- float16ArrayType ('fl16') and float32ArrayType ('fl32')
- a 66 KB hybrid printer test profile and the script that produced it
Two places where ICC.2 disagrees with the reference implementation are
resolved in favour of the reference, since interop follows it, and both
are documented at the site: spectralViewingConditions stores its two
trailing CIEXYZ triples as float32 rather than the XYZNumber Table 69
specifies, and the 1/65535 scaling for a uInt16-encoded swpt appears
nowhere in ICC.2 at all.
One change is unconditional and deliberate. Raising the extendedCLUT
output-channel ceiling invalidated the overflow guard inside CubeSize,
where the constant 15 was load-bearing because it matched the old
parser limit, so a crafted profile could wrap the allocation size and
be read out of bounds. The overflow-safe multiply now lives in
cmsStageAllocCLutFloatGranular and cmsStageAllocCLut16bitGranular
unconditionally, because that weakness is reachable in master through
the public API. A default build therefore differs from master in one
respect: input master would accept and then corrupt is now rejected.
CMS_USE_ICCMAX_SPECTRAL and CMS_NO_HALF_SUPPORT are mutually exclusive
and produce an #error, because ICC.2 encodes spectralRange as
float16Number.
The test profile's DToB3 carries a curve set of four shaper curves: three
singleSampledCurves and, deliberately, one segmentedCurve holding an ICC.2
formulaCurveSegment of function type 0003h with identity parameters, so a
single fixture exercises both curve encodings and an ICC.2-only formula
type without changing any output value. It is curve 0 rather than the last
so the curves after it shift in the position table. Its header declares
version 5.1 rather than 5.0, which is the case that proves the version
check tolerates a minor bump.
Tests: 158 with the macro undefined, unchanged from master, and 174
with it defined.
7e3a6a8 to
72cfecd
Compare
|
Dear Max, I have spent some time carefully reviewing the code you submitted. Regardless of whether it was written with the help of AI or not, which in the end is just another development tool, I have to say that it is quite impressive. Thank you again for this excellent contribution. I would like to share a few thoughts about the scope of this patch and how I think we could integrate it into LittleCMS. As you already know, maintaining an open source project is not easy. You receive contributions from many different people, and deciding what to do with them is often the hardest part. The issue is that once you accept a contribution, it effectively becomes your responsibility forever. You are the one who will have to maintain that code, and if something breaks in the future, it becomes your problem to fix. As a general rule, I rarely accept external contributions larger than about twenty lines. Nowadays I only accept bug fixes, and almost never new functionality. The reason is that several contributions I have received recently, most of them AI-generated and supposedly fixing "security vulnerabilities," turned out to be quite harmful and broke functionality that had previously worked perfectly. Because of this, for the last couple of years I have personally reviewed every line of code that goes into the project, knowing that if I accept it, I will eventually be responsible for maintaining it. I believe I mentioned before that I am no longer adding new functionality to the open source version of LittleCMS. At this point I only accept security fixes, build system improvements, and occasionally small changes to support new platforms. As I also mentioned previously, I am developing a commercial version that will include significant new functionality. Unfortunately, continuing to develop major new features under GPLv3 no longer makes much sense for me, since AI systems are able to learn and reuse the algorithms anyway. Your contribution is particularly significant because it does much more than add a new feature. It also opens the door to iccMAX support, something that until now was completely outside my plans for the open source branch. Please don't interpret this as me saying that I don't want the code. Quite the opposite. I think it is an excellent piece of work, and it would be a real shame to let it go unused. However, I think we need a credible deployment strategy that allows us to introduce this functionality without making disruptive changes to the core library and, from my perspective, without cannibalizing the commercial branch that I am currently developing. By the way, I noticed that you have run into what is probably the biggest architectural limitation of the current code base, namely the fixed number of channels. Changing that is such a fundamental redesign that I have never dared to merge it into the main branch. In fact, I have an experimental branch on GitHub, although I am not even sure whether it is public, that does exactly this. The problem is that the security implications and the increase in minimum system requirements are simply too significant for a library that is currently deployed in production in so many places. I also noticed that the AI once again flagged CubeSize(). I honestly do not know why AI systems are so obsessed with that function. I receive two or three security reports every week claiming that CubeSize() contains a vulnerability. When I ask for an ICC profile that actually triggers the reported problem, the answer is always that the analysis was mistaken. LittleCMS has a C API, and C is inherently unsafe. The library guarantees that using untrusted ICC profiles is safe, but it does not, and cannot, guarantee that calling the API with invalid parameters is safe. In C you can always pass an arbitrary integer where a pointer is expected, and the compiler will happily accept it. That is simply the nature of the language. With all that in mind, my proposal would be to first define the list of new plug-in entry points that we need. I think we could probably have that ready within a month, although August is always slow because everyone is on holiday. From there I could prepare a roadmap for gradually introducing those new plug-in interfaces. As you know, I typically make a new LittleCMS release approximately every six months. After that, we can decide whether this functionality should be distributed as a plug-in that ships together with LittleCMS or whether it would be better as a completely separate package. Personally, I think the latter may be the cleaner solution. Finally, it would also be interesting to see what the ICC decides regarding this area. I have not been following closely whether ICC 4.5 will include standardized spectral data or not, although I do not think that is the most important issue at this stage. I would be very interested to hear your thoughts. Warmest regards, Marti |
|
Closing this in favour of #596, which carries my reply and the proposed plug-in entry point list. This PR was written before our conversation about a plug-in architecture, so it is the wrong shape now: it puts iccMAX support in the core behind a compile-time macro, which is not where either of us wants it to end up. I would rather not leave a 4,900 line pull request sitting open against your review queue. The work is not lost. The macro-guarded implementation stays on Happy to reopen this or raise something narrower whenever the entry point discussion in #596 reaches a point where code would help. |
Hi Marti,
I would really prefer to not add limited spectral tags for printers to the V4 specification - especially if the reason is because little-cms doesn't support V5. With V5 we have a robust complete environment for dealing with spectral encoding. I would rather not have some half-baked kludge injected into ICC.1 that we have to support and explain and figure out how it works with everything else. So instead I propose that we add limited iccMAX support for Hybrid Printer Profiles with Reflectance into little-cms. This PR represents this effort.
The ICS for this was explicitly limited to allow for relatively minor additions to implement the ability to get spectral reflectances for systems that have already implemented V4 MPE support. The processing elements are limited to curves, cluts, and matrices - though V5 has singleSampledCurves, some additional parametric functions and an extended CLUT that allows for a little more flexibility in how the encoded lut data is stored (internally it's still 32-bit float). These changes were relatively easy to add to little-cms along with supporting the embedded v5 sub-tag and support for the spectralWhitePointTag and spectralViewingConditionsTag.
A small hybrid printer profile was added for testing, and work was put in to allow profile authors to create a hybrid printer profile if they choose. This new functionality is guarded by a define that you have to opt in for (so people don't get it if they don't want it).
I hope that you will consider moving this forward as it will simplify things on the specification side and potentially enable further iccMAX functionality in other areas in the future.
Max Derhak