support gradients and stroke+fill

support gradients and stroke+fill

To draw paths that are stroked and filled with a pair of colors,
reduce the fill by the width of the stroke. This is implemented
with path ops subtracting the resolved stroke shape from the
fill shape. This permits rendering the result without requiring
an offscreen bitmap.

The implementation for stroke+fill requires a new entry into
the graphics engine, so a bit was added to device caps for that.

Extract the gradient information out of the axial gradient
function descriptions, and when possible, use Skia to draw
the linear gradient directly. This requires making the function
descriptions non-opaque, and adding a bit to device caps for
another entry into the graphics engine.

BUG=

Committed: https://pdfium.googlesource.com/pdfium/+/342f6fa66f6d843fe07d9b6a133656f83c8d62f6

[caryclark, 2016-03-24 16:42:27]

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

[caryclark, 2016-03-24 16:42:28]

PTAL. This is mostly rearranging foxit's code. Let me know what
rewriting-refactoring is required to land.

[dsinclair, 2016-03-24 17:43:38]

Overall, this looks good. The biggest thing I can see is if you could pull out
the other half of the methods you're short circuiting into their own functions
would make things a bit nicer.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_render/fp...
File core/fpdfapi/fpdf_render/fpdf_render_pattern.cpp (right):

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_render/fp...
core/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:868:
buffer.Initialize(m_pContext, m_pDevice, &clip_rect, m_pCurObj, 150);
Can the rest of this be moved to a function? Would also be nice to know when we
can remove this, if we get to all skia, can this chunk be removed? Or if we
remove AGG?

https://codereview.chromium.org/1828283002/diff/1/core/fxge/ge/fx_ge_device.cpp
File core/fxge/ge/fx_ge_device.cpp (right):

https://codereview.chromium.org/1828283002/diff/1/core/fxge/ge/fx_ge_device.c...
core/fxge/ge/fx_ge_device.cpp:232: if (!(m_RenderCaps & FXRC_GET_BITS)) {
similar, can this be moved to a function with a note of if it can be removed?

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
File core/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:446: }
nit: No {}s

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:512: FX_FLOAT t_min = 0, t_max = 1.0f;
nit: one per line.

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:526: if (nFuncs != 1)
Can we do this earlier before all the other work?

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:533: case 2:  // exp interp function
Can you expand 'exp interp' as I don't know what those mean. Exponential
Interpolation?

Or, better, can we add a enum FunctionTypes {} and name the numbers?

[Tom Sepez, 2016-03-24 18:07:08]

As is the case with most of these kinds of CLs, your code is fine, but there's a
pile of technical debt we can clean up in the process ...

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/fpdf...
File core/fpdfapi/fpdf_page/fpdf_page_func.cpp (right):

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:837: CPDF_Function* pFunc = NULL;
nit: move to line 846?

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:861: pFunc->m_nType = type;
Shouldn't the ctors of the various function sub-classes be setting this?

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
File core/fpdfapi/fpdf_page/pageint.h (right):

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:392: CPDF_Function();
nit: probably should take the type as a parameter, then the various sub-class
ctors don't have to muck with m_nType.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:393: int m_nInputs, m_nOutputs;
nit: one per line, also blank line between methods and members.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:396: int m_nType;
nit: I'd put this as the first member.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:397: FX_BOOL Init(CPDF_Object* pObj);
nit: move these to line 393.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:414: int m_nOrigOutputs;
nit: may save 8 bytes per instance on 64 bits if this came before m_Exponent

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
File core/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:162: const CPDF_ExpIntFunc* expIntFunc =
nit: want to assert on type before the cast?

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:183: const CPDF_StitchFunc* stitchFunc =
dittp

https://codereview.chromium.org/1828283002/diff/1/core/include/fxge/fx_ge.h
File core/include/fxge/fx_ge.h (right):

https://codereview.chromium.org/1828283002/diff/1/core/include/fxge/fx_ge.h#n...
core/include/fxge/fx_ge.h:593: virtual void* GetPlatformSurface() { return NULL;
}
nit: should any of these be const?

https://codereview.chromium.org/1828283002/diff/1/core/include/fxge/fx_ge.h#n...
core/include/fxge/fx_ge.h:594: 
nit: might as well kill these blank lines?

[caryclark, 2016-03-25 20:47:56]

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/fpdf...
File core/fpdfapi/fpdf_page/fpdf_page_func.cpp (right):

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:837: CPDF_Function* pFunc = NULL;
On 2016/03/24 18:07:07, Tom Sepez wrote:
> nit: move to line 846?

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/fpdf...
core/fpdfapi/fpdf_page/fpdf_page_func.cpp:861: pFunc->m_nType = type;
On 2016/03/24 18:07:07, Tom Sepez wrote:
> Shouldn't the ctors of the various function sub-classes be setting this?

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
File core/fpdfapi/fpdf_page/pageint.h (right):

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:392: CPDF_Function();
On 2016/03/24 18:07:07, Tom Sepez wrote:
> nit: probably should take the type as a parameter, then the various sub-class
> ctors don't have to muck with m_nType.

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:393: int m_nInputs, m_nOutputs;
On 2016/03/24 18:07:07, Tom Sepez wrote:
> nit: one per line, also blank line between methods and members.

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:396: int m_nType;
On 2016/03/24 18:07:08, Tom Sepez wrote:
> nit: I'd put this as the first member.

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:397: FX_BOOL Init(CPDF_Object* pObj);
On 2016/03/24 18:07:07, Tom Sepez wrote:
> nit: move these to line 393.

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_page/page...
core/fpdfapi/fpdf_page/pageint.h:414: int m_nOrigOutputs;
On 2016/03/24 18:07:08, Tom Sepez wrote:
> nit: may save 8 bytes per instance on 64 bits if this came before m_Exponent

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_render/fp...
File core/fpdfapi/fpdf_render/fpdf_render_pattern.cpp (right):

https://codereview.chromium.org/1828283002/diff/1/core/fpdfapi/fpdf_render/fp...
core/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:868:
buffer.Initialize(m_pContext, m_pDevice, &clip_rect, m_pCurObj, 150);
On 2016/03/24 17:43:38, dsinclair wrote:
> Can the rest of this be moved to a function? Would also be nice to know when
we
> can remove this, if we get to all skia, can this chunk be removed? Or if we
> remove AGG?

I don't know yet what if any of this code can be completely obsoleted. For
instance, because some shaders' functional descriptions are implemented directly
with PostScript, it's unlikely that all of this code will ever be replaced by
Skia native code.

The good news is that nothing here depends on or uses AGG, but it's premature to
restructure anticipating the portions that may be removed.

https://codereview.chromium.org/1828283002/diff/1/core/fxge/ge/fx_ge_device.cpp
File core/fxge/ge/fx_ge_device.cpp (right):

https://codereview.chromium.org/1828283002/diff/1/core/fxge/ge/fx_ge_device.c...
core/fxge/ge/fx_ge_device.cpp:232: if (!(m_RenderCaps & FXRC_GET_BITS)) {
On 2016/03/24 17:43:38, dsinclair wrote:
> similar, can this be moved to a function with a note of if it can be removed?

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
File core/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:162: const CPDF_ExpIntFunc* expIntFunc =
On 2016/03/24 18:07:08, Tom Sepez wrote:
> nit: want to assert on type before the cast?

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:183: const CPDF_StitchFunc* stitchFunc =
On 2016/03/24 18:07:08, Tom Sepez wrote:
> dittp

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:446: }
On 2016/03/24 17:43:38, dsinclair wrote:
> nit: No {}s

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:512: FX_FLOAT t_min = 0, t_max = 1.0f;
On 2016/03/24 17:43:38, dsinclair wrote:
> nit: one per line.

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:526: if (nFuncs != 1)
On 2016/03/24 17:43:38, dsinclair wrote:
> Can we do this earlier before all the other work?

Done.

https://codereview.chromium.org/1828283002/diff/1/core/fxge/skia/fx_skia_devi...
core/fxge/skia/fx_skia_device.cpp:533: case 2:  // exp interp function
On 2016/03/24 17:43:38, dsinclair wrote:
> Can you expand 'exp interp' as I don't know what those mean. Exponential
> Interpolation?
> 
> Or, better, can we add a enum FunctionTypes {} and name the numbers?

Done.

https://codereview.chromium.org/1828283002/diff/1/core/include/fxge/fx_ge.h
File core/include/fxge/fx_ge.h (right):

https://codereview.chromium.org/1828283002/diff/1/core/include/fxge/fx_ge.h#n...
core/include/fxge/fx_ge.h:593: virtual void* GetPlatformSurface() { return NULL;
}
On 2016/03/24 18:07:08, Tom Sepez wrote:
> nit: should any of these be const?

Done.

https://codereview.chromium.org/1828283002/diff/1/core/include/fxge/fx_ge.h#n...
core/include/fxge/fx_ge.h:594: 
On 2016/03/24 18:07:08, Tom Sepez wrote:
> nit: might as well kill these blank lines?

Done.

[Tom Sepez, 2016-03-25 21:03:00]

lgtm

[caryclark, 2016-03-25 21:07:59]

The CQ bit was checked by caryclark@google.com

[commit-bot: I haz the power, 2016-03-25 21:08:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828283002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828283002/20001

[commit-bot: I haz the power, 2016-03-25 21:08:33]

Description was changed from

==========
support gradients and stroke+fill

To draw paths that are stroked and filled with a pair of colors,
reduce the fill by the width of the stroke. This is implemented
with path ops subtracting the resolved stroke shape from the
fill shape. This permits rendering the result without requiring
an offscreen bitmap.

The implementation for stroke+fill requires a new entry into
the graphics engine, so a bit was added to device caps for that.

Extract the gradient information out of the axial gradient
function descriptions, and when possible, use Skia to draw
the linear gradient directly. This requires making the function
descriptions non-opaque, and adding a bit to device caps for
another entry into the graphics engine.

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

to

==========
support gradients and stroke+fill

To draw paths that are stroked and filled with a pair of colors,
reduce the fill by the width of the stroke. This is implemented
with path ops subtracting the resolved stroke shape from the
fill shape. This permits rendering the result without requiring
an offscreen bitmap.

The implementation for stroke+fill requires a new entry into
the graphics engine, so a bit was added to device caps for that.

Extract the gradient information out of the axial gradient
function descriptions, and when possible, use Skia to draw
the linear gradient directly. This requires making the function
descriptions non-opaque, and adding a bit to device caps for
another entry into the graphics engine.

BUG=

Committed:
https://pdfium.googlesource.com/pdfium/+/342f6fa66f6d843fe07d9b6a133656f83c8d...
==========

[commit-bot: I haz the power, 2016-03-25 21:08:34]

Committed patchset #2 (id:20001) as
https://pdfium.googlesource.com/pdfium/+/342f6fa66f6d843fe07d9b6a133656f83c8d...