[PDF] Add support for shaders.

[PDF] Add support for Shaders.

- Shaders, or as they are referred to in PDF, patterns, are drawn in the coordinate system of the initial page, so when we canonicalize them, we have to consider the current transform and where they are constructed.

- Image shaders are tiled by default, this makes repeat and mirror modes easy, but means we have to draw a pattern as large as the current clip to support clamp mode.

- Gradient shaders are implemented with type 4 functions, which are basically small snippets of post script code.  I've tried to make the code generation modular and heavily commented to make it easy to understand or expand.

Committed: http://code.google.com/p/skia/source/detail?r=905

[reed1, 2011-03-07 16:53:45]

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h#newcod...
include/pdf/SkPDFDevice.h:180: void updateGSFromPaint(SkPaint newPaint, bool
forText);
Why are we forcing the copy of newPaint here? Shouldn't that be a hidden impl
detail, and keep the const paint& signature?

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h
File include/pdf/SkPDFShader.h (right):

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode29
include/pdf/SkPDFShader.h:29: /** \class SkPDFShader
Its OK with me if we want to call this SkPDFPattern -- if that is closer to what
it is (from a PDF perspective). Your call.

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode57
include/pdf/SkPDFShader.h:57: static SkPDFShader* getPDFShader(SkShader* shader,
const SkMatrix& matrix,
Can shader be const here?

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h#newcode29
include/pdf/SkPDFStream.h:29: A stream object in a PDF.  Note, all streams must
be indirect objects.
What does indirect mean here?

http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode31
src/pdf/SkPDFShader.cpp:31: void transformBBox(const SkMatrix& matrix, SkRect*
bbox) {
Can these be static? If so, do they also need to be inside namespace?

http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode136
src/pdf/SkPDFShader.cpp:136: const SkScalar div = SkIntToScalar(255);
Would be faster to invert div once, and then use SkScalarMul in the loop. Have
you tried this to see if it is correct enough?

http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode376
src/pdf/SkPDFShader.cpp:376: range->append(new SkPDFInt(1))->unref();
I wonder if a helper appendInt(int) would be useful. We seem to do the above
pattern quite a lot.

[Steve VanDeBogart, 2011-03-07 19:11:43]

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h#newcod...
include/pdf/SkPDFDevice.h:180: void updateGSFromPaint(SkPaint newPaint, bool
forText);
On 2011/03/07 16:53:46, reed1 wrote:
> Why are we forcing the copy of newPaint here? Shouldn't that be a hidden impl
> detail, and keep the const paint& signature?

It is an implementation detail, but as far as I know, the caller doesn't care if
it's a const ref or a copy instance.   I'm happy to move the copy inside the
function if you prefer, but I don't see the difference - maybe C++ ignorance on
my part.

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h
File include/pdf/SkPDFShader.h (right):

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode29
include/pdf/SkPDFShader.h:29: /** \class SkPDFShader
On 2011/03/07 16:53:46, reed1 wrote:
> Its OK with me if we want to call this SkPDFPattern -- if that is closer to
what
> it is (from a PDF perspective). Your call.

I think shader is a better name.  Someone familiar with PDF will be able to find
this code based on the PDF output it generates, so I think shader is fine.

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode57
include/pdf/SkPDFShader.h:57: static SkPDFShader* getPDFShader(SkShader* shader,
const SkMatrix& matrix,
On 2011/03/07 16:53:46, reed1 wrote:
> Can shader be const here?

I did try that, but asABitmap isn't const.

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h#newcode29
include/pdf/SkPDFStream.h:29: A stream object in a PDF.  Note, all streams must
be indirect objects.
On 2011/03/07 16:53:46, reed1 wrote:
> What does indirect mean here?

Meaning it can not be used directly in another pdf object like an array or
dictionary.  An SkPDFObjRef has to be used to include a stream in another kind
of object.

http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode31
src/pdf/SkPDFShader.cpp:31: void transformBBox(const SkMatrix& matrix, SkRect*
bbox) {
On 2011/03/07 16:53:46, reed1 wrote:
> Can these be static? If so, do they also need to be inside namespace?

Sorry, Chrome style creeping in, changed.

http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode136
src/pdf/SkPDFShader.cpp:136: const SkScalar div = SkIntToScalar(255);
On 2011/03/07 16:53:46, reed1 wrote:
> Would be faster to invert div once, and then use SkScalarMul in the loop. Have
> you tried this to see if it is correct enough?

Done.

http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode376
src/pdf/SkPDFShader.cpp:376: range->append(new SkPDFInt(1))->unref();
On 2011/03/07 16:53:46, reed1 wrote:
> I wonder if a helper appendInt(int) would be useful. We seem to do the above
> pattern quite a lot.

Indeed.  After working with the primitives a bit I've noticed a few places where
convenience wrappers could make things easier.  The one I noticed most recently
was SkPDFDict::insertObjectReference().  I will add appendInt (and maybe
appendScalar) to my list to look at.

[reed1, 2011-03-07 19:22:52]

On 2011/03/07 19:11:43, Steve VanDeBogart wrote:
> http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h
> File include/pdf/SkPDFDevice.h (right):
> 
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h#newcod...
> include/pdf/SkPDFDevice.h:180: void updateGSFromPaint(SkPaint newPaint, bool
> forText);
> On 2011/03/07 16:53:46, reed1 wrote:
> > Why are we forcing the copy of newPaint here? Shouldn't that be a hidden
impl
> > detail, and keep the const paint& signature?
> 
> It is an implementation detail, but as far as I know, the caller doesn't care
if
> it's a const ref or a copy instance.   I'm happy to move the copy inside the
> function if you prefer, but I don't see the difference - maybe C++ ignorance
on
> my part.

The difference is that (a) now the copies happen at all the call-sites, adding a
little code-bloat, and (b) it we *must* pay for the copy, since its part of the
signature. If in some later impl you don't need to change your local paint, then
you can just not make a copy internally (but only if you make the signature be
const &.)

> 
> http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h
> File include/pdf/SkPDFShader.h (right):
> 
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode29
> include/pdf/SkPDFShader.h:29: /** \class SkPDFShader
> On 2011/03/07 16:53:46, reed1 wrote:
> > Its OK with me if we want to call this SkPDFPattern -- if that is closer to
> what
> > it is (from a PDF perspective). Your call.
> 
> I think shader is a better name.  Someone familiar with PDF will be able to
find
> this code based on the PDF output it generates, so I think shader is fine.
> 
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode57
> include/pdf/SkPDFShader.h:57: static SkPDFShader* getPDFShader(SkShader*
shader,
> const SkMatrix& matrix,
> On 2011/03/07 16:53:46, reed1 wrote:
> > Can shader be const here?
> 
> I did try that, but asABitmap isn't const.
> 
> http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h
> File include/pdf/SkPDFStream.h (right):
> 
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h#newcode29
> include/pdf/SkPDFStream.h:29: A stream object in a PDF.  Note, all streams
must
> be indirect objects.
> On 2011/03/07 16:53:46, reed1 wrote:
> > What does indirect mean here?
> 
> Meaning it can not be used directly in another pdf object like an array or
> dictionary.  An SkPDFObjRef has to be used to include a stream in another kind
> of object.

OK. Should we expand the comment to expound on that, or will that be immediately
clear to other readers?

> 
> http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp
> File src/pdf/SkPDFShader.cpp (right):
> 
> http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode31
> src/pdf/SkPDFShader.cpp:31: void transformBBox(const SkMatrix& matrix, SkRect*
> bbox) {
> On 2011/03/07 16:53:46, reed1 wrote:
> > Can these be static? If so, do they also need to be inside namespace?
> 
> Sorry, Chrome style creeping in, changed.
> 
>
http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode136
> src/pdf/SkPDFShader.cpp:136: const SkScalar div = SkIntToScalar(255);
> On 2011/03/07 16:53:46, reed1 wrote:
> > Would be faster to invert div once, and then use SkScalarMul in the loop.
Have
> > you tried this to see if it is correct enough?
> 
> Done.
> 
>
http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode376
> src/pdf/SkPDFShader.cpp:376: range->append(new SkPDFInt(1))->unref();
> On 2011/03/07 16:53:46, reed1 wrote:
> > I wonder if a helper appendInt(int) would be useful. We seem to do the above
> > pattern quite a lot.
> 
> Indeed.  After working with the primitives a bit I've noticed a few places
where
> convenience wrappers could make things easier.  The one I noticed most
recently
> was SkPDFDict::insertObjectReference().  I will add appendInt (and maybe
> appendScalar) to my list to look at.

[reed1, 2011-03-07 19:29:37]

On 2011/03/07 19:22:52, reed1 wrote:
> On 2011/03/07 19:11:43, Steve VanDeBogart wrote:
> > http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h
> > File include/pdf/SkPDFDevice.h (right):
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h#newcod...
> > include/pdf/SkPDFDevice.h:180: void updateGSFromPaint(SkPaint newPaint, bool
> > forText);
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Why are we forcing the copy of newPaint here? Shouldn't that be a hidden
> impl
> > > detail, and keep the const paint& signature?
> > 
> > It is an implementation detail, but as far as I know, the caller doesn't
care
> if
> > it's a const ref or a copy instance.   I'm happy to move the copy inside the
> > function if you prefer, but I don't see the difference - maybe C++ ignorance
> on
> > my part.
> 
> The difference is that (a) now the copies happen at all the call-sites, adding
a
> little code-bloat, and (b) it we *must* pay for the copy, since its part of
the
> signature. If in some later impl you don't need to change your local paint,
then
> you can just not make a copy internally (but only if you make the signature be
> const &.)
> 
> > 
> > http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h
> > File include/pdf/SkPDFShader.h (right):
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode29
> > include/pdf/SkPDFShader.h:29: /** \class SkPDFShader
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Its OK with me if we want to call this SkPDFPattern -- if that is closer
to
> > what
> > > it is (from a PDF perspective). Your call.
> > 
> > I think shader is a better name.  Someone familiar with PDF will be able to
> find
> > this code based on the PDF output it generates, so I think shader is fine.
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode57
> > include/pdf/SkPDFShader.h:57: static SkPDFShader* getPDFShader(SkShader*
> shader,
> > const SkMatrix& matrix,
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Can shader be const here?
> > 
> > I did try that, but asABitmap isn't const.

It is now (as of rev. 898)

> > 
> > http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h
> > File include/pdf/SkPDFStream.h (right):
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h#newcode29
> > include/pdf/SkPDFStream.h:29: A stream object in a PDF.  Note, all streams
> must
> > be indirect objects.
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > What does indirect mean here?
> > 
> > Meaning it can not be used directly in another pdf object like an array or
> > dictionary.  An SkPDFObjRef has to be used to include a stream in another
kind
> > of object.
> 
> OK. Should we expand the comment to expound on that, or will that be
immediately
> clear to other readers?
> 
> > 
> > http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp
> > File src/pdf/SkPDFShader.cpp (right):
> > 
> >
http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode31
> > src/pdf/SkPDFShader.cpp:31: void transformBBox(const SkMatrix& matrix,
SkRect*
> > bbox) {
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Can these be static? If so, do they also need to be inside namespace?
> > 
> > Sorry, Chrome style creeping in, changed.
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode136
> > src/pdf/SkPDFShader.cpp:136: const SkScalar div = SkIntToScalar(255);
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Would be faster to invert div once, and then use SkScalarMul in the loop.
> Have
> > > you tried this to see if it is correct enough?
> > 
> > Done.
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode376
> > src/pdf/SkPDFShader.cpp:376: range->append(new SkPDFInt(1))->unref();
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > I wonder if a helper appendInt(int) would be useful. We seem to do the
above
> > > pattern quite a lot.
> > 
> > Indeed.  After working with the primitives a bit I've noticed a few places
> where
> > convenience wrappers could make things easier.  The one I noticed most
> recently
> > was SkPDFDict::insertObjectReference().  I will add appendInt (and maybe
> > appendScalar) to my list to look at.

[Steve VanDeBogart, 2011-03-07 22:20:47]

On 2011/03/07 19:22:52, reed1 wrote:
> On 2011/03/07 19:11:43, Steve VanDeBogart wrote:
> > http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h
> > File include/pdf/SkPDFDevice.h (right):
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFDevice.h#newcod...
> > include/pdf/SkPDFDevice.h:180: void updateGSFromPaint(SkPaint newPaint, bool
> > forText);
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Why are we forcing the copy of newPaint here? Shouldn't that be a hidden
> impl
> > > detail, and keep the const paint& signature?
> > 
> > It is an implementation detail, but as far as I know, the caller doesn't
care
> if
> > it's a const ref or a copy instance.   I'm happy to move the copy inside the
> > function if you prefer, but I don't see the difference - maybe C++ ignorance
> on
> > my part.
> 
> The difference is that (a) now the copies happen at all the call-sites, adding
a
> little code-bloat, and (b) it we *must* pay for the copy, since its part of
the
> signature. If in some later impl you don't need to change your local paint,
then
> you can just not make a copy internally (but only if you make the signature be
> const &.)

Thanks for the enlightenment.  Fixed.
> 
> > 
> > http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h
> > File include/pdf/SkPDFShader.h (right):
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode29
> > include/pdf/SkPDFShader.h:29: /** \class SkPDFShader
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Its OK with me if we want to call this SkPDFPattern -- if that is closer
to
> > what
> > > it is (from a PDF perspective). Your call.
> > 
> > I think shader is a better name.  Someone familiar with PDF will be able to
> find
> > this code based on the PDF output it generates, so I think shader is fine.
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFShader.h#newcode57
> > include/pdf/SkPDFShader.h:57: static SkPDFShader* getPDFShader(SkShader*
> shader,
> > const SkMatrix& matrix,
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Can shader be const here?
> > 
> > I did try that, but asABitmap isn't const.
>
> It is now (as of rev. 898)

Done.

> > 
> > http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h
> > File include/pdf/SkPDFStream.h (right):
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/include/pdf/SkPDFStream.h#newcode29
> > include/pdf/SkPDFStream.h:29: A stream object in a PDF.  Note, all streams
> must
> > be indirect objects.
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > What does indirect mean here?
> > 
> > Meaning it can not be used directly in another pdf object like an array or
> > dictionary.  An SkPDFObjRef has to be used to include a stream in another
kind
> > of object.
> 
> OK. Should we expand the comment to expound on that, or will that be
immediately
> clear to other readers?

Probably will be clear, but doesn't hurt to be more descriptive.  I clarified a
bit.


> > http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp
> > File src/pdf/SkPDFShader.cpp (right):
> > 
> >
http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode31
> > src/pdf/SkPDFShader.cpp:31: void transformBBox(const SkMatrix& matrix,
SkRect*
> > bbox) {
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Can these be static? If so, do they also need to be inside namespace?
> > 
> > Sorry, Chrome style creeping in, changed.
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode136
> > src/pdf/SkPDFShader.cpp:136: const SkScalar div = SkIntToScalar(255);
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > Would be faster to invert div once, and then use SkScalarMul in the loop.
> Have
> > > you tried this to see if it is correct enough?
> > 
> > Done.
> > 
> >
>
http://codereview.appspot.com/4239061/diff/1/src/pdf/SkPDFShader.cpp#newcode376
> > src/pdf/SkPDFShader.cpp:376: range->append(new SkPDFInt(1))->unref();
> > On 2011/03/07 16:53:46, reed1 wrote:
> > > I wonder if a helper appendInt(int) would be useful. We seem to do the
above
> > > pattern quite a lot.
> > 
> > Indeed.  After working with the primitives a bit I've noticed a few places
> where
> > convenience wrappers could make things easier.  The one I noticed most
> recently
> > was SkPDFDict::insertObjectReference().  I will add appendInt (and maybe
> > appendScalar) to my list to look at.

[reed1, 2011-03-08 12:54:52]

LGTM

[twiz1, 2011-03-09 20:23:05]

I believe that this change broke the skia sample app on Windows.

Fix incoming.

http://codereview.appspot.com/4239061/diff/10001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

http://codereview.appspot.com/4239061/diff/10001/src/pdf/SkPDFShader.cpp#newc...
src/pdf/SkPDFShader.cpp:132: SkScalar
colorData[info.fColorCount][kColorComponents];
The use of dynamic stack-arrays here has broken the SampleApp  on windows.  On
which platform was this code compiled?  GCC?  

I'll quickly submit a CL for review that replaces these with calls to alloca.

[Steve VanDeBogart, 2011-03-09 21:41:33]

http://codereview.appspot.com/4239061/diff/10001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

http://codereview.appspot.com/4239061/diff/10001/src/pdf/SkPDFShader.cpp#newc...
src/pdf/SkPDFShader.cpp:132: SkScalar
colorData[info.fColorCount][kColorComponents];
On 2011/03/09 20:23:05, twiz1 wrote:
> The use of dynamic stack-arrays here has broken the SampleApp  on windows.  On
> which platform was this code compiled?  GCC?  
> 
> I'll quickly submit a CL for review that replaces these with calls to alloca.

Sorry about that, I thought I had removed those.  Looks like  there are two of
these.  The one in interpolateColorCode can be changed to a static value - that
generality is never used.  Let me know if you want me to take care of this.

[twiz1, 2011-03-09 21:45:07]

On 2011/03/09 21:41:33, Steve VanDeBogart wrote:
> http://codereview.appspot.com/4239061/diff/10001/src/pdf/SkPDFShader.cpp
> File src/pdf/SkPDFShader.cpp (right):
> 
>
http://codereview.appspot.com/4239061/diff/10001/src/pdf/SkPDFShader.cpp#newc...
> src/pdf/SkPDFShader.cpp:132: SkScalar
> colorData[info.fColorCount][kColorComponents];
> On 2011/03/09 20:23:05, twiz1 wrote:
> > The use of dynamic stack-arrays here has broken the SampleApp  on windows. 
On
> > which platform was this code compiled?  GCC?  
> > 
> > I'll quickly submit a CL for review that replaces these with calls to
alloca.
> 
> Sorry about that, I thought I had removed those.  Looks like  there are two of
> these.  The one in interpolateColorCode can be changed to a static value -
that
> generality is never used.  Let me know if you want me to take care of this.

Thanks for the offer, Steve, but I beat you to it. = )

Please see CL: http://codereview.appspot.com/4270041/