Use number of components from ICC profile and alternateCS

core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp

 // Copyright 2014 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
  
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "../../../include/fpdfapi/fpdf_page.h"
 #include "../../../include/fpdfapi/fpdf_module.h"
 #include "../../../include/fxcodec/fx_codec.h"
 #include "pageint.h"
@@ skipping 499 matching lines @@
         lab[1] = (FX_FLOAT)(pSrcBuf[1] - 128);
         lab[2] = (FX_FLOAT)(pSrcBuf[2] - 128);
         GetRGB(lab, R, G, B);
         pDestBuf[0] = (FX_INT32)(B * 255);
         pDestBuf[1] = (FX_INT32)(G * 255);
         pDestBuf[2] = (FX_INT32)(R * 255);
         pDestBuf += 3;
         pSrcBuf += 3;
     }
 }
-CPDF_IccProfile::CPDF_IccProfile(FX_LPCBYTE pData, FX_DWORD dwSize, int nComponents)
+CPDF_IccProfile::CPDF_IccProfile(FX_LPCBYTE pData, FX_DWORD dwSize)
 {
-    m_bsRGB = nComponents == 3 && dwSize == 3144 && FXSYS_memcmp32(pData + 0x190, "sRGB IEC61966-2.1", 17) == 0;
-    m_pTransform = NULL;
-    if (!m_bsRGB && CPDF_ModuleMgr::Get()->GetIccModule()) {
-        m_pTransform = CPDF_ModuleMgr::Get()->GetIccModule()->CreateTransform_sRGB(pData, dwSize, nComponents);
+    if (dwSize == 3144 && FXSYS_memcmp32(pData + 0x190, "sRGB IEC61966-2.1", 17) == 0) {
+        m_bsRGB = TRUE;
+        m_pTransform = NULL;
+        m_srcnComponents = 3;

[jun_fang, 2014/08/25 18:56:51]

In some cases, m_nSrcComponents may not initialized. It shall be initialized to
be a default value out of this condition.

[Bo Xu, 2014/08/25 20:28:17]

Done.

+    }
+    else if (CPDF_ModuleMgr::Get()->GetIccModule()) {
+        m_bsRGB = FALSE;
+        m_pTransform = CPDF_ModuleMgr::Get()->GetIccModule()->CreateTransform_sRGB(pData, dwSize, &m_srcnComponents);

[Tom Sepez, 2014/08/25 18:18:51]

If neither expression in this if-statement is true, we run the risk of leaving
m_bsRGB, m_pTransform, and m_srcnComponent unitialized.  One common pattern for
these kinds of constructors is to do something like

CPDF_IccProfile::CPDF_IccProfile(FX_LPCBYTE pData, FX_DWORD dwSize) :
    m_bsRGB(FALSE),
    m_pTransform(NULL),
    m_srcnComponents(0) {
  if ( ... ) {
    m_bsRGB = TRUE;
    m_srcnComponents = 3;
  } else if ( ... ) {
    m_pTransform = ... ( ..., &m_nsrcnComponents)
  }
}

[Bo Xu, 2014/08/25 20:28:17]

That's right. Done.

     }
 }
 CPDF_IccProfile::~CPDF_IccProfile()
 {
     if (m_pTransform) {
         CPDF_ModuleMgr::Get()->GetIccModule()->DestroyTransform(m_pTransform);
     }
 }
 class CPDF_ICCBasedCS : public CPDF_ColorSpace
 {
@@ skipping 40 matching lines @@
     if (m_pProfile && m_pDocument) {
         m_pDocument->GetPageData()->ReleaseIccProfile(NULL, m_pProfile);
     }
 }
 FX_BOOL CPDF_ICCBasedCS::v_Load(CPDF_Document* pDoc, CPDF_Array* pArray)
 {
     CPDF_Stream* pStream = pArray->GetStream(1);
     if (pStream == NULL) {
         return FALSE;
     }
+    m_pProfile = pDoc->LoadIccProfile(pStream);
+    if (!m_pProfile) {
+        return FALSE;
+    }
+    m_nComponents = m_pProfile->GetComponents(); //Try using the nComponents from ICC profile
     CPDF_Dictionary* pDict = pStream->GetDict();
-    m_nComponents = pDict ? pDict->GetInteger(FX_BSTRC("N")) : 0;
-    if (m_nComponents != 1 && m_nComponents != 3 && m_nComponents != 4) {
-        return FALSE;
+    FX_INT32 Dict_nComponents = pDict ? pDict->GetInteger(FX_BSTRC("N")) : 0;

[Tom Sepez, 2014/08/25 18:18:51]

nit: nDictComponents for consistency perhaps.

[Bo Xu, 2014/08/25 20:28:17]

Done.

+    if (m_pProfile->m_pTransform == NULL) { // No valid ICC profile or using sRGB
+        CPDF_Object* pAlterCSObj = pDict ? pDict->GetElementValue(FX_BSTRC("Alternate")) : NULL;
+        if (pAlterCSObj) {
+            CPDF_ColorSpace* alter_cs = CPDF_ColorSpace::Load(pDoc, pAlterCSObj);

[Tom Sepez, 2014/08/25 18:18:51]

nit: If you wanted to do some cleanup along the way, this probably should be
pAlterCS (or pAlternateCS) to keep with the style of the file.  In chromium,
you're right that we vastly prefer the _ style identifiers, but for now, I've
been suggesting that we keep to whatever dominant style convention is already at
play in the file. 

[Bo Xu, 2014/08/25 20:28:17]

Done.

+            if (alter_cs) {
+                if (m_nComponents == 0) { // NO valid ICC profile
+                    if (alter_cs->CountComponents() > 0) { // Use Alternative colorspace
+                        m_nComponents = alter_cs->CountComponents();
+                        m_pAlterCS = alter_cs;
+                        m_bOwn = TRUE;
+                    }
+                    else { // No valid alternative colorspace
+                        alter_cs->ReleaseCS();
+                        if (Dict_nComponents == 1 || Dict_nComponents == 3 || Dict_nComponents == 4 ) {

[Tom Sepez, 2014/08/25 18:18:51]

nit: prefer early return as in
if (Dict_nComponents != 1 && Dict_nComponents != 3 && Dict_nComponents != 4 ) {
    return FALSE;
}
m_nComponents = ... 

[Bo Xu, 2014/08/25 20:28:17]

Done.

+                            m_nComponents = Dict_nComponents;
+                        }
+                        else {
+                            return FALSE;
+                        }
+                    }
+
+                }
+                else { // Using sRGB
+                    if (alter_cs->CountComponents() != m_nComponents) {
+                        alter_cs->ReleaseCS();
+                    }
+                    else {
+                        m_pAlterCS = alter_cs;
+                        m_bOwn = TRUE;
+                    }
+                }
+            }
+        }
+        if (!m_pAlterCS) {
+            if (m_nComponents == 1) {
+                m_pAlterCS = GetStockCS(PDFCS_DEVICEGRAY);
+            }
+            if (m_nComponents == 3) {

[Tom Sepez, 2014/08/25 18:18:51]

nit: else if

+                m_pAlterCS = GetStockCS(PDFCS_DEVICERGB);
+            }
+            else if (m_nComponents == 4) {
+                m_pAlterCS = GetStockCS(PDFCS_DEVICECMYK);
+            }
+        }
     }
     CPDF_Array* pRanges = pDict->GetArray(FX_BSTRC("Range"));
     m_pRanges = FX_Alloc(FX_FLOAT, m_nComponents * 2);
     for (int i = 0; i < m_nComponents * 2; i ++) {
         if (pRanges) {
             m_pRanges[i] = pRanges->GetNumber(i);
         } else if (i % 2) {
             m_pRanges[i] = 1.0f;
         } else {
             m_pRanges[i] = 0;
         }
     }
-    m_pProfile = pDoc->LoadIccProfile(pStream, m_nComponents);
-    if (!m_pProfile) {
-        return FALSE;
-    }
-    if (m_pProfile->m_pTransform == NULL) {
-        CPDF_Object* pAlterCSObj = pDict ? pDict->GetElementValue(FX_BSTRC("Alternate")) : NULL;
-        if (pAlterCSObj) {
-            CPDF_ColorSpace* alter_cs = CPDF_ColorSpace::Load(pDoc, pAlterCSObj);
-            if (alter_cs) {
-                if (alter_cs->CountComponents() > m_nComponents) {
-                    alter_cs->ReleaseCS();
-                } else {
-                    m_pAlterCS = alter_cs;
-                    m_bOwn = TRUE;
-                }
-            }
-        }
-        if (!m_pAlterCS) {
-            if (m_nComponents == 3) {
-                m_pAlterCS = GetStockCS(PDFCS_DEVICERGB);
-            } else if (m_nComponents == 4) {
-                m_pAlterCS = GetStockCS(PDFCS_DEVICECMYK);
-            } else {
-                m_pAlterCS = GetStockCS(PDFCS_DEVICEGRAY);
-            }
-        }
-    }
     return TRUE;
 }
 FX_BOOL CPDF_ICCBasedCS::GetRGB(FX_FLOAT* pBuf, FX_FLOAT& R, FX_FLOAT& G, FX_FLOAT& B) const
 {
     if (m_pProfile && m_pProfile->m_bsRGB) {
         R = pBuf[0];
         G = pBuf[1];
         B = pBuf[2];
         return TRUE;
     }
@@ skipping 760 matching lines @@
     PatternValue* pvalue = (PatternValue*)m_pBuffer;
     return pvalue->m_nComps ? pvalue->m_Comps : NULL;
 }
 FX_BOOL CPDF_Color::IsEqual(const CPDF_Color& other) const
 {
     if (m_pCS != other.m_pCS || m_pCS == NULL) {
         return FALSE;
     }
     return FXSYS_memcmp32(m_pBuffer, other.m_pBuffer, m_pCS->GetBufSize()) == 0;
 }