idea: add annotation to SkPaint

idea: add annotation to SkPaint
Committed: https://code.google.com/p/skia/source/detail?r=4555

[reed1, 2012-06-29 13:37:25]

Not sure at all that SkMetaData is the right level of features for storing
annotation content, but its a starting point...

- just a string enough?
- just a string,string dictionary enough?

other comments?

This is a brain-storm-alternative to the addRectURL CL of yore. Just want to
consider alternatives, for the current ask (rect-url) and for the future of skia
greatness.

[bungeman, 2012-06-29 14:33:32]

SkMetaData seems like a nice way to pass named parameters.

https://codereview.appspot.com/6355050/diff/1/include/core/SkAnnotation.h
File include/core/SkAnnotation.h (right):

https://codereview.appspot.com/6355050/diff/1/include/core/SkAnnotation.h#new...
include/core/SkAnnotation.h:30: uint32_t    fFlags;
In addition to flags and metadata, add a string or guid 'type'? This could
alternatively be in the metadata by convention, but that means looking into the
metadata for the device to do quick reject.

[Steve VanDeBogart, 2012-06-29 17:09:20]

https://codereview.appspot.com/6355050/diff/1/include/core/SkAnnotation.h
File include/core/SkAnnotation.h (right):

https://codereview.appspot.com/6355050/diff/1/include/core/SkAnnotation.h#new...
include/core/SkAnnotation.h:17: kDontDraw_Flag  = 1 << 0,
This means don't draw the rectangle, right?  Should there be a comment
explaining that?

https://codereview.appspot.com/6355050/diff/1/include/core/SkAnnotation.h#new...
include/core/SkAnnotation.h:23: uint32_t    getFlags() const { return fFlags; }
If we have an SkMetaData it's not clear we also need a flags mechanism.

https://codereview.appspot.com/6355050/diff/1/include/core/SkAnnotation.h#new...
include/core/SkAnnotation.h:26: const SkMetaData& metaData() const { return
fMetaData; }
I don't know about XPS, but all annotations require the annotation type and text
to go with it.  Some require more.

How about including an enum type, an SkString/SkData and an SkMetaData* for
extra settings?

https://codereview.appspot.com/6355050/diff/1/include/core/SkPaint.h
File include/core/SkPaint.h (right):

https://codereview.appspot.com/6355050/diff/1/include/core/SkPaint.h#newcode586
include/core/SkPaint.h:586: SkAnnotation* getAnnotation() const { return
fAnnotation; }
Right now we only intend to use this with drawRect, maybe we should note that
here as well as the expectation of not drawing anything?

[reed1, 2012-06-29 18:58:28]

Yes, the point of the flag was to tell skia to not draw the primitive.

Do we (the 4 of us) think metadata is overkill? Would type(string) + data be
sufficient? It certainly would account for URLs.

What is the "right" namespace for the "type"?
- simple-enum
- GUID
- string

[bsalomon, 2012-06-29 19:37:09]

On 2012/06/29 18:58:28, reed1 wrote:
> Yes, the point of the flag was to tell skia to not draw the primitive.
> 
> Do we (the 4 of us) think metadata is overkill? Would type(string) + data be
> sufficient? It certainly would account for URLs.
> 
> What is the "right" namespace for the "type"?
> - simple-enum
> - GUID
> - string

Maybe we should have a constrained metadata/dictionary that only has interfaces
for pre-flattened things (strings, basic c types, guids, serialized pictures,
etc but not ptrs) with a standard "type" key that identifies the type of
annotation.

[Steve VanDeBogart, 2012-06-29 20:13:14]

On 2012/06/29 18:58:28, reed1 wrote:
> Yes, the point of the flag was to tell skia to not draw the primitive.
> 
> Do we (the 4 of us) think metadata is overkill? Would type(string) + data be
> sufficient? It certainly would account for URLs.

I think that metadata might be overkilll and type + data should be sufficient. 
We can change it in the future if need be, but I doubt that will come to pass.

> What is the "right" namespace for the "type"?
> - simple-enum
> - GUID
> - string

I think enum is sufficient for type - there are a limited number of annotation
types and we probably only want to support a subset of them, so explicitly
adding the ones we know how to deal with is probably best unless XPS is
different in this regard.

[reed1, 2012-06-29 20:42:59]

patch #3 experiment:

- add type (string)
- restrict metadata to just SkData (nice for sharing blobs) for now
- assumes we can add more types later
    - prehaps set() -> setData() in that case

[Steve VanDeBogart, 2012-06-29 20:51:27]

I would propose the following, but patch set 3 is fine by me as well.

class SkAnnotation : public SkFlattenable {
public:
    enum Flags {
        // If set, the associated drawing primitive should not be drawn
        kDontDraw_Flag  = 1 << 0,
    };

    enum Type {
        kURLLink_Type = 1 << 0,
    };

    SkAnnotation(Type type, SkData* data, uint32_t flags = kDontDraw_Flag);
    virtual ~SkAnnotation();

    uint32_t flags() const { return fFlags; }
    Type type() const { return fType; }
    SkData* data() const { return fData; }

    void writeToStream(SkWStream*) const;
    size_t readFromStream(SkStream*);

private:
    uint32_t    fFlags;
    Type        fType;
    SkData*     fData;

    typedef SkFlattenable INHERITED;
};

[reed1, 2012-06-29 20:59:34]

I think I understand. restrict it to a single data blob for now, and use an enum
for the "type".

Your enum syntax for Type implies that its a bitfield, and not a straight enum.
Was that your intention?

[Steve VanDeBogart, 2012-06-29 21:01:53]

On 2012/06/29 20:59:34, reed1 wrote:
> I think I understand. restrict it to a single data blob for now, and use an
enum
> for the "type".
> 
> Your enum syntax for Type implies that its a bitfield, and not a straight
enum.
> Was that your intention?

No, just copy paste.

[reed1, 2012-06-29 21:11:29]

Since we will undoubtedly change our mind going forward, I also propose we add a
Skia function (for now) that Chrome/WebKit should call (for now).

SkAddRectURL(SkCanvas*, const SkRect&, const char*);
or
SkAddRectURL(SkCanvas*, const SkRect&, SkData*);

That way we don't have to coordinate between the two/three code trees as we want
to make changes to the specifics of our annotation process.

[Steve VanDeBogart, 2012-06-29 21:23:16]

On 2012/06/29 21:11:29, reed1 wrote:
> Since we will undoubtedly change our mind going forward, I also propose we add
a
> Skia function (for now) that Chrome/WebKit should call (for now).
> 
> SkAddRectURL(SkCanvas*, const SkRect&, const char*);
> or
> SkAddRectURL(SkCanvas*, const SkRect&, SkData*);
> 
> That way we don't have to coordinate between the two/three code trees as we
want
> to make changes to the specifics of our annotation process.

Seems good. You could even put it on Canvas.

[reed1, 2012-07-02 20:32:02]

patch #4

- add unit-test, to help document dictionary behavior
- hide methods we may not need (title, stream)
- implement flattening

Do we need a title/type, given that we think multiple chunks for the payload is
useful? I guess we need some "documented" examples that want more than one
payload.

[Steve VanDeBogart, 2012-07-09 20:03:15]

On 2012/07/02 20:32:02, reed1 wrote:
> patch #4
> 
> - add unit-test, to help document dictionary behavior
> - hide methods we may not need (title, stream)
> - implement flattening
> 
> Do we need a title/type, given that we think multiple chunks for the payload
is
> useful? I guess we need some "documented" examples that want more than one
> payload.

I haven't heard anything about XPS, so maybe I'm missing part of the story, but
this solution seems overly complicated especially given the use cases that we
are currently aware of.

If you would prefer a bag of key value pairs, then it probably makes sense to
remove the specific fields (fType, which is dead code right now anyway) in favor
of well known keys (SkAnnotation::kTypeKey).
But I prefer a simpler interface like the one in comment #8.

[reed1, 2012-07-10 20:42:14]

patch #7

SkAnnotationRectWithURL(...)

    Helper function to allow this to be used by Chrome, but hiding the
implementation for now

Experimenting with skipping draws with no-draw annotations (raster and gpu at
the moment).

[reed1, 2012-07-11 19:27:39]

patch #8 Summary (ready for review/checkin)

1. Only the helper function SkAnnotateRectWithURL() should be used publicly for
now
2. Annotations are immutable, which allows SkPaint to cache if it is in a
no-draw state, which can be queried via an inline test.
3. Devices have to opt-out for now if they shouldn't draw the annotation. Raster
and GPU are doing that for rects/paths, which should be enough for now.

Next:

4. PDF and XPS should respect the URL annotation, and opt-out for all
no-draw-annotations

Later optimization idea:

5. Have canvas know that its Device never wants any no-draw-annotations, and
have it opt-out before it calls the device.

[bungeman, 2012-07-11 19:35:11]

LGTM, even if it does punt on the type.

On 2012/07/11 19:27:39, reed1 wrote:
> patch #8 Summary (ready for review/checkin)
> 
> 1. Only the helper function SkAnnotateRectWithURL() should be used publicly
for
> now
> 2. Annotations are immutable, which allows SkPaint to cache if it is in a
> no-draw state, which can be queried via an inline test.
> 3. Devices have to opt-out for now if they shouldn't draw the annotation.
Raster
> and GPU are doing that for rects/paths, which should be enough for now.
> 
> Next:
> 
> 4. PDF and XPS should respect the URL annotation, and opt-out for all
> no-draw-annotations
> 
> Later optimization idea:
> 
> 5. Have canvas know that its Device never wants any no-draw-annotations, and
> have it opt-out before it calls the device.

[bsalomon, 2012-07-11 19:43:49]

LGTM

On 2012/07/11 19:35:11, bungeman wrote:
> LGTM, even if it does punt on the type.
> 
> On 2012/07/11 19:27:39, reed1 wrote:
> > patch #8 Summary (ready for review/checkin)
> > 
> > 1. Only the helper function SkAnnotateRectWithURL() should be used publicly
> for
> > now
> > 2. Annotations are immutable, which allows SkPaint to cache if it is in a
> > no-draw state, which can be queried via an inline test.
> > 3. Devices have to opt-out for now if they shouldn't draw the annotation.
> Raster
> > and GPU are doing that for rects/paths, which should be enough for now.
> > 
> > Next:
> > 
> > 4. PDF and XPS should respect the URL annotation, and opt-out for all
> > no-draw-annotations
> > 
> > Later optimization idea:
> > 
> > 5. Have canvas know that its Device never wants any no-draw-annotations, and
> > have it opt-out before it calls the device.