Subset decoding benchmarks

bench/SubsetBenchPriv.h

+/*
+ * Copyright 2015 The Android Open Source Project
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef SubsetBenchPriv_DEFINED
+#define SubsetBenchPriv_DEFINED
+
+#include "SkCodec.h"
+#include "SkImageGenerator.h"
+
+/*
+ * Returns true if a subset decode succeeds, false otherwise
+ */
+static bool valid_subset_bench(SkMemoryStream* stream, SkColorType colorType, bool useCodec) {

[scroggo, 2015/06/01 17:25:14]

Does this need to be an SkMemoryStream? In general, a parameter type should be
as general as it can be. Maybe it should be an SkData, and this function can
create the SkMemoryStream. (Or even call NewFromData, in the case of SkCodec.)

It looks like all the callers are going to start with an SkString path. Maybe
you should just take a path and consolidate the code for reading it and creating
the stream?

[msarett, 2015/06/01 20:37:01]

Yes, I think this makes a lot more sense.

+    if (useCodec) {
+        SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream->duplicate()));

[scroggo, 2015/06/01 17:25:14]

Not sure whether we need to duplicate the stream here. I suppose it would be a
problem if the stream did not duplicate, but an SkMemoryStream should never
fail. 

[msarett, 2015/06/01 20:37:01]

Acknowledged.

+        if (NULL == codec) {
+            return false;

[scroggo, 2015/06/01 17:25:14]

Can you add a debug statement? This seems like a bug.

If you make this function take an SkString of the path, as I suggested above,
you can make all the error conditions print the file name, so we know which file
failed.

[msarett, 2015/06/01 20:37:01]

I agree!  Adding informative error messages through this function.

+        }
+
+        const SkImageInfo info = codec->getInfo();
+        SkAutoTDeleteArray<uint8_t> row(SkNEW_ARRAY(uint8_t, info.minRowBytes()));
+        SkScanlineDecoder* scanlineDecoder = codec->getScanlineDecoder(info);
+        if (NULL == scanlineDecoder) {
+            return false;

[scroggo, 2015/06/01 17:25:14]

Maybe add a debug statement here, as well?

[msarett, 2015/06/01 20:37:01]

Done.

+        }
+
+        const uint32_t subsetWidth = info.width() / 2;

[scroggo, 2015/06/01 17:25:14]

It seems somewhat weird that we decode an arbitrary subset here. Is the thinking
that if we can successfully decode the last subset, we ought to be able to
decode them all?

I was imagining that valid_subset_bench would catch where we cannot even begin -
i.e. getScanlineDecoder returns NULL or buildTileIndex fails (the former can
just happen on some images; the latter *should* always succeed, but it could
fail...)

If we fail partway, maybe that is something we can depend on DM to catch?

[msarett, 2015/06/01 20:37:01]

Yeah I thought this was awkward too.  I didn't want to duplicate all of the
different benchmark code here, but I thought we might want to check that subsets
actually decode.  As you say, they *should* work if the scanline/buildTileIndex
work, but there isn't a guarantee.  Similarly, I was thinking all subsets
*should* work if one works but there isn't a guarantee for that either.  So
essentially, you're right, decoding an arbitrary subset does not provide us with
anything useful beyond what we already check.

I would agree that depending on dm is a reasonable solution.

+        const uint32_t subsetHeight = info.height() / 2;
+        SkBitmap bitmap;
+        if (!bitmap.tryAllocPixels(info.makeWH(subsetWidth, subsetHeight))) {
+            SkDebugf("Could not allocate memory.  Aborting bench.\n");

[scroggo, 2015/06/01 17:25:14]

Skipping*

[msarett, 2015/06/01 20:37:01]

Acknowledged.

+            return false;
+        }
+
+        if (SkImageGenerator::kSuccess != scanlineDecoder->skipScanlines(subsetHeight)) {

[scroggo, 2015/06/01 17:25:14]

We probably want a debug statement here, too. Although we *do* want to exit
here, since we don't have anything to time, this is an error, so we should not
fail silently.

[msarett, 2015/06/01 20:37:01]

Acknowledged.

+            return false;
+        }
+        uint32_t bpp = info.bytesPerPixel();
+        for (uint32_t y = 0; y < subsetHeight; y++) {
+            if (SkImageGenerator::kSuccess != scanlineDecoder->getScanlines(row.get(), 1, 0)) {
+                return false;
+            }
+            memcpy(bitmap.getAddr(0, y), row.get(), subsetWidth * bpp);

[scroggo, 2015/06/01 17:25:14]

This seems unnecessary. We're trying to make sure it works, but this does not
give us any information about whether it succeeded.

[msarett, 2015/06/01 20:37:01]

Acknowledged.

+        }
+    } else {
+        SkAutoTDelete<SkImageDecoder> decoder(SkImageDecoder::Factory(stream));
+        if (NULL == decoder) {
+            return false;
+        }
+        int width, height;
+        if (!decoder->buildTileIndex(stream->duplicate(), &width, &height)) {

[scroggo, 2015/06/01 17:25:14]

If width and height are not 1, isn't this a bug for it to fail? Probably worth a
debug statement.

[msarett, 2015/06/01 20:37:02]

Acknowledged.

+            return false;
+        }
+        const uint32_t subsetWidth = width / 2;
+        const uint32_t subsetHeight = height / 2;
+        SkBitmap bitmap;
+        SkIRect rect = SkIRect::MakeXYWH(subsetWidth, subsetHeight, subsetWidth,
+                subsetHeight);
+        if (!decoder->decodeSubset(&bitmap, rect, colorType)) {
+            return false;
+        }
+    }
+    return true;
+}
+
+#endif // SubsetBenchPriv_DEFINED