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 d06c2c3399e4858e30626945948f4933f5d059c6..c4b4fd12e6c0eb45804413dc07a7a5e1b3bc411b 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -278,6 +278,45 @@ public:
     Result getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes);
 
     /**
+     *  If decoding to YUV is supported, this returns true and populates "sizes" with
+     *  the appropriate sizes of the Y, U, and V planes.
+     */
+    bool getYUV8Sizes(SkISize sizes[3]) const {

[scroggo, 2016/01/04 18:29:15]

crrev.com/863053002 proposed suggesting both a logical size and an optimal size.
IIRC the implementation in SkImageDecoder is a little wacky (sizes means one
thing sometimes and another other times), and that was part of cleaning it up.
Would it make sense to include that option here?

[msarett, 2016/01/04 21:46:46]

I think this is the hard question for this API.  I commented on this a bit when
I published the first Patch Set, but we might as well move the discussion here.

The jpeg library (whichever one it is) requires that the width and height of the
input memory be padded to a multiple of 8.  Thus, SkImageDecoder makes a
distinction between AllocatedSize and ActualSize.

It will suggest AllocatedSize, and then, after it finishes the decode, it will
change the size ptr to the ActualSize.  Chromium does something very similar.

This is actually unnecessary for the height.  We can always use the ActualSize. 
The jpeg library takes as input an array of row ptrs, so if we need padding, we
can trick it by passing in ptrs to a garbage row.

The width is what causes us a problem.  Here a few suggestions.  I imagine that
there are more solutions:
(1) When the client calls getYUV8Sizes(), we return the AllocatedWidth.  After
completing the decode in getYUV8Planes(), set the width to ActualWidth.  This is
what SkImageDecoder does.  And this is currently what my implementation does. 
In this implementation, 'sizes' cannot be const but 'rowBytes' can be const.
(2) Allow the client to query for 'ActualWidth' and 'AllocatedWidth' (or logical
size and optimal size, as you call them).  If they call getYUV8Planes() with
'ActualWidth', we will be able to complete the decode slowly using memcpys.  If
they call with 'AllocatedWidth', we can decode directly into their memory.  I
still think we would need to set the width to 'ActualWidth' at the end of this
decode.  Which I still think is messy and confusing.
(3) Make the query function suggest a value for "sizes" and "rowBytes".  Clients
must provide memory with enough rowBytes for us to perform the decode.  This
works until the last row (which will not necessarily be padded).  We could get
around this with a memcpy() (which I don't like because it is slow).  Or we
could require that the last row also be padded (which is weird because we are
changing the definition of rowBytes).  But at least sizes and rowBytes could
both be const.

I think (3) is the best, but we still have to sort out what to do with the
bottom row.

[msarett, 2016/01/12 14:25:27]

I have invented "widthBytes" which almost means the same thing as "rowBytes". 
Please see the latest Patch Set 4.

+        if (nullptr == sizes) {
+            return false;
+        }
+
+        return this->onGetYUV8Sizes(sizes);
+    }
+
+    /**

[scroggo, 2016/01/04 18:29:15]

nit: Typically the first line will describe generally what the method does.

[msarett, 2016/01/12 14:25:27]

Done.

+     *  @param sizes      Sizes of each of the Y, U, and V planes
+     *  @param pixels     Memory for each of the Y, U, and V planes

[scroggo, 2016/01/04 18:29:15]

Maybe this is just due to my lack of knowledge about the YUV data, but it might
be nice to describe how big we expect the memory to be; at least how many bytes
per logical pixel?

[msarett, 2016/01/04 21:46:46]

Will add comments.  Each Y, U, or V sample is a single byte.  Generally, the
size of the Y plane will equal the size of the image, while the U and V planes
may be downsampled further.

[msarett, 2016/01/12 14:25:27]

Added more detail.

+     *  @param rowBytes   Row bytes for each of the Y, U, and V planes
+     *  @param colorSpace If nullptr, this parameter is ignored.  Otherwise,

[scroggo, 2016/01/04 18:29:15]

Do we need this here? In
https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
we discussed returning it in the query step (named queryYUV8 in that CL;
getYUV8Sizes here). It looks like since we only ever return one answer, we could
put it in the query step for now, and then revisit if necessary?

[msarett, 2016/01/04 21:46:46]

Happy to add this to the query step.

What is this for?  It seems to me like this provides no useful information?  Of
course, I have no idea what kRec601_SkYUVColorSpace is?

IMO, it could possibly be more useful to report the type of YUV color space more
specifically (ex: 444, 422, ...).  See:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
But it looks like no one uses this information either.

+     *                    if the decode is successful, this will bet set to
+     *                    kJPEG_SkYUVColorSpace (JPEG is currently the only
+     *                    format that supports YUV).
+     *
+     *  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 getYUV8Sizes().
+     */
+    Result getYUV8Planes(SkISize sizes[3], void* pixels[3], size_t rowBytes[3],

[reed1, 2016/01/04 15:50:55]

can sizes[] be const? Seems much simplier if they are.

[scroggo, 2016/01/04 18:29:15]

Looks like rowBytes can also be const?

Alternatively, maybe we could use a struct similar to crrev.com/1396323007?

[msarett, 2016/01/04 21:46:46]

Depends on how we choose to deal with AllocatedSize vs. ActualSize.  Ideally, we
will make it const.  See the discussion above regarding some different options.
Yes.
+1.  I'm happy to use structs for "sizes" and "rowBytes".

[msarett, 2016/01/12 14:25:27]

Using structs now.

"sizes" and "widthBytes" are const.

+            SkYUVColorSpace* colorSpace) {
+        if (nullptr == sizes || nullptr == pixels || nullptr == pixels[0] ||
+                nullptr == pixels[1] || nullptr == pixels[2] || nullptr == rowBytes) {
+            return kInvalidInput;
+        }
+
+        if (!this->rewindIfNeeded()) {
+            return kCouldNotRewind;
+        }
+
+        return this->onGetYUV8Planes(sizes, pixels, rowBytes, colorSpace);
+    }
+
+    /**
      *  Some images may initially report that they have alpha due to the format
      *  of the encoded data, but then never use any colors which have alpha
      *  less than 100%. This function can be called *after* decoding to
@@ -454,7 +493,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();
     }
@@ -481,7 +520,16 @@ protected:
                                SkPMColor ctable[], int* ctableCount,
                                int* rowsDecoded) = 0;
 
-    virtual bool onGetValidSubset(SkIRect* /* desiredSubset */) const {
+    virtual bool onGetYUV8Sizes(SkISize* /*sizes*/) const {
+        return false;
+    }
+
+    virtual Result onGetYUV8Planes(SkISize* /*sizes*/, void** /*pixels*/, size_t* /*rowBytes*/,

[scroggo, 2016/01/04 18:29:15]

Why does this use different notations from getYUV8Planes?

+            SkYUVColorSpace* /*colorSpace*/) {
+        return kUnimplemented;
+    }
+
+    virtual bool onGetValidSubset(SkIRect* /*desiredSubset*/) const {
         // By default, subsets are not supported.
         return false;
     }