CalRGB color correction

CalRGB color correction

BUG=pdfium:19
R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/e7e6ca4

[Lei Zhang, 2014-07-24 20:35:19]

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:261: static void
MatInv_3by3(const FX_FLOAT* m, FX_FLOAT* m_inv)
Is there no utility library in pdfium that does this already?

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:272: FX_FLOAT det = a*(e*i -
f*h) - b*(i*d - f*g) + c*(d*h - e*g);
can |det| be 0?

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:310: for (size_t j = 0; j < 3;
j++)
indent this for loop?

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:319: if (R1 > 1) {
In pdf/instance.cc, we have a ClipToRange() function. You may want to copy it
over and use it here.

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:337: int scale = (int)(R1 *
1023);
Ditto. Turn this into a function and call it 3 times.

[Bo Xu, 2014-07-25 00:32:44]

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:261: static void
MatInv_3by3(const FX_FLOAT* m, FX_FLOAT* m_inv)
PDF matrix is a coordinate transformation matrix but not a general matrix. I
re-structure the code and put the matrix related codes to util part.

On 2014/07/24 20:35:19, Lei Zhang wrote:
> Is there no utility library in pdfium that does this already?

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:272: FX_FLOAT det = a*(e*i -
f*h) - b*(i*d - f*g) + c*(d*h - e*g);
Fixed.

On 2014/07/24 20:35:19, Lei Zhang wrote:
> can |det| be 0?

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:310: for (size_t j = 0; j < 3;
j++)
On 2014/07/24 20:35:19, Lei Zhang wrote:
> indent this for loop?

Re-structured.

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:319: if (R1 > 1) {
On 2014/07/24 20:35:19, Lei Zhang wrote:
> In pdf/instance.cc, we have a ClipToRange() function. You may want to copy it
> over and use it here.

I think we may not want to port code that in the chromium part into pdfium just
to preserve its independence?

https://codereview.chromium.org/403163002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:337: int scale = (int)(R1 *
1023);
On 2014/07/24 20:35:19, Lei Zhang wrote:
> Ditto. Turn this into a function and call it 3 times.

Done.

[Lei Zhang, 2014-07-25 00:42:05]

lgtm

https://codereview.chromium.org/403163002/diff/40001/core/include/fxcrt/fx_ba...
File core/include/fxcrt/fx_basic.h (right):

https://codereview.chromium.org/403163002/diff/40001/core/include/fxcrt/fx_ba...
core/include/fxcrt/fx_basic.h:1596: public:
The public here seems redundant? Same with CFX_Matrix_3by3.

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

https://codereview.chromium.org/403163002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:211: static float
RGB_Conversion(FX_FLOAT colorComponent)
do you want to return a FX_FLOAT?

[Bo Xu, 2014-07-25 00:48:08]

https://codereview.chromium.org/403163002/diff/40001/core/include/fxcrt/fx_ba...
File core/include/fxcrt/fx_basic.h (right):

https://codereview.chromium.org/403163002/diff/40001/core/include/fxcrt/fx_ba...
core/include/fxcrt/fx_basic.h:1596: public:
On 2014/07/25 00:42:05, Lei Zhang wrote:
> The public here seems redundant? Same with CFX_Matrix_3by3.

Done.

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

https://codereview.chromium.org/403163002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:211: static float
RGB_Conversion(FX_FLOAT colorComponent)
On 2014/07/25 00:42:05, Lei Zhang wrote:
> do you want to return a FX_FLOAT?

Right!

[Bo Xu, 2014-07-25 00:51:05]

Committed patchset #4 manually as re7e6ca4 (presubmit successful).