Add getYUV8Planes() API to SkCodec

src/codec/SkJpegCodec.cpp

Index: src/codec/SkJpegCodec.cpp
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index 7db772da6386ad9a3b0002d5334598a625fd6fa2..439d45e466c4391390405bd9aa52f3bc1323fac9 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp
@@ -452,3 +452,179 @@ bool SkJpegCodec::onSkipScanlines(int count) {
 
     return (uint32_t) count == jpeg_skip_scanlines(fDecoderMgr->dinfo(), count);
 }
+
+static bool is_yuv_supported(jpeg_decompress_struct* dinfo) {
+    // Scaling is not supported in raw data mode.
+    SkASSERT(dinfo->scale_num == dinfo->scale_denom);
+
+    // I can't imagine that this would ever change, but we do depend on it.
+    SkASSERT(8 == DCTSIZE);

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

Can this be a compile time assert (i.e. static_assert)?

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

Yes!

+
+    if (JCS_YCbCr != dinfo->jpeg_color_space) {
+        return false;
+    }
+
+    SkASSERT(3 == dinfo->num_components);
+    SkASSERT(dinfo->comp_info);
+
+    // TODO: It is possible to perform a YUV decode for any combination of
+    //       horizontal and vertical sampling that is supported by
+    //       libjpeg/libjpeg-turbo.  However, we will start by supporting
+    //       only the common cases.
+    bool unitUV = (1 == dinfo->comp_info[1].h_samp_factor) &&

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

Instead of creating this boolean that is just read at the end, what if we
instead said:

if (!<unitUV>) {
  return false;
}

...

return <commonY>

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

Done.

+                  (1 == dinfo->comp_info[1].v_samp_factor) &&
+                  (1 == dinfo->comp_info[2].h_samp_factor) &&
+                  (1 == dinfo->comp_info[2].v_samp_factor);
+
+    int hSampY = dinfo->comp_info[0].h_samp_factor;
+    int vSampY = dinfo->comp_info[0].v_samp_factor;
+    // TODO: Chromium reports the YUV subsampling factors to the client.

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

nit: blank line before comment.

[msarett, 2016/01/13 20:50:25]

Done.

+    //       Ex: 444, 422, 420, etc.
+    //       Could it be useful to do the same here?

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

If we're going to replace/merge with Chromium, it would make sense to report the
same.

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

Currently, the gpu determines the subsampling type by looking at the Y, U, and V
sizes.

While chrome has a function that reports an enum, AFAICT it is not used.  Other
than to confirm that the subsampling ratio is not YUV_UNKNOWN.

I don't think we need to provide this unless somebody starts asking for it.

+    bool commonY = (1 == hSampY && 1 == vSampY) ||
+                   (2 == hSampY && 1 == vSampY) ||
+                   (2 == hSampY && 2 == vSampY) ||
+                   (1 == hSampY && 2 == vSampY) ||
+                   (4 == hSampY && 1 == vSampY) ||
+                   (4 == hSampY && 2 == vSampY);
+
+    return unitUV && commonY;
+}
+
+bool SkJpegCodec::onGetYUV8Sizes(SkISize sizes[3]) const {
+    jpeg_decompress_struct* dinfo = fDecoderMgr->dinfo();
+    if (!is_yuv_supported(dinfo)) {
+        return false;
+    }
+
+    sizes[0].set(dinfo->comp_info[0].width_in_blocks * 8, dinfo->comp_info[0].downsampled_height);
+    sizes[1].set(dinfo->comp_info[1].width_in_blocks * 8, dinfo->comp_info[1].downsampled_height);
+    sizes[2].set(dinfo->comp_info[2].width_in_blocks * 8, dinfo->comp_info[2].downsampled_height);
+    return true;
+}
+
+SkCodec::Result SkJpegCodec::onGetYUV8Planes(SkISize sizes[3], void* pixels[3], size_t rowBytes[3],
+        SkYUVColorSpace* colorSpace) {
+    SkISize recommendedSizes[3];
+    // This will check is_yuv_supported(), so we don't need to here.
+    bool supportsYUV = this->onGetYUV8Sizes(recommendedSizes);
+    if (!supportsYUV || memcmp((const void*) recommendedSizes, (const void*) sizes, sizeof(sizes)))
+    {
+        fDecoderMgr->returnFailure("onGetYUV8Sizes", kInvalidInput);

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

return

[msarett, 2016/01/13 20:50:25]

Done.

+    }
+
+    // Set the jump location for libjpeg errors
+    if (setjmp(fDecoderMgr->getJmpBuf())) {
+        return fDecoderMgr->returnFailure("setjmp", kInvalidInput);
+    }
+
+    // Get a pointer to the decompress info since we will use it quite frequently
+    jpeg_decompress_struct* dinfo = fDecoderMgr->dinfo();
+
+    dinfo->raw_data_out = TRUE;
+    if (!jpeg_start_decompress(dinfo)) {
+        return fDecoderMgr->returnFailure("startDecompress", kInvalidInput);
+    }
+
+    // A previous implementation claims that the return value of is_yuv_supported()
+    // may change after calling jpeg_start_decompress().  It looks to me like this
+    // was caused by a bug in the old code, but we'll be safe and check here.
+    SkASSERT(is_yuv_supported(dinfo));
+
+    // TODO: As mentioned in is_yuv_supported(), we are requiring that U and V be
+    //       the same size and Y be equal to the image size (possibly with padding
+    //       on the width).  This is by far the most common case, but technically we
+    //       could support other combinations of sampling factors.
+    SkASSERT(sizes[1] == sizes[2]);
+    SkASSERT((uint32_t) sizes[0].height() == dinfo->output_height);
+
+    // Build a JSAMPIMAGE to handle output from libjpeg-turbo.  A JSAMPIMAGE has
+    // a 2-D array of pixels for each of the components (Y, U, V) in the image.
+    // Cheat Sheet:
+    //     JSAMPIMAGE == JSAMPLEARRAY* == JSAMPROW** == JSAMPLE***
+    JSAMPARRAY yuv[3];
+    // Set aside enough space for pointers to rows of Y, U, and V.
+    JSAMPROW rowptrs[16 + 8 + 8];
+    // TODO: Note that if we support more sampling factors, the amount of space set
+    //       aside here and/or which pointers we use for Y, U, and V may change.
+    //       This would also add complexity to the code below.
+    yuv[0] = &rowptrs[0]; // Y rows (8 or 16)

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

nit: Line up these comments?

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

Done.

+    yuv[1] = &rowptrs[16]; // U rows (8)
+    yuv[2] = &rowptrs[24]; // V rows (8)
+
+    // Initialize rowptrs.
+    int numYRowsPerBlock = 8 * dinfo->comp_info[0].v_samp_factor;
+    for (int i = 0; i < numYRowsPerBlock; i++) {
+        rowptrs[i] = (JSAMPROW) SkTAddOffset<void>(pixels[0], i * rowBytes[0]);

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

I think part of the point of SkTAddOffset is that it does the cast for you?
Can't this be

  rowptrs[i] = SkTAddOffset<JSAMPROW>(...);

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

Done.

+    }
+    for (int i = 0; i < 8; i++) {
+        rowptrs[i + 16] =(JSAMPROW) SkTAddOffset<void>(pixels[1], i * rowBytes[1]);

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

nit: space between "=" and the rhs.

[msarett, 2016/01/13 20:50:25]

Done.

+        rowptrs[i + 24] = (JSAMPROW) SkTAddOffset<void>(pixels[2], i * rowBytes[2]);
+    }
+
+    // After each loop iteration, we will increment pointers to Y, U, and V.
+    int blockIncrementY = numYRowsPerBlock * rowBytes[0];
+    int blockIncrementU = 8 * rowBytes[1];

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

Do all these "8"s refer to DCTSIZE? Should we use that constant instead?

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

Yes.

Should we use that constant instead?

Done.  I'm indifferent.  I'm not sure if it makes things more or less clear.

[scroggo, 2016/01/13 19:34:48]

I think it's more clear. It is more obvious to me why you would be using DCTSIZE
than 8

+    int blockIncrementV = 8 * rowBytes[2];
+
+    uint32_t numRowsPerBlock = numYRowsPerBlock;
+    // We intentionally round down here.  As a final step, we may need to decode one

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

Why do we intentionally round down?

And why might we need to decode one more partial block?

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

I'll improve this comment.

Essentially, if the image height is 21, we will first:
First, handle two normal blocks of 8 rows.
Second, handle the final five rows in a special case at the end.  This case is
"special" because we need to provide three fake rows to libjpeg-turbo.

I wanted to keep all of the dummyRow and remainingRows checks outside of the
main loop.  Maybe this isn't worth it?  I doubt the performance impact is
significant.

+    // more partial block.
+    int numIters = dinfo->output_height / numRowsPerBlock;

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

can be const?

[msarett, 2016/01/13 20:50:25]

Done.

+    int iters = 0;
+    while (iters < numIters) {

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

It looks like iters is only used inside the loop. Can this be a for loop?

[msarett, 2016/01/13 20:50:25]

Done.

+        JDIMENSION linesRead = jpeg_read_raw_data(dinfo, yuv, numRowsPerBlock);
+        if (linesRead < numRowsPerBlock) {
+            // FIXME: We will treat this as an error for now.
+            //        Can we fill memory and still display an image?
+            return kIncompleteInput;
+        }
+
+        // Update rowptrs.
+        for (int i = 0; i < numYRowsPerBlock; i++) {
+            rowptrs[i] += blockIncrementY;
+        }
+        for (int i = 0; i < 8; i++) {
+            rowptrs[i + 16] += blockIncrementU;
+            rowptrs[i + 24] += blockIncrementV;
+        }
+        iters++;
+    }
+
+    uint32_t remainingRows = dinfo->output_height - dinfo->output_scanline;
+    SkASSERT(remainingRows == dinfo->output_height % numRowsPerBlock);
+    SkASSERT(dinfo->output_scanline == numIters * numRowsPerBlock);
+    if (remainingRows > 0) {
+        // libjpeg-turbo needs memory to be padded by the block sizes.  We will fulfill
+        // this requirement using a dummy row buffer.
+        SkAutoTMalloc<JSAMPLE> dummyRow(rowBytes[0]);
+        for (int i = remainingRows; i < numYRowsPerBlock; i++) {
+            rowptrs[i] = dummyRow.get();
+        }
+        int remainingUVRows = dinfo->comp_info[1].downsampled_height - 8 * numIters;
+        for (int i = remainingUVRows; i < 8; i++) {
+            rowptrs[i + 16] = dummyRow.get();
+            rowptrs[i + 24] = dummyRow.get();
+        }
+
+        JDIMENSION linesRead = jpeg_read_raw_data(dinfo, yuv, numRowsPerBlock);

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

Should this use |remainingRows| instead of |numRowsPerBlock|?

[msarett, 2016/01/13 20:50:25]

Actually it must use |numRowsPerBlock|.  libjpeg/turbo expects us to provide
memory for numRowsPerBlock and to ask for numRowsPerBlock.  Even though, the
extra memory will not be populated with YUV values.

+        if (linesRead < remainingRows) {
+            // FIXME: We will treat this as an error for now.
+            //        Can we fill memory and still display an image?
+            return kIncompleteInput;

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

Doesn't kIncompleteInput mean (in getPixels etc) that we do display what we can
and fill the rest? It seems like these should be kInvalidInput until we
implement filling?

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

Done.

+        }
+    }
+
+    // FIXME: This is ugly.  We should find a solution where the input memory is

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

Does making the query call return both the logical and optimal sizes (as in
crrev.com/863053002) help with this problem?

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

Yes I believe so.

+    //        padded appropriately, but we are able to report the true width from
+    //        the start.
+    sizes[0].fWidth = dinfo->comp_info[0].downsampled_width;
+    sizes[1].fWidth = dinfo->comp_info[1].downsampled_width;
+    sizes[2].fWidth = dinfo->comp_info[2].downsampled_width;
+
+    if (nullptr != colorSpace) {
+        *colorSpace = kJPEG_SkYUVColorSpace;
+    }
+
+    return kSuccess;
+}