python/corrade: make our own buffer protocol for array views.

THe one from pybind11 is severely limited and allocation-happy. Lots of
tech debt and TODOs resolved now, yay.

doc/python/corrade.containers.rst

@@ -51,26 +51,16 @@
     Unlike in C++, the view keeps a reference to the original memory owner
     object, meaning that calling :py:`del` on the original object will *not*
     invalidate the view. Slicing a view creates a new view referencing the same
-    original object, without any dependency on the previous view.
+    original object, without any dependency on the previous view. That means a
+    long chained slicing operation will not cause increased memory usage.
 
     .. code:: pycon
 
         >>> b.obj is a
         True
-        >>> b[1:4].obj is a
+        >>> b[1:4][:-1].obj is a
         True
 
-    .. block-danger:: Pybind11 Buffer Protocol issues
-
-        Currently, due to how buffer protocol is exposed in pybind11, there are
-        several issues with converting array views from and to buffer objects:
-
-        -   readonly property is not preserved when converting an ArrayView
-            to Python `memoryview`
-        -   subsequent slicing and view conversion keeps reference to the
-            previous view instance instead of just to the original object,
-            creating unnecesarily long GC chains
-
     `Comparison to Python's memoryview`_
     ====================================
 

src/python/corrade/PyArrayView.h

@@ -38,6 +38,10 @@ template<class T> struct PyArrayView: Containers::ArrayView<T> {
     /*implicit*/PyArrayView() noexcept: obj{py::none{}} {}
     explicit PyArrayView(Containers::ArrayView<T> view, py::object obj) noexcept: Containers::ArrayView<T>{view}, obj{std::move(obj)} {}
 
+    /* Turning this into a reference so buffer protocol can point to it instead
+       of needing to allocate */
+    std::size_t& sizeRef() { return Containers::ArrayView<T>::_size; }
+
     py::object obj;
 };
 
@@ -46,6 +50,15 @@ template<unsigned dimensions, class T> struct PyStridedArrayView: Containers::St
     /*implicit*/ PyStridedArrayView() noexcept: obj{py::none{}} {}
     explicit PyStridedArrayView(Containers::StridedArrayView<dimensions, T> view, py::object obj) noexcept: Containers::StridedArrayView<dimensions, T>{view}, obj{std::move(obj)} {}
 
+    /* Turning this into a reference so buffer protocol can point to it instead
+       of needing to allocate */
+    Containers::StridedDimensions<dimensions, std::size_t>& sizeRef() {
+        return Containers::StridedArrayView<dimensions, T>::_size;
+    }
+    Containers::StridedDimensions<dimensions, std::ptrdiff_t>& strideRef() {
+        return Containers::StridedArrayView<dimensions, T>::_stride;
+    }
+
     py::object obj;
 };
 

src/python/corrade/PyBuffer.h

@@ -0,0 +1,82 @@
+#ifndef corrade_PyBuffer_h
+#define corrade_PyBuffer_h
+/*
+    This file is part of Magnum.
+
+    Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019
+              Vladimír Vondruš <mosra@centrum.cz>
+
+    Permission is hereby granted, free of charge, to any person obtaining a
+    copy of this software and associated documentation files (the "Software"),
+    to deal in the Software without restriction, including without limitation
+    the rights to use, copy, modify, merge, publish, distribute, sublicense,
+    and/or sell copies of the Software, and to permit persons to whom the
+    Software is furnished to do so, subject to the following conditions:
+
+    The above copyright notice and this permission notice shall be included
+    in all copies or substantial portions of the Software.
+
+    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+    THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+    DEALINGS IN THE SOFTWARE.
+*/
+
+#include <pybind11/pybind11.h>
+#include <Corrade/Containers/Pointer.h>
+#include <Corrade/Containers/StridedArrayView.h>
+
+#include "bootstrap.h"
+
+namespace corrade {
+
+/* pybind's py::buffer_info is EXTREMELY USELESS IT HURTS (and also allocates
+   like hell), doing my own thing here instead. IMAGINE, I can pass flags to
+   say what features I'm able to USE! WOW! */
+
+template<class Class, bool(*getter)(Class&, Py_buffer&, int)> void enableBetterBufferProtocol(py::object& object) {
+    auto& typeObject = reinterpret_cast<PyHeapTypeObject&>(*object.ptr());
+    /* Sanity check -- we expect pybind set up its own buffer functions before
+       us */
+    CORRADE_INTERNAL_ASSERT(typeObject.as_buffer.bf_getbuffer == pybind11::detail::pybind11_getbuffer);
+    CORRADE_INTERNAL_ASSERT(typeObject.as_buffer.bf_releasebuffer == pybind11::detail::pybind11_releasebuffer);
+
+    typeObject.as_buffer.bf_getbuffer = [](PyObject *obj, Py_buffer *buffer, int flags) {
+        /* Stolen from pybind11::class_::def_buffer(). Not sure what exactly
+           it does, but I assume caster is implicitly convertible to Class& and
+           thus can magically access the actual Class from the PyObject. */
+        pybind11::detail::make_caster<Class> caster;
+        CORRADE_INTERNAL_ASSERT(!PyErr_Occurred() && buffer);
+        CORRADE_INTERNAL_ASSERT(caster.load(obj, /*convert=*/false));
+
+        /* Zero-initialize the output and ask the class to fill it. If that
+           fails for some reason, give up */
+        *buffer = Py_buffer{};
+        if(!getter(caster, *buffer, flags)) {
+            CORRADE_INTERNAL_ASSERT(!buffer->obj);
+            CORRADE_INTERNAL_ASSERT(PyErr_Occurred());
+            return -1;
+        }
+
+        /* Set the memory owner to the object and increase its reference count.
+           We need to keep the object around because buffer->shapes /
+           buffer->strides might be refering to it, moreover setting it to
+           something else (like ArrayView's memory owner object) would mean
+           Python calls the releasebuffer on that object instead of on us,
+           leading to reference count getting negative in many cases. */
+        CORRADE_INTERNAL_ASSERT(!buffer->obj);
+        buffer->obj = obj;
+        Py_INCREF(buffer->obj);
+        return 0;
+    };
+    /* No need to release anything, we haven't made any garbage in the first
+       place */
+    typeObject.as_buffer.bf_releasebuffer = nullptr;
+}
+
+}
+
+#endif

src/python/corrade/containers.cpp

@@ -26,16 +26,26 @@
 #include <pybind11/pybind11.h>
 #include <pybind11/numpy.h> /* so ArrayView is convertible from python array */
 #include <Corrade/Containers/Array.h>
+#include <Corrade/Containers/ScopeGuard.h>
 #include <Corrade/Utility/FormatStl.h>
 
 #include "corrade/bootstrap.h"
 #include "corrade/PyArrayView.h"
 #include "corrade/PybindExtras.h"
+#include "corrade/PyBuffer.h"
 
 namespace corrade {
 
 namespace {
 
+const char* const FormatStrings[]{
+    /* 0. Representing bytes as unsigned. Not using 'c' because then it behaves
+       differently from bytes/bytearray, where you can do `a[0] = ord('A')`. */
+    "B",
+};
+template<class> constexpr std::size_t formatIndex();
+template<> constexpr std::size_t formatIndex<char>() { return 0; }
+
 struct Slice {
     std::size_t start;
     std::size_t stop;
@@ -61,6 +71,32 @@ Slice calculateSlice(py::slice slice, std::size_t containerSize) {
     return Slice{std::size_t(start), std::size_t(stop), step};
 }
 
+template<class T> bool arrayViewBufferProtocol(T& self, Py_buffer& buffer, int flags) {
+    if((flags & PyBUF_WRITABLE) == PyBUF_WRITABLE && !std::is_const<typename T::Type>::value) {
+        PyErr_SetString(PyExc_BufferError, "array view is not writable");
+        return false;
+    }
+
+    /* I hate the const_casts but I assume this is to make editing easier, NOT
+       to make it possible for users to stomp on these values. */
+    buffer.ndim = 1;
+    buffer.itemsize = sizeof(typename T::Type);
+    buffer.len = sizeof(typename T::Type)*self.size();
+    buffer.buf = const_cast<typename std::decay<typename T::Type>::type*>(self.data());
+    buffer.readonly = std::is_const<typename T::Type>::value;
+    if((flags & PyBUF_FORMAT) == PyBUF_FORMAT)
+        buffer.format = const_cast<char*>(FormatStrings[formatIndex<typename std::decay<typename T::Type>::type>()]);
+    if(flags != PyBUF_SIMPLE) {
+        /* The view is immutable (can't change its size after it has been
+           constructed), so referencing the size directly is okay */
+        buffer.shape = reinterpret_cast<Py_ssize_t*>(&self.sizeRef());
+        if((flags & PyBUF_STRIDES) == PyBUF_STRIDES)
+            buffer.strides = &buffer.itemsize;
+    }
+
+    return true;
+}
+
 template<class T> void arrayView(py::class_<PyArrayView<T>>& c) {
     /* Implicitly convertible from a buffer */
     py::implicitly_convertible<py::buffer, PyArrayView<T>>();
@@ -70,27 +106,25 @@ template<class T> void arrayView(py::class_<PyArrayView<T>>& c) {
         .def(py::init(), "Default constructor")
 
         /* Buffer protocol */
-        .def(py::init([](py::buffer buffer) {
+        .def(py::init([](py::buffer other) {
-            py::buffer_info info = buffer.request(!std::is_const<T>::value);
+            Py_buffer buffer{};
-
+            if(PyObject_GetBuffer(other.ptr(), &buffer, (std::is_const<T>::value ? 0 : PyBUF_WRITABLE)) != 0)
-            if(info.ndim != 1)
+                throw py::error_already_set{};
-                throw py::buffer_error{Utility::formatString("expected one dimension but got {}", info.ndim)};
+
-            if(info.strides[0] != info.itemsize)
+            Containers::ScopeGuard e{&buffer, PyBuffer_Release};
-                throw py::buffer_error{Utility::formatString("expected stride of {} but got {}", info.itemsize, info.strides[0])};
+
-
+            if(buffer.ndim != 1)
-            // TODO: need to take buffer.obj, not buffer!
+                throw py::buffer_error{Utility::formatString("expected one dimension but got {}", buffer.ndim)};
-            return PyArrayView<T>{{static_cast<T*>(info.ptr), std::size_t(info.shape[0]*info.itemsize)}, buffer};
+            if(buffer.strides && buffer.strides[0] != buffer.itemsize)
+                throw py::buffer_error{Utility::formatString("expected stride of {} but got {}", buffer.itemsize, buffer.strides[0])};
+
+            /* reinterpret_borrow converts PyObject* to an (automatically
+               refcounted) py::object. We take the underlying object instead of
+               the buffer because we no longer care about the buffer
+               descriptor -- that could allow the GC to haul away a bit more
+               garbage */
+            return PyArrayView<T>{{static_cast<T*>(buffer.buf), std::size_t(buffer.len)}, py::reinterpret_borrow<py::object>(buffer.obj)};
         }), "Construct from a buffer")
-        .def_buffer([](const PyArrayView<T>& self) -> py::buffer_info {
-            return py::buffer_info{
-                const_cast<char*>(self.data()),
-                sizeof(T),
-                py::format_descriptor<T>::format(),
-                1,
-                {self.size()},
-                {1} // TODO: need to pass self.obj to the buffer, not self!
-            };
-        })
 
         /* Length and memory owning object */
         .def("__len__", &PyArrayView<T>::size, "View size")
@@ -121,6 +155,8 @@ template<class T> void arrayView(py::class_<PyArrayView<T>>& c) {
             /* Usual business */
             return py::cast(PyArrayView<T>{self.slice(calculated.start, calculated.stop), self.obj});
         }, "Slice the view");
+
+    enableBetterBufferProtocol<PyArrayView<T>, arrayViewBufferProtocol>(c);
 }
 
 template<class T> void mutableArrayView(py::class_<PyArrayView<T>>& c) {
@@ -202,42 +238,76 @@ template<class T> const T& dimensionsTupleGet(const typename DimensionsTuple<3,
     CORRADE_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */
 }
 
+template<class T> bool stridedArrayViewBufferProtocol(T& self, Py_buffer& buffer, int flags) {
+    if((flags & PyBUF_STRIDES) != PyBUF_STRIDES) {
+        /* TODO: allow this if the array actually *is* contiguous? */
+        PyErr_SetString(PyExc_BufferError, "array view is not contiguous");
+        return false;
+    }
+
+    if((flags & PyBUF_WRITABLE) == PyBUF_WRITABLE && !std::is_const<typename T::Type>::value) {
+        PyErr_SetString(PyExc_BufferError, "array view is not writable");
+        return false;
+    }
+
+    /* I hate the const_casts but I assume this is to make editing easier, NOT
+       to make it possible for users to stomp on these values. */
+    buffer.ndim = T::Dimensions;
+    buffer.itemsize = sizeof(typename T::Type);
+    buffer.len = sizeof(typename T::Type);
+    for(std::size_t i = 0; i != T::Dimensions; ++i)
+        buffer.len *= self.sizeRef()[i];
+    buffer.buf = const_cast<typename std::decay<typename T::ErasedType>::type*>(self.data());
+    buffer.readonly = std::is_const<typename T::Type>::value;
+    if((flags & PyBUF_FORMAT) == PyBUF_FORMAT)
+        buffer.format = const_cast<char*>(FormatStrings[formatIndex<typename std::decay<typename T::Type>::type>()]);
+    /* The view is immutable (can't change its size after it has been
+       constructed), so referencing the size/stride directly is okay */
+    buffer.shape = const_cast<Py_ssize_t*>(reinterpret_cast<const Py_ssize_t*>(self.sizeRef().begin()));
+    buffer.strides = const_cast<Py_ssize_t*>(reinterpret_cast<const Py_ssize_t*>(self.strideRef().begin()));
+
+    return true;
+}
+
+inline std::size_t largerStride(std::size_t a, std::size_t b) {
+    return a < b ? b : a; /* max(), but named like this to avoid clashes */
+}
+
 template<unsigned dimensions, class T> void stridedArrayView(py::class_<PyStridedArrayView<dimensions, T>>& c) {
     c
         /* Constructor */
         .def(py::init(), "Default constructor")
 
         /* Buffer protocol */
-        .def(py::init([](py::buffer buffer) {
+        .def(py::init([](py::buffer other) {
-            py::buffer_info info = buffer.request(!std::is_const<T>::value);
+            Py_buffer buffer{};
-
+            if(PyObject_GetBuffer(other.ptr(), &buffer, PyBUF_STRIDES|(std::is_const<T>::value ? 0 : PyBUF_WRITABLE)) != 0)
-            if(info.ndim != dimensions)
+                throw py::error_already_set{};
-                throw py::buffer_error{Utility::formatString("expected {} dimensions but got {}", dimensions, info.ndim)};
+
-
+            Containers::ScopeGuard e{&buffer, PyBuffer_Release};
+
+            if(buffer.ndim != dimensions)
+                throw py::buffer_error{Utility::formatString("expected {} dimensions but got {}", dimensions, buffer.ndim)};
+
+            Containers::StaticArrayView<dimensions, const std::size_t> sizes{reinterpret_cast<std::size_t*>(buffer.shape)};
+            Containers::StaticArrayView<dimensions, const std::ptrdiff_t> strides{reinterpret_cast<std::ptrdiff_t*>(buffer.strides)};
+            /* Calculate total memory size that spans the whole view. Mainly to
+               make the constructor assert happy, not used otherwise */
+            std::size_t size = 0;
+            for(std::size_t i = 0; i != dimensions; ++i)
+                size = largerStride(buffer.shape[i]*(buffer.strides[i] < 0 ? -buffer.strides[i] : buffer.strides[i]), size);
+
+            /* reinterpret_borrow converts PyObject* to an (automatically
+               refcounted) py::object. We take the underlying object instead of
+               the buffer because we no longer care about the buffer
+               descriptor -- that could allow the GC to haul away a bit more
+               garbage */
             return PyStridedArrayView<dimensions, T>{{
-                {static_cast<T*>(info.ptr), std::size_t(info.size*info.strides[0])},
+                {static_cast<T*>(buffer.buf), size},
-                Containers::StaticArrayView<dimensions, const std::size_t>{reinterpret_cast<std::size_t*>(&info.shape[0])},
+                Containers::StaticArrayView<dimensions, const std::size_t>{reinterpret_cast<std::size_t*>(buffer.shape)},
-                Containers::StaticArrayView<dimensions, const std::ptrdiff_t>{reinterpret_cast<std::ptrdiff_t*>(&info.strides[0])}},
+                Containers::StaticArrayView<dimensions, const std::ptrdiff_t>{reinterpret_cast<std::ptrdiff_t*>(buffer.strides)}},
-                // TODO: need to take buffer.obj, not buffer!
+                py::reinterpret_borrow<py::object>(buffer.obj)};
-                buffer};
         }), "Construct from a buffer")
-        .def_buffer([](const PyStridedArrayView<dimensions, T>& self) -> py::buffer_info {
-            std::vector<py::ssize_t> shape(dimensions);
-            Containers::StridedDimensions<dimensions, std::size_t> selfSize{self.size()};
-            for(std::size_t i = 0; i != dimensions; ++i) shape[i] = selfSize[i];
-
-            std::vector<py::ssize_t> strides(dimensions);
-            Containers::StridedDimensions<dimensions, std::ptrdiff_t> selfStride{self.stride()};
-            for(std::size_t i = 0; i != dimensions; ++i) strides[i] = selfStride[i];
-            return py::buffer_info{
-                const_cast<void*>(self.data()),
-                sizeof(T),
-                py::format_descriptor<T>::format(),
-                dimensions,
-                shape, strides
-                // TODO: need to pass self.obj to the buffer, not self!
-            };
-        })
 
         // TODO: construct from a buffer + size/stride
 
@@ -266,6 +336,8 @@ template<unsigned dimensions, class T> void stridedArrayView(py::class_<PyStride
             const Slice calculated = calculateSlice(slice, Containers::StridedDimensions<dimensions, const std::size_t>{self.size()}[0]);
             return py::cast(PyStridedArrayView<dimensions, T>(self.slice(calculated.start, calculated.stop).every(calculated.step), self.obj));
         }, "Slice the view");
+
+    enableBetterBufferProtocol<PyStridedArrayView<dimensions, T>, stridedArrayViewBufferProtocol>(c);
 }
 
 template<class T> void stridedArrayView1D(py::class_<PyStridedArrayView<1, T>>& c) {

src/python/corrade/test/test_containers.py

@@ -66,6 +66,14 @@ class ArrayView(unittest.TestCase):
         del b
         self.assertTrue(sys.getrefcount(a), a_refcount)
 
+    def test_init_buffer_memoryview_obj(self):
+        a = b'hello'
+        v = memoryview(a)
+        b = containers.ArrayView(v)
+        # memoryview's buffer protocol returns itself, not the underlying
+        # bytes, as it manages the Py_buffer instance. So this is expected.
+        self.assertIs(b.obj, v)
+
     def test_init_buffer_mutable(self):
         a = bytearray(b'hello')
         b = containers.MutableArrayView(a)
@@ -79,16 +87,18 @@ class ArrayView(unittest.TestCase):
         self.assertIs(b.obj, a)
         self.assertEqual(len(b), 3*4)
 
+    @unittest.skip("there doesn't seem to be a way to make memoryview give back N-dimensional array that can't be represented as linear memory")
     def test_init_buffer_unexpected_dimensions(self):
         a = memoryview(b'123456').cast('b', shape=[2, 3])
         self.assertEqual(bytes(a), b'123456')
-        with self.assertRaisesRegex(BufferError, "expected one dimension but got 2"):
+        with self.assertRaisesRegex(BufferError, "but what"):
             b = containers.ArrayView(a)
 
     def test_init_buffer_unexpected_stride(self):
         a = memoryview(b'hello')[::2]
         self.assertEqual(bytes(a), b'hlo')
-        with self.assertRaisesRegex(BufferError, "expected stride of 1 but got 2"):
+        # Error emitted by memoryview, not us
+        with self.assertRaisesRegex(BufferError, "memoryview: underlying buffer is not C-contiguous"):
             b = containers.ArrayView(a)
 
     def test_init_buffer_mutable_from_immutable(self):
@@ -183,12 +193,24 @@ class ArrayView(unittest.TestCase):
         b_refcount = sys.getrefcount(b)
         self.assertEqual(sys.getrefcount(a), a_refcount + 1)
 
-        # TODO: fix when pybind is replaced
-
         c = memoryview(b)
-        self.assertEqual(c.obj, b) # TODO: should be a
+        # Unlike slicing, ArrayView's buffer protocol returns a reference to
-        self.assertEqual(sys.getrefcount(b), b_refcount + 1) # TODO: should not hcange
+        # itself -- it needs to be kept around because the Py_buffer refers to
-        self.assertEqual(sys.getrefcount(a), a_refcount + 1) # TODO: should be +2
+        # its internals for size. Also returning a reference to the underlying
+        # buffer would mean the underlying buffer's releasebuffer function gets
+        # called instead of ours which is *not* wanted.
+        self.assertEqual(c.obj, b)
+        self.assertEqual(sys.getrefcount(b), b_refcount + 1)
+        self.assertEqual(sys.getrefcount(a), a_refcount + 1)
+
+        with self.assertRaisesRegex(TypeError, "cannot modify read-only memory"):
+            c[-1] = ord('?')
+
+    def test_convert_mutable_memoryview(self):
+        a = bytearray(b'World is hell!')
+        b = memoryview(containers.MutableArrayView(a))
+        b[-1] = ord('?')
+        self.assertEqual(a, b'World is hell?')
 
 class StridedArrayView1D(unittest.TestCase):
     def test_init(self):
@@ -235,12 +257,13 @@ class StridedArrayView1D(unittest.TestCase):
         del b
         self.assertTrue(sys.getrefcount(a), a_refcount)
 
-    @unittest.expectedFailure
     def test_init_buffer_memoryview_obj(self):
         a = b'hello'
         v = memoryview(a)
         b = containers.StridedArrayView1D(v)
-        self.assertIs(b.obj, a) # TODO: it's b because pybind is stupid
+        # memoryview's buffer protocol returns itself, not the underlying
+        # bytes, as it manages the Py_buffer instance. So this is expected.
+        self.assertIs(b.obj, v)
 
     def test_init_buffer_mutable(self):
         a = bytearray(b'hello')
@@ -334,18 +357,28 @@ class StridedArrayView1D(unittest.TestCase):
         b_refcount = sys.getrefcount(b)
         self.assertEqual(sys.getrefcount(a), a_refcount + 1)
 
-        # TODO: fix when pybind is replaced
-
         c = memoryview(b)
         self.assertEqual(c.ndim, 1)
         self.assertEqual(len(c), len(a))
         self.assertEqual(bytes(c), a)
-        self.assertEqual(c.obj, b) # TODO: should be a
+        # Unlike slicing, StridedArrayView's buffer protocol returns a
-        self.assertEqual(sys.getrefcount(b), b_refcount + 1) # TODO: should not change
+        # reference to itself and not the underlying buffer -- it needs to be
-        self.assertEqual(sys.getrefcount(a), a_refcount + 1) # TODO: should be +2
+        # kept around because the Py_buffer refers to its internals for size.
+        # Also returning a reference to the underlying buffer would mean the
+        # underlying buffer's releasebuffer function gets called instead of
+        # ours which is *not* wanted.
+        self.assertEqual(c.obj, b)
+        self.assertEqual(sys.getrefcount(b), b_refcount + 1)
+        self.assertEqual(sys.getrefcount(a), a_refcount + 1)
 
-        c[-1] = ord('?') # TODO: wrong, should fail
+        with self.assertRaisesRegex(TypeError, "cannot modify read-only memory"):
-        self.assertEqual(a, b'World is hell?') # TODO: wrong
+            c[-1] = ord('?')
+
+    def test_convert_mutable_memoryview(self):
+        a = bytearray(b'World is hell!')
+        b = memoryview(containers.MutableStridedArrayView1D(a))
+        b[-1] = ord('?')
+        self.assertEqual(a, b'World is hell?')
 
 class StridedArrayView2D(unittest.TestCase):
     def test_init(self):
@@ -370,9 +403,9 @@ class StridedArrayView2D(unittest.TestCase):
              b'89abcdef')
         a_refcount = sys.getrefcount(a)
 
-        b = containers.StridedArrayView2D(memoryview(a).cast('b', shape=[3, 8]))
+        v = memoryview(a).cast('b', shape=[3, 8])
         # TODO: construct as containers.StridedArrayView2D(a, (3, 8), (8, 1))
-        #self.assertIs(b.obj, a) # TODO
+        b = containers.StridedArrayView2D(v)
         self.assertEqual(len(b), 3)
         self.assertEqual(bytes(b), b'01234567'
                                    b'456789ab'
@@ -439,20 +472,17 @@ class StridedArrayView2D(unittest.TestCase):
             b = containers.MutableStridedArrayView2D(a)
 
     def test_slice(self):
-        a = (b'01234567'
+        a = memoryview(b'01234567'
-             b'456789ab'
+                       b'456789ab'
-             b'89abcdef')
+                       b'89abcdef').cast('b', shape=[3, 8])
         a_refcount = sys.getrefcount(a)
 
-        v = memoryview(a).cast('b', shape=[3, 8])
+        b = containers.StridedArrayView2D(a)
-        v_refcount = sys.getrefcount(v)
-
-        # TODO: Pybind refcounts against v, not a
-
-        b = containers.StridedArrayView2D(v)
         b_refcount = sys.getrefcount(b)
+        # memoryview's buffer protocol returns itself, not the underlying
+        # bytes, as it manages the Py_buffer instance. So this is expected.
+        self.assertEqual(b.obj, a)
         self.assertEqual(sys.getrefcount(a), a_refcount + 1)
-        self.assertEqual(sys.getrefcount(v), v_refcount + 1) # TODO: should not change
 
         # When slicing, b's refcount should not change but a's refcount should
         # increase
@@ -462,30 +492,25 @@ class StridedArrayView2D(unittest.TestCase):
         self.assertEqual(c.stride, (8, 1))
         self.assertEqual(bytes(c), b'01234567456789ab')
         self.assertEqual(sys.getrefcount(b), b_refcount)
-        self.assertEqual(sys.getrefcount(a), a_refcount + 1) # TODO: should be +2
+        self.assertEqual(sys.getrefcount(a), a_refcount + 2)
-        self.assertEqual(sys.getrefcount(v), v_refcount + 2) # TODO: should not change
 
         # Deleting a slice should reduce a's refcount again, keep b's unchanged
         del c
         self.assertEqual(sys.getrefcount(b), b_refcount)
         self.assertEqual(sys.getrefcount(a), a_refcount + 1)
-        self.assertEqual(sys.getrefcount(v), v_refcount + 1) # TODO: should not change
 
     def test_slice_multidimensional(self):
-        a = (b'01234567'
+        a = memoryview(b'01234567'
-             b'456789ab'
+                       b'456789ab'
-             b'89abcdef')
+                       b'89abcdef').cast('b', shape=[3, 8])
         a_refcount = sys.getrefcount(a)
 
-        v = memoryview(a).cast('b', shape=[3, 8])
+        b = containers.StridedArrayView2D(a)
-        v_refcount = sys.getrefcount(v)
-
-        # TODO: Pybind refcounts against v, not a
-
-        b = containers.StridedArrayView2D(v)
         b_refcount = sys.getrefcount(b)
+        # memoryview's buffer protocol returns itself, not the underlying
+        # bytes, as it manages the Py_buffer instance. So this is expected.
+        self.assertEqual(b.obj, a)
         self.assertEqual(sys.getrefcount(a), a_refcount + 1)
-        self.assertEqual(sys.getrefcount(v), v_refcount + 1) # TODO: should not change
 
         # When slicing, b's refcount should not change but a's refcount should
         # increase
@@ -496,14 +521,12 @@ class StridedArrayView2D(unittest.TestCase):
         self.assertEqual(bytes(c[0]), b'89a')
         self.assertEqual(bytes(c[1]), b'cde')
         self.assertEqual(sys.getrefcount(b), b_refcount)
-        self.assertEqual(sys.getrefcount(a), a_refcount + 1) # TODO: should be +2
+        self.assertEqual(sys.getrefcount(a), a_refcount + 2)
-        self.assertEqual(sys.getrefcount(v), v_refcount + 2) # TODO: should not change
 
         # Deleting a slice should reduce a's refcount again, keep b's unchanged
         del c
         self.assertEqual(sys.getrefcount(b), b_refcount)
         self.assertEqual(sys.getrefcount(a), a_refcount + 1)
-        self.assertEqual(sys.getrefcount(v), v_refcount + 1) # TODO: should not change
 
     def test_slice_invalid(self):
         with self.assertRaisesRegex(ValueError, "slice step cannot be zero"):
@@ -600,29 +623,25 @@ class StridedArrayView2D(unittest.TestCase):
         self.assertEqual(bytes(d), b'3377bb')
 
     def test_convert_memoryview(self):
-        a = (b'01234567'
+        a = memoryview(b'01234567'
-             b'456789ab'
+                       b'456789ab'
-             b'89abcdef')
+                       b'89abcdef').cast('b', shape=[3, 8])
         a_refcount = sys.getrefcount(a)
-        v = memoryview(a).cast('b', shape=[3, 8])
-        v_refcount = sys.getrefcount(v)
 
-        b = containers.StridedArrayView2D(v)
+        b = containers.StridedArrayView2D(a)
         b_refcount = sys.getrefcount(b)
-        self.assertEqual(sys.getrefcount(a), a_refcount + 1) # TODO: should be + 2
+        # memoryview's buffer protocol returns itself, not the underlying
-        self.assertEqual(sys.getrefcount(v), v_refcount + 1) # TODO: should not change
+        # bytes, as it manages the Py_buffer instance. So this is expected.
+        self.assertEqual(b.obj, a)
+        self.assertEqual(sys.getrefcount(a), a_refcount + 1)
 
         c = memoryview(b)
         self.assertEqual(c.ndim, 2)
         self.assertEqual(c.shape, (3, 8))
         self.assertEqual(c.strides, (8, 1))
-        self.assertEqual(c.obj, v) # TODO: should be a
+        self.assertEqual(c.obj, a)
-        self.assertEqual(sys.getrefcount(b), b_refcount + 1) # TODO: should not change
+        self.assertEqual(sys.getrefcount(a), a_refcount + 1)
-        self.assertEqual(sys.getrefcount(a), a_refcount + 1) # TODO: should be + 2
+        self.assertEqual(sys.getrefcount(b), b_refcount + 1)
-        self.assertEqual(sys.getrefcount(v), v_refcount + 1) # TODO: should not change
-
-        c[2, 1] = ord('!') # TODO: wrong, should fail
-        self.assertEqual(chr(c[2, 1]), '!') # TODO: should be 9
 
 class StridedArrayView3D(unittest.TestCase):
     def test_init_buffer(self):