Implement SkAshmemDiscardableMemory

src/ports/SkDiscardableMemory_ashmem.cpp

+/*
+ * Copyright 2013 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include <unistd.h>
+#include <sys/mman.h>
+#include "SkDiscardableMemory.h"
+#include "SkTypes.h"
+#include "android/ashmem.h"
+
+////////////////////////////////////////////////////////////////////////////////
+namespace {
+/**
+ *  DiscardableMemory implementation that uses the Android kernel's
+ *  ashmem (Android shared memory).
+ */
+class SkAshmemDiscardableMemory : public SkDiscardableMemory {
+public:
+    SkAshmemDiscardableMemory(int fd, void* address, size_t size);
+    virtual ~SkAshmemDiscardableMemory();
+    virtual bool lock() SK_OVERRIDE;
+    virtual void* data() SK_OVERRIDE;
+    virtual void unlock() SK_OVERRIDE;
+private:
+    bool fLocked;

[scroggo, 2013/11/22 14:34:14]

nit: names should line up.

[hal.canary, 2013/11/22 15:17:54]

Done.

+    int fFd;
+    void* fMemory;
+    const size_t fSize;
+};
+
+SkAshmemDiscardableMemory::SkAshmemDiscardableMemory(int fd,
+                                                     void* address,
+                                                     size_t size)
+    : fLocked(true)
+    , fFd(fd)
+    , fMemory(address)
+    , fSize(size) {
+    SkASSERT(fFd >= 0);
+    SkASSERT(fMemory != NULL);
+    SkASSERT(fSize > 0);
+}
+
+SkAshmemDiscardableMemory::~SkAshmemDiscardableMemory() {
+    SkASSERT(!fLocked);
+    if (NULL != fMemory) {
+        munmap(fMemory, fSize);
+    }
+    if (fFd != -1) {
+        close(fFd);
+    }
+}
+
+bool SkAshmemDiscardableMemory::lock() {
+    SkASSERT(!fLocked);
+    if (-1 == fFd) {
+        return fLocked = false;

[scroggo, 2013/11/22 14:34:14]

I find this confusing to look at. Is it okay to say

fLocked = false;
return fLocked; // or false

[hal.canary, 2013/11/22 15:17:54]

Done.

+    }
+    SkASSERT(fMemory != NULL);
+    if (fLocked || (ASHMEM_NOT_PURGED == ashmem_pin_region(fFd, 0, 0))) {
+        return fLocked = true;
+    } else {
+        munmap(fMemory, fSize);
+        fMemory = NULL;
+
+        close(fFd);
+        fFd = -1;
+        return fLocked = false;
+    }
+}
+
+void* SkAshmemDiscardableMemory::data() {
+    SkASSERT(fLocked);
+    return fLocked ? fMemory : NULL;
+}
+
+void SkAshmemDiscardableMemory::unlock() {
+    SkASSERT(fLocked);
+    if (fLocked && (fFd != -1)) {
+        ashmem_unpin_region(fFd, 0, 0);
+    }
+    fLocked = false;
+}
+}  // namespace
+////////////////////////////////////////////////////////////////////////////////
+
+SkDiscardableMemory* SkDiscardableMemory::Create(size_t bytes) {
+    // ashmem likes lengths on page boundaries.
+    const size_t mask = getpagesize() - 1;
+    size_t size = (bytes + mask) & ~mask;
+
+    static const char name[] = "Skia_Ashmem_Discardable_Memory";
+    int fd = ashmem_create_region(name, size);
+    if (fd < 0) {
+        return NULL;
+    }
+    if (0 != ashmem_set_prot_region(fd, PROT_READ | PROT_WRITE)) {
+        close(fd);
+        return NULL;
+    }
+    void* addr = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+    if (MAP_FAILED == addr) {
+        close(fd);
+        return NULL;
+    }
+    if (-1 == ashmem_pin_region(fd, 0, 0)) {

[scroggo, 2013/11/22 14:34:14]

Is this step necessary? It seems that SkImageRef_ashmem skips this step.

[hal.canary, 2013/11/22 15:17:54]

SkImageRef_ashmem does not begin in a locked state.  the SkDiscardableMemory
contract is different.

[scroggo, 2013/11/22 15:37:05]

SkImageRef_ashmem does not begin in a locked state, but its analogous function
to this one, allocPixelRef(), is only called inside onLockPixels, so it is being
set to a locked state.

Does your test pass if you remove this call to ashmem_pin_region? (I'm not sure
that's a good indicator of what is correct; your test only covers a pretty
simple case. That said, I've yet to figure out how to write a good test for
Ashmem. I wonder if Android has any interesting tests we could mimic...)

[scroggo, 2013/12/06 16:07:51]

Did you see my last comment? I don't think this recreates the same behavior that
SkImageRefAshmem had.

[hal.canary, 2013/12/06 20:14:42]

I had missed your last comment.  Thanks for reminding me.  Yes, you are correct:
that first call to pin the memory was unnecessary, as I verified by checking the
documentation.  I am removing that call and adding a comment to the constructor
to justify why I can initialize isLocked to true.

+        // Purge status doesn't matter, only check for errors.
+        munmap(addr, size);
+        close(fd);
+        return NULL;
+    }
+
+    return SkNEW_ARGS(SkAshmemDiscardableMemory, (fd, addr, size));
+}
+