Use number of components from ICC profile and alternateCS

core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp

Index: core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp
diff --git a/core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp b/core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp
index f1640ad14af135dc3419d531bac17c30e96711fd..323a0565227aaed1554a9c2c3251c3105098da0b 100644
--- a/core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp
+++ b/core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp
@@ -517,12 +517,16 @@ void CPDF_LabCS::TranslateImageLine(FX_LPBYTE pDestBuf, FX_LPCBYTE pSrcBuf, int
         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()
@@ -583,49 +587,69 @@ FX_BOOL CPDF_ICCBasedCS::v_Load(CPDF_Document* pDoc, CPDF_Array* pArray)
     if (pStream == NULL) {
         return FALSE;
     }
-    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;
-    }
-    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);
+    m_pProfile = pDoc->LoadIccProfile(pStream);
     if (!m_pProfile) {
         return FALSE;
     }
-    if (m_pProfile->m_pTransform == NULL) {
+    m_nComponents = m_pProfile->GetComponents(); //Try using the nComponents from ICC profile
+    CPDF_Dictionary* pDict = pStream->GetDict();
+    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 (alter_cs->CountComponents() > m_nComponents) {
-                    alter_cs->ReleaseCS();
-                } else {
-                    m_pAlterCS = alter_cs;
-                    m_bOwn = TRUE;
+                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) {
+            }
+            else if (m_nComponents == 4) {
                 m_pAlterCS = GetStockCS(PDFCS_DEVICECMYK);
-            } else {
-                m_pAlterCS = GetStockCS(PDFCS_DEVICEGRAY);
             }
         }
     }
+    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;
+        }
+    }
     return TRUE;
 }
 FX_BOOL CPDF_ICCBasedCS::GetRGB(FX_FLOAT* pBuf, FX_FLOAT& R, FX_FLOAT& G, FX_FLOAT& B) const