As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 803124 - Implement isPointInStroke
: Implement isPointInStroke
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla19
Assigned To: Tobias Schneider [:tobytailor]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 812039
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-18 08:21 PDT by Tobias Schneider [:tobytailor]
Modified: 2013-03-23 10:23 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
fixed
fixed


Attachments
Implement mozIsPointInStroke (39.23 KB, patch)
2012-10-18 08:21 PDT, Tobias Schneider [:tobytailor]
no flags Details | Diff | Splinter Review
Patch v2 (18.59 KB, patch)
2012-10-18 11:48 PDT, Tobias Schneider [:tobytailor]
no flags Details | Diff | Splinter Review
Patch v3 (20.80 KB, patch)
2012-10-18 12:02 PDT, Tobias Schneider [:tobytailor]
no flags Details | Diff | Splinter Review
Patch v4 (20.85 KB, patch)
2012-10-18 13:21 PDT, Tobias Schneider [:tobytailor]
no flags Details | Diff | Splinter Review
Patch v5 (20.99 KB, patch)
2012-10-19 00:54 PDT, Tobias Schneider [:tobytailor]
no flags Details | Diff | Splinter Review
Patch v6 (28.25 KB, patch)
2012-10-25 01:17 PDT, Tobias Schneider [:tobytailor]
jmuizelaar: review+
Details | Diff | Splinter Review
Patch v7 (29.76 KB, patch)
2012-10-29 08:59 PDT, Tobias Schneider [:tobytailor]
jmuizelaar: review+
Details | Diff | Splinter Review
Patch to remove prefix (24.69 KB, patch)
2012-11-22 05:40 PST, Tobias Schneider [:tobytailor]
jmuizelaar: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Tobias Schneider [:tobytailor] 2012-10-18 08:21:17 PDT
Created attachment 672798 [details] [diff] [review]
Implement mozIsPointInStroke

In addition to isPointInPath, the Canvas API should also provide isPointInStroke for hit testing. Right now there is no efficient way to test if a point lies inside the filled AND stroked region of a path. (except of checking for nontransparend pixels using getImageData, which is awful slow if 2D rendering is hw accelerated).
Comment 1 User image Tobias Schneider [:tobytailor] 2012-10-18 11:48:29 PDT
Created attachment 672886 [details] [diff] [review]
Patch v2
Comment 2 User image Tobias Schneider [:tobytailor] 2012-10-18 12:02:59 PDT
Created attachment 672894 [details] [diff] [review]
Patch v3
Comment 3 User image Tobias Schneider [:tobytailor] 2012-10-18 13:21:02 PDT
Created attachment 672928 [details] [diff] [review]
Patch v4
Comment 4 User image Tobias Schneider [:tobytailor] 2012-10-19 00:54:13 PDT
Created attachment 673128 [details] [diff] [review]
Patch v5
Comment 5 User image Jeff Muizelaar [:jrmuizel] 2012-10-19 10:27:26 PDT
Comment on attachment 673128 [details] [diff] [review]
Patch v5

Review of attachment 673128 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/test_mozIsPointInStroke.html
@@ +33,5 @@
> +
> +ok(ctx.mozIsPointInStroke(20, 20) === false);
> +ok(ctx.mozIsPointInStroke(80, 20) === false);
> +ok(ctx.mozIsPointInStroke(80, 30) === false);
> +ok(ctx.mozIsPointInStroke(20, 30) === false);

This test should include curves and transformed paths/transformed userspace. It might also be worth testing different cap modes.
Comment 6 User image Tobias Schneider [:tobytailor] 2012-10-25 01:17:33 PDT
Created attachment 675015 [details] [diff] [review]
Patch v6
Comment 7 User image Jeff Muizelaar [:jrmuizel] 2012-10-25 11:00:44 PDT
Comment on attachment 675015 [details] [diff] [review]
Patch v6

Review of attachment 675015 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/test_mozIsPointInStroke.html
@@ +191,5 @@
> +ok(ctx.mozIsPointInStroke(14, 15) === true, 'ctx.mozIsPointInStroke(14, 15) === true');
> +ok(ctx.mozIsPointInStroke(81, 15) === true, 'ctx.mozIsPointInStroke(81, 15) === true');
> +ok(ctx.mozIsPointInStroke(79, 41) === true, 'ctx.mozIsPointInStroke(79, 41) === true');
> +ok(ctx.mozIsPointInStroke(73, 21) === true, 'ctx.mozIsPointInStroke(73, 21) === true');
> +

How about a final test that does a curve scaled by 3x?

Also you should test changing the scale during path construction.

::: gfx/2d/PathSkia.cpp
@@ +184,5 @@
> +                    int32_t(SkFloatToScalar(transformed.y + 1)));
> +
> +  SkRegion pathRegion;
> +
> +  return pathRegion.setPath(strokePath, pointRect);

This should use SkPathContainsPoint if possible.
Comment 8 User image Tobias Schneider [:tobytailor] 2012-10-25 12:44:40 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> 
> How about a final test that does a curve scaled by 3x?
>
> Also you should test changing the scale during path construction.

Sounds reasonable, will add those tests.

> > +  return pathRegion.setPath(strokePath, pointRect);
> 
> This should use SkPathContainsPoint if possible.

Don't think SkPathContainsPoint is a standard Skia API, is it? Seems to be an internal helper defined and used in Chromium (http://code.google.com/p/chromium/source/search?q=SkPathContainsPoint&origq=SkPathContainsPoint&btnG=Search+Trunk) and itself uses setPath().
Comment 9 User image Jeff Muizelaar [:jrmuizel] 2012-10-25 13:08:57 PDT
(In reply to Tobias Schneider from comment #8)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
>
> Don't think SkPathContainsPoint is a standard Skia API, is it? Seems to be
> an internal helper defined and used in Chromium
> (http://code.google.com/p/chromium/source/
> search?q=SkPathContainsPoint&origq=SkPathContainsPoint&btnG=Search+Trunk)
> and itself uses setPath().

True. I thought they had fixed this in Skia.
Comment 10 User image Jeff Muizelaar [:jrmuizel] 2012-10-25 13:45:04 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Tobias Schneider from comment #8)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> >
> > Don't think SkPathContainsPoint is a standard Skia API, is it? Seems to be
> > an internal helper defined and used in Chromium
> > (http://code.google.com/p/chromium/source/
> > search?q=SkPathContainsPoint&origq=SkPathContainsPoint&btnG=Search+Trunk)
> > and itself uses setPath().
> 
> True. I thought they had fixed this in Skia.

Actually, the function is SkPath::contains(x,y)
Comment 11 User image Tobias Schneider [:tobytailor] 2012-10-29 08:59:04 PDT
Created attachment 676169 [details] [diff] [review]
Patch v7
Comment 12 User image Ryan VanderMeulen [:RyanVM] 2012-10-31 17:46:12 PDT
Pushed to Try since I don't see any results posted here.
https://tbpl.mozilla.org/?tree=Try&rev=079dc1ebac54
Comment 13 User image Ryan VanderMeulen [:RyanVM] 2012-11-01 06:20:04 PDT
Unfortunately, this causes hangs in android armv6 builds.
https://tbpl.mozilla.org/?tree=Try&rev=7f236ae2ee94
Comment 14 User image Jeff Muizelaar [:jrmuizel] 2012-11-01 06:38:05 PDT
I wonder if it still hangs on armv6 without the test changes. I find it pretty unlikely that this would cause all the mochitests to fail...
Comment 15 User image Tobias Schneider [:tobytailor] 2012-11-02 08:23:08 PDT
Didn't happen in https://tbpl.mozilla.org/?tree=Try&rev=8cab7ad6c0bf.
Comment 16 User image Ryan VanderMeulen [:RyanVM] 2012-11-06 15:26:14 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/69c7ee7683e0
Comment 17 User image Ed Morley [:emorley] 2012-11-07 05:00:21 PST
https://hg.mozilla.org/mozilla-central/rev/69c7ee7683e0
Comment 18 User image :Ms2ger (⌚ UTC+1/+2) 2012-11-07 07:44:15 PST
Was this ever even suggested on a standards list?
Comment 19 User image Jeff Muizelaar [:jrmuizel] 2012-11-07 07:56:48 PST
(In reply to :Ms2ger from comment #18)
> Was this ever even suggested on a standards list?

I don't believe it was. Would you be willing to do so?
Comment 20 User image :Ms2ger (⌚ UTC+1/+2) 2012-11-07 07:59:18 PST
I have no idea what this API is good for; it's probably better to let someone who does suggest it.
Comment 22 User image Hixie (not reading bugmail) 2012-11-21 16:36:24 PST
There's no need for this to be prefixed, by the way. It'll either be added, in which case I'll ensure it's compatible and so it won't conflict, or it won't, in which case nothing with this name will be added and it still won't conflict.

The idea seems pretty sound, the main thing that would stop me from adding it to the spec right now is lack of interest from other vendors. I'll ask around, but if you can convince another vendor to indicate an interest that would certainly help.
Comment 23 User image Henri Sivonen (:hsivonen) 2012-11-22 04:11:45 PST
Can this be unprefixed, please? As Hixie says, the prefix is useless and unprefixing later will cause breakage later.
Comment 24 User image Tobias Schneider [:tobytailor] 2012-11-22 05:40:36 PST
Created attachment 684393 [details] [diff] [review]
Patch to remove prefix
Comment 25 User image Ryan VanderMeulen [:RyanVM] 2012-12-16 16:41:44 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed5cbd0b622
Comment 26 User image Ed Morley [:emorley] 2012-12-17 05:54:58 PST
https://hg.mozilla.org/mozilla-central/rev/eed5cbd0b622
Comment 27 User image Masatoshi Kimura [:emk] 2012-12-20 22:46:29 PST
Comment on attachment 684393 [details] [diff] [review]
Patch to remove prefix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: proliferating more -moz prefixes
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): maybe low, simple renaming
String or UUID changes made by this patch: none
Comment 28 User image bhavana bajaj [:bajaj] 2012-12-31 11:18:24 PST
Comment on attachment 684393 [details] [diff] [review]
Patch to remove prefix

Approving based on the m-c testing.
Comment 29 User image Masatoshi Kimura [:emk] 2012-12-31 23:41:32 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/a55bc8cb8a8e

Note You need to log in before you can comment on or make changes to this bug.