Randomize seed for SkDiscretePathEffect::filterPath()

Randomize seed for SkDiscretePathEffect::filterPath()

Fix for https://code.google.com/p/skia/issues/detail?id=2581. Randomizes
the seed in SkDiscretePathEffect::filterPath(). Prior to this we were
using the path length as a seed.

Now, if we have two different paths with identical contents and we
apply an SkDiscretePathEffect to each, we obtain two different random
paths. Previously, we would obtain two overlapping paths (identical
path contents leading to the same seed).

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/39e58adb99111acbcc0b115e44812a3090bd6a2b

[rs.prinja, 2014-06-03 11:17:01]

The CQ bit was checked by rs.prinja@samsung.com

[rs.prinja, 2014-06-03 11:17:02]

The CQ bit was unchecked by rs.prinja@samsung.com

[scroggo, 2014-06-05 16:09:30]

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
File include/effects/SkDiscretePathEffect.h (right):

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:12: #include "SkRandom.h"
No need for this include here, since the header does not appear to reference
anything from SkRandom.

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:24: static SkDiscretePathEffect*
Create(SkScalar segLength, SkScalar deviation, uint32_t seedAssist=1) {
Please use a line break so the line is not over 100 chars. Skia's coding style
guidelines are here:
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:34: SkDiscretePathEffect(SkScalar
segLength, SkScalar deviation, uint32_t seedAssist=1);
In general, we only want a default parameter on the user-facing API. (We also
don't want these two to get out of sync.)

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:44: static uint32_t fObjectIdGenerator;
This is not needed. As discussed in skbug.com/2581, we want to only make it
random based on the seed and and path length.

(As a side note, though it's not mentioned in the style guide, we generally use
a different naming convention for static variables. This should be
gObjectIdGenerator. It also need not be in the class definition. See
SkNextPixelRefGenerationID() in
https://skia.googlesource.com/skia/+/1b918af0d86cc4bebdebd5fd1d76f63359009f01...)

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:45: uint32_t fObjectId;
Please remove.

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
File src/effects/SkDiscretePathEffect.cpp (right):

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:15: #include "SkTime.h"
Not used. Please remove.

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:26:
SkDiscretePathEffect::SkDiscretePathEffect(SkScalar segLength, SkScalar
deviation, uint32_t seedAssist)
Over 100 chars.

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:40: uint32_t seed = fObjectId +
SkScalarRoundToInt(meas.getLength());
As stated elsewhere, please just use the length and the seed.

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:43: seed += SkTime::GetMSecs() / 1000;
Please remove.

[rs.prinja, 2014-06-07 10:46:05]

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
File include/effects/SkDiscretePathEffect.h (right):

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:12: #include "SkRandom.h"
On 2014/06/05 16:09:30, scroggo wrote:
> No need for this include here, since the header does not appear to reference
> anything from SkRandom.

Done.

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:24: static SkDiscretePathEffect*
Create(SkScalar segLength, SkScalar deviation, uint32_t seedAssist=1) {
On 2014/06/05 16:09:30, scroggo wrote:
> Please use a line break so the line is not over 100 chars. Skia's coding style
> guidelines are here:
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...

Done.

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:34: SkDiscretePathEffect(SkScalar
segLength, SkScalar deviation, uint32_t seedAssist=1);
On 2014/06/05 16:09:30, scroggo wrote:
> In general, we only want a default parameter on the user-facing API. (We also
> don't want these two to get out of sync.)

Done.

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:44: static uint32_t fObjectIdGenerator;
On 2014/06/05 16:09:30, scroggo wrote:
> This is not needed. As discussed in skbug.com/2581, we want to only make it
> random based on the seed and and path length.
> 
> (As a side note, though it's not mentioned in the style guide, we generally
use
> a different naming convention for static variables. This should be
> gObjectIdGenerator. It also need not be in the class definition. See
> SkNextPixelRefGenerationID() in
>
https://skia.googlesource.com/skia/+/1b918af0d86cc4bebdebd5fd1d76f63359009f01...)

Done.

https://codereview.chromium.org/311803002/diff/40001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:45: uint32_t fObjectId;
On 2014/06/05 16:09:30, scroggo wrote:
> Please remove.

Done.

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
File src/effects/SkDiscretePathEffect.cpp (right):

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:15: #include "SkTime.h"
On 2014/06/05 16:09:30, scroggo wrote:
> Not used. Please remove.

Done.

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:26:
SkDiscretePathEffect::SkDiscretePathEffect(SkScalar segLength, SkScalar
deviation, uint32_t seedAssist)
On 2014/06/05 16:09:30, scroggo wrote:
> Over 100 chars.

Done.

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:40: uint32_t seed = fObjectId +
SkScalarRoundToInt(meas.getLength());
On 2014/06/05 16:09:30, scroggo wrote:
> As stated elsewhere, please just use the length and the seed.

Done.

https://codereview.chromium.org/311803002/diff/40001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:43: seed += SkTime::GetMSecs() / 1000;
On 2014/06/05 16:09:30, scroggo wrote:
> Please remove.

Done.

[scroggo, 2014-06-09 14:01:04]

Changes look good so far. A couple more comments:

https://codereview.chromium.org/311803002/diff/60001/include/effects/SkDiscre...
File include/effects/SkDiscretePathEffect.h (right):

https://codereview.chromium.org/311803002/diff/60001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:25: uint32_t seedAssist=1) {
Please add a comment explaining what seedAssist does. (It might be worth noting
that filtering the same path multiple times will result in the same segments,
but a different seedAssist value can be used to get a different set of segments
for the same path.)

https://codereview.chromium.org/311803002/diff/60001/src/effects/SkDiscretePa...
File src/effects/SkDiscretePathEffect.cpp (right):

https://codereview.chromium.org/311803002/diff/60001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:81: buffer.writeScalar(fPerterb);
We need to flatten and unflatten fSeedAssist:

buffer.writeUInt(fSeedAssist);

(Side note for Skia developers: we do *not* need to modify the picture version
or handle backwards compatibility because Chrome does not use
SkDiscretePathEffect.)

https://codereview.chromium.org/311803002/diff/60001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:86: fPerterb = buffer.readScalar();
fSeedAssist = buffer.readUInt();

[rs.prinja, 2014-06-11 04:15:16]

https://codereview.chromium.org/311803002/diff/60001/include/effects/SkDiscre...
File include/effects/SkDiscretePathEffect.h (right):

https://codereview.chromium.org/311803002/diff/60001/include/effects/SkDiscre...
include/effects/SkDiscretePathEffect.h:25: uint32_t seedAssist=1) {
On 2014/06/09 14:01:04, scroggo wrote:
> Please add a comment explaining what seedAssist does. (It might be worth
noting
> that filtering the same path multiple times will result in the same segments,
> but a different seedAssist value can be used to get a different set of
segments
> for the same path.)

Done.

https://codereview.chromium.org/311803002/diff/60001/src/effects/SkDiscretePa...
File src/effects/SkDiscretePathEffect.cpp (right):

https://codereview.chromium.org/311803002/diff/60001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:81: buffer.writeScalar(fPerterb);
On 2014/06/09 14:01:04, scroggo wrote:
> We need to flatten and unflatten fSeedAssist:
> 
> buffer.writeUInt(fSeedAssist);
> 
> (Side note for Skia developers: we do *not* need to modify the picture version
> or handle backwards compatibility because Chrome does not use
> SkDiscretePathEffect.)

Done.

https://codereview.chromium.org/311803002/diff/60001/src/effects/SkDiscretePa...
src/effects/SkDiscretePathEffect.cpp:86: fPerterb = buffer.readScalar();
On 2014/06/09 14:01:04, scroggo wrote:
> fSeedAssist = buffer.readUInt();

Done.

[scroggo, 2014-06-11 14:10:24]

On 2014/06/11 04:15:16, rs.prinja wrote:
>
https://codereview.chromium.org/311803002/diff/60001/include/effects/SkDiscre...
> File include/effects/SkDiscretePathEffect.h (right):
> 
>
https://codereview.chromium.org/311803002/diff/60001/include/effects/SkDiscre...
> include/effects/SkDiscretePathEffect.h:25: uint32_t seedAssist=1) {
> On 2014/06/09 14:01:04, scroggo wrote:
> > Please add a comment explaining what seedAssist does. (It might be worth
> noting
> > that filtering the same path multiple times will result in the same
segments,
> > but a different seedAssist value can be used to get a different set of
> segments
> > for the same path.)
> 
> Done.
> 
>
https://codereview.chromium.org/311803002/diff/60001/src/effects/SkDiscretePa...
> File src/effects/SkDiscretePathEffect.cpp (right):
> 
>
https://codereview.chromium.org/311803002/diff/60001/src/effects/SkDiscretePa...
> src/effects/SkDiscretePathEffect.cpp:81: buffer.writeScalar(fPerterb);
> On 2014/06/09 14:01:04, scroggo wrote:
> > We need to flatten and unflatten fSeedAssist:
> > 
> > buffer.writeUInt(fSeedAssist);
> > 
> > (Side note for Skia developers: we do *not* need to modify the picture
version
> > or handle backwards compatibility because Chrome does not use
> > SkDiscretePathEffect.)
> 
> Done.
> 
>
https://codereview.chromium.org/311803002/diff/60001/src/effects/SkDiscretePa...
> src/effects/SkDiscretePathEffect.cpp:86: fPerterb = buffer.readScalar();
> On 2014/06/09 14:01:04, scroggo wrote:
> > fSeedAssist = buffer.readUInt();
> 
> Done.

lgtm. You'll still need reed@'s approval for the API change before you can
submit.

[reed1, 2014-06-11 14:46:31]

Its a small matter, but I think

1. the default value should be 0
2. we can just XOR the seedassist with the "computed" seed
3. we don't need to document "how" we use their seed, just that "seedassist is
used to modify our rand seed..."

[rs.prinja, 2014-06-12 01:14:53]

On 2014/06/11 14:46:31, reed1 wrote:
> Its a small matter, but I think
> 
> 1. the default value should be 0
> 2. we can just XOR the seedassist with the "computed" seed
> 3. we don't need to document "how" we use their seed, just that "seedassist is
> used to modify our rand seed..."

done

[reed1, 2014-06-12 16:13:35]

lgtm

[rs.prinja, 2014-06-13 05:33:20]

The CQ bit was checked by rs.prinja@samsung.com

[commit-bot: I haz the power, 2014-06-13 05:33:37]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/rs.prinja@samsung.com/311803002/100001

[commit-bot: I haz the power, 2014-06-13 05:55:12]

Change committed as 39e58adb99111acbcc0b115e44812a3090bd6a2b