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..2fd0ceb642bcf3c6538608d2bee5cd707889c302
--- /dev/null
+++ b/src/codec/SkRawCodec.cpp
@@ -0,0 +1,228 @@
+/*
+ * 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 "SkStream.h"
+#include "SkTemplates.h"
+#include "SkTypes.h"
+#include "SkSwizzler.h"
+#include "SkJpegCodec.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 <cstring>
+#include <cmath>
+#include <memory>
+#include <vector>
+
+namespace {
+
+/* 
+ * For the dng sdk stream implementation we need to provide the full
+ * length of the image file. However, that is not too bad because we
+ * would need it anyway for the dng rendering pipeline.
+ */
+class SkDngStream : public dng_stream {
+public:
+    SkDngStream(size_t size, void* data)

[scroggo, 2016/01/06 22:30:20]

nit: I think we would typically switch the order:

(void* data, size_t size)

(This also matches the order of your variables, FWIW)

[yujieqin, 2016/01/07 09:22:26]

Done.

+        : fData(data)
+        , fDataSize(size) {}
+
+    ~SkDngStream() override {}
+
+protected:
+    uint64 DoGetLength() override { return fDataSize; }
+    void DoRead(void* data, uint32 count, uint64 offset) override {
+        if (fDataSize < offset + count) {
+            ThrowReadFile();
+        }
+        memcpy(data, reinterpret_cast<const uint8*>(fData) + offset, count);
+    }
+
+private:
+    const void* fData;

[scroggo, 2016/01/06 22:30:20]

Add a comment that this is owned by the codec, rather than the stream?

On the other hand, that seems weird to me. fDngStorage is never used directly by
the codec; all it does is free it. I think it would be cleaner if the stream
classes (which actually use fData, and more logically own it) handle the free.

[yujieqin, 2016/01/07 09:22:26]

We may revisit here when we change the PiexStream implementation as planned.
Currently we need to share the data buffered from a SkStream to both DngStream
and PiexStream.

+    const size_t fDataSize;
+};
+
+class SkPiexStream : public ::piex::StreamInterface {
+   public:
+    SkPiexStream(size_t size, void* data)
+        : fData(data)
+        , fDataSize(size) {}
+
+    ::piex::Error GetData(const size_t offset, const size_t length,
+                          uint8* data) override {
+        if (offset >= fDataSize || fDataSize - offset < length) {
+          return ::piex::Error::kFail;
+        }
+        memcpy(data, reinterpret_cast<const uint8*>(fData) + offset, length);
+        return ::piex::Error::kOk;
+    }
+
+   private:
+    const void* fData;
+    const size_t fDataSize;
+};
+
+bool ReadDng(dng_stream* stream, dng_host* host, dng_info* info,
+             SkAutoTDelete<dng_negative>* negative) {

[scroggo, 2016/01/06 22:30:20]

I mentioned this before, but I didn't see any response:

Why not return a dng_negative* from this function? Then you can check whether
it's null (instead of a separate boolean).

[yujieqin, 2016/01/07 09:22:26]

Oh, we misunderstood your point before. It should be better now.

+    const uint32 kPreferredSize = 0;
+    const uint32 kMinimumSize = 0;
+    const uint32 kMaximumSize = 0;
+    try {
+        host->SetPreferredSize(kPreferredSize);
+        host->SetMinimumSize(kMinimumSize);
+        host->SetMaximumSize(kMaximumSize);
+
+        host->ValidateSizes();
+
+        // Read stage 1 image into the negative.
+        {
+            info->Parse(*host, *stream);
+            info->PostParse(*host);
+
+            if (!info->IsValidDNG()) {
+                return false;
+            }
+
+            negative->reset(host->Make_dng_negative());
+            (*negative)->Parse(*host, *stream, *info);
+            (*negative)->PostParse(*host, *stream, *info);
+        }
+
+        (*negative)->SynchronizeMetadata();
+    } catch (const dng_exception& exception) {

[msarett, 2016/01/06 22:50:25]

Can this be a single catch block?  Since both do the same thing?

[yujieqin, 2016/01/07 09:22:26]

Done.

+        return false;
+    } catch (...) {
+        return false;
+    }
+
+    return true;
+}
+
+bool PrepareStage3(dng_stream* stream, dng_host* host, dng_info* info,
+                   dng_negative* negative) {
+    if (negative->Stage3Image() == nullptr) {
+        negative->ReadStage1Image(*host, *stream, *info);
+
+        if (info->fMaskIndex != -1) {
+            negative->ReadTransparencyMask(*host, *stream, *info);
+        }
+
+        negative->ValidateRawImageDigest(*host);
+        if (negative->IsDamaged()) {
+            return false;
+        }
+
+        const int32 kMosaicPlane = -1;
+        negative->BuildStage2Image(*host);
+        negative->BuildStage3Image(*host, kMosaicPlane);
+    }
+
+    return true;
+}
+
+}  // namespace
+
+SkCodec* SkRawCodec::NewFromStream(SkStream* stream) {
+    SkAutoMalloc data;
+    const size_t dataSize = SkCopyStreamToStorage(&data, stream);
+
+    SkPiexStream piexStream(dataSize, data.get());
+    ::piex::PreviewImageData imageData;
+    if (::piex::IsRaw(&piexStream) &&
+        ::piex::GetPreviewImageData(&piexStream, &imageData) == ::piex::Error::kOk) {
+        return SkJpegCodec::NewFromStream(new SkMemoryStream(

[scroggo, 2016/01/06 22:30:20]

nit: This is hard to read since the code lines up with the condition. I think
this would be better as:

::piex::Get...)
{
    return ...
}

[yujieqin, 2016/01/07 09:22:26]

Done.

+            reinterpret_cast<const uint8*>(data.get()) + imageData.jpeg_offset,

[scroggo, 2016/01/06 22:30:20]

This should be indented 8 spaces, rather than 4

[yujieqin, 2016/01/07 09:22:26]

Done.

+            imageData.jpeg_length, true));
+    }
+
+    SkDngStream dngStream(dataSize, data.get());
+    dng_host host;
+    dng_info info;
+    SkAutoTDelete<dng_negative> negative;
+    if (!ReadDng(&dngStream, &host, &info, &negative)) {
+        return nullptr;
+    }
+
+    const SkImageInfo& imageInfo = SkImageInfo::Make(
+        negative->DefaultCropSizeH().As_real64(),
+        negative->DefaultCropSizeV().As_real64(),
+        kN32_SkColorType, kOpaque_SkAlphaType);
+    return new SkRawCodec(imageInfo, dataSize, data.detach());
+}
+
+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();
+    SkAutoTDelete<dng_image> finalDngImage;
+    try {
+	SkDngStream dngStream(fDngLength, fDngStorage);

[scroggo, 2016/01/06 22:30:20]

nit: this should be indented (although I think it would be better to factor out
the exception try/catch block).

[yujieqin, 2016/01/07 09:22:26]

Could you explain a bit more here about:
* indented
* factor out the try/catch

[scroggo, 2016/01/07 13:53:20]

try {
    SkDngStream dngStream(fDngLength, fDngStorage); // indented four spaces from
"try"
    dng_host host; // just like every other line inside this try block
I imagined a mini library that hid the try/catch from this code, and instead
returned a boolean (or a pointer to an object, with null on failure/exception).
If this (i.e. the raw_codec target) makes Derek happy, though, I'm happy, so
long as we include some build guards so a client can build without exceptions.
e.g.

#ifdef SK_CODEC_DECODES_RAW // only defined if SK_HAS_EXCEPTIONS is defined
#include "SkRawCodec.h"
#endif

...

#ifdef SK_CODEC_DECODES_RAW
codec.reset(SkRawCodec::NewFromStream(...)
#endif

etc.

[yujieqin, 2016/01/07 15:52:01]

We added the SK_CODEC_DECODES_RAW in codec.gyp:raw_codec. If one removes the
dependency to codec.gyp:raw_codec in codec.gyp:skia_codec, everything builds
fine and RAW support is disabled.

+
+        dng_host host;
+        dng_info info;
+        SkAutoTDelete<dng_negative> negative;
+        if (!ReadDng(&dngStream, &host, &info, &negative) ||
+            !PrepareStage3(&dngStream, &host, &info, negative.get())) {
+            return kInvalidInput;
+        }
+
+        dng_render render(host, *negative);
+        render.SetFinalSpace(dng_space_sRGB::Get());
+        render.SetFinalPixelType(ttByte);
+
+        finalDngImage.reset(render.Render());
+    } catch (const dng_exception& exception) {
+        return kInvalidInput;
+    } catch (...) {
+        return kInvalidInput;
+    }
+
+    void* dstRow = dst;
+    uint8 srcRow[width * 3];
+    SkAutoTDelete<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler(
+            SkSwizzler::kRGB, nullptr, requestedInfo, options));
+    if (!swizzler) {

[msarett, 2016/01/06 22:50:25]

Maybe this instead can be:
SkASSERT(swizzler);

I think that if we fail to create a swizzler here, it is due to a programming
error.

[yujieqin, 2016/01/07 09:22:26]

Done.

+        return kInvalidInput;
+    }
+    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);
+        finalDngImage->Get(buffer, dng_image::edge_zero);
+        swizzler->swizzle(dstRow, &srcRow[0]);
+        dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
+    }
+    return kSuccess;
+}
+
+SkRawCodec::~SkRawCodec() {
+  sk_free(fDngStorage);

[scroggo, 2016/01/06 22:30:20]

nit: This should be indented four spaces (although really I think fDngStorage
should be owned by the stream instead).

[yujieqin, 2016/01/07 09:22:26]

Done.

+}
+
+SkRawCodec::SkRawCodec(const SkImageInfo& srcInfo, size_t length, void* storage)
+    : INHERITED(srcInfo, nullptr), fDngLength(length), fDngStorage(storage) {}