Add getYUV8Planes() API to SkCodec

include/codec/SkCodec.h

Index: include/codec/SkCodec.h
diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index 597ebd039e8a806bc09cf4c6f7eb0d1b8746ded8..8fd3187cf22904d443bb96dfd13a0dbc3dff8462 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -277,6 +277,81 @@ public:
      */
     Result getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes);
 
+    struct YUVSizeInfo {

[reed1, 2016/01/22 16:01:46]

Just so I understand the difference...

Size means logical number of bytes for width/height
Bytes means allocation number of bytes for width
?

[msarett, 2016/01/22 16:14:14]

Yes that's correct.

+        SkISize fYSize;
+        SkISize fUSize;
+        SkISize fVSize;
+
+        /**
+         * While the widths of the Y, U, and V planes are not restricted, the
+         * implementation requires that the width of the memory allocated for
+         * each plane be a multiple of DCTSIZE (which is always 8).
+         *
+         * This struct allows us to inform the client how many "widthBytes"
+         * that we need.  Note that we use the new idea of "widthBytes"
+         * because this idea is distinct from "rowBytes" (used elsewhere in
+         * Skia).  "rowBytes" allow the last row of the allocation to not
+         * include any extra padding, while, in this case, every single row of
+         * the allocation must be at least "widthBytes".
+         */
+        size_t fYWidthBytes;
+        size_t fUWidthBytes;
+        size_t fVWidthBytes;
+    };
+
+    /**
+     *  If decoding to YUV is supported, this returns true.  Otherwise, this
+     *  returns false and does not modify any of the parameters.
+     *
+     *  @param sizeInfo   Output parameter indicating the sizes and required
+     *                    allocation widths of the Y, U, and V planes.
+     *  @param colorSpace Output parameter.  If non-NULL this is set to kJPEG,
+     *                    otherwise this is ignored.
+     */
+    bool queryYUV8(YUVSizeInfo* sizeInfo, SkYUVColorSpace* colorSpace) const {
+        if (nullptr == sizeInfo) {

[reed1, 2016/01/22 16:01:46]

interesting: why is one parameter optional and the other required? Seems like if
the caller is just interested in knowing if YUV is supported than null for both
makes some sense. Or, since the colorspace is just an enum, seems like requiring
both is not onerous...

[msarett, 2016/01/22 16:14:14]

I'm fine with making both optional or both required.  What do you think is best?

For more background:

Making "colorSpace" optional is the same behavior from the old API.  IMO, this
makes sense because it seems fairly useless here.  We always return kJPEG. 
However, I've kept it because Brian says the GPU needs this enum to distinguish
JPEG from other types of YUV.

I'm not sure I see the point of making sizeInfo optional.  What kind of client
wants to know YUV is supported, but doesn't need the size of the planes?

[msarett, 2016/01/22 21:08:45]

Any thoughts?

+            return false;
+        }
+
+        return this->onQueryYUV8(sizeInfo, colorSpace);
+    }
+
+    /**
+     *  Returns kSuccess, or another value explaining the type of failure.
+     *  This always attempts to perform a full decode.  If the client only
+     *  wants size, it should call queryYUV8().
+     *
+     *  @param sizeInfo   Contains sizes of each of the Y, U, and V planes.
+     *                    The size of the Y plane will be the same size as the
+     *                    image.  The size of the U and V planes may be the
+     *                    same size as the image, or they may be a fraction of
+     *                    the size of the image (due to sampling).  It is
+     *                    quite common for the width to be sampled at a
+     *                    different rate than the height.  2 and 4 are common
+     *                    sampling rates.  The U and V planes are often the
+     *                    same size.
+     *                    Also contains, the width in bytes of each row of the
+     *                    Y, U, and V planes.  Each Y, U, or V "sample" is a
+     *                    single byte.  Also, the implementation requires that
+     *                    the width of the memory allocated be padded to a

[reed1, 2016/01/22 18:40:16]

This comment seems very JPEG specific. Do we need so much detail here, given the
comments on the YUVSizeInfo type itself?

Should the comments here just say something like:

the sizeinfo needs to match exactly the values returned by the query, except
that the widthbytes fields may be larger than the values returned by the query
(but not smaller).

?

[msarett, 2016/01/22 21:08:45]

I think the detail is a bit much.  Fixed.

So are we ok with keeping this parameter (and giving the client flexibility to
choose their own WidthBytes)?

+     *                    multiple of 8.  So the widthBytes should at least
+     *                    equal the Y, U, and V widths rounded up to the
+     *                    nearest multiples of 8.
+     *  @param planes     Memory for each of the Y, U, and V planes.
+     */
+    Result getYUV8Planes(const YUVSizeInfo& sizeInfo, void* planes[3]) {

[reed1, 2016/01/22 16:01:46]

Why do we pass the info back in? Can it (legally) be different from what was
returned by the query method?

[msarett, 2016/01/22 16:14:14]

The WidthBytes may be larger than the recommended WidthBytes (similar to
rowBytes I guess).

Having said that, I would be in favor of removing this parameter.  This means we
require that they use the WidthBytes returned from the query() function.  This
will always be aligned to 8 bytes.  Seems reasonable enough.

What do you think?

+        if (nullptr == planes || nullptr == planes[0] || nullptr == planes[1] ||
+                nullptr == planes[2]) {
+            return kInvalidInput;
+        }
+
+        if (!this->rewindIfNeeded()) {
+            return kCouldNotRewind;
+        }
+
+        return this->onGetYUV8Planes(sizeInfo, planes);
+    }
+
     /**
      * The remaining functions revolve around decoding scanlines.
      */
@@ -442,7 +517,7 @@ public:
 protected:
     SkCodec(const SkImageInfo&, SkStream*);
 
-    virtual SkISize onGetScaledDimensions(float /* desiredScale */) const {
+    virtual SkISize onGetScaledDimensions(float /*desiredScale*/) const {
         // By default, scaling is not supported.
         return this->getInfo().dimensions();
     }
@@ -469,7 +544,15 @@ protected:
                                SkPMColor ctable[], int* ctableCount,
                                int* rowsDecoded) = 0;
 
-    virtual bool onGetValidSubset(SkIRect* /* desiredSubset */) const {
+    virtual bool onQueryYUV8(YUVSizeInfo*, SkYUVColorSpace*) const {
+        return false;
+    }
+
+    virtual Result onGetYUV8Planes(const YUVSizeInfo&, void*[3] /*planes*/) {
+        return kUnimplemented;
+    }
+
+    virtual bool onGetValidSubset(SkIRect* /*desiredSubset*/) const {
         // By default, subsets are not supported.
         return false;
     }