CMM performs repeated conditionals and conversions in CIccPCS::Check

[maxderhak]

It would be better to refactor things so that operations being performed in CIccPCS::Check() were checked and added (only as needed) as new transforms in the transform sequence chain. This would allow checks to be performed during Begin() transform initialization (and not during transform application), operations that don't need to be done wouldn't get injected in the transform sequence chain thus allowing for a better ability to concatenate/remove transforms (thus simplifying the transform sequence chain) and improve application performance.

To state it more simply - there is a lot or redundant work being done during profile application that isn't really necessary.

[xsscx]

@maxderhak

brainstorming .. can you offer insight and feedback?

Refactoring for CIccPCS::Check

Constructor and Destructor Refactoring so that checks and operations are only performed as needed and are injected into the transform sequence chain during the Begin() initialization, rather than during the application of transforms with potential considerations for:

1. Thread Safety
2. Input Validation and Sanitization
3. Graceful Handling of Unexpected Conditions
4. Fuzzing with AFL++ and LibFuzzer

CIccPCS::CIccPCS() Constructor

CIccPCS::CIccPCS() 
  : m_bIsV2Lab(false), 
    m_Space(icSigUnknownData), 
    m_bLastPcsXform(false), 
    m_Convert{}
{
  // Constructor logic is now handled in the initializer list, ensuring consistent initialization
}

CIccPCS::Reset()

void CIccPCS::Reset(icColorSpaceSignature StartSpace, bool bUseLegacyPCS)
{
  // Clear state before applying new settings
  m_bIsV2Lab = IsSpacePCS(StartSpace) && bUseLegacyPCS;
  m_Space = StartSpace;

  // Handle potential reinitialization of m_Convert if necessary
  // (assuming m_Convert might need to be reset or initialized based on the new space)
  m_Convert = {}; // Reset m_Convert to default state
}

CIccPCS::Check()

const icFloatNumber *CIccPCS::Check(const icFloatNumber *SrcPixel, const CIccXform *pXform)
{
  // Retrieve next transform's source space and related properties
  icColorSpaceSignature NextSpace = pXform->GetSrcSpace();
  bool bIsV2 = pXform->UseLegacyPCS();
  bool bIsNextV2Lab = bIsV2 && (NextSpace == icSigLabData);
  bool bNoClip = pXform->NoClipPCS();

  // Early exit if last PCS transform was applied
  if (m_bLastPcsXform) {
    return SrcPixel;
  }

  // Initialize the return value to source pixel
  const icFloatNumber *rv = SrcPixel;

  // Perform the necessary color space conversions based on the current and next space
  if (m_bIsV2Lab && !bIsNextV2Lab) {
    // Convert from Lab v2 to Lab v4, and optionally to XYZ
    Lab2ToLab4(m_Convert, SrcPixel, bNoClip);
    if (NextSpace == icSigXYZData) {
      LabToXyz(m_Convert, m_Convert, bNoClip);
    }
    rv = m_Convert;
  } 
  else if (!m_bIsV2Lab && bIsNextV2Lab) {
    // Convert from XYZ to Lab v4, and then from Lab v4 to Lab v2
    if (m_Space == icSigXYZData) {
      XyzToLab(m_Convert, SrcPixel, bNoClip);
      SrcPixel = m_Convert;
    }
    Lab4ToLab2(m_Convert, SrcPixel);
    rv = m_Convert;
  } 
  else if (m_Space != NextSpace) {
    // Handle conversions between XYZ and Lab color spaces
    if (m_Space == icSigXYZData && NextSpace == icSigLabData) {
      XyzToLab(m_Convert, SrcPixel, bNoClip);
      rv = m_Convert;
    } 
    else if (m_Space == icSigLabData && NextSpace == icSigXYZData) {
      LabToXyz(m_Convert, SrcPixel, bNoClip);
      rv = m_Convert;
    }
  }

  // Update the internal state based on the current transform
  m_Space = pXform->GetDstSpace();
  m_bIsV2Lab = bIsV2 && (m_Space == icSigLabData);
  m_bLastPcsXform = (pXform->GetXformType() == icXformTypePCS);

  return rv;
}

CIccPCS::CheckLast()

void CIccPCS::CheckLast(icFloatNumber *Pixel, icColorSpaceSignature DestSpace, bool bNoClip)
{

   // Validate inputs
    if (Pixel == nullptr) {
        LogError("CIccPCS::CheckLast: Pixel pointer is null");
        throw std::invalid_argument("Pixel pointer is null");
    }

    if (DestSpace != icSigXYZData && DestSpace != icSigLabData) {
        LogError("CIccPCS::CheckLast: Invalid destination color space");
        throw std::invalid_argument("Invalid destination color space");
    }

    try {
        // Early exit if no transformation is needed
        if (m_Space == DestSpace && !m_bIsV2Lab) {
            LogInfo("CIccPCS::CheckLast: No transformation needed, exiting");
            return;
        }

        // Convert Lab v2 to Lab v4 if necessary
        if (m_bIsV2Lab) {
            LogInfo("CIccPCS::CheckLast: Converting Lab v2 to Lab v4");
            Lab2ToLab4(Pixel, Pixel, bNoClip);
        }

        // Perform the necessary final conversion based on the destination space
        if (m_Space == icSigXYZData && DestSpace == icSigLabData) {
            LogInfo("CIccPCS::CheckLast: Converting XYZ to Lab");
            XyzToLab(Pixel, Pixel, bNoClip);
        }
        else if (m_Space == icSigLabData && DestSpace == icSigXYZData) {
            LogInfo("CIccPCS::CheckLast: Converting Lab to XYZ");
            LabToXyz(Pixel, Pixel, bNoClip);
        }
    }
    catch (const std::exception &e) {
        std::stringstream ss;
        ss << "CIccPCS::CheckLast: Exception occurred - " << e.what();
        LogError(ss.str());
        std::cerr << ss.str() << std::endl;
        throw; // Re-throw the exception after logging
    }
}

[maxderhak]

Here are a few observations about CIccPCS::Check() and CIccPCS::CheckLast().

1. There are a set of conditionals that are performed. The state of these comparisons for each transform are constant, and known by the end of the CIccCmm::Begin() function.
2. There are operations that are sometimes performed depending on the results of the conditionals. Lab2ToLab4, Lab4ToLab2, LabToXYZ, XYZToLab

Using these observations, the operations that are sometimes performed could be implemented as xform objects. The conditionals could be performed during the begin function to insert instances of these objects. This may result in no objects being injected. After all objects are injected then a check should be made to potentially concatenate or simplify the transform chain. For example: if successive matrix operations are performed (because no intermediate transforms are injected), or Lab2ToLab4 followed by LabToXYZ might be replaced by a Lab2ToXYZ (since the Lab2ToLab4 does an internal conversion to XYZ), or pairs of objects may form logical Identity transforms and be removed (IE LabToXYZ followed by XYZToLab). Multiple passes through transform contatenation/simplification may be needed.

The CIccPCS::Check and CIccPCS::CheckLast() might be changed to return a list of objects to inject (rather than perform pixel transforms) and called during the Begin function rather than during the Apply.

Once this is done the apply transforms becomes simpler since the output of each transform will be directly applied by the next transform - no checks or function calls are needed to tweak things.

Note: I'm not sure about all the checks that have been added to CheclLast are needed as colorspace checks are performed during begin and functions never pass null into the function.

[maxderhak]

I've begun work on this in the PCS_Refactor branch. Preliminary testing shows that it works. It needs more extensive testing to make sure I haven't missed something.

[maxderhak]

PCS_Refactor branch has been merged into the main branch