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..b44755e4e324ecbe4acda23b187e04ed46295e30
--- /dev/null
+++ b/src/codec/SkRawCodec.cpp
@@ -0,0 +1,450 @@
+/*
+ * 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 "SkCodecPriv.h"
+#include "SkColorPriv.h"
+#include "SkData.h"
+#if !defined(GOOGLE3)
+#include "SkJpegCodec.h"
+#endif
+#include "SkRawCodec.h"
+#include "SkRefCnt.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_memory.h"
+#include "dng_render.h"
+#include "dng_stream.h"
+
+#include "src/piex.h"
+
+#include <cmath>  // for std::round,floor,ceil
+#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()) {
+            *result = static_cast<size_t>(sum);
+            return true;
+        }
+    }
+    return false;
+}
+
+class SkDngMemoryAllocator : public dng_memory_allocator {
+public:
+    virtual ~SkDngMemoryAllocator() {}

[scroggo, 2016/01/20 18:13:36]

nit: I think we generally drop the virtual and use override:

    ~SkDngMemoryAllocator() override

[yujieqin, 2016/01/20 21:42:19]

Done.

+
+    dng_memory_block* Allocate(uint32 size) override {
+        // To avoid arbitary allocation requests which might lead to out-of-memory, limit the
+        // amount of memory that can be allocated at once. The memory limit is based on experiments
+        // and supposed to be sufficient for all valid DNG images.
+        if (size > 300 * 1024 * 1024) {  // 300 MB
+            ThrowMemoryFull();
+        }
+        return dng_memory_allocator::Allocate(size);
+    }
+};
+
+}  // 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 an SkMemoryStream from the offset with size.
+     * Note: for performance reason, this function is destructive to the SkRawStream. One should
+     *       abandon current object after the function call.
+     */
+    SkMemoryStream* transferBuffer(size_t offset, size_t size) {
+        SkAutoTUnref<SkData> data(SkData::NewUninitialized(size));
+        if (offset > fStreamBuffer.bytesWritten()) {
+            // If the offset is not buffered, read from fStream directly and skip the buffering.
+            const size_t skipLength = offset - fStreamBuffer.bytesWritten();
+            if (fStream->skip(skipLength) != skipLength) {
+                return nullptr;
+            }
+            const size_t bytesRead = fStream->read(data->writable_data(), size);
+            if (bytesRead < size) {
+                data.reset(SkData::NewSubset(data.get(), 0, bytesRead));
+            }
+        } else {
+            const size_t alreadyBuffered = SkTMin(fStreamBuffer.bytesWritten() - offset, size);
+            if (alreadyBuffered > 0 &&

[scroggo, 2016/01/20 18:13:36]

It looks like we forgot to consider the case where offset ==
fStreamBuffer.bytesWritten(), making alreadyBuffered zero. Did this cause a
crash?

If so, that seems like a bug in SkDynamicMemoryWStream. I think it's fine to
still have this workaround - maybe that class has a reason not to be robust? -
but maybe file a bug and reference it here?

[yujieqin, 2016/01/20 21:42:20]

Yes, it seems like the SkDynamicMemoryWStream.read() will return "false" if the
requested "size  == 0". There is no crash. But we still need to distinguish this
case and the normal read failure,  that's why I add this workaround.

As I don't have enough knowledge about Skia framework yet, I am not sure if it
is a bug or work as expected.

[scroggo, 2016/01/21 16:36:09]

I filed skbug.com/4836 to investigate.

+                !fStreamBuffer.read(data->writable_data(), offset, alreadyBuffered)) {
+                return nullptr;
+            }
+
+            const size_t remaining = size - alreadyBuffered;
+            if (remaining) {
+                auto* dst = static_cast<uint8_t*>(data->writable_data()) + alreadyBuffered;
+                const size_t bytesRead = fStream->read(dst, remaining);
+                size_t newSize;
+                if (bytesRead < remaining) {
+                    if (!safe_add_to_size_t(alreadyBuffered, bytesRead, &newSize)) {
+                        return nullptr;
+                    }
+                    data.reset(SkData::NewSubset(data.get(), 0, newSize));
+                }
+            }
+        }
+        return new SkMemoryStream(data);
+    }
+
+    // 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) {  // 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()) {  // already buffered to newSize
+            return true;
+        }
+        if (fWholeStreamRead) {  // newSize is larger than the whole stream.
+            return false;
+        }
+
+        const size_t sizeToRead = newSize - fStreamBuffer.bytesWritten();
+        SkAutoTMalloc<uint8> tempBuffer(sizeToRead);
+        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;
+};
+
+class SkDngImage {
+public:
+    static SkDngImage* NewFromStream(SkRawStream* stream) {
+        SkAutoTDelete<SkDngImage> dngImage(new SkDngImage(stream));
+        if (!dngImage->readDng()) {
+            return nullptr;
+        }
+
+        SkASSERT(dngImage->fNegative);
+        return dngImage.release();
+    }
+
+    /*
+     * Renders the DNG image to the size. The DNG SDK only allows scaling close to integer factors
+     * down to 80 pixels on the short edge. The rendered image will be close to the specified size,
+     * but there is no guarantee that any of the edges will match the requested size. E.g.
+     *   100% size:              4000 x 3000
+     *   requested size:         1600 x 1200
+     *   returned size could be: 2000 x 1500
+     */
+    dng_image* render(int width, int height) {
+        // render() takes ownership of fHost, fInfo and fNegative when available.
+        SkAutoTDelete<dng_host> host;
+        SkAutoTDelete<dng_info> info;
+        SkAutoTDelete<dng_negative> negative;
+        if (!fHost || !fInfo || !fNegative) {
+            SkCodecPrintf("Warning: SkDngImage::render() is called multiple times.");

[scroggo, 2016/01/20 18:13:36]

I don't think this print statement is still necessary. I believe Matt originally
suggested it because this method was called by both onGetScaledDimensions and
onGetPixels. But now that it's only called by onGetPixels, this just means that
the client discarded their copy of the final output to save memory. So even
though this is going to be slow, I don't think the client should be doing
anything smarter to avoid it.

[yujieqin, 2016/01/20 21:42:19]

Removed.

+            if (!this->readDng()) {
+                return nullptr;
+            }
+        } else {
+            host.reset(fHost.release());
+            info.reset(fInfo.release());
+            negative.reset(fNegative.release());
+        }
+
+        // DNG SDK preserves the aspect ratio, so it only needs to know the longer dimension.
+        const int preferredSize = SkTMax(width, height);
+        try {
+            host->SetPreferredSize(preferredSize);
+            host->ValidateSizes();
+
+            negative->ReadStage1Image(*host, *fStream, *info);
+
+            if (info->fMaskIndex != -1) {
+                negative->ReadTransparencyMask(*host, *fStream, *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;
+        }
+    }
+
+    const SkImageInfo& getImageInfo() const {
+        return fImageInfo;
+    }
+
+    bool isXtransImage() const {
+        return fIsXtransImage;
+    }
+
+private:
+    bool readDng() {
+        // Due to the limit of DNG SDK, we need to reset host and info.
+        fHost.reset(new dng_host(&fAllocator));
+        fInfo.reset(new dng_info);
+        try {
+            fHost->ValidateSizes();
+            fInfo->Parse(*fHost, *fStream);
+            fInfo->PostParse(*fHost);
+            if (!fInfo->IsValidDNG()) {
+                return false;
+            }
+
+            fNegative.reset(fHost->Make_dng_negative());
+            fNegative->Parse(*fHost, *fStream, *fInfo);
+            fNegative->PostParse(*fHost, *fStream, *fInfo);
+            fNegative->SynchronizeMetadata();
+
+            fImageInfo = SkImageInfo::Make(fNegative->DefaultCropSizeH().As_real64(),
+                                           fNegative->DefaultCropSizeV().As_real64(),
+                                           kN32_SkColorType, kOpaque_SkAlphaType);
+            fIsXtransImage = fNegative->GetMosaicInfo() != nullptr 
+                ? (fNegative->GetMosaicInfo()->fCFAPatternSize.v == 6
+                   && fNegative->GetMosaicInfo()->fCFAPatternSize.h == 6)
+                : false;
+            return true;
+        } catch (...) {
+            fNegative.reset(nullptr);
+            return false;
+        }
+    }
+
+    SkDngImage(SkRawStream* stream)
+        : fStream(stream) {}
+
+    SkDngMemoryAllocator fAllocator;
+    SkAutoTDelete<SkRawStream> fStream;
+    SkAutoTDelete<dng_host> fHost;
+    SkAutoTDelete<dng_info> fInfo;
+    SkAutoTDelete<dng_negative> fNegative;
+
+    SkImageInfo fImageInfo;
+    bool fIsXtransImage;
+};
+
+/*
+ * Tries to handle the image with PIEX. If PIEX returns kOk and finds the preview image, create a
+ * SkJpegCodec. If PIEX returns kFail, then the file is invalid, return nullptr. In other cases,
+ * fallback to create SkRawCodec for DNG images.
+ */
+SkCodec* SkRawCodec::NewFromStream(SkStream* stream) {
+    SkAutoTDelete<SkRawStream> rawStream(new SkRawStream(stream));
+    ::piex::PreviewImageData imageData;
+    if (::piex::IsRaw(rawStream.get())) {
+        ::piex::Error error = ::piex::GetPreviewImageData(rawStream.get(), &imageData);

[msarett, 2016/01/20 20:23:17]

This has the potential to "reallocate" the SkRawStream->fStreamBuffer over and
over again.  See the sequence of calls from Canon_5D2.dng:

::piex::IsRaw
bufferMoreData 5000
::piex::GetPreviewImageData
bufferMoreData 108308
bufferMoreData 108310
bufferMoreData 108312
bufferMoreData 108316
bufferMoreData 108322
bufferMoreData 108324
bufferMoreData 108328
bufferMoreData 108334
bufferMoreData 108336
bufferMoreData 108340
bufferMoreData 108346
bufferMoreData 108348
bufferMoreData 108352
bufferMoreData 108358
bufferMoreData 108360
bufferMoreData 108364
bufferMoreData 108366
bufferMoreData 108370
bufferMoreData 108372
bufferMoreData 108376
bufferMoreData 108378
bufferMoreData 108382
bufferMoreData 108384
bufferMoreData 108388
bufferMoreData 108394
bufferMoreData 108396
bufferMoreData 108400
bufferMoreData 108406
bufferMoreData 108408
bufferMoreData 108412
bufferMoreData 108418
bufferMoreData 108420
bufferMoreData 108424
bufferMoreData 108430
bufferMoreData 108432
bufferMoreData 108436
bufferMoreData 108442
bufferMoreData 108444
bufferMoreData 108448
bufferMoreData 108454
bufferMoreData 108456
bufferMoreData 108460
bufferMoreData 108466
bufferMoreData 108468
bufferMoreData 108472
bufferMoreData 108478
bufferMoreData 108480
bufferMoreData 108484
bufferMoreData 108490
bufferMoreData 108492
bufferMoreData 108496
bufferMoreData 108502
bufferMoreData 108504
bufferMoreData 108508
bufferMoreData 108514
bufferMoreData 108516
bufferMoreData 108520
bufferMoreData 108526
bufferMoreData 108528
bufferMoreData 108532
bufferMoreData 108538
bufferMoreData 108540
bufferMoreData 108544
bufferMoreData 108550
bufferMoreData 108552
bufferMoreData 108556
bufferMoreData 108562
bufferMoreData 108564
bufferMoreData 108568
bufferMoreData 108572
bufferMoreData 111464
bufferMoreData 111498
bufferMoreData 111500
bufferMoreData 111502
bufferMoreData 111506
bufferMoreData 111512
bufferMoreData 111514
bufferMoreData 111518
bufferMoreData 111524
bufferMoreData 111526
bufferMoreData 111530
bufferMoreData 111536
bufferMoreData 111538
bufferMoreData 111542
bufferMoreData 111548
bufferMoreData 111550
bufferMoreData 111554
bufferMoreData 111556
bufferMoreData 111560
bufferMoreData 111562
bufferMoreData 111566
bufferMoreData 111568
bufferMoreData 111572
bufferMoreData 111574
bufferMoreData 111578
bufferMoreData 111582
bufferMoreData 111584
bufferMoreData 111586
bufferMoreData 111590
bufferMoreData 111596
bufferMoreData 111598
bufferMoreData 111602
bufferMoreData 111608
bufferMoreData 111610
bufferMoreData 111614
bufferMoreData 111618
bufferMoreData 111620
bufferMoreData 111622
bufferMoreData 111626
bufferMoreData 111632
bufferMoreData 111634
bufferMoreData 111638
bufferMoreData 111644
bufferMoreData 111646
bufferMoreData 111650
bufferMoreData 111656
bufferMoreData 111658
bufferMoreData 111662
bufferMoreData 111668
bufferMoreData 111670
bufferMoreData 111674
bufferMoreData 111680
bufferMoreData 111682
bufferMoreData 111686
bufferMoreData 111692
bufferMoreData 111694
bufferMoreData 111698
bufferMoreData 111704
bufferMoreData 111706
bufferMoreData 111710
bufferMoreData 111716
bufferMoreData 111718
bufferMoreData 111722
bufferMoreData 111728
bufferMoreData 111730
bufferMoreData 111734
bufferMoreData 111742
bufferMoreData 111892
bufferMoreData 111894
bufferMoreData 111896
bufferMoreData 111900
bufferMoreData 111906
bufferMoreData 111908
bufferMoreData 111912
bufferMoreData 111918
bufferMoreData 111920
bufferMoreData 111924
bufferMoreData 111930
bufferMoreData 111932
bufferMoreData 111936
bufferMoreData 111942
bufferMoreData 111944
bufferMoreData 111948
bufferMoreData 111950
bufferMoreData 111954
bufferMoreData 111956
bufferMoreData 111960
bufferMoreData 111962
bufferMoreData 111966
bufferMoreData 111968
bufferMoreData 111972
bufferMoreData 111976
bufferMoreData 111978
bufferMoreData 111980
bufferMoreData 111984
bufferMoreData 111990
bufferMoreData 111992
bufferMoreData 111996
bufferMoreData 112002
bufferMoreData 112004
bufferMoreData 112008
bufferMoreData 112012
bufferMoreData 112014
bufferMoreData 112016
bufferMoreData 112020
bufferMoreData 112026
bufferMoreData 112028
bufferMoreData 112032
bufferMoreData 112038
bufferMoreData 112040
bufferMoreData 112044
bufferMoreData 112050
bufferMoreData 112052
bufferMoreData 112056
bufferMoreData 112062
bufferMoreData 112064
bufferMoreData 112068
bufferMoreData 112074
bufferMoreData 112076
bufferMoreData 112080
bufferMoreData 112086
bufferMoreData 112088
bufferMoreData 112092
bufferMoreData 112098
bufferMoreData 112100
bufferMoreData 112104
bufferMoreData 112110
bufferMoreData 112112
bufferMoreData 112116
bufferMoreData 112122
bufferMoreData 112124
bufferMoreData 112128
bufferMoreData 112136
bufferMoreData 112286
bufferMoreData 112288
bufferMoreData 112290
bufferMoreData 112294
bufferMoreData 112298
bufferMoreData 112610
bufferMoreData 112618
bufferMoreData 112638
bufferMoreData 112698


It's not so bad because the implementation makes good use of a linked list, but
it's not great either.

[yujieqin, 2016/01/20 21:42:20]

I am also aware of this issue. There is surely still room for improvement. As we
did not see big issue so far, I would propose to keep it as it is, and try to
optimize here later. FIXME added.

+
+        if (error == ::piex::Error::kOk && imageData.preview_length > 0) {

[msarett, 2016/01/20 20:23:17]

Bad Case Stream Memory Use for Raw Image with Piex Preview:

Nikon_P330.NRW

Original Stream: 27 MB

Assume that the Java client buffers the stream in order to call:
BitmapFactory.decodeBounds()
rewind()
BitmapFactory.decode()

Java Buffer for Stream: 27 MB

SkRawStream will buffer the original stream as it reads up to the preview:
SkRawStream Buffer: 24.5 MB (often this is less - this is a bad case example)

Then we will copy to an SkMemoryStream for a Jpeg decode:
SkMemoryStream: 2.5 MB

So we will peak at about 81 MB (immediately before the SkRawStream is deleted).

Is this acceptable?  Can we do better?  Seems like it would be useful to know
whether or not there is a preview as soon as possible from Piex.  Because, if
there is a preview, maybe we can stop/limit our buffering?

[yujieqin, 2016/01/20 21:42:20]

The position of embedded preview is unpredictable (every camera can do what it
what). So there is not guarantee we can win from such change as you mentioned.
As said, we may investigate later to see if we can improve some formats. But so
far, as we tested in the head of Android, we don't see memory issue.

[msarett, 2016/01/21 15:46:16]

Acknowledged.

[scroggo, 2016/01/21 16:36:09]

We've already done something to help this problem - transferBuffer will skip to
reach preview_offset and copy directly to the output rather than buffering. But
I don't know whether GetPreviewImageData reads all the way to that offset
anyway, in which case, yes, we would be buffering all that data unnecessarily.

[msarett, 2016/01/21 16:48:49]

Good point.

Unfortunately, in my testing, it looks like Piex generally needs to read up to
the Jpeg preview (and the start of the jpeg - maybe just the header) before
being done.

+#if !defined(GOOGLE3)
+            // transferBuffer() is destructive to the rawStream. Abandon the rawStream after this
+            // function call.
+            SkMemoryStream* memoryStream =
+                    rawStream->transferBuffer(imageData.preview_offset, imageData.preview_length);

[msarett, 2016/01/20 20:23:17]

This operation involves allocating a new 1-4 MB buffer for the Jpeg stream, and
then copying 1-4 MB from the the fStreamBuffer into the Jpeg stream.

The creation of this new stream is followed by the deletion of the old stream
(generally much larger than 1-4 MB).

In general, I guess this is a memory win.  But it often involves a large
allocation to buffer something that is potentially already a buffer?

I think this should be cleaned up, but is quite complicated by:
(1) How much of the SkRawStream is buffered?
(2) Is the original SkStream a memory stream or something less versatile?

I think this is fine for now.  But can we add a FIXME?

[scroggo, 2016/01/20 20:36:24]

(1) Not sure if this is where you were going with this (I think so...) but if
the raw stream is buffered into chunks (which it is) we could just reuse the
chunks that intersect the data. I'm not sure SkDynamicMemoryWStream is that
flexible, though. But we could potentially add that.
(2) If the original was an SkMemoryStream (we don't know, and it generally will
not be on Android, unless we make it), we could ref it to avoid the new
allocation and copy - at the expense of holding onto more memory.

[msarett, 2016/01/20 20:43:52]

(1) Agreed.
(2) Seems like a bad idea to hold onto more memory - the stream is often much
larger than 4 MB.  I was thinking maybe realloc()?  But I don't think this can
work if we need to start at an offset of the original ptr.  Maybe there is
nothing we can do here?

[yujieqin, 2016/01/20 21:42:20]

Added FIXME. But, as said, the location of the embedded image is quite various,
we may not win too much here in general cases.

[msarett, 2016/01/21 15:46:16]

Acknowledged.

+            return memoryStream ? SkJpegCodec::NewFromStream(memoryStream) : nullptr;
+#else
+            return nullptr;
+#endif
+        } else if (error == ::piex::Error::kFail) {
+            return nullptr;
+        }
+    }
+
+    SkAutoTDelete<SkDngImage> dngImage(SkDngImage::NewFromStream(rawStream.release()));

[msarett, 2016/01/20 20:23:17]

It looks to me like ::piex::IsRaw() returns true for all RAW images (even the
DNGs that don't have a preview).

Can we move this code inside that "if" statement and return nullptr here?  That
way we only have to read 5000 bytes to determine success/failure.

This will prevent us from the reading an entire, arbitrarily long garbage stream
before realizing that we don't have a RAW image.

[yujieqin, 2016/01/20 21:42:20]

Similar to the type checks in SkCodec.cpp, the ::piex::IsRaw() only offers a
quick and dirty check, which is not 100% accurate. We did know some cases the
PIEX can not recognize DNG.

Instead, DNG SDK is dedicated for DNG format, it uses more complicated
validation. That is why I put it after the general PIEX cases. So that we can
avoid missing support for some DNGs.

[msarett, 2016/01/21 15:46:16]

Understood.

Would it be possible to make it a long term goal to extend Piex/DNG_SDK to be
able to recognize all relevant RAW images without reading the entire stream?  Do
some DNGs really not indicate that they are DNGs until the end of the stream?

[scroggo, 2016/01/21 16:36:10]

FWIW, SkCodec.cpp's type checks may offer false positives (e.g. first few bytes
looked like a PNG, but the rest is garbage). It sounds like ::piex::IsRaw also
has false negatives.

[yujieqin, 2016/01/21 17:44:26]

Unfortunately, yes. Due to the variations of RAW formats and to balance the
performance, ::piex::IsRaw() has false negatives.

+    if (!dngImage) {
+        return nullptr;
+    }
+
+    return new SkRawCodec(dngImage.release());

[msarett, 2016/01/20 20:23:17]

Bad Case Memory Use for Dng Image without Preview:

HTC.dng

Original Stream: 40 MB

Assume that the Java client buffers the stream in order to call:
BitmapFactory.decodeBounds()
rewind()
BitmapFactory.decode()

Java Buffer for Stream: 40 MB

SkRawStream Buffer for Stream: 40 MB

This peaks at about 120 MB.  This is just the stream (not including the memory
for the output image).

SkRawStream is forced to immediately buffer the whole stream because GetLength()
is always called first.  Can we somehow provide a length (or length estimate)
without reading the entire steam here?

[yujieqin, 2016/01/20 21:42:20]

Unfortunately, the length is required by DNG SDK, we can not avoid that.

One option I think we may try later is, we could check if the data stream for
SkRawStream is rewindable. If "Yes" (the case you mentioned), then we may avoid
the buffering in SkRawStream. What do you think?

Luckily we don't see real memory issue during out test on the head build of
Android with current CL.

[msarett, 2016/01/21 15:46:16]

Can we make it a longer term goal to "fix" the DNG SDK to not immediately
require the length?
I think that would be great!  There are certainly cases where it will be
rewindable, and we have the opportunity to save this memory.  I'm fine if this
is a follow-up.
Acknowledged.

+}
+
+SkCodec::Result SkRawCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst,
+                                        size_t dstRowBytes, const Options& options,
+                                        SkPMColor ctable[], int* ctableCount,
+                                        int* rowsDecoded) {
+    if (!conversion_possible(requestedInfo, this->getInfo())) {
+        SkCodecPrintf("Error: cannot convert input type to output type.\n");
+        return kInvalidConversion;
+    }
+
+    SkAutoTDelete<SkSwizzler> swizzler(SkSwizzler::CreateSwizzler(
+            SkSwizzler::kRGB, nullptr, requestedInfo, options));
+    if (!swizzler) {

[msarett, 2016/01/20 20:23:17]

I think that, because conversion_possible succeeded, we should
SkASSERT(swizzler).

Only (N32, Opaque) and (565, Opaque) will be approved by conversion_possible. 
And the swizzler must support (and does support) both of those conversions from
kRGB.

[yujieqin, 2016/01/20 21:42:19]

I think the 565 does not work in current test, the swizzler == nullptr. (using
the testing DNG images)

Did we do something wrong here about the swizzler?

[msarett, 2016/01/21 15:46:16]

No, SkSwizzler had a bug where we forgot to add the case for RGB->565.  This has
been fixed.

When you are able to rebase, you will get this fix.  Let me know if you still
need help rebasing.

[yujieqin, 2016/01/21 16:40:01]

Yeah, after rebase, I don't need this check anymore. :)

+        SkCodecPrintf("Error: cannot create swizzler.\n");
+        return kInvalidConversion;
+    }
+
+    const int width = requestedInfo.width();
+    const int height = requestedInfo.height();
+    SkAutoTDelete<dng_image> image(fDngImage->render(width, height));
+    if (!image) {
+        return kInvalidInput;
+    }
+
+    // Because the DNG SDK can not guarantee to render to requested size, we allow a small
+    // difference. Only the overlapping region will be converted in onGetPixels().

[scroggo, 2016/01/20 18:13:36]

nit: "in onGetPixels()" seems unnecessary here - this method *is* onGetPixels.

[yujieqin, 2016/01/20 21:42:20]

Done.

+    const float maxDiffRatio = 1.03f;
+    const dng_point& imageSize = image->Size();
+    if (imageSize.h / width > maxDiffRatio || imageSize.h < width ||
+        imageSize.v / height > maxDiffRatio || imageSize.v < height) {
+        return SkCodec::kInvalidScale;

[scroggo, 2016/01/20 18:13:36]

Is this code ever reached? This gets called by SkCodec::getPixels, but only if
onDimensionsSupported() returns true. Ideally that would catch any problems so
we don't have to render the dng_image only to find out it's too big.

[yujieqin, 2016/01/20 21:42:20]

adaubert@ wrote a test to verify this part could happen.

[adaubert, 2016/01/21 10:43:30]

In practice we should be safe and this case should never happen. But due to the
flexibility and diversity of the DNG standard, we can't guarantee that this case
would not happen at all.
To ensure that we don't run into a strange case, where the rendered dng_image is
way larger than the requested size, we check here.

[scroggo, 2016/01/21 16:36:10]

I guess that's fine. My real concern would be if getScaledDimensions returned a
scale that getPixels did not support. We already have a test for that in
CodexTest (although it only tests one image).

+    }
+
+    void* dstRow = dst;
+    uint8_t srcRow[width * 3];
+
+    dng_pixel_buffer buffer;
+    buffer.fData = &srcRow[0];
+    buffer.fPlane = 0;
+    buffer.fPlanes = 3;
+    buffer.fColStep = buffer.fPlanes;
+    buffer.fPlaneStep = 1;
+    buffer.fPixelType = ttByte;
+    buffer.fPixelSize = sizeof(uint8_t);
+    buffer.fRowStep = sizeof(srcRow);
+
+    for (int i = 0; i < height; ++i) {
+        buffer.fArea = dng_rect(i, 0, i + 1, width);
+
+        try {
+            image->Get(buffer, dng_image::edge_zero);
+        } catch (...) {
+            *rowsDecoded = i;
+            return kIncompleteInput; 
+        }
+
+        swizzler->swizzle(dstRow, &srcRow[0]);
+        dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
+    }
+    return kSuccess;
+}
+
+SkISize SkRawCodec::onGetScaledDimensions(float desiredScale) const {
+    SkASSERT(desiredScale <= 1.f);
+    const SkISize dim = this->getInfo().dimensions();
+
+    // Limits the minimum size to be 80 on the short edge.
+    const float shortEdge = SkTMin(dim.fWidth, dim.fHeight);
+    if (desiredScale < 80.f / shortEdge) {
+        desiredScale = 80.f / shortEdge;
+    }
+
+    // For Xtrans images, the integer-factor scaling does not support the half-size scaling case
+    // (stronger downscalings are fine). In this case, returns the non-scaled version.

[scroggo, 2016/01/20 18:13:36]

So this is saying that scaling by a half is not supported, but scaling to a
smaller size is okay?

In general, the motivation for downscaling is to save memory, so it's too bad
that we're suggesting the full size image when they wanted a downscaled version.
What if we instead suggested something a little smaller than a scale factor of
.5f?

Matt, I think you decided to have SkJpegCodec round up, but I do not remember
why. Is that correct?

FWIW, we should probably update SkCodec to return two sizes, like
SkImageGenerator (now) does. In which case we would give back both the full size
and whatever the smaller size is.

[msarett, 2016/01/20 20:23:17]

I've changed that code multiple times for multiple different reasons.  Right
now, it rounds up.  (Side Note: IIRC the reason is consistency.  libjpeg-turbo
will decode a 41x41 image at a 1/4 scale to 11x11 - it always rounds up.  So if
we also round up - a request for 0.15 goes to 1/4 instead of 1/8, the decoded
output will always be >= than the request.  This is at least easier to predict.)

What we really should do is suggest two candidates - one larger and one smaller.
Agreed.  I have no preference for what to do right now.

Side Note 2:

In past releases, BitmapFactory has been buggy/inconsistent regarding whether it
will actually respect the sampleSize that is requested.  The new implementation
will always get within one.  Ex: 25x25 with sampleSize=2 always decodes to 12x12
OR 13x13, but never 25x25 or 6x6 etc.

RAW will not maintain that behavior, but it's undocumented and I don't
anticipate anybody will notice or care.

[yujieqin, 2016/01/20 21:42:20]

Just for clarification, does it mean we can keep our current solution for now?
:)

[adaubert, 2016/01/21 10:43:30]

Downscaling to a smaller size, in this case 1/3, would be Ok. Like Yujie said,
it's up to you guys what you would prefer.

[msarett, 2016/01/21 15:46:16]

Leon?

[scroggo, 2016/01/21 16:36:09]

My preference would be 1/3 for now. The caller can always decide that that's too
small. Thinking about it now, telling the caller they can decode to full size is
probably never interesting - they should always assume that is an option. 

[yujieqin, 2016/01/21 17:44:26]

Sure, 1/3 it is now. :)

+    if (fDngImage->isXtransImage() && desiredScale >= 0.5f) {
+        return dim;
+    }
+
+    // Round to integer-factors.
+    const float finalScale = std::floor(1.f/ desiredScale);
+    return SkISize::Make(std::floor(dim.fWidth / finalScale),
+                         std::floor(dim.fHeight / finalScale));
+}
+
+bool SkRawCodec::onDimensionsSupported(const SkISize& dim) {

[scroggo, 2016/01/20 18:13:36]

All this scaling code seems complex. Should we add tests to verify it's doing
what we expect it to?

test_dimensions in CodexTest already checks that anything returned by
getScaledDimensions is supported (at least for sampleSizes of 1 to 32, for the
image you've checked in, although I don't know whether that's an XtransImage or
not). It doesn't give us any meaningful data about whether those scaled
dimensions made sense. I'm not sure how best to handle that, though. We could
input the sizes we expect after testing and verifying that the sizes make sense.
That would require entering a lot of sizes though...

[yujieqin, 2016/01/20 21:42:20]

* The image in current CL is not xtrans.
* I think more tests is always good. Could we do that after this CL? I think if
the CodexTest I changed is working, we may first rely on that?

[scroggo, 2016/01/21 16:36:09]

I think that's a fine start.

+    const SkISize fullDim = this->getInfo().dimensions();
+    const float fullShortEdge = SkTMin(fullDim.fWidth, fullDim.fHeight);
+    const float shortEdge = SkTMin(dim.fWidth, dim.fHeight);
+
+    SkISize sizeFloor = this->onGetScaledDimensions(1.f / std::floor(fullShortEdge / shortEdge));
+    SkISize sizeCeil = this->onGetScaledDimensions(1.f / std::ceil(fullShortEdge / shortEdge));
+    return sizeFloor == dim || sizeCeil == dim;
+}
+
+SkRawCodec::~SkRawCodec() {}
+
+SkRawCodec::SkRawCodec(SkDngImage* dngImage)
+    : INHERITED(dngImage->getImageInfo(), nullptr)
+    , fDngImage(dngImage) {}