Fix extraction of colour components in CPDF_DIBSource::DownSampleScanline32Bit

(Reland) Fix extraction of colour components in CPDF_DIBSource::DownSampleScanline32Bit

Previously, if |m_bpc| was < 8 (e.g. 4), this function may still try to
access the source components as if |m_bpc| == 8. Even when it fell into
the codepath that tried to do the right thing in this case, it was
wrong.

BUG=554151
R=tsepez@chromium.org, thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/9b99615806e358fdb396d1cb162ee2e69c2a20ec

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

[Oliver Chang, 2015-11-11 18:38:23]

Description was changed from

==========
Fix extraction of colour components in CPDF_DIBSource::DownSampleScanline32Bit

Previously, if |m_bpc| was < 8 (e.g. 4), this function may still try to
access the source components as if |m_bpc| == 8. Even when it fell into
the codepath that tried to do the right thing in this case, it was
wrong.

BUG=554151
==========

to

==========
Fix extraction of colour components in CPDF_DIBSource::DownSampleScanline32Bit

Previously, if |m_bpc| was < 8 (e.g. 4), this function may still try to
access the source components as if |m_bpc| == 8. Even when it fell into
the codepath that tried to do the right thing in this case, it was
wrong.

BUG=554151
R=tsepez@chromium.org,thestig@chromium.org
==========

[Oliver Chang, 2015-11-11 18:40:06]

ochang@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[Oliver Chang, 2015-11-11 18:42:28]

ptal

[Tom Sepez, 2015-11-11 21:09:45]

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:22: unsigned int
_GetBits8(const uint8_t* pData, size_t bitpos, size_t nbits) {
follow-up: bitpos needs to be an uint64_t.

kMaxImageDimension is 0x01ffff, so the maximum bytes in an image won't fit in a
size_t on 32 bit platforms, so image size is limited only by memory there. Its
conceivable that we might have a 1GB image even on such a platform, so bitpos ==
8G won't fit into a size_t there either.  This needs to percolate down through
all the other arithmetic.  Sigh.

nit: lose the leading _ as it is now namespaced.

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:23: unsigned int byte =
pData[bitpos / 8];
ASSERT(nbits == 1 || nbits == 2 || nbits == 4 ||  nbits == 8 || nbits == 16);
ASSERT((bitpos & (nbits - 1)) == 0);

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:31: return (byte >> (6 -
bitpos % 8)) & 0x03;
follow-up: There's probably a general form of this that's faster than branching
and handles n == 1, 2, 4 (but keep the n == 8, n==16 cases).

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1449: byte_offset *=
m_bpc;
nit: this can probably be an infix expression since we are not using safe types.
 And using byte_offset for the temporary is a little misleading since we want to
create a count of bits, then derive bytes from it.

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1470: 
I think there's a problem with {} nesting, I'd expect to see an else branch here
for the n == 8 case.

[Tom Sepez, 2015-11-11 21:39:09]

>
https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
> core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1470: 
> I think there's a problem with {} nesting, I'd expect to see an else branch
here
> for the n == 8 case.

Nevermind,  I can't read, I pulled the patch and am looking at it in my editor.

[Tom Sepez, 2015-11-11 21:43:02]

Anyways, I think the code is correct, so LGTM after adding the assert, and you
can fix the others as you like at your leisure.

[Oliver Chang, 2015-11-11 22:30:59]

thanks for the review! will address the follow-ups in separate CL(s) when I get
the time.

mind taking a quick sanity check at the differences between PS#1 and #2?

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:22: unsigned int
_GetBits8(const uint8_t* pData, size_t bitpos, size_t nbits) {
On 2015/11/11 21:09:44, Tom Sepez wrote:
> follow-up: bitpos needs to be an uint64_t.
> 
> kMaxImageDimension is 0x01ffff, so the maximum bytes in an image won't fit in
a
> size_t on 32 bit platforms, so image size is limited only by memory there. Its
> conceivable that we might have a 1GB image even on such a platform, so bitpos
==
> 8G won't fit into a size_t there either.  This needs to percolate down through
> all the other arithmetic.  Sigh.
> 
> nit: lose the leading _ as it is now namespaced.

Done. will fix up callers in a separate CL.

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:23: unsigned int byte =
pData[bitpos / 8];
On 2015/11/11 21:09:45, Tom Sepez wrote:
> ASSERT(nbits == 1 || nbits == 2 || nbits == 4 ||  nbits == 8 || nbits == 16);
> ASSERT((bitpos & (nbits - 1)) == 0);

Done.

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:31: return (byte >> (6 -
bitpos % 8)) & 0x03;
On 2015/11/11 21:09:45, Tom Sepez wrote:
> follow-up: There's probably a general form of this that's faster than
branching
> and handles n == 1, 2, 4 (but keep the n == 8, n==16 cases). 
> 

Acknowledged.

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1449: byte_offset *=
m_bpc;
On 2015/11/11 21:09:45, Tom Sepez wrote:
> nit: this can probably be an infix expression since we are not using safe
types.
>  And using byte_offset for the temporary is a little misleading since we want
to
> create a count of bits, then derive bytes from it.

Done. I initially used a safe size_t, but neglected to make it read a bit nicer
after I changed it back.

https://codereview.chromium.org/1433423002/diff/1/core/src/fpdfapi/fpdf_rende...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1470: 
On 2015/11/11 21:09:45, Tom Sepez wrote:
> I think there's a problem with {} nesting, I'd expect to see an else branch
here
> for the n == 8 case.

Acknowledged.

[Tom Sepez, 2015-11-11 23:33:59]

https://codereview.chromium.org/1433423002/diff/20001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1433423002/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:998: int R =
GetBits8(src_scan, src_bit_pos, m_bpc);
Just noticed this: Anything that GetBits8 is assigned to should be unsigned int.
Esp. if we shift it around later. Sorry.

[Tom Sepez, 2015-11-11 23:37:13]

On 2015/11/11 23:33:59, Tom Sepez wrote:
>
https://codereview.chromium.org/1433423002/diff/20001/core/src/fpdfapi/fpdf_r...
> File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):
> 
>
https://codereview.chromium.org/1433423002/diff/20001/core/src/fpdfapi/fpdf_r...
> core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:998: int R =
> GetBits8(src_scan, src_bit_pos, m_bpc);
> Just noticed this: Anything that GetBits8 is assigned to should be unsigned
int.
> Esp. if we shift it around later. Sorry.
Actually, lets do this in a follow-up.  LGTM++

[Oliver Chang, 2015-11-11 23:38:10]

On 2015/11/11 23:37:13, Tom Sepez wrote:
> On 2015/11/11 23:33:59, Tom Sepez wrote:
> >
>
https://codereview.chromium.org/1433423002/diff/20001/core/src/fpdfapi/fpdf_r...
> > File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1433423002/diff/20001/core/src/fpdfapi/fpdf_r...
> > core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:998: int R =
> > GetBits8(src_scan, src_bit_pos, m_bpc);
> > Just noticed this: Anything that GetBits8 is assigned to should be unsigned
> int.
> > Esp. if we shift it around later. Sorry.
> Actually, lets do this in a follow-up.  LGTM++

So much cleanup needed in this file.. thanks again!

[Oliver Chang, 2015-11-11 23:46:31]

Committed patchset #3 (id:40001) manually as
9b99615806e358fdb396d1cb162ee2e69c2a20ec (presubmit successful).

[Oliver Chang, 2015-11-12 00:03:35]

Description was changed from

==========
Fix extraction of colour components in CPDF_DIBSource::DownSampleScanline32Bit

Previously, if |m_bpc| was < 8 (e.g. 4), this function may still try to
access the source components as if |m_bpc| == 8. Even when it fell into
the codepath that tried to do the right thing in this case, it was
wrong.

BUG=554151
R=tsepez@chromium.org, thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/9b99615806e358fdb396d1cb162ee2e69c2a...
==========

to

==========
(Reland) Fix extraction of colour components in
CPDF_DIBSource::DownSampleScanline32Bit

Previously, if |m_bpc| was < 8 (e.g. 4), this function may still try to
access the source components as if |m_bpc| == 8. Even when it fell into
the codepath that tried to do the right thing in this case, it was
wrong.

BUG=554151
R=tsepez@chromium.org, thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/9b99615806e358fdb396d1cb162ee2e69c2a...
==========

[Oliver Chang, 2015-11-12 00:04:04]

Committed patchset #5 (id:80001) manually as
e21fe98d5b5da7da01503b985b07b90c8e811689 (presubmit successful).

[Oliver Chang, 2015-11-12 06:05:12]

Description was changed from

==========
(Reland) Fix extraction of colour components in
CPDF_DIBSource::DownSampleScanline32Bit

Previously, if |m_bpc| was < 8 (e.g. 4), this function may still try to
access the source components as if |m_bpc| == 8. Even when it fell into
the codepath that tried to do the right thing in this case, it was
wrong.

BUG=554151
R=tsepez@chromium.org, thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/9b99615806e358fdb396d1cb162ee2e69c2a...

Committed:
https://pdfium.googlesource.com/pdfium/+/e21fe98d5b5da7da01503b985b07b90c8e81...
==========

to

==========
(Reland) Fix extraction of colour components in
CPDF_DIBSource::DownSampleScanline32Bit

Previously, if |m_bpc| was < 8 (e.g. 4), this function may still try to
access the source components as if |m_bpc| == 8. Even when it fell into
the codepath that tried to do the right thing in this case, it was
wrong.

BUG=554151
R=tsepez@chromium.org, thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/9b99615806e358fdb396d1cb162ee2e69c2a...

Committed:
https://pdfium.googlesource.com/pdfium/+/e21fe98d5b5da7da01503b985b07b90c8e81...
==========

[Lei Zhang, 2015-11-12 23:42:22]

https://codereview.chromium.org/1433423002/diff/80001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1433423002/diff/80001/BUILD.gn#newcode156
BUILD.gn:156: 'testing/utils/path_service.cpp',
This is a bad merge, and now the PDFium DEPS roll is failing. Please fix it and
I'll try rolling DEPS again.

[Lei Zhang, 2015-11-12 23:44:31]

On 2015/11/12 23:42:22, Lei Zhang wrote:
> https://codereview.chromium.org/1433423002/diff/80001/BUILD.gn
> File BUILD.gn (right):
> 
> https://codereview.chromium.org/1433423002/diff/80001/BUILD.gn#newcode156
> BUILD.gn:156: 'testing/utils/path_service.cpp',
> This is a bad merge, and now the PDFium DEPS roll is failing. Please fix it
and
> I'll try rolling DEPS again.

Basically https://codereview.chromium.org/1419763005 got reverted. :-P

[Oliver Chang, 2015-11-12 23:45:55]

On 2015/11/12 23:44:31, Lei Zhang wrote:
> On 2015/11/12 23:42:22, Lei Zhang wrote:
> > https://codereview.chromium.org/1433423002/diff/80001/BUILD.gn
> > File BUILD.gn (right):
> > 
> > https://codereview.chromium.org/1433423002/diff/80001/BUILD.gn#newcode156
> > BUILD.gn:156: 'testing/utils/path_service.cpp',
> > This is a bad merge, and now the PDFium DEPS roll is failing. Please fix it
> and
> > I'll try rolling DEPS again.
> 
> Basically https://codereview.chromium.org/1419763005 got reverted. :-P

Oh my bad, must've derped when rebasing this CL.