SkValue: to/from SkImage

src/effects/SkToFromValue.cpp

Index: src/effects/SkToFromValue.cpp
diff --git a/src/effects/SkToFromValue.cpp b/src/effects/SkToFromValue.cpp
index b981d1715ee4d475f9b90363cdb10a301e0e74cb..42a6a6aee2a3b9c209f8b6dcf140bdf68bc7bb2f 100644
--- a/src/effects/SkToFromValue.cpp
+++ b/src/effects/SkToFromValue.cpp
@@ -6,6 +6,8 @@
  */
 
 #include "SkArithmeticMode.h"
+#include "SkData.h"
+#include "SkImage.h"
 #include "SkLerpXfermode.h"
 #include "SkMatrix.h"
 #include "SkPixelXorXfermode.h"
@@ -41,6 +43,15 @@ template<> bool SkFromValue<uint32_t>(const SkValue& val, uint32_t* x) {
     return true;
 }
 
+template<> bool SkFromValue<SkData*>(const SkValue& val, SkData** dst) {
+    REQUIRE(   val.type() == SkValue::Bytes

[mtklein, 2016/01/21 21:21:30]

Guess we'd better put isData() out in public for now.

[hal.canary, 2016/01/21 22:03:23]

done

+            || val.type() == SkValue::U16s
+            || val.type() == SkValue::U32s
+            || val.type() == SkValue::F32s);
+    *dst = val.bytes();  // no ref taken
+    return true;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 
 template<> SkValue SkToValue<SkMatrix>(const SkMatrix& mat) {
@@ -127,5 +138,29 @@ template<> bool SkFromValue< SkAutoTUnref<SkXfermode> >(
 
 ////////////////////////////////////////////////////////////////////////////////
 
+enum { kEncoded, kUniqueID };  // SkValue::Image

[mtklein, 2016/01/21 21:21:29]

?

[hal.canary, 2016/01/21 22:03:23]

moved.

+
+template<> SkValue SkToValue<SkImage>(const SkImage* image) {
+    SkAutoTUnref<SkData> encoded(image->encode());

[mtklein, 2016/01/22 14:09:18]

you know, this is probably the most controversial line of the whole CL.
it's not clear to me that we should be encoding these images.
that means,
   1) they must be encoded and decoded if you're going to use SkValues, which
may not be necessary
   2) the client gets no choice in how that encoding or decoding happens

Let's leave images for later to think more about.

+    if (!encoded || 0 == encoded->size()) {

[mtklein, 2016/01/21 21:21:30]

why 0 == encoded->size()?

[hal.canary, 2016/01/21 22:03:23]

removed.

+        return SkValue();
+    }
+    auto val = SkValue::Object(SkValue::Image);
+    val.set(kEncoded, SkValue::FromBytes(encoded));
+    val.set(kUniqueID, SkValue::FromU32(image->uniqueID()));

[mtklein, 2016/01/21 21:21:30]

This seems like a bad idea, especially if we ignore it in SkFromValue().

Those uniqueIDs are unique to the current process, but have nothing to do with
the image itself.

[hal.canary, 2016/01/21 22:03:23]

a serializer can use them to de-dup images.  Or anything else with a uniqueID.

[mtklein, 2016/01/22 14:09:18]

this unique ID is not part of the value of the image.  it does not belong in its
value.

+    return val;
+}
+
+template<> bool SkFromValue< SkAutoTUnref<SkImage> >(

[mtklein, 2016/01/21 21:21:30]

template<> bool SkFromValue<SkAutoTUnref<SkImage>>(const SkValue& val,
SkAutoTUnref<SkImage>* dst) {

is 100 columns.

[hal.canary, 2016/01/21 22:03:23]

yes but my *editor* doesn't know about how C++11 allows >> to sometimes not be a
shift operator.

[mtklein, 2016/01/22 14:09:18]

are you seriously proposing that we write code strangely to coddle the
eccentricities of the editor you have chosen to use?

+        const SkValue& val, SkAutoTUnref<SkImage>* dst) {
+    REQUIRE(val.type() == SkValue::Image);
+    SkData* encoded;
+    REQUIRE(getT(val, kEncoded, &encoded));
+    dst->reset(SkImage::NewFromEncoded(encoded));
+    return true;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
 #undef REQUIRE