add local caching for skia draws

add local caching for skia draws

PDFium assumes the lowest common denominator
and draws many strings and paths that can
be accumulated.

Defer canvas->restore() calls until required
because the clip changed.

Defer text and path draws as long as subsequent
calls concatenate additional data.

Include debugging switch to allow disabling cache
at compile-time while bugs are shaken out.

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

[caryclark, 2016-06-28 18:24:43]

Description was changed from

==========
wip; add local caching for skia draws

PDFium assumes the lowest common denominator
and draws many strings and paths that could
be accumulated.

BUG=
==========

to

==========
add local caching for skia draws

PDFium assumes the lowest common denominator
and draws many strings and paths that can
be accumulated.

Defer canvas->restore() calls until required
because the clip changed.

Defer text and path draws as long as subsequent
calls concatenate additional data.
==========

[caryclark, 2016-06-28 18:24:43]

caryclark@google.com changed reviewers:
+ dsinclair@chromium.org, reed@google.com

[caryclark, 2016-06-28 18:25:04]

The CQ bit was checked by caryclark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 18:25:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 18:42:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 18:42:21]

Dry run: This issue passed the CQ dry run.

[caryclark, 2016-06-28 20:55:41]

Description was changed from

==========
add local caching for skia draws

PDFium assumes the lowest common denominator
and draws many strings and paths that can
be accumulated.

Defer canvas->restore() calls until required
because the clip changed.

Defer text and path draws as long as subsequent
calls concatenate additional data.
==========

to

==========
add local caching for skia draws

PDFium assumes the lowest common denominator
and draws many strings and paths that can
be accumulated.

Defer canvas->restore() calls until required
because the clip changed.

Defer text and path draws as long as subsequent
calls concatenate additional data.

Include debugging switch to allow disabling cache
at compile-time while bugs are shaken out.
==========

[dsinclair, 2016-06-29 13:22:28]

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
File core/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:482: // they can be combined
nit: . at end of sentence.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:491: SkiaState() {
Initializer list?

SkiaState() : m_pFont(nullptr), m_pCache(nullptr), ... etc

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:712: // start here; more work to do thinking
through flushing
Is this still relevant? If so, can you add a TODO(caryclark)

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:728: pDriver->SkiaCanvas()->save();
There is a save() but no restore below()?

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:810: if (!pState->m_DashArray &&
!refState.m_DashArray)
Can't this just be removed in favour of the check on 812?

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:993: delete m_pCache;
Not needed with unique_ptr

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
File core/fxge/skia/fx_skia_device.h (right):

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.h:18: class SkiaState;
nit: Move this up with the other classes.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.h:130: void Dump();
const?

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.h:137: SkiaState* m_pCache;
std::unique_ptr

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.h:141: friend class SkiaState;
nit: Move this up to just after the private:

[caryclark, 2016-06-29 13:55:11]

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
File core/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:482: // they can be combined
On 2016/06/29 13:22:28, dsinclair wrote:
> nit: . at end of sentence.

Done.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:491: SkiaState() {
On 2016/06/29 13:22:28, dsinclair wrote:
> Initializer list?
> 
> SkiaState() : m_pFont(nullptr), m_pCache(nullptr), ... etc

Done.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:712: // start here; more work to do thinking
through flushing
On 2016/06/29 13:22:28, dsinclair wrote:
> Is this still relevant? If so, can you add a TODO(caryclark)

Removed (Done).

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:728: pDriver->SkiaCanvas()->save();
On 2016/06/29 13:22:28, dsinclair wrote:
> There is a save() but no restore below()?

That's correct. The save here is balanced by a subsequent restore when the
commands are flushed because the cache is no longer valid.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:810: if (!pState->m_DashArray &&
!refState.m_DashArray)
On 2016/06/29 13:22:28, dsinclair wrote:
> Can't this just be removed in favour of the check on 812?

The first condition returns false when both items are false.
The second condition returns true when either item is false.
I can't think of a way to remove one or the other.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:993: delete m_pCache;
On 2016/06/29 13:22:28, dsinclair wrote:
> Not needed with unique_ptr

Done.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
File core/fxge/skia/fx_skia_device.h (right):

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.h:18: class SkiaState;
On 2016/06/29 13:22:28, dsinclair wrote:
> nit: Move this up with the other classes.

Done.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.h:130: void Dump();
On 2016/06/29 13:22:28, dsinclair wrote:
> const?

Done.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.h:137: SkiaState* m_pCache;
On 2016/06/29 13:22:28, dsinclair wrote:
> std::unique_ptr

Done.

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.h:141: friend class SkiaState;
On 2016/06/29 13:22:28, dsinclair wrote:
> nit: Move this up to just after the private:

Done.

[dsinclair, 2016-06-29 13:58:34]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-06-29 13:58:35]

lgtm

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
File core/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/2064753002/diff/160001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device.cpp:810: if (!pState->m_DashArray &&
!refState.m_DashArray)
On 2016/06/29 13:55:10, caryclark wrote:
> On 2016/06/29 13:22:28, dsinclair wrote:
> > Can't this just be removed in favour of the check on 812?
> 
> The first condition returns false when both items are false.
> The second condition returns true when either item is false.
> I can't think of a way to remove one or the other.


I can't read, I mis-read the second statement as returning false. Ignore me.

[commit-bot: I haz the power, 2016-06-29 13:58:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 14:10:53]

Description was changed from

==========
add local caching for skia draws

PDFium assumes the lowest common denominator
and draws many strings and paths that can
be accumulated.

Defer canvas->restore() calls until required
because the clip changed.

Defer text and path draws as long as subsequent
calls concatenate additional data.

Include debugging switch to allow disabling cache
at compile-time while bugs are shaken out.
==========

to

==========
add local caching for skia draws

PDFium assumes the lowest common denominator
and draws many strings and paths that can
be accumulated.

Defer canvas->restore() calls until required
because the clip changed.

Defer text and path draws as long as subsequent
calls concatenate additional data.

Include debugging switch to allow disabling cache
at compile-time while bugs are shaken out.

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

[commit-bot: I haz the power, 2016-06-29 14:10:55]

Committed patchset #10 (id:180001) as
https://pdfium.googlesource.com/pdfium/+/e89391e2cbec0788d39985df9c0967dd467c...

[commit-bot: I haz the power, 2016-06-29 14:10:57]

CQ bit was unchecked.

[Lei Zhang, 2016-06-29 18:53:50]

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

[Lei Zhang, 2016-06-29 18:53:51]

https://codereview.chromium.org/2064753002/diff/180001/core/fxge/skia/DEPS
File core/fxge/skia/DEPS (right):

https://codereview.chromium.org/2064753002/diff/180001/core/fxge/skia/DEPS#ne...
core/fxge/skia/DEPS:2: '+fpdfsdk/include',
This is a layering violation. core/ should not depend on fpdfsdk/ and public/.

https://codereview.chromium.org/2064753002/diff/180001/core/fxge/skia/fx_skia...
File core/fxge/skia/fx_skia_device_unittest.cpp (right):

https://codereview.chromium.org/2064753002/diff/180001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device_unittest.cpp:114: FPDF_BITMAP bitmap =
FPDFBitmap_Create(w, h, 1);
Just create a CFX_DIBitmap instead? That's what you really want below anyway.

[caryclark, 2016-06-29 19:25:41]

https://codereview.chromium.org/2064753002/diff/180001/core/fxge/skia/DEPS
File core/fxge/skia/DEPS (right):

https://codereview.chromium.org/2064753002/diff/180001/core/fxge/skia/DEPS#ne...
core/fxge/skia/DEPS:2: '+fpdfsdk/include',
On 2016/06/29 18:53:50, Lei Zhang wrote:
> This is a layering violation. core/ should not depend on fpdfsdk/ and public/.

Sounds good. If unit tests belong in core directories, how should they depend on
the includes they need to compile?

https://codereview.chromium.org/2064753002/diff/180001/core/fxge/skia/fx_skia...
File core/fxge/skia/fx_skia_device_unittest.cpp (right):

https://codereview.chromium.org/2064753002/diff/180001/core/fxge/skia/fx_skia...
core/fxge/skia/fx_skia_device_unittest.cpp:114: FPDF_BITMAP bitmap =
FPDFBitmap_Create(w, h, 1);
On 2016/06/29 18:53:51, Lei Zhang wrote:
> Just create a CFX_DIBitmap instead? That's what you really want below anyway.

I copied the code construction used by pdfium_test to best emulate how the
tested routines are used. Feel free to edit it however you like.