Convert CPDF_Parser::m_CrossRef to a std::map.

Convert CPDF_Parser::m_CrossRef to a std::map.

This improvements memory usage in case PDFs have sparse object numbers.

The map holds a struct that currently has only one member. The struct
will contain more members as other internal data structures get
converted.

BUG=pdfium:111
R=weili@chromium.org

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

[Lei Zhang, 2015-12-18 01:36:04]

thestig@chromium.org changed reviewers:
+ weili@chromium.org

[Lei Zhang, 2015-12-18 01:36:04]

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
File core/include/fpdfapi/fpdf_parser.h (left):

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
core/include/fpdfapi/fpdf_parser.h:397: CPDF_SecurityHandler*
GetSecurityHandler() {
Some dead code here.

[Wei Li, 2015-12-18 19:20:34]

Thank you for taking on this endeavor. :)

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
File core/include/fpdfapi/fpdf_parser.h (right):

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
core/include/fpdfapi/fpdf_parser.h:419: void SetLastObjNum(FX_DWORD num);
I think this may not be needed. The intent of calling this was to actually
re-size the Object array. For map, it is not needed.

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
core/include/fpdfapi/fpdf_parser.h:420: FX_DWORD GetLastObjNum() const;
To get the largest obj number in pdf, it can directly use the last key in object
map.

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
core/include/fpdfapi/fpdf_parser.h:421: FX_DWORD GetSize() const;
Similarly, GetSize for a parser could be ambiguous. Maybe we can get rid of it
by directly using GetLargestObjNum from the map?

https://codereview.chromium.org/1539573003/diff/1/core/src/fpdfapi/fpdf_edit/...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1539573003/diff/1/core/src/fpdfapi/fpdf_edit/...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:1324: for (; objnum < nOldSize;
objnum++) {
Not matter much. This sequential search could also be replaced by map iteration.

[Lei Zhang, 2015-12-19 02:09:52]

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
File core/include/fpdfapi/fpdf_parser.h (right):

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
core/include/fpdfapi/fpdf_parser.h:419: void SetLastObjNum(FX_DWORD num);
On 2015/12/18 19:20:34, Wei Li wrote:
> I think this may not be needed. The intent of calling this was to actually
> re-size the Object array. For map, it is not needed.

Done. I think that works because all the callers are external and only care
about this after the parsing has finished.

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
core/include/fpdfapi/fpdf_parser.h:420: FX_DWORD GetLastObjNum() const;
On 2015/12/18 19:20:34, Wei Li wrote:
> To get the largest obj number in pdf, it can directly use the last key in
object
> map. 

But if the map is empty, there's no last key. Thus we need a helper function to
check if the map is empty, which this still does.

https://codereview.chromium.org/1539573003/diff/1/core/include/fpdfapi/fpdf_p...
core/include/fpdfapi/fpdf_parser.h:421: FX_DWORD GetSize() const;
On 2015/12/18 19:20:34, Wei Li wrote:
> Similarly, GetSize for a parser could be ambiguous. Maybe we can get rid of it
> by directly using GetLargestObjNum from the map?

I replaced it with IsValidObjectNumber().

https://codereview.chromium.org/1539573003/diff/1/core/src/fpdfapi/fpdf_edit/...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1539573003/diff/1/core/src/fpdfapi/fpdf_edit/...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:1324: for (; objnum < nOldSize;
objnum++) {
On 2015/12/18 19:20:34, Wei Li wrote:
> Not matter much. This sequential search could also be replaced by map
iteration.

Previously, if we only had 1 element in |m_CrossRef| with objnum = 5, we would
iterate through this loop for objnums [objnum ... 5].

If we change this code to only iterate through the map, then we'd only iterate
through this loop for [5], which is a behavior change I'd like to avoid.

[Wei Li, 2015-12-21 18:06:46]

LGTM

https://codereview.chromium.org/1539573003/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1539573003/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:24: const int32_t kMaxRef =
1048576;
Maybe still write as 1<<20 or put in the comment to help readers know how you
get this number.

[Lei Zhang, 2015-12-21 19:40:04]

https://codereview.chromium.org/1539573003/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1539573003/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:24: const int32_t kMaxRef =
1048576;
On 2015/12/21 18:06:46, Wei Li wrote:
> Maybe still write as 1<<20 or put in the comment to help readers know how you
> get this number.

Actually, do you think we should leave this constant at this value or bump it
up? In theory, can't a valid PDF file have a xref section with a size that's
bigger than this? e.g.

xref
0 1
NNNNNNNNNN GGGGG n
1 0
NNNNNNNNNN GGGGG n
...

Each subsection is ~29 bytes, and there's 65536 possible object numbers = ~1.9
million bytes.

[Wei Li, 2015-12-21 21:45:04]

https://codereview.chromium.org/1539573003/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1539573003/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:24: const int32_t kMaxRef =
1048576;
On 2015/12/21 19:40:04, Lei Zhang wrote:
> On 2015/12/21 18:06:46, Wei Li wrote:
> > Maybe still write as 1<<20 or put in the comment to help readers know how
you
> > get this number.
> 
> Actually, do you think we should leave this constant at this value or bump it
> up? In theory, can't a valid PDF file have a xref section with a size that's
> bigger than this? e.g.
> 
> xref
> 0 1
> NNNNNNNNNN GGGGG n
> 1 0
> NNNNNNNNNN GGGGG n
> ...
> 
> Each subsection is ~29 bytes, and there's 65536 possible object numbers = ~1.9
> million bytes.

Isn't the theoretical bound of max ref num 10<<10 since it can have 10 digits?
But considering the size of a pdf file, this estimate seems not bad. I am fine
with either way as long as we have a comment here that we can check later.

[Lei Zhang, 2015-12-22 19:32:41]

On 2015/12/21 21:45:04, Wei Li(ooo) wrote:
>
https://codereview.chromium.org/1539573003/diff/40001/core/src/fpdfapi/fpdf_p...
> File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):
> 
>
https://codereview.chromium.org/1539573003/diff/40001/core/src/fpdfapi/fpdf_p...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:24: const int32_t kMaxRef
=
> 1048576;
> On 2015/12/21 19:40:04, Lei Zhang wrote:
> > On 2015/12/21 18:06:46, Wei Li wrote:
> > > Maybe still write as 1<<20 or put in the comment to help readers know how
> you
> > > get this number.
> > 
> > Actually, do you think we should leave this constant at this value or bump
it
> > up? In theory, can't a valid PDF file have a xref section with a size that's
> > bigger than this? e.g.
> > 
> > xref
> > 0 1
> > NNNNNNNNNN GGGGG n
> > 1 0
> > NNNNNNNNNN GGGGG n
> > ...
> > 
> > Each subsection is ~29 bytes, and there's 65536 possible object numbers =
~1.9
> > million bytes.
> 
> Isn't the theoretical bound of max ref num 10<<10 since it can have 10 digits?
> But considering the size of a pdf file, this estimate seems not bad. I am fine
> with either way as long as we have a comment here that we can check later.

I thought that's the theoretical bound on the size of the PDF file, whereas I'm
more interested in the size of the xref table. Anyway, I'll just add a comment
and land.

[Lei Zhang, 2015-12-22 20:04:49]

On 2015/12/22 19:32:41, Lei Zhang wrote:
> I thought that's the theoretical bound on the size of the PDF file, whereas
I'm
> more interested in the size of the xref table. Anyway, I'll just add a comment
> and land.

And for some reason I thought object numbers are limited in the same way as
generation numbers, but that was wrong, so the upper bound on that is 10<<10
since that's the limit in the xref table.

[Lei Zhang, 2015-12-22 21:42:00]

Description was changed from

==========
Convert CPDF_Parser::m_CrossRef to a std::map.

This improvements memory usage in case PDFs have sparse object numbers.

The map holds a struct that currently has only one member. The struct
will contain more members as other internal data structures get
converted.

BUG=pdfium:111
==========

to

==========
Convert CPDF_Parser::m_CrossRef to a std::map.

This improvements memory usage in case PDFs have sparse object numbers.

The map holds a struct that currently has only one member. The struct
will contain more members as other internal data structures get
converted.

BUG=pdfium:111
R=weili@chromium.org

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

[Lei Zhang, 2015-12-22 21:42:01]

Committed patchset #4 (id:60001) manually as
f6dafc90ec1a0df8d618efc68c0223f0cdd80ffb (presubmit successful).