Use number of components from ICC profile and alternateCS

Use number of components from ICC profile and alternate color space

BUG=406806
Committed: https://pdfium.googlesource.com/pdfium/+/be83103

[Bo Xu, 2014-08-24 21:56:22]

Hi Tom, Please review this one.

For this bug and some previous bugs, the number of components from /N entry in
the color space dictionary is hacked. Previously, memory allocation is based on
this number, but it should depends on the actual ICC profile or alternative
color space. If neither of them are valid, we can try to use the /N entry (if
it's valid).

[Bo Xu, 2014-08-25 17:01:27]

Committed patchset #1 manually as be83103 (presubmit successful).

[Bo Xu, 2014-08-25 17:03:09]

On 2014/08/25 17:01:27, Bo Xu wrote:
> Committed patchset #1 manually as be83103 (presubmit successful).

Hi Tom, I accidentally landed this patch, is there a way to revert this?

[Tom Sepez, 2014-08-25 18:18:52]

I pinged nico about the best way to revert.  In the mean time, here are a few
thoughts.  Also, If there's a way to break this into smaller pieces, that might
be easier to review.  I think its on the right track.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:529: m_pTransform =
CPDF_ModuleMgr::Get()->GetIccModule()->CreateTransform_sRGB(pData, dwSize,
&m_srcnComponents);
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)
  }
}

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:596: FX_INT32 Dict_nComponents =
pDict ? pDict->GetInteger(FX_BSTRC("N")) : 0;
nit: nDictComponents for consistency perhaps.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:600: CPDF_ColorSpace* alter_cs =
CPDF_ColorSpace::Load(pDoc, pAlterCSObj);
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.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:610: if (Dict_nComponents == 1
|| Dict_nComponents == 3 || Dict_nComponents == 4 ) {
nit: prefer early return as in
if (Dict_nComponents != 1 && Dict_nComponents != 3 && Dict_nComponents != 4 ) {
    return FALSE;
}
m_nComponents = ...

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:634: if (m_nComponents == 3) {
nit: else if

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/pageint.h (right):

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/pageint.h:473: FX_INT32 GetComponents() { return
m_srcnComponents; }
nit: const method.

[Bo Xu, 2014-08-25 18:37:02]

A revert of this CL (patchset #1) has been created in
https://codereview.chromium.org/504883003/ by bo_xu@foxitsoftware.com.

The reason for reverting is: Needs to address comments before landing.

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

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:525: m_srcnComponents = 3;
In some cases, m_nSrcComponents may not initialized. It shall be initialized to
be a default value out of this condition.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/pageint.h (right):

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/pageint.h:475: FX_INT32               
m_srcnComponents;
should be m_nSrcComponents?

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_icc.cpp (right):

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_icc.cpp:10: #define N_COMPONENT_LAB 3
It's better to use const variables rather than macro like:
const FX_DWORD N_COMPONENT_LAB = 3;

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_icc.cpp:69: {
FX_DWORD components = N_COMPONENT_DEFAULT;

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_icc.cpp:72: return N_COMPONENT_LAB;
use |components = N_COMPONENT_LAB| to replace the return here.

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_icc.cpp:86: }
return components;

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_icc.cpp:96: *nSrcComponents = 0;
Need to check whether nSrcComponents is NULL.

[Bo Xu, 2014-08-25 20:28:18]

New patch set in https://codereview.chromium.org/503883002/

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:525: m_srcnComponents = 3;
On 2014/08/25 18:56:51, jun_fang wrote:
> In some cases, m_nSrcComponents may not initialized. It shall be initialized
to
> be a default value out of this condition.

Done.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:529: m_pTransform =
CPDF_ModuleMgr::Get()->GetIccModule()->CreateTransform_sRGB(pData, dwSize,
&m_srcnComponents);
On 2014/08/25 18:18:51, Tom Sepez wrote:
> 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)
>   }
> }

That's right. Done.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:596: FX_INT32 Dict_nComponents =
pDict ? pDict->GetInteger(FX_BSTRC("N")) : 0;
On 2014/08/25 18:18:51, Tom Sepez wrote:
> nit: nDictComponents for consistency perhaps.

Done.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:600: CPDF_ColorSpace* alter_cs =
CPDF_ColorSpace::Load(pDoc, pAlterCSObj);
On 2014/08/25 18:18:51, Tom Sepez wrote:
> 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. 

Done.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:610: if (Dict_nComponents == 1
|| Dict_nComponents == 3 || Dict_nComponents == 4 ) {
On 2014/08/25 18:18:51, Tom Sepez wrote:
> nit: prefer early return as in
> if (Dict_nComponents != 1 && Dict_nComponents != 3 && Dict_nComponents != 4 )
{
>     return FALSE;
> }
> m_nComponents = ... 

Done.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/pageint.h (right):

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/pageint.h:473: FX_INT32 GetComponents() { return
m_srcnComponents; }
On 2014/08/25 18:18:51, Tom Sepez wrote:
> nit: const method.

Done.

https://codereview.chromium.org/493163003/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/pageint.h:475: FX_INT32               
m_srcnComponents;
On 2014/08/25 18:56:51, jun_fang wrote:
> should be m_nSrcComponents?

Done.

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_icc.cpp (right):

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_icc.cpp:10: #define N_COMPONENT_LAB 3
On 2014/08/25 18:56:51, jun_fang wrote:
> It's better to use const variables rather than macro like:
> const FX_DWORD N_COMPONENT_LAB = 3;

Done.

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_icc.cpp:69: {
On 2014/08/25 18:56:51, jun_fang wrote:
> FX_DWORD components = N_COMPONENT_DEFAULT;

Done.

https://codereview.chromium.org/493163003/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_icc.cpp:96: *nSrcComponents = 0;
On 2014/08/25 18:56:51, jun_fang wrote:
> Need to check whether nSrcComponents is NULL.

I have changed nSrcComponents from pointer to reference.