Prototype of RAW decoding in Skia.

src/codec/SkRawCodec.cpp

Index: src/codec/SkRawCodec.cpp
diff --git a/src/codec/SkRawCodec.cpp b/src/codec/SkRawCodec.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..5bd746b541a386a3b1a22ed56d49965aabdbf596
--- /dev/null
+++ b/src/codec/SkRawCodec.cpp
@@ -0,0 +1,367 @@
+/*
+ * Copyright 2015 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "SkCodec.h"
+#include "SkRawCodec.h"
+#include "SkCodecPriv.h"
+#include "SkColorPriv.h"
+#include "SkJpegCodec.h"
+#include "SkStream.h"
+#include "SkStreamPriv.h"
+#include "SkSwizzler.h"
+#include "SkTemplates.h"
+#include "SkTypes.h"
+
+#include "dng_color_space.h"
+#include "dng_exceptions.h"
+#include "dng_host.h"
+#include "dng_info.h"
+#include "dng_render.h"
+#include "dng_stream.h"
+
+#include "src/piex.h"
+
+#include <limits>
+
+namespace {
+
+// T must be unsigned type.
+template <class T>
+bool safe_add_to_size_t(T arg1, T arg2, size_t* result) {
+    SkASSERT(arg1 >= 0);
+    SkASSERT(arg2 >= 0);
+    if (arg1 >= 0 && arg2 <= std::numeric_limits<T>::max() - arg1) {
+        T sum = arg1 + arg2;
+        if (sum <= std::numeric_limits<size_t>::max()) {

[scroggo, 2016/01/12 21:54:39]

How can this be false if the above if statement is true?

[yujieqin, 2016/01/13 15:29:42]

T could be uint64 in our use case, so we need this check.

+            *result = static_cast<size_t>(sum);
+            return true;
+        }
+    }
+    return false;
+}
+
+dng_negative* read_dng(dng_stream* stream, dng_host* host, dng_info* info) {
+    try {
+        SkAutoTDelete<dng_negative> negative;
+        host->ValidateSizes();
+
+        info->Parse(*host, *stream);
+        info->PostParse(*host);
+
+        if (!info->IsValidDNG()) {
+            return nullptr;
+        }
+
+        negative.reset(host->Make_dng_negative());
+        negative->Parse(*host, *stream, *info);
+        negative->PostParse(*host, *stream, *info);
+        negative->SynchronizeMetadata();
+
+        return negative.release();
+    } catch (...) {
+        return nullptr;
+    }
+}
+
+/*
+ * Renders the DNG image, which is loaded by read_dng().

[scroggo, 2016/01/12 21:54:39]

I find this comment confusing. Is it saying that it calls read_dng? Or that
read_dng already loaded the image? But it looks like read_dng created a
"negative", not an "image". I think a clearer comment might say something like:

Renders the DNG image to the preferred size.

If you want to say something about read_dng, I think it probably makes sense to
specify it along with the parameter descriptions. e.g.

host: already initialized by read_dng
info: already initialized by read_dng
negative: loaded by read_dng

(That said, down below, I suggested making this method a member method, in which
case you do not need to pass the parameters in.)

[yujieqin, 2016/01/13 15:29:42]

moved into the class SkDngImage, then we don't need the old comments anymore.

+ *
+ * @param preferredSize
+ * Determines the preferred size of the long edge. The rendered image will be close to this size,

[scroggo, 2016/01/12 21:54:39]

nit: Specifies the preferred size ...

sounds better to me.

[yujieqin, 2016/01/13 15:29:42]

Done.

+ * but there is no guarantee that any of the edges will match the preferred size. E.g.
+ *   100% size:              4000 x 3000
+ *   preferred size:         1600 (1600 x 1200)
+ *   returned size could be: 2000 x 1500
+ *
+ * @param stream
+ * Contains the data of the image.

[scroggo, 2016/01/12 21:54:39]

I think this is more clear as: Contains the compressed data representing the
image.

[yujieqin, 2016/01/13 15:29:42]

Comment removed.

+ *
+ * @param host
+ * Contains the connection to DNG SDK.
+ *
+ * @param info
+ * Contains the infomation of the DNG image.
+ *
+ * @param negative
+ * Contains the parsed information of the DNG image.
+ *
+ */
+dng_image* render_dng_for_preferred_size(const int preferredSize, dng_stream* stream, 
+                                         dng_host* host, dng_info* info, dng_negative* negative) {
+    try {
+        host->SetPreferredSize(preferredSize);
+        host->ValidateSizes();
+
+        negative->ReadStage1Image(*host, *stream, *info);
+
+        if (info->fMaskIndex != -1) {
+            negative->ReadTransparencyMask(*host, *stream, *info);
+        }
+
+        negative->ValidateRawImageDigest(*host);
+        if (negative->IsDamaged()) {
+            return nullptr;
+        }
+
+        const int32 kMosaicPlane = -1;
+        negative->BuildStage2Image(*host);
+        negative->BuildStage3Image(*host, kMosaicPlane);
+
+        dng_render render(*host, *negative);
+        render.SetFinalSpace(dng_space_sRGB::Get());
+        render.SetFinalPixelType(ttByte);
+
+        dng_point stage3_size = negative->Stage3Image()->Size();
+        render.SetMaximumSize(SkTMax(stage3_size.h, stage3_size.v));
+
+        return render.Render();
+    } catch (...) {
+        return nullptr;
+    }
+}
+
+}  // namespace
+
+// Note: this class could throw exception if it is used as dng_stream.
+class SkRawStream : public dng_stream, public ::piex::StreamInterface {
+public:
+    // Note that this call will take the ownership of stream.
+    explicit SkRawStream(SkStream* stream)
+        : fStream(stream), fWholeStreamRead(false) {}
+
+    ~SkRawStream() override {}
+
+    // Creates a SkMemoryStream from the offset with size.
+    SkMemoryStream* copyBuffer(size_t offset, size_t size) {

[scroggo, 2016/01/12 21:54:39]

I think this method could be more efficient. Assuming this is called before we
reach |offset| in |fStream|, it is going to buffer:

a) the stream from its current offset until |offset|
b) the stream from |offset| to |offset| + |size|

You don't actually care about (a) (maybe its zero in practice, but then it seems
like we don't need the parameter |offset|), and (b) is just going to be copied
again. You never need the first copy.

I think what you really want is something like the following:

if (!fStream->skip(offset - fStreamBuffer.bytesWritten()) {
    return nullptr;
}

// Note that there is a TODO in SkStream to eliminate getMemoryBase
// This is how you use an SkData to do the same thing:
SkAutoTUnref<SkData> data(SkData::NewUninitialized(size));
if (fStream->read(data->writable_data(), size) != size) {
    return nullptr;
}

return new SkMemoryStream(data);

Two caveats:
- This is destructive to the SkRawStream. In practice, I think this is fine
because you never use it after calling copyBuffer. But I do think it is worth a
comment.
- I'm assuming that you have not already buffered past offset. If you have, you
can read fStreamBuffer into the new SkData, and then copy any remaining bytes
directly into it. This will be only slightly more complicated.

[yujieqin, 2016/01/13 15:29:42]

Agreed, I think it is a good improvement if the offset is not buffered yet. (in
most of the case, this can be actually true).

+        size_t sum;
+        if (!safe_add_to_size_t(offset, size, &sum) || !this->bufferMoreData(sum)) {
+            return nullptr;
+        }
+        SkAutoTDelete<SkMemoryStream> newStream(new SkMemoryStream(size));
+        if (!fStreamBuffer.read(const_cast<void*>(newStream->getMemoryBase()), offset, size)) {
+            return nullptr;
+        }
+        return newStream.release();
+    }
+
+    // For PIEX
+    ::piex::Error GetData(const size_t offset, const size_t length,
+                          uint8* data) override {
+        if (offset == 0 && length == 0) {
+            return ::piex::Error::kOk;
+        }
+        size_t sum;
+        if (!safe_add_to_size_t(offset, length, &sum) || !this->bufferMoreData(sum)) {
+            return ::piex::Error::kFail;
+        }
+        if (!fStreamBuffer.read(data, offset, length)) {
+            return ::piex::Error::kFail;
+        }
+        return ::piex::Error::kOk;
+    }
+
+protected:
+    // For dng_stream
+    uint64 DoGetLength() override {
+        if (!this->bufferMoreData(kReadToEnd)) {  // read whole stream
+            ThrowReadFile();
+        }
+        return fStreamBuffer.bytesWritten();
+    }
+
+    // For dng_stream
+    void DoRead(void* data, uint32 count, uint64 offset) override {
+        if (count == 0 && offset == 0) {
+            return;
+        }
+        size_t sum;
+        if (!safe_add_to_size_t(static_cast<uint64>(count), offset, &sum) ||
+            !this->bufferMoreData(sum)) {
+            ThrowReadFile();
+        }
+
+        if (!fStreamBuffer.read(data, offset, count)) {
+            ThrowReadFile();
+        }
+    }
+
+private:
+    // Note: if the newSize == kReadToEnd (0), this function will read to the end of stream..
+    bool bufferMoreData(size_t newSize) {
+        if (newSize == kReadToEnd) {
+            if (fWholeStreamRead) {  // the stream is already read-to-end.
+                return true;
+            }
+
+            // TODO: optimize for the special case when the input is SkMemoryStream.
+            return SkStreamCopy(&fStreamBuffer, fStream.get());
+        }
+
+        if (newSize <= fStreamBuffer.bytesWritten()) {  // the stream is already buffered to newSize.

[scroggo, 2016/01/12 21:54:39]

nit: 100 chars

[yujieqin, 2016/01/13 15:29:42]

Done.

+            return true;
+        }
+        if (fWholeStreamRead) {  // newSize is larger than the whole stream.
+            return false;
+        }
+
+        const size_t sizeToRead = newSize - fStreamBuffer.bytesWritten();
+        SkAutoTMalloc<uint8> tempBuffer;
+        tempBuffer.realloc(sizeToRead);

[scroggo, 2016/01/12 21:54:39]

nit: You can combine these two lines into:

SkAutoTMalloc<uint8> tempBuffer(sizeToRead);

[yujieqin, 2016/01/13 15:29:42]

Thanks.

+        const size_t bytesRead = fStream->read(tempBuffer.get(), sizeToRead);
+        if (bytesRead != sizeToRead) {
+            return false;
+        }
+        return fStreamBuffer.write(tempBuffer.get(), bytesRead);
+    }
+
+    SkAutoTDelete<SkStream> fStream;
+    bool fWholeStreamRead;
+
+    SkDynamicMemoryWStream fStreamBuffer;
+
+    const size_t kReadToEnd = 0;
+};
+
+SkCodec* SkRawCodec::NewFromStream(SkStream* stream) {
+    SkAutoTDelete<SkRawStream> rawStream(new SkRawStream(stream));
+    ::piex::PreviewImageData imageData;
+    if (::piex::IsRaw(rawStream.get()) &&
+        ::piex::GetPreviewImageData(rawStream.get(), &imageData) == ::piex::Error::kOk)
+    {
+        SkMemoryStream* memoryStream =
+                rawStream->copyBuffer(imageData.jpeg_offset, imageData.jpeg_length);
+        return memoryStream ? SkJpegCodec::NewFromStream(memoryStream) : nullptr;

[scroggo, 2016/01/12 21:54:39]

We'll need to skip the JPEG code ing Google3 (i.e. "#if !defined(GOOGLE3)", as
in SkCodec.cpp), unless it turns out we cannot build SkRawCodec at all in
Google3 (I don't know its restrictions; I don't remember why SkJpegCodec is not
allowed there).

[yujieqin, 2016/01/13 15:29:42]

Done

+    }
+
+    SkAutoTDelete<dng_host> host(new dng_host);
+    SkAutoTDelete<dng_info> info(new dng_info);
+    SkAutoTDelete<dng_negative> negative(read_dng(rawStream.get(), host.get(), info.get()));
+    if (!negative) {
+        return nullptr;
+    }
+
+    const SkImageInfo& imageInfo = SkImageInfo::Make(

[scroggo, 2016/01/12 21:54:39]

I don't think this is right to make this a reference. SkImageInfo::Make will
return a copy, unless it's inlined, in which case you're essentially calling the
constructor. Either way, this should just be "const SkImageInfo"

[yujieqin, 2016/01/13 15:29:41]

Removed.

+        negative->DefaultCropSizeH().As_real64(),
+        negative->DefaultCropSizeV().As_real64(),
+        kN32_SkColorType, kOpaque_SkAlphaType);
+    return new SkRawCodec(imageInfo, rawStream.release(), host.release(),
+                          info.release(), negative.release());
+}
+
+SkCodec::Result SkRawCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst,
+                                        size_t dstRowBytes, const Options& options,
+                                        SkPMColor ctable[], int* ctableCount,
+                                        int* rowsDecoded) {
+    const int width = requestedInfo.width();
+    const int height = requestedInfo.height();
+    if (!fDngImage ||
+        (fDngImage->Size().h != width || fDngImage->Size().v != height))
+    {
+        // Reset the DNG image, so that we can rerender it for different size.
+        if (fDngImage) {
+            SkCodecPrintf("Warning: For RAW images, calling onGetPixels() with a size that \
+                    does not match the result of onGetScaledDimensions() forces the decoder to \
+                    render the image multiple times.");
+            fDngImage.reset();
+            fDngHost.reset(new dng_host);

[scroggo, 2016/01/12 21:54:39]

Can these fields not be reused?

I also notice that the SkRawCodec never cares about the dng_host, dng_info, or
dng_negative. It only hangs on to them to pass them to static functions. Would
it make sense to abstract this into its own class?

class SkDngImage {
private:
    SkDngImage(...); // constructor
    fDngImage;
    fDngInfo;
    fDngNegative;
    fDngHost;
    fRawStream;
public:
    static SkDngImage* NewFromStream(SkRawStream*) {
        // returns nullptr when read_dng would have failed
    }

    bool /* or SkCodec::Result? */ scale(int width, int height) {
        int prefSize = SkTMax(width, height);
        // does what render_dng_for_preferred_size does now
        if (!fDngImage) {
            return false; // or kInvalidInput;
        }
        // Possibly check for matching width/height here...
    }
}

Alternatively, should read_dng and render_dng_for_preferred_size be private
member functions on SkRawCodec? Then we do not have to pass all these pointers
around all the time.

[yujieqin, 2016/01/13 15:29:42]

Done.

+            fDngInfo.reset(new dng_info);
+            fDngNegative.reset(read_dng(fRawStream, fDngHost.get(), fDngInfo.get()));
+            if (!fDngNegative) {
+                return kInvalidInput;
+            }
+        }
+
+        const int preferredSize = SkTMax(width, height);
+        fDngImage.reset(render_dng_for_preferred_size(preferredSize, fRawStream, fDngHost.get(),
+                                                      fDngInfo.get(), fDngNegative.get()));
+        if (!fDngImage) {
+            return kInvalidInput;
+        }
+
+        const dng_point& imageSize = fDngImage->Size();
+        if (imageSize.h != width || imageSize.v != height) {
+            return kInvalidScale;
+        }
+    }
+
+    void* dstRow = dst;
+    uint8 srcRow[width * 3];
+    SkAutoTDelete<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler(
+            SkSwizzler::kRGB, nullptr, requestedInfo, options));
+    SkASSERT(swizzler);
+    for (int i = 0; i < height; ++i) {
+        dng_pixel_buffer buffer;
+        buffer.fData = &srcRow[0];
+        buffer.fArea = dng_rect(i, 0, i + 1, width);
+        buffer.fPlane = 0;
+        buffer.fPlanes = 3;
+        buffer.fColStep = buffer.fPlanes;
+        buffer.fPlaneStep = 1;
+        buffer.fPixelType = ttByte;
+        buffer.fPixelSize = sizeof(uint8);
+        buffer.fRowStep = sizeof(srcRow);
+
+        try {
+            fDngImage->Get(buffer, dng_image::edge_zero);
+        } catch (...) {
+            return kInvalidInput;
+        }
+
+        swizzler->swizzle(dstRow, &srcRow[0]);
+        dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
+    }
+    return kSuccess;
+}
+
+SkISize SkRawCodec::onGetScaledDimensions(float desiredScale) const {
+    SkISize dim = this->getInfo().dimensions();
+    if (desiredScale >= 1.0) {

[scroggo, 2016/01/12 21:54:39]

This is redundant. We already call this in getScaledDimensions (which calls this
function - this function should not otherwise be called directly).

[yujieqin, 2016/01/13 15:29:42]

Done.

+        return dim;
+    }
+
+    if (fDngImage) {
+        SkCodecPrintf("Warning: Multiple calls to onGetScaledDimensions() for RAW image. \
+                Every call requires the image to be re-rendered.");
+    }
+
+    // SkCodec treats zero dimensional images as errors, so the minimum size
+    // that we will recommend is 1x1.
+    dim.fWidth = SkTMax(1, SkScalarRoundToInt(desiredScale * dim.fWidth));
+    dim.fHeight = SkTMax(1, SkScalarRoundToInt(desiredScale * dim.fHeight));
+
+    // DNG SDK preserves the aspect ratio, so it only needs to know the longer dimension.
+    const int preferredSize = SkTMax(dim.fWidth, dim.fHeight);
+    fDngImage.reset(render_dng_for_preferred_size(preferredSize, fRawStream, fDngHost.get(),
+                                                  fDngInfo.get(), fDngNegative.get()));
+    if (!fDngImage) {
+        return SkISize::Make(0, 0);
+    }
+
+    dng_point imageSize = fDngImage->Size();
+    dim.fWidth = imageSize.h;
+    dim.fHeight = imageSize.v;
+
+    return dim;
+}
+
+bool SkRawCodec::onDimensionsSupported(const SkISize& dim) {
+    const SkImageInfo& info = this->getInfo();
+    return dim.width() >= 1 && dim.width() <= info.width()

[scroggo, 2016/01/12 21:54:39]

This is misleading. It reports that it can support any arbitrary scale, but this
is not true. That said, the function was really put in place to early exit if
the codec knows it cannot support this scale. In your case, it seems that the
codec does not know. Perhaps we should update the header to state that it
returns false positives?

[yujieqin, 2016/01/13 15:29:41]

We now offer a more reliable check. 

+            && dim.height() >= 1 && dim.height() <= info.height();
+}
+
+SkRawCodec::~SkRawCodec() {
+    delete fRawStream;
+}
+
+SkRawCodec::SkRawCodec(const SkImageInfo& srcInfo, SkRawStream* stream, dng_host* host,
+                       dng_info* info, dng_negative* negative)
+    : INHERITED(srcInfo, nullptr)
+    , fRawStream(stream)
+    , fDngHost(host)
+    , fDngInfo(info)
+    , fDngNegative(negative) {}