From b6b8f2727f6b08bbc307aeebac23beeba1e4984f Mon Sep 17 00:00:00 2001 From: mtklein Date: Wed, 21 Oct 2015 10:46:02 -0700 Subject: [PATCH] SkRemote: refactoring - Cache becomes CachingEncoder that wraps another Encoder - Encoders provide IDs - syntaxy improvements to Client - ID isn't really protocol sensitive. - I don't think we need Type::kNone. No diffs. https://gold.skia.org/search2?issue=1418863002&unt=true&query=source_type%3Dgm&master=false&include=true BUG=skia: Review URL: https://codereview.chromium.org/1418863002 --- dm/DMSrcSink.cpp | 7 +- src/core/SkRemote.cpp | 284 +++++++++++++++++-------------------------- src/core/SkRemote.h | 109 ++++++++--------- src/core/SkRemote_protocol.h | 26 ---- 4 files changed, 166 insertions(+), 260 deletions(-) diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp index 24ae2d6..83b89f4 100644 --- a/dm/DMSrcSink.cpp +++ b/dm/DMSrcSink.cpp @@ -1163,10 +1163,11 @@ Error ViaPipe::draw(const Src& src, SkBitmap* bitmap, SkWStream* stream, SkStrin Error ViaRemote::draw(const Src& src, SkBitmap* bitmap, SkWStream* stream, SkString* log) const { return draw_to_canvas(fSink, bitmap, stream, log, src.size(), [&](SkCanvas* canvas) { - SkAutoTDelete cache(fCache ? SkRemote::Cache::CreateAlwaysCache() - : SkRemote::Cache::CreateNeverCache()); SkRemote::Server server(canvas); - SkRemote::Client client(cache, &server); + SkAutoTDelete cache(fCache + ? SkRemote::Encoder::CreateCachingEncoder(&server) + : nullptr); + SkRemote::Client client(cache.get() ? cache.get() : &server); return src.draw(&client); }); } diff --git a/src/core/SkRemote.cpp b/src/core/SkRemote.cpp index 7c662d3..21fbdf3 100644 --- a/src/core/SkRemote.cpp +++ b/src/core/SkRemote.cpp @@ -72,210 +72,141 @@ namespace SkRemote { // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // - class LookupScope { + class CachingEncoder final : public Encoder { public: - LookupScope(Cache* cache, Encoder* encoder) : fCache(cache), fEncoder(encoder) {} - ~LookupScope() { for (ID id : fToUndefine) { fEncoder->undefine(id); } } - void undefineWhenDone(ID id) { fToUndefine.push_back(id); } - - template - ID lookup(const T& val) { - ID id; - if (!fCache->lookup(val, &id, this)) { - fEncoder->define(id, val); - } - return id; - } + explicit CachingEncoder(Encoder* wrapped) : fWrapped(wrapped) {} private: - Cache* fCache; - Encoder* fEncoder; - SkSTArray<4, ID> fToUndefine; - }; + struct Undef { + Encoder* fEncoder; + template + void operator()(const T&, ID* id) const { fEncoder->undefine(*id); } + }; + ~CachingEncoder() override { + Undef undef{fWrapped}; + fMatrix .foreach(undef); + fMisc .foreach(undef); + fPath .foreach(undef); + fStroke .foreach(undef); + fShader .foreach(undef); + fXfermode.foreach(undef); + } - Cache* Cache::CreateNeverCache() { - struct NeverCache final : public Cache { - NeverCache() - : fNextMatrix (Type::kMatrix) - , fNextMisc (Type::kMisc) - , fNextPath (Type::kPath) - , fNextStroke (Type::kStroke) - , fNextShader (Type::kShader) - , fNextXfermode(Type::kXfermode) - {} - void cleanup(Encoder*) override {} - - static bool Helper(ID* next, ID* id, LookupScope* ls) { - *id = ++(*next); - ls->undefineWhenDone(*id); - return false; + template + ID define(Map* map, const T& v) { + if (const ID* id = map->find(v)) { + return *id; } + ID id = fWrapped->define(v); + map->set(v, id); + return id; + } - bool lookup(const SkMatrix&, ID* id, LookupScope* ls) override { - return Helper(&fNextMatrix, id, ls); - } - bool lookup(const Misc&, ID* id, LookupScope* ls) override { - return Helper(&fNextMisc, id, ls); - } - bool lookup(const SkPath&, ID* id, LookupScope* ls) override { - return Helper(&fNextPath, id, ls); - } - bool lookup(const Stroke&, ID* id, LookupScope* ls) override { - return Helper(&fNextStroke, id, ls); - } - bool lookup(const SkShader* shader, ID* id, LookupScope* ls) override { - if (!shader) { - *id = ID(Type::kShader); - return true; // Null IDs are always defined. - } - return Helper(&fNextShader, id, ls); - } - bool lookup(const SkXfermode* xfermode, ID* id, LookupScope* ls) override { - if (!xfermode) { - *id = ID(Type::kXfermode); - return true; // Null IDs are always defined. - } - return Helper(&fNextXfermode, id, ls); - } + ID define(const SkMatrix& v) override { return this->define(&fMatrix, v); } + ID define(const Misc& v) override { return this->define(&fMisc, v); } + ID define(const SkPath& v) override { return this->define(&fPath, v); } + ID define(const Stroke& v) override { return this->define(&fStroke, v); } + ID define(SkShader* v) override { return this->define(&fShader, v); } + ID define(SkXfermode* v) override { return this->define(&fXfermode, v); } - ID fNextMatrix, - fNextMisc, - fNextPath, - fNextStroke, - fNextShader, - fNextXfermode; - }; - return new NeverCache; - } + void undefine(ID) override {} - // These can't be declared locally inside AlwaysCache because of the templating. :( - namespace { - template - static bool always_cache_lookup(const T& val, Map* map, ID* next, ID* id) { - if (const ID* found = map->find(val)) { - *id = *found; - return true; - } - *id = ++(*next); - map->set(val, *id); - return false; - } + void save() override { fWrapped-> save(); } + void restore() override { fWrapped->restore(); } - struct Undefiner { - Encoder* fEncoder; + void setMatrix(ID matrix) override { fWrapped->setMatrix(matrix); } - template - void operator()(const T&, ID* id) const { fEncoder->undefine(*id); } - }; + void clipPath(ID path, SkRegion::Op op, bool aa) override { + fWrapped->clipPath(path, op, aa); + } + void fillPath(ID path, ID misc, ID shader, ID xfermode) override { + fWrapped->fillPath(path, misc, shader, xfermode); + } + void strokePath(ID path, ID misc, ID shader, ID xfermode, ID stroke) override { + fWrapped->strokePath(path, misc, shader, xfermode, stroke); + } - // Maps const T* -> ID, and refs the key. nullptr always maps to ID(kType). + // Maps const T* -> ID, and refs the key. template class RefKeyMap { public: RefKeyMap() {} - ~RefKeyMap() { fMap.foreach([](const T* key, ID*) { key->unref(); }); } + ~RefKeyMap() { fMap.foreach([](const T* key, ID*) { SkSafeUnref(key); }); } - void set(const T* key, const ID& id) { - SkASSERT(key && id.type() == kType); - fMap.set(SkRef(key), id); + void set(const T* key, ID id) { + SkASSERT(id.type() == kType); + fMap.set(SkSafeRef(key), id); } void remove(const T* key) { - SkASSERT(key); fMap.remove(key); - key->unref(); + SkSafeUnref(key); } const ID* find(const T* key) const { - static const ID nullID(kType); - return key ? fMap.find(key) : &nullID; + return fMap.find(key); } template - void foreach(const Fn& fn) { fMap.foreach(fn); } + void foreach(const Fn& fn) { + fMap.foreach(fn); + } private: SkTHashMap fMap; }; - } // namespace - - Cache* Cache::CreateAlwaysCache() { - struct AlwaysCache final : public Cache { - AlwaysCache() - : fNextMatrix (Type::kMatrix) - , fNextMisc (Type::kMisc) - , fNextPath (Type::kPath) - , fNextStroke (Type::kStroke) - , fNextShader (Type::kShader) - , fNextXfermode(Type::kXfermode) - {} - - void cleanup(Encoder* encoder) override { - Undefiner undef{encoder}; - fMatrix .foreach(undef); - fMisc .foreach(undef); - fPath .foreach(undef); - fStroke .foreach(undef); - fShader .foreach(undef); - fXfermode.foreach(undef); - } + SkTHashMap fMatrix; + SkTHashMap fMisc; + SkTHashMap fPath; + SkTHashMap fStroke; + RefKeyMap fShader; + RefKeyMap fXfermode; - bool lookup(const SkMatrix& matrix, ID* id, LookupScope*) override { - return always_cache_lookup(matrix, &fMatrix, &fNextMatrix, id); - } - bool lookup(const Misc& misc, ID* id, LookupScope*) override { - return always_cache_lookup(misc, &fMisc, &fNextMisc, id); - } - bool lookup(const SkPath& path, ID* id, LookupScope*) override { - return always_cache_lookup(path, &fPath, &fNextPath, id); - } - bool lookup(const Stroke& stroke, ID* id, LookupScope*) override { - return always_cache_lookup(stroke, &fStroke, &fNextStroke, id); - } - bool lookup(const SkShader* shader, ID* id, LookupScope*) override { - return always_cache_lookup(shader, &fShader, &fNextShader, id); - } - bool lookup(const SkXfermode* xfermode, ID* id, LookupScope*) override { - return always_cache_lookup(xfermode, &fXfermode, &fNextXfermode, id); - } + Encoder* fWrapped; + }; - SkTHashMap fMatrix; - SkTHashMap fMisc; - SkTHashMap fPath; - SkTHashMap fStroke; - RefKeyMap fShader; - RefKeyMap fXfermode; - - ID fNextMatrix, - fNextMisc, - fNextPath, - fNextStroke, - fNextShader, - fNextXfermode; - }; - return new AlwaysCache; - } + Encoder* Encoder::CreateCachingEncoder(Encoder* wrapped) { return new CachingEncoder(wrapped); } + + // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // + + // Calls Encoder::define() when created, Encoder::undefine() when destroyed. + class Client::AutoID : ::SkNoncopyable { + public: + template + explicit AutoID(Encoder* encoder, const T& val) + : fEncoder(encoder) + , fID(encoder->define(val)) {} + ~AutoID() { if (fEncoder) fEncoder->undefine(fID); } + + AutoID(AutoID&& o) : fEncoder(o.fEncoder), fID(o.fID) { + o.fEncoder = nullptr; + } + AutoID& operator=(AutoID&&) = delete; + + operator ID () const { return fID; } + + private: + Encoder* fEncoder; + const ID fID; + }; + + template + Client::AutoID Client::id(const T& val) { return AutoID(fEncoder, val); } // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // - Client::Client(Cache* cache, Encoder* encoder) + Client::Client(Encoder* encoder) : SkCanvas(1,1) - , fCache(cache) , fEncoder(encoder) {} - Client::~Client() { - fCache->cleanup(fEncoder); - } - void Client::willSave() { fEncoder->save(); } void Client::didRestore() { fEncoder->restore(); } void Client::didConcat (const SkMatrix&) { this->didSetMatrix(this->getTotalMatrix()); } void Client::didSetMatrix(const SkMatrix& matrix) { - LookupScope ls(fCache, fEncoder); - fEncoder->setMatrix(ls.lookup(matrix)); + fEncoder->setMatrix(this->id(matrix)); } void Client::onDrawOval(const SkRect& oval, const SkPaint& paint) { @@ -306,17 +237,16 @@ namespace SkRemote { } void Client::onDrawPath(const SkPath& path, const SkPaint& paint) { - LookupScope ls(fCache, fEncoder); - ID p = ls.lookup(path), - m = ls.lookup(Misc::CreateFrom(paint)), - s = ls.lookup(paint.getShader()), - x = ls.lookup(paint.getXfermode()); + auto p = this->id(path), + m = this->id(Misc::CreateFrom(paint)), + s = this->id(paint.getShader()), + x = this->id(paint.getXfermode()); if (paint.getStyle() == SkPaint::kFill_Style) { fEncoder->fillPath(p, m, s, x); } else { // TODO: handle kStrokeAndFill_Style - fEncoder->strokePath(p, m, s, x, ls.lookup(Stroke::CreateFrom(paint))); + fEncoder->strokePath(p, m, s, x, this->id(Stroke::CreateFrom(paint))); } } @@ -368,20 +298,26 @@ namespace SkRemote { } void Client::onClipPath(const SkPath& path, SkRegion::Op op, ClipEdgeStyle edgeStyle) { - LookupScope ls(fCache, fEncoder); - fEncoder->clipPath(ls.lookup(path), op, edgeStyle == kSoft_ClipEdgeStyle); + fEncoder->clipPath(this->id(path), op, edgeStyle == kSoft_ClipEdgeStyle); } // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // Server::Server(SkCanvas* canvas) : fCanvas(canvas) {} - void Server::define(ID id, const SkMatrix& v) { fMatrix .set(id, v); } - void Server::define(ID id, const Misc& v) { fMisc .set(id, v); } - void Server::define(ID id, const SkPath& v) { fPath .set(id, v); } - void Server::define(ID id, const Stroke& v) { fStroke .set(id, v); } - void Server::define(ID id, SkShader* v) { fShader .set(id, v); } - void Server::define(ID id, SkXfermode* v) { fXfermode.set(id, v); } + template + ID Server::define(Type type, Map* map, const T& val) { + ID id(type, fNextID++); + map->set(id, val); + return id; + } + + ID Server::define(const SkMatrix& v) { return this->define(Type::kMatrix, &fMatrix, v); } + ID Server::define(const Misc& v) { return this->define(Type::kMisc, &fMisc, v); } + ID Server::define(const SkPath& v) { return this->define(Type::kPath, &fPath, v); } + ID Server::define(const Stroke& v) { return this->define(Type::kStroke, &fStroke, v); } + ID Server::define(SkShader* v) { return this->define(Type::kShader, &fShader, v); } + ID Server::define(SkXfermode* v) { return this->define(Type::kXfermode, &fXfermode, v); } void Server::undefine(ID id) { switch(id.type()) { @@ -391,8 +327,6 @@ namespace SkRemote { case Type::kStroke: return fStroke .remove(id); case Type::kShader: return fShader .remove(id); case Type::kXfermode: return fXfermode.remove(id); - - case Type::kNone: SkASSERT(false); }; } diff --git a/src/core/SkRemote.h b/src/core/SkRemote.h index 6e95f3a..cbfcf9b 100644 --- a/src/core/SkRemote.h +++ b/src/core/SkRemote.h @@ -18,7 +18,26 @@ // TODO: document namespace SkRemote { - // TODO: document + + // General purpose identifier. Holds a Type and a 56-bit value. + class ID { + public: + ID() {} + ID(Type type, uint64_t val) { + fVal = (uint64_t)type << 56 | val; + SkASSERT(this->type() == type && this->val() == val); + } + + Type type() const { return (Type)(fVal >> 56); } + uint64_t val() const { return fVal & ~((uint64_t)0xFF << 56); } + + bool operator==(ID o) const { return fVal == o.fVal; } + + private: + uint64_t fVal; + }; + + // Fields from SkPaint used by stroke, fill, and text draws. struct Misc { SkColor fColor; SkFilterQuality fFilterQuality; @@ -28,7 +47,7 @@ namespace SkRemote { void applyTo(SkPaint*) const; }; - // TODO: document + // Fields from SkPaint used by stroke draws only. struct Stroke { SkScalar fWidth, fMiter; SkPaint::Cap fCap; @@ -42,12 +61,14 @@ namespace SkRemote { struct Encoder { virtual ~Encoder() {} - virtual void define(ID, const SkMatrix&) = 0; - virtual void define(ID, const Misc&) = 0; - virtual void define(ID, const SkPath&) = 0; - virtual void define(ID, const Stroke&) = 0; - virtual void define(ID, SkShader*) = 0; - virtual void define(ID, SkXfermode*) = 0; + static Encoder* CreateCachingEncoder(Encoder*); + + virtual ID define(const SkMatrix&) = 0; + virtual ID define(const Misc&) = 0; + virtual ID define(const SkPath&) = 0; + virtual ID define(const Stroke&) = 0; + virtual ID define(SkShader*) = 0; + virtual ID define(SkXfermode*) = 0; virtual void undefine(ID) = 0; @@ -64,41 +85,17 @@ namespace SkRemote { virtual void strokePath(ID path, ID misc, ID shader, ID xfermode, ID stroke) = 0; }; - class LookupScope; - - // The Cache interface encapsulates the caching logic of the Client. - // - // Each lookup() method must always fill ID* with a valid value, - // but ID may be cached. If so, the lookup() method returns true; - // if not the lookup() method returns false and the Client must - // then define() this ID -> Thing mapping before using the ID. - // - // The Caches may also add IDs to the LookupScope's list of IDs to - // undefine() on destruction. This lets the Cache purge IDs. - struct Cache { - virtual ~Cache() {} - - static Cache* CreateNeverCache(); // Never caches anything. - static Cache* CreateAlwaysCache(); // Caches by value (not deep pointer equality). - // TODO: static Cache* CreateDeepCache(); // Caches by deep value. - - virtual bool lookup(const SkMatrix&, ID*, LookupScope*) = 0; - virtual bool lookup(const Misc&, ID*, LookupScope*) = 0; - virtual bool lookup(const SkPath&, ID*, LookupScope*) = 0; - virtual bool lookup(const Stroke&, ID*, LookupScope*) = 0; - virtual bool lookup(const SkShader*, ID*, LookupScope*) = 0; - virtual bool lookup(const SkXfermode*, ID*, LookupScope*) = 0; - - virtual void cleanup(Encoder*) = 0; - }; - - // TODO: document + // An SkCanvas that translates to Encoder calls. class Client final : public SkCanvas { public: - Client(Cache*, Encoder*); - ~Client(); + explicit Client(Encoder*); private: + class AutoID; + + template + AutoID id(const T&); + void willSave() override; void didRestore() override; @@ -123,22 +120,21 @@ namespace SkRemote { void onDrawPosTextH(const void*, size_t, const SkScalar[], SkScalar, const SkPaint&) override; - Cache* fCache; Encoder* fEncoder; }; - // TODO: document + // An Encoder that translates back to SkCanvas calls. class Server final : public Encoder { public: explicit Server(SkCanvas*); private: - void define(ID, const SkMatrix&) override; - void define(ID, const Misc&) override; - void define(ID, const SkPath&) override; - void define(ID, const Stroke&) override; - void define(ID, SkShader*) override; - void define(ID, SkXfermode*) override; + ID define(const SkMatrix&) override; + ID define(const Misc&) override; + ID define(const SkPath&) override; + ID define(const Stroke&) override; + ID define(SkShader*) override; + ID define(SkXfermode*) override; void undefine(ID) override; @@ -185,25 +181,22 @@ namespace SkRemote { template class ReffedIDMap { public: - ReffedIDMap() { - // A null ID always maps to nullptr. - fMap.set(ID(kType), nullptr); - } + ReffedIDMap() {} ~ReffedIDMap() { // A well-behaved client always cleans up its definitions. - SkASSERT(fMap.count() == 1); + SkASSERT(fMap.count() == 0); } void set(const ID& id, T* val) { - SkASSERT(id.type() == kType && val); - fMap.set(id, SkRef(val)); + SkASSERT(id.type() == kType); + fMap.set(id, SkSafeRef(val)); } void remove(const ID& id) { SkASSERT(id.type() == kType); T** val = fMap.find(id); - SkASSERT(val && *val); - (*val)->unref(); + SkASSERT(val); + SkSafeUnref(*val); fMap.remove(id); } @@ -218,6 +211,9 @@ namespace SkRemote { SkTHashMap fMap; }; + template + ID define(Type, Map*, const T&); + IDMap fMatrix; IDMap fMisc; IDMap fPath; @@ -226,6 +222,7 @@ namespace SkRemote { ReffedIDMap fXfermode; SkCanvas* fCanvas; + uint64_t fNextID = 0; }; } // namespace SkRemote diff --git a/src/core/SkRemote_protocol.h b/src/core/SkRemote_protocol.h index d34a4b6..b89d491 100644 --- a/src/core/SkRemote_protocol.h +++ b/src/core/SkRemote_protocol.h @@ -15,8 +15,6 @@ namespace SkRemote { // It is safe to append to this enum without breaking protocol compatibility. // Resorting, deleting, or inserting anywhere but the end will break compatibility. enum class Type : uint8_t { - kNone, - kMatrix, kMisc, kPath, @@ -25,30 +23,6 @@ namespace SkRemote { kXfermode, }; - class ID { - public: - explicit ID(Type type = Type::kNone) : fVal((uint64_t)type << 56) {} - ID(Type type, uint64_t val) { - fVal = (uint64_t)type << 56 | val; - SkASSERT(this->type() == type && this->val() == val); - } - - Type type() const { return (Type)(fVal >> 56); } - uint64_t val() const { return fVal & ~((uint64_t)0xFF << 56); } - - bool operator==(ID o) const { return fVal == o.fVal; } - ID operator++() { - ++fVal; - SkASSERT(this->val() != 0); // Overflow is particularly bad as it'd change our Type. - return *this; - } - - private: - // High 8 bits hold a Type. Low 56 bits are unique within that Type. - // Any change to this format will break protocol compatibility. - uint64_t fVal; - }; - } // namespace SkRemote #endif//SkRemote_protocol_DEFINED -- 2.7.4