Remove the GetValidBpc check in application callers and move it to where m_bpc is assigned.

The problem of using GetValidBpc() in each function call is it could result in mismatch as seen in this case:
in ContinueToLoadMask(), m_bpc is re-assigned to 1 if m_bImageMask==1 regardless of the value from GetValidBpc().
This will result in mismatch if another function use the value from GetValidBpc().

The solution could be checking m_bImageMask in another function to make sure m_bpc is consistent, but that makes the code too cumbersome.
Also, we have to bring and are bringing in more and more GetValidBpc check, and this will continue with other buggy documents. So better to fix it now.

The original rational to use GetValidBpc() in where m_bpc is used is to respect the "raw" data from parsing.
However, if it will be ignored anyway and using value from GetValidBpc(), we'd better correct it at the very beginning.

BUG=408541
R=tsepez@chromium.org

Review URL: https://codereview.chromium.org/518443002

2 files changed, 67 insertions, 75 deletions

diff --git a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
index 5f0abde2be..f3a1996ab4 100644
--- a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
+++ b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
@@ -253,20 +253,19 @@ FX_BOOL CPDF_DIBSource::Load(CPDF_Document* pDoc, const CPDF_Stream* pStream, CP
 }
 int	CPDF_DIBSource::ContinueToLoadMask()
 {
-    FX_DWORD bpc = GetValidBpc();
     if (m_bImageMask) {
         m_bpp = 1;
-        bpc = 1;
+        m_bpc = 1;
         m_nComponents = 1;
         m_AlphaFlag = 1;
-    } else if (bpc * m_nComponents == 1) {
+    } else if (m_bpc * m_nComponents == 1) {
         m_bpp = 1;
-    } else if (bpc * m_nComponents <= 8) {
+    } else if (m_bpc * m_nComponents <= 8) {
         m_bpp = 8;
     } else {
         m_bpp = 24;
     }
-    if (!bpc || !m_nComponents) {
+    if (!m_bpc || !m_nComponents) {
         return 0;
     }
     FX_SAFE_DWORD pitch = m_Width;
@@ -451,6 +450,8 @@ int	CPDF_DIBSource::ContinueLoadDIBSource(IFX_Pause* pPause)
 }
 FX_BOOL CPDF_DIBSource::LoadColorInfo(CPDF_Dictionary* pFormResources, CPDF_Dictionary* pPageResources)
 {
+    m_bpc_orig = m_pDict->GetInteger(FX_BSTRC("BitsPerComponent"));
+    ValidateBpc();
     if (m_pDict->GetInteger("ImageMask")) {
         m_bImageMask = TRUE;
     }
@@ -492,7 +493,6 @@ FX_BOOL CPDF_DIBSource::LoadColorInfo(CPDF_Dictionary* pFormResources, CPDF_Dict
     if (m_pColorSpace == NULL) {
         return FALSE;
     }
-    m_bpc = m_pDict->GetInteger(FX_BSTRC("BitsPerComponent"));
     m_Family = m_pColorSpace->GetFamily();
     m_nComponents = m_pColorSpace->CountComponents();
     if (m_Family == PDFCS_ICCBASED && pCSObj->GetType() == PDFOBJ_NAME) {
@@ -565,8 +565,7 @@ int CPDF_DIBSource::CreateDecoder()
     if (decoder.IsEmpty()) {
         return 1;
     }
-    FX_DWORD bpc = GetValidBpc();
-    if (bpc == 0) {
+    if (m_bpc == 0) {
         return 0;
     }
     FX_LPCBYTE src_data = m_pStreamAcc->GetData();
@@ -579,17 +578,17 @@ int CPDF_DIBSource::CreateDecoder()
                      m_nComponents, pParams ? pParams->GetInteger(FX_BSTR("ColorTransform"), 1) : 1);
         if (NULL == m_pDecoder) {
             FX_BOOL bTransform = FALSE;
-            int comps, tmp_bpc;
+            int comps, bpc;
             ICodec_JpegModule* pJpegModule = CPDF_ModuleMgr::Get()->GetJpegModule();
-            if (pJpegModule->LoadInfo(src_data, src_size, m_Width, m_Height, comps, tmp_bpc, bTransform)) {
+            if (pJpegModule->LoadInfo(src_data, src_size, m_Width, m_Height, comps, bpc, bTransform)) {
                 m_nComponents = comps;
-                m_bpc = bpc = tmp_bpc;
+                m_bpc = bpc;
                 m_pDecoder = CPDF_ModuleMgr::Get()->GetJpegModule()->CreateDecoder(src_data, src_size, m_Width, m_Height,
                              m_nComponents, bTransform);
             }
         }
     } else if (decoder == FX_BSTRC("FlateDecode")) {
-        m_pDecoder = FPDFAPI_CreateFlateDecoder(src_data, src_size, m_Width, m_Height, m_nComponents, bpc, pParams);
+        m_pDecoder = FPDFAPI_CreateFlateDecoder(src_data, src_size, m_Width, m_Height, m_nComponents, m_bpc, pParams);
     } else if (decoder == FX_BSTRC("JPXDecode")) {
         LoadJpxBitmap();
         return m_pCachedBitmap != NULL ? 1 : 0;
@@ -603,10 +602,10 @@ int CPDF_DIBSource::CreateDecoder()
         m_Status = 1;
         return 2;
     } else if (decoder == FX_BSTRC("RunLengthDecode")) {
-        m_pDecoder = CPDF_ModuleMgr::Get()->GetCodecModule()->GetBasicModule()->CreateRunLengthDecoder(src_data, src_size, m_Width, m_Height, m_nComponents, bpc);
+        m_pDecoder = CPDF_ModuleMgr::Get()->GetCodecModule()->GetBasicModule()->CreateRunLengthDecoder(src_data, src_size, m_Width, m_Height, m_nComponents, m_bpc);
     }
     if (m_pDecoder) {
-        FX_SAFE_DWORD requested_pitch = bpc;
+        FX_SAFE_DWORD requested_pitch = m_bpc;
         requested_pitch *= m_nComponents;
         requested_pitch *= m_Width;
         requested_pitch += 7;
@@ -855,17 +854,16 @@ int CPDF_DIBSource::StartLoadMaskDIB()
 }
 void CPDF_DIBSource::LoadPalette()
 {
-    FX_DWORD bpc = GetValidBpc();
-    if (bpc == 0) {
+    if (m_bpc == 0) {
         return;
     }
-    if (bpc * m_nComponents > 8) {
+    if (m_bpc * m_nComponents > 8) {
         return;
     }
     if (m_pColorSpace == NULL) {
         return;
     }
-    if (bpc * m_nComponents == 1) {
+    if (m_bpc * m_nComponents == 1) {
         if (m_bDefaultDecode && (m_Family == PDFCS_DEVICEGRAY || m_Family == PDFCS_DEVICERGB)) {
             return;
         }
@@ -889,16 +887,16 @@ void CPDF_DIBSource::LoadPalette()
         }
         return;
     }
-    if (m_pColorSpace == CPDF_ColorSpace::GetStockCS(PDFCS_DEVICEGRAY) && bpc == 8 && m_bDefaultDecode) {
+    if (m_pColorSpace == CPDF_ColorSpace::GetStockCS(PDFCS_DEVICEGRAY) && m_bpc == 8 && m_bDefaultDecode) {
     } else {
-        int palette_count = 1 << (bpc * m_nComponents);
+        int palette_count = 1 << (m_bpc * m_nComponents);
         CFX_FixedBufGrow<FX_FLOAT, 16> color_values(m_nComponents);
         FX_FLOAT* color_value = color_values;
         for (int i = 0; i < palette_count; i ++) {
             int color_data = i;
             for (FX_DWORD j = 0; j < m_nComponents; j ++) {
-                int encoded_component = color_data % (1 << bpc);
-                color_data /= 1 << bpc;
+                int encoded_component = color_data % (1 << m_bpc);
+                color_data /= 1 << m_bpc;
                 color_value[j] = m_pCompData[j].m_DecodeMin + m_pCompData[j].m_DecodeStep * encoded_component;
             }
             FX_FLOAT R = 0, G = 0, B = 0;
@@ -917,49 +915,46 @@ void CPDF_DIBSource::LoadPalette()
         }
     }
 }
-FX_DWORD CPDF_DIBSource::GetValidBpc() const
+void CPDF_DIBSource::ValidateBpc()
 {
-    FX_DWORD bpc = m_bpc;
+    m_bpc = m_bpc_orig;
 	CPDF_Object * pFilter = m_pDict ? m_pDict->GetElementValue(FX_BSTRC("Filter")) : NULL;
     if (pFilter) {
         if (pFilter->GetType() == PDFOBJ_NAME) {
             CFX_ByteString filter = pFilter->GetString();
             if (filter == FX_BSTRC("CCITTFaxDecode") || filter == FX_BSTRC("JBIG2Decode")) {
-                bpc = 1;
+                m_bpc = 1;
             }
             if (filter == FX_BSTRC("RunLengthDecode") || filter == FX_BSTRC("DCTDecode")) {
-                bpc = 8;
+                m_bpc = 8;
             }
         } else if (pFilter->GetType() == PDFOBJ_ARRAY) {
             CPDF_Array *pArray = (CPDF_Array *)pFilter;
             if (pArray->GetString(pArray->GetCount() - 1) == FX_BSTRC("CCITTFacDecode") ||
                     pArray->GetString(pArray->GetCount() - 1) == FX_BSTRC("JBIG2Decode")) {
-                bpc = 1;
+                m_bpc = 1;
             }
             if (pArray->GetString(pArray->GetCount() - 1) == FX_BSTRC("RunLengthDecode") ||
                     pArray->GetString(pArray->GetCount() - 1) == FX_BSTRC("DCTDecode")) {
-                bpc = 8;
+                m_bpc = 8;
             }
         }
     }
-    if (bpc != 1 && bpc != 2 && bpc != 4 && bpc != 8 && bpc != 16) {
-        bpc = 0;
+    if (m_bpc != 1 && m_bpc != 2 && m_bpc != 4 && m_bpc != 8 && m_bpc != 16) {
+        m_bpc = 0;
     }
-
-    return bpc;
 }
 #define NORMALCOLOR_MAX(color, max) (color) > (max) ? (max) : (color) < 0 ? 0 : (color);
 void CPDF_DIBSource::TranslateScanline24bpp(FX_LPBYTE dest_scan, FX_LPCBYTE src_scan) const
 {
-    FX_DWORD bpc = GetValidBpc();
-    if (bpc == 0) {
+    if (m_bpc == 0) {
         return;
     }
-    int max_data = (1 << bpc) - 1;
+    int max_data = (1 << m_bpc) - 1;
     if (m_bDefaultDecode) {
         if (m_Family == PDFCS_DEVICERGB || m_Family == PDFCS_CALRGB) {
             FX_LPCBYTE src_pos = src_scan;
-            switch (bpc) {
+            switch (m_bpc) {
                 case 16:
                     for (int col = 0; col < m_Width; col ++) {
                         *dest_scan++ = src_pos[4];
@@ -980,12 +975,12 @@ void CPDF_DIBSource::TranslateScanline24bpp(FX_LPBYTE dest_scan, FX_LPCBYTE src_
                     int src_bit_pos = 0;
                     int dest_byte_pos = 0;
                     for (int column = 0; column < m_Width; column ++) {
-                        int R = _GetBits8(src_scan, src_bit_pos, bpc);
-                        src_bit_pos += bpc;
-                        int G = _GetBits8(src_scan, src_bit_pos, bpc);
-                        src_bit_pos += bpc;
-                        int B = _GetBits8(src_scan, src_bit_pos, bpc);
-                        src_bit_pos += bpc;
+                        int R = _GetBits8(src_scan, src_bit_pos, m_bpc);
+                        src_bit_pos += m_bpc;
+                        int G = _GetBits8(src_scan, src_bit_pos, m_bpc);
+                        src_bit_pos += m_bpc;
+                        int B = _GetBits8(src_scan, src_bit_pos, m_bpc);
+                        src_bit_pos += m_bpc;
                         R = NORMALCOLOR_MAX(R, max_data);
                         G = NORMALCOLOR_MAX(G, max_data);
                         B = NORMALCOLOR_MAX(B, max_data);
@@ -997,7 +992,7 @@ void CPDF_DIBSource::TranslateScanline24bpp(FX_LPBYTE dest_scan, FX_LPCBYTE src_
                     break;
             }
             return;
-        } else if (bpc == 8) {
+        } else if (m_bpc == 8) {
             if (m_nComponents == m_pColorSpace->CountComponents())
                 m_pColorSpace->TranslateImageLine(dest_scan, src_scan, m_Width, m_Width, m_Height,
                                                   m_bLoadMask && m_GroupFamily == PDFCS_DEVICECMYK && m_Family == PDFCS_DEVICECMYK);
@@ -1007,7 +1002,7 @@ void CPDF_DIBSource::TranslateScanline24bpp(FX_LPBYTE dest_scan, FX_LPCBYTE src_
     CFX_FixedBufGrow<FX_FLOAT, 16> color_values1(m_nComponents);
     FX_FLOAT* color_values = color_values1;
     FX_FLOAT R = 0.0f, G = 0.0f, B = 0.0f;
-    if (bpc == 8) {
+    if (m_bpc == 8) {
         int src_byte_pos = 0;
         int dest_byte_pos = 0;
         for (int column = 0; column < m_Width; column ++) {
@@ -1035,13 +1030,12 @@ void CPDF_DIBSource::TranslateScanline24bpp(FX_LPBYTE dest_scan, FX_LPCBYTE src_
     } else {
         int src_bit_pos = 0;
         int dest_byte_pos = 0;
-        FX_DWORD bpc = GetValidBpc();
         for (int column = 0; column < m_Width; column ++) {
             for (FX_DWORD color = 0; color < m_nComponents; color ++) {
-                int data = _GetBits8(src_scan, src_bit_pos, bpc);
+                int data = _GetBits8(src_scan, src_bit_pos, m_bpc);
                 color_values[color] = m_pCompData[color].m_DecodeMin +
                                       m_pCompData[color].m_DecodeStep * data;
-                src_bit_pos += bpc;
+                src_bit_pos += m_bpc;
             }
             if (m_bLoadMask && m_GroupFamily == PDFCS_DEVICECMYK && m_Family == PDFCS_DEVICECMYK) {
                 FX_FLOAT k = 1.0f - color_values[3];
@@ -1070,12 +1064,11 @@ FX_LPBYTE CPDF_DIBSource::GetBuffer() const
 }
 FX_LPCBYTE CPDF_DIBSource::GetScanline(int line) const
 {
-    FX_DWORD bpc = GetValidBpc();
-    if (bpc == 0) {
+    if (m_bpc == 0) {
         return NULL;
     }
     FX_SAFE_DWORD src_pitch = m_Width;
-    src_pitch *= bpc;
+    src_pitch *= m_bpc;
     src_pitch *= m_nComponents;
     src_pitch += 7;
     src_pitch /= 8;
@@ -1100,7 +1093,7 @@ FX_LPCBYTE CPDF_DIBSource::GetScanline(int line) const
         FXSYS_memset8(pLineBuf, 0xff, m_Pitch);
         return pLineBuf;
     }
-    if (bpc * m_nComponents == 1) {
+    if (m_bpc * m_nComponents == 1) {
         if (m_bImageMask && m_bDefaultDecode) {
             for (FX_DWORD i = 0; i < src_pitch_value; i++) {
                 m_pLineBuf[i] = ~pSrcLine[i];
@@ -1132,17 +1125,17 @@ FX_LPCBYTE CPDF_DIBSource::GetScanline(int line) const
         }
         return m_pLineBuf;
     }
-    if (bpc * m_nComponents <= 8) {
-        if (bpc == 8) {
+    if (m_bpc * m_nComponents <= 8) {
+        if (m_bpc == 8) {
             FXSYS_memcpy32(m_pLineBuf, pSrcLine, src_pitch_value);
         } else {
             int src_bit_pos = 0;
             for (int col = 0; col < m_Width; col ++) {
                 int color_index = 0;
                 for (FX_DWORD color = 0; color < m_nComponents; color ++) {
-                    int data = _GetBits8(pSrcLine, src_bit_pos, bpc);
-                    color_index |= data << (color * bpc);
-                    src_bit_pos += bpc;
+                    int data = _GetBits8(pSrcLine, src_bit_pos, m_bpc);
+                    color_index |= data << (color * m_bpc);
+                    src_bit_pos += m_bpc;
                 }
                 m_pLineBuf[col] = color_index;
             }
@@ -1169,7 +1162,7 @@ FX_LPCBYTE CPDF_DIBSource::GetScanline(int line) const
         return m_pLineBuf;
     }
     if (m_bColorKey) {
-        if (m_nComponents == 3 && bpc == 8) {
+        if (m_nComponents == 3 && m_bpc == 8) {
             FX_LPBYTE alpha_channel = m_pMaskedLine + 3;
             for (int col = 0; col < m_Width; col ++) {
                 FX_LPCBYTE pPixel = pSrcLine + col * 3;
@@ -1214,10 +1207,9 @@ void CPDF_DIBSource::DownSampleScanline(int line, FX_LPBYTE dest_scan, int dest_
         return;
     }
 
-    FX_DWORD bpc = GetValidBpc();
     FX_DWORD src_width = m_Width;
     FX_SAFE_DWORD pitch = src_width;
-    pitch *= bpc;
+    pitch *= m_bpc;
     pitch *= m_nComponents;
     pitch += 7;
     pitch /= 8;
@@ -1241,7 +1233,7 @@ void CPDF_DIBSource::DownSampleScanline(int line, FX_LPBYTE dest_scan, int dest_
             pSrcLine = m_pStreamAcc->GetData() + line * src_pitch;
         }
     }
-    int orig_Bpp = bpc * m_nComponents / 8;
+    int orig_Bpp = m_bpc * m_nComponents / 8;
     int dest_Bpp = dest_bpp / 8;
     if (pSrcLine == NULL) {
         FXSYS_memset32(dest_scan, 0xff, dest_Bpp * clip_width);
@@ -1257,7 +1249,7 @@ void CPDF_DIBSource::DownSampleScanline(int line, FX_LPBYTE dest_scan, int dest_
     }
 
     CFX_FixedBufGrow<FX_BYTE, 128> temp(orig_Bpp);
-    if (bpc * m_nComponents == 1) {
+    if (m_bpc * m_nComponents == 1) {
         FX_DWORD set_argb = (FX_DWORD) - 1, reset_argb = 0;
         if (m_bImageMask) {
             if (m_bDefaultDecode) {
@@ -1325,15 +1317,15 @@ void CPDF_DIBSource::DownSampleScanline(int line, FX_LPBYTE dest_scan, int dest_
             }
         }
         return;
-    } else if (bpc * m_nComponents <= 8) {
-        if (bpc < 8) {
+    } else if (m_bpc * m_nComponents <= 8) {
+        if (m_bpc < 8) {
             int src_bit_pos = 0;
             for (FX_DWORD col = 0; col < src_width; col ++) {
                 int color_index = 0;
                 for (FX_DWORD color = 0; color < m_nComponents; color ++) {
-                    int data = _GetBits8(pSrcLine, src_bit_pos, bpc);
-                    color_index |= data << (color * bpc);
-                    src_bit_pos += bpc;
+                    int data = _GetBits8(pSrcLine, src_bit_pos, m_bpc);
+                    color_index |= data << (color * m_bpc);
+                    src_bit_pos += m_bpc;
                 }
                 m_pLineBuf[col] = color_index;
             }
@@ -1382,14 +1374,14 @@ void CPDF_DIBSource::DownSampleScanline(int line, FX_LPBYTE dest_scan, int dest_
     } else {
         int last_src_x = -1;
         FX_ARGB last_argb;
-        FX_FLOAT orig_Not8Bpp = (FX_FLOAT)bpc * (FX_FLOAT)m_nComponents / 8.0f;
-        FX_FLOAT unit_To8Bpc = 255.0f / ((1 << bpc) - 1);
+        FX_FLOAT orig_Not8Bpp = (FX_FLOAT)m_bpc * (FX_FLOAT)m_nComponents / 8.0f;
+        FX_FLOAT unit_To8Bpc = 255.0f / ((1 << m_bpc) - 1);
         for (int i = 0; i < clip_width; i ++) {
             int dest_x = clip_left + i;
             FX_DWORD src_x = (bFlipX ? (dest_width - dest_x - 1) : dest_x) * (FX_INT64)src_width / dest_width;
             src_x %= src_width;
             FX_LPCBYTE pSrcPixel = NULL;
-            if (bpc % 8 == 0) {
+            if (m_bpc % 8 == 0) {
                 pSrcPixel = pSrcLine + src_x * orig_Bpp;
             } else {
                 pSrcPixel = pSrcLine + (int)(src_x * orig_Not8Bpp);
@@ -1408,14 +1400,14 @@ void CPDF_DIBSource::DownSampleScanline(int line, FX_LPBYTE dest_scan, int dest_
                         }
                         m_pColorSpace->TranslateImageLine(color, temp, 1, 0, 0, m_bLoadMask && m_GroupFamily == PDFCS_DEVICECMYK && m_Family == PDFCS_DEVICECMYK);
                     } else {
-                        if (bpc < 8) {
+                        if (m_bpc < 8) {
                             int src_bit_pos = 0;
                             if (src_x % 2) {
                                 src_bit_pos = 4;
                             }
                             for (FX_DWORD i = 0; i < m_nComponents; i ++) {
-                                temp[i] = (FX_BYTE)(_GetBits8(pSrcPixel, src_bit_pos, bpc) * unit_To8Bpc);
-                                src_bit_pos += bpc;
+                                temp[i] = (FX_BYTE)(_GetBits8(pSrcPixel, src_bit_pos, m_bpc) * unit_To8Bpc);
+                                src_bit_pos += m_bpc;
                             }
                             m_pColorSpace->TranslateImageLine(color, temp, 1, 0, 0, m_bLoadMask && m_GroupFamily == PDFCS_DEVICECMYK && m_Family == PDFCS_DEVICECMYK);
                         } else {
@@ -1428,7 +1420,7 @@ void CPDF_DIBSource::DownSampleScanline(int line, FX_LPBYTE dest_scan, int dest_
                 }
                 if (m_bColorKey) {
                     int alpha = 0xff;
-                    if (m_nComponents == 3 && bpc == 8) {
+                    if (m_nComponents == 3 && m_bpc == 8) {
                         alpha = (pSrcPixel[0] < m_pCompData[0].m_ColorKeyMin ||
                                  pSrcPixel[0] > m_pCompData[0].m_ColorKeyMax ||
                                  pSrcPixel[1] < m_pCompData[1].m_ColorKeyMin ||
diff --git a/core/src/fpdfapi/fpdf_render/render_int.h b/core/src/fpdfapi/fpdf_render/render_int.h
index fda3666871..af0e74e5e4 100644
--- a/core/src/fpdfapi/fpdf_render/render_int.h
+++ b/core/src/fpdfapi/fpdf_render/render_int.h
@@ -419,13 +419,13 @@ protected:
     void				LoadPalette();
     FX_BOOL				CreateDecoder();
     void				TranslateScanline24bpp(FX_LPBYTE dest_scan, FX_LPCBYTE src_scan) const;
-    FX_DWORD            GetValidBpc() const;
+    void                ValidateBpc();
     CPDF_Document*		m_pDocument;
     const CPDF_Stream*	m_pStream;
     CPDF_StreamAcc*		m_pStreamAcc;
     const CPDF_Dictionary*	m_pDict;
     CPDF_ColorSpace*	m_pColorSpace;
-    FX_DWORD			m_Family, m_bpc, m_nComponents, m_GroupFamily;
+    FX_DWORD			m_Family, m_bpc, m_bpc_orig, m_nComponents, m_GroupFamily;
     FX_BOOL				m_bLoadMask;
     FX_BOOL				m_bDefaultDecode, m_bImageMask, m_bColorKey;
     DIB_COMP_DATA*		m_pCompData;