Prototype of RAW decoding in Skia.

src/codec/SkRawCodec.cpp

+/*
+ * 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 <cmath>
+#include <cstring>
+#include <limits>
+#include <memory>
+#include <vector>

[scroggo, 2016/01/08 17:12:25]

Why do you need to include <vector>? I don't see any vectors?

Which makes me take another look at the rest of your includes. Do you need
cmath? We have a lot of our own functions in SkMath.h. Maybe you can use code
already supplied by Skia?

string.h is included by SkTypes. Do you need cstring here?

[yujieqin, 2016/01/11 14:03:07]

Done.

+
+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()) {
+            *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;
+    }
+}
+
+dng_image* render_dng_for_prefered_size(const int prefered_size, dng_stream* stream, 

[scroggo, 2016/01/11 17:44:02]

Can you add comments explaining what this does, how the parameters are used, and
why it might fail?

A comment explaining what preferred_size means (the size of the longer side out
of W and H, after scaling) would also be nice.

Also, "prefered" is spelled incorrectly, here and elsewhere. It should be
"preferred".

One final nit: variables should be camelCase, so this parameter should be
"preferredSize" (unlike the name of the method, which is correctly
using_underscores).

[yujieqin, 2016/01/12 10:47:40]

Done.

+                                        dng_host* host, dng_info* info, dng_negative* negative) {
+    try {
+        host->SetPreferredSize(prefered_size);
+        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), fStreamBufferSize(0) {}
+
+    ~SkRawStream() override {}
+
+    // Creates a SkMemoryStream from the offset with size.
+    SkMemoryStream* copyBuffer(size_t offset, size_t size) {
+        size_t sum;
+        if (!safe_add_to_size_t(offset, size, &sum) ||

[scroggo, 2016/01/08 17:12:25]

nit: I think these can fit on one line?

if (!safe_add_to_size_t || !bufferMoreData(sum)) {

[yujieqin, 2016/01/11 14:03:07]

Done.

+            !bufferMoreData(sum)) {
+            return nullptr;
+        }
+        return new SkMemoryStream(fStreamBuffer.get() + offset, size, true);
+    }
+
+    // For PIEX
+    ::piex::Error GetData(const size_t offset, const size_t length,
+                          uint8* data) override {
+        size_t sum;
+        if (!safe_add_to_size_t(offset, length, &sum) ||

[scroggo, 2016/01/08 17:12:25]

one line?

[yujieqin, 2016/01/11 14:03:07]

Done.

+            !bufferMoreData(sum)) {

[scroggo, 2016/01/08 17:12:25]

Sorry, something else I missed on previous reviews: in Skia, we use the this
pointer when calling member functions on the same object, e.g.

!this->bufferMoreData(sum)

here and elsewhere.

[yujieqin, 2016/01/11 14:03:08]

Done.

+          return ::piex::Error::kFail;
+        }
+        memcpy(data, fStreamBuffer.get() + offset, length);
+        return ::piex::Error::kOk;
+    }
+
+protected:
+    // For dng_stream
+    uint64 DoGetLength() override {
+        if (!bufferMoreData(0, true)) {  // read whole stream
+            ThrowReadFile();
+        }
+        return fStreamBufferSize;
+    }
+
+    // For dng_stream
+    void DoRead(void* data, uint32 count, uint64 offset) override {
+        size_t sum;
+        if (!safe_add_to_size_t(static_cast<uint64>(count), offset, &sum) ||
+            !bufferMoreData(sum)) {
+            ThrowReadFile();
+        }
+        memcpy(data, fStreamBuffer.get() + offset, count);
+    }
+
+private:
+    // Note: if the readToEnd is "true", the newSize will be basically ignored in this function.
+    bool bufferMoreData(size_t newSize, bool readToEnd = false) {

[scroggo, 2016/01/08 17:12:26]

Instead of having two mutually-exclusive parameters, why not make newSize == 0 a
sentinel that means readToEnd?

You could even define a constant:

// Sentinel to tell bufferMoreData to buffer the entire stream.
const size_t kReadToEnd = 0;

// If newSize is kReadToEnd, buffer the entire stream.
bool bufferMoreData(size_t newSize) {
  if (kReadToEnd == newSize) {
     ...
  }
  ...
}

// elsewhere:
this->bufferMoreData(kReadToEnd)

[yujieqin, 2016/01/11 14:03:08]

Done.

+        if (readToEnd) {
+            if (fWholeStreamRead) {  // the stream is already read-to-end.
+                return true;
+            }
+
+            SkDynamicMemoryWStream tempStream;
+            if (!SkStreamCopy(&tempStream, fStream.get())) {
+                return false;
+            }
+
+            size_t wholeStreamSize;
+            if (!safe_add_to_size_t(fStreamBufferSize, tempStream.bytesWritten(), 
+                                    &wholeStreamSize)) {
+                return false;
+            }
+
+            fStreamBuffer.realloc(wholeStreamSize);
+            uint8_t* appendDst = fStreamBuffer.get() + fStreamBufferSize;
+            tempStream.copyTo(appendDst);
+            fStreamBufferSize = wholeStreamSize;

[scroggo, 2016/01/08 17:12:26]

I think you can

  return true;

here. Then you do not need the else statement.

[yujieqin, 2016/01/11 14:03:07]

Done.

+        } else {
+            if (newSize <= fStreamBufferSize) {  // the stream is already buffered to newSize.
+                return true;
+            }
+            if (fWholeStreamRead) {  // newSize is larger than the whole stream.
+                return false;
+            }
+
+            fStreamBuffer.realloc(newSize);
+            uint8_t* appendDst = fStreamBuffer.get() + fStreamBufferSize;
+            const size_t sizeToRead = newSize - fStreamBufferSize;
+            const size_t bytesRead = fStream->read(appendDst, sizeToRead);
+            if (bytesRead < sizeToRead) {
+                return false;
+            }
+            fStreamBufferSize = newSize;
+        }
+        return true;
+    }
+
+    SkAutoTDelete<SkStream> fStream;
+    bool fWholeStreamRead;
+
+    SkAutoTMalloc<uint8> fStreamBuffer;
+    size_t fStreamBufferSize;
+};
+
+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;
+    }
+
+    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(
+        negative->DefaultCropSizeH().As_real64(),
+        negative->DefaultCropSizeV().As_real64(),
+        kN32_SkColorType, kOpaque_SkAlphaType);
+    return new SkRawCodec(imageInfo, rawStream.detach(), host.release(), info.release(), negative.release());

[msarett, 2016/01/08 14:38:34]

nit: Line is too long

[scroggo, 2016/01/08 17:12:25]

Also:

For historical reasons, SkAutoTDelete has two methods which do the same thing:
detach() and release().

(The history: SkAutoTDelete is older than unique_ptr, and we chose a different
name from the eventual unique_ptr. We added release() when we implemented it
with unique_ptr, and I think we'll eventually switch.)

I don't have a strong preference between the two (although maybe we should go
with release() for new code, to facilitate an eventual removal), but I think you
should be consistent on a single line of code. 

[yujieqin, 2016/01/11 14:03:07]

Done.

+}
+
+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) {

[msarett, 2016/01/08 14:38:34]

I think here, you are assuming that getScaledDimensions() has already been
called, and then the client is calling getPixels() with the same size returned
by getScaledDimensions()?

In that case, I think this will work.

However, the client is not required to use the size returned by
getScaledDimensions().  I think this will break if the client calls
getScaledDimensions(), but then decides to call getPixels() on a different size.

I'm in favor of not repeating the render_dng_for_prefered_size() call, but to do
that, I think you'll need to make sure the current size of fDngImage matches the
request.

[yujieqin, 2016/01/11 14:03:07]

Done.

[scroggo, 2016/01/11 17:44:02]

How was this fixed?

[adaubert, 2016/01/11 17:57:05]

It was fixed in the way that the size of the image rendered by the
render_dng_for_prefered_size will be checked against the requested size. If the
sizes are different the error kInvalidScale will be returned.

* So if the caller called getScaledDimensions() before and then calls
onGetPixels() with exactly these size, it will work.
* If the caller called onGetPixels() directly, by requesting random sizes (so
not calling getScaledDimensions() before, or not calling  onGetPixels() with the
100% size), then the fDngImage will be created on the fly by trying to match the
requested size. If the requested size does not match the rendered size, then an
error will be returned.

[msarett, 2016/01/11 19:12:44]

Ah I see.  This is not quite enough though.

After the client calls getScaledDimensions(), they should still be able to call
getPixels() on *any* valid size and get the correct result.

For example, maybe they don't like the recommended scaled dimensions, and decide
to just decode to the original size.  Or maybe they will call
getScaledDimensions() more than once, and then choose whichever size they like
best.

Essentially, onGetScaledDimensions() should behave as if it is truly const.  I
am noticing now that you have marked some fields as mutable.  I think this is
fine, as long as they are only used to cache the result of
onGetScaledDimensions() (in case it should match the request to onGetPixels()).

On the other hand, I would not say you *must* cache it.  I am fine with doing
the scaling calculation twice.

[yujieqin, 2016/01/12 10:47:40]

We prefer the caller use the size from onGetScaledDimensions(), because this
makes the process much faster (in such case, we only parse and render the image
once). Anyhow, we add the functionality to also support different size requests,
but this will cause multiple re-parsing and re-rendering.

Some more info:
The only reason to render the image already in onGetScaledDimensions() is: we
can only know the valid scaled image size after rendering. (due to the way the
preferred size is handled by DNG SDK.)

[msarett, 2016/01/12 14:01:29]

Ok thanks I understand.  Maybe returning kInvalidInput is a good way to prevent
from the client from doing extra work?  What do you think Leon?

I think I would still prefer being more flexible but add warnings:
<on call to onGetPixels() with dimensions that don't match
onGetScaledDimensions()>
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.")
(if the requested size is unsupported, this should still return kInvalidScale
later)

<on multiple calls to onGetScaledDimensions()>
SkCodecPrintf("Warning: Multiple calls to onGetScaledDimensions() for RAW image.
 Every call requires the image to be re-rendered.")

(no need to quote me exactly on these message if you have something more useful
to say :))

[yujieqin, 2016/01/12 15:11:09]

Added the warnings.

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

Right, but this is a virtual function, and other SkCodecs expect to be able to
check a couple of times, and potentially use a different scale. FWIW,
SkImageGenerator built on the SkCodec API, and has the method
computeScaledDimensions (introduced in crrev.com/1396323007) which returns up to
two options for supported dimensions. It is possible we'll convert SkCodec's API
to match this one. If we cannot update the underlying libraries to do the check
more efficiently (is that not possible?), I suppose we'll just have to return
one possible set of supported dimensions, in order to run faster and throw away
less work. Do you have a rough idea of how long it takes to call
getScaledDimensions? 
So this means that the fully decoded raw image is stored in the SkCodec? How big
is a typical DngImage? SkCodec was designed around the idea that it could be
used for a "lazy" decode - we only decode enough initially to know the size, and
then when the client wants the actual image (e.g. at draw time, or in a
background thread) they can call getPixels. But this means that we may keep
around the DngImage at times when we do not need it.
Did your team also write the DNG SDK? Is it possible to change that restriction?

I think with the current implementation, we should also put a note on
getScaledDimensions, that it is an expensive call (which holds on to a lot of
RAM) for these images.

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

We refactor the whole logic of scaling DNG images. We now do cheap computation
for onGetScaledDimensions() and made the onDimensionsSupported() more reliable.
The heavy work is now done only in onGEtPixels(). This change also implies the
rendering of DNG image is done "lazy", only in onGetPixels().

The DNG SDK is from Adobe. We think our new solution for handling scale request
should be okay.

Will the onGetPixels() be called multiple time with the same resolution for our
use case? If not, we can even clean up all the DNG stuff (free up memory) except
the rawStream and recreate everything from the rawStream if onGetPixels() get
called again.

[msarett, 2016/01/13 17:15:14]

Good question.  I believe it might.  Certainly we have designed to make that use
case possible, but I'm not sure how frequent it is.  Leon may know more.

[scroggo, 2016/01/13 18:24:44]

In the current common use case, Android will only call onGetPixels once and then
throw away the codec.
I think that makes the most sense. The only reason to call onGetPixels again
later is if we threw away the output in order to save memory. Hanging on to the
fDngImage seems to defeat that purpose.

[yujieqin, 2016/01/14 09:33:11]

Optimized the workflow for the that onGetPixels() will be called only once.
(But one can still call onGetPixels() multiple time, but it is not efficient.)

[scroggo, 2016/01/14 15:26:37]

Yeah, I think it's fine that it's not efficient. Again, the only reason to call
it again is because we wanted to free up memory. Caching the (larger) dng_image
defeats that purpose.

+        const int preferedSize = SkTMax(width, height);
+        fDngImage.reset(render_dng_for_prefered_size(preferedSize, fRawStream, fDngHost.get(),
+                                                     fDngInfo.get(), fDngNegative.get()));
+        if (!fDngImage) {
+            return kInvalidInput;

[scroggo, 2016/01/08 17:12:26]

Arguably this could be kInvalidScale, although I don't know if you can tell the
difference.

[yujieqin, 2016/01/11 14:03:07]

We think "kInvalidInput" should be correct for this case, because the ckeck for
the size is done below.

[scroggo, 2016/01/11 17:44:02]

So will render_dng_for_prefered_size succeed even if it's given a bad scale
value? (Or is there such thing? Will it always succeed if the input data is
valid?)

[adaubert, 2016/01/11 17:57:05]

It will always succeed if the input data is valid.
The preferredSize is only a "wish" and the dng_sdk tries to make it, but it does
not guarantee that preferredSize will be delivered.

+        }
+    }
+
+    SkDEBUGCODE(dng_point imageSize = fDngImage->Size();)
+    SkASSERT(imageSize.h == width);
+    SkASSERT(imageSize.v == height);
+
+    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();
+    // 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));
+
+    const int preferedSize = SkTMax(dim.fWidth, dim.fHeight);

[msarett, 2016/01/08 14:38:34]

Just for my curiosity, why does dng only need to know the max scaled dimension?

[yujieqin, 2016/01/11 14:03:07]

The DNG SDK only need the long edge size, because it always preservers the
aspect ratio.

[scroggo, 2016/01/11 17:44:02]

Good to know. Can you add a comment? Matt and I will need to maintain this code
in Skia, and that will help us understand what's going on the next time we look
at this code.

Something like:

// DNG SDK preserves the aspect ratio, so it only needs to know the longer
dimension.

[yujieqin, 2016/01/12 10:47:40]

Comment added.

FYI, for maintaining the RAW related code in skia (also the deps to DNG SDK and
PIEX), we also commit to do that.

+    fDngImage.reset(render_dng_for_prefered_size(preferedSize, fRawStream, fDngHost.get(),
+                                                 fDngInfo.get(), fDngNegative.get()));
+    if (!fDngImage) {
+      return this->getInfo().dimensions();

[msarett, 2016/01/08 14:38:34]

In general, if the exact requested scale is not supported, we try to suggest a
scale that is supported and is as close as we can get to the request.

When does "render_dng_for_prefered_size" fail?  Is this an error case?  Or can
we choose a closer match for the request?

Based on your implementation of onDimensionsSupported(), I think this is an
error case (so maybe the current implementation is fine?  or maybe we should
return (0, 0)?)

[scroggo, 2016/01/08 17:12:25]

(On another note, this should be indented four spaces.)

[yujieqin, 2016/01/11 14:03:07]

Yes, this is actually an error case.

[yujieqin, 2016/01/11 14:03:08]

Done.

+    }
+
+    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()
+            && dim.height() >= 1 && dim.height() <= info.height();
+}
+
+SkRawCodec::~SkRawCodec() {
+    delete fRawStream;
+}
+
+SkRawCodec::SkRawCodec(const SkImageInfo& srcInfo, SkRawStream* stream, dng_host* host, dng_info* info,

[msarett, 2016/01/08 14:38:34]

nit: Line greater than 100 chars

[yujieqin, 2016/01/11 14:03:07]

Done.

+                       dng_negative* negative)
+    : INHERITED(srcInfo, nullptr), fRawStream(stream), fDngHost(host), fDngInfo(info), fDngNegative(negative) {}

[msarett, 2016/01/08 14:38:34]

nit: Line greater than 100 chars

[scroggo, 2016/01/08 17:12:25]

FWIW, in Skia we often will put each member on its own line, e.g.

SkRawCodec::SkRawCodec(...)
    : INHERITED(srcInfo, nullptr)
    , fRawStream(stream)
    , ...

[yujieqin, 2016/01/11 14:03:07]

Done.

[yujieqin, 2016/01/11 14:03:08]

Done.