Give each opaque buffer its own key

CHANGELOG.rst

@@ -47,6 +47,8 @@ Features:
 Fixes:
 
 - ``av.dump_codecs()`` no longer drops the canonical names ``h264``, ``hevc``, ``av1``, ``dirac``, and ``ilbc``, each of which was overwritten by the row of whichever encoder it resolved to.
+- Reading a :class:`.Stream` or its :attr:`~av.stream.Stream.index_entries` after the container is closed now raises instead of reading freed memory, since ``avformat_close_input()`` frees the underlying ``AVStream``. Holding an ``index_entries`` also keeps its container alive, and an :class:`.IndexEntry` is a copy, so it stays readable after the close and is unaffected by the demuxer reallocating the index.
+- Attaching one object to the ``opaque`` of more than one frame or packet no longer loses it. The objects were keyed by ``id()``, so every holder shared an entry and whichever was freed first took it away from the rest.
 - ``Frame.side_data`` now satisfies the ``Mapping`` protocol it advertises: iteration yields :class:`~av.sidedata.sidedata.Type` keys, so ``items()``, ``keys()``, and ``values()`` work instead of raising ``KeyError``. Values remain reachable positionally by an integer or, for the first time, a slice. Its type stub was a ``TypedDict`` with a single literal key, and is now ``SideDataContainer``.
 - Fix crashes from indexes that were turned into C pointer arithmetic without being range checked. ``MotionVectors[i]`` only checked the upper bound, so a negative index read off the front of the buffer (``mvs[-1]`` now returns the last vector, as with any sequence); ``VideoFormatComponent`` and ``AudioPlane`` accepted any index at all; and ``BitmapSubtitlePlane`` and ``VideoBlockParams`` were missing their lower bounds.
 - Frames returned by flushing a codec context directly (``CodecContext.decode()`` with no packet) now carry the stream's ``time_base`` instead of ``None``.

av/audio/stream.py

@@ -8,6 +8,11 @@
 @cython.cclass
 class AudioStream(Stream):
     def __repr__(self):
+        if not self._is_open():
+            return (
+                f"<av.{self.__class__.__name__} (container closed) at 0x{id(self):x}>"
+            )
+
         if self.codec_context is None:
             return f"<av.AudioStream #{self.index} audio/<nocodec> at 0x{id(self):x}>"
         form = self.format.name if self.format else None

av/filter/loudnorm.py

@@ -23,10 +23,10 @@ def stats(loudnorm_args: str, stream: AudioStream) -> bytes:
         loudnorm_args = loudnorm_args + ":print_format=json"
 
     container: Container = stream.container
-    format_ptr: cython.pointer[AVFormatContext] = container.ptr
-    container.ptr = cython.NULL  # Prevent double-free
-
     stream_index: cython.int = stream.index
+
+    format_ptr: cython.pointer[AVFormatContext] = container.ptr
+    container.ptr = cython.NULL
     py_args: bytes = loudnorm_args.encode("utf-8")
     c_args: cython.p_const_char = py_args
     result: cython.p_char

av/index.pxd

@@ -1,12 +1,14 @@
 cimport libav as lib
 
+from av.stream cimport Stream
+
 
 cdef class IndexEntry:
-    cdef const lib.AVIndexEntry *ptr
+    cdef lib.AVIndexEntry entry
     cdef void _init(self, const lib.AVIndexEntry *ptr)
 
 cdef class IndexEntries:
-    cdef lib.AVStream *stream_ptr
-    cdef void _init(self, lib.AVStream *ptr)
+    cdef Stream stream
+    cdef void _init(self, Stream stream)
 
-cdef IndexEntries wrap_index_entries(lib.AVStream *ptr)
+cdef IndexEntries wrap_index_entries(Stream stream)

av/index.py

@@ -1,5 +1,6 @@
 import cython
 import cython.cimports.libav as lib
+from cython.cimports.av.stream import Stream
 from cython.cimports.libc.stdint import int64_t
 
 _cinit_bypass_sentinel = cython.declare(object, object())
@@ -28,7 +29,7 @@ def __cinit__(self, sentinel):
 
     @cython.cfunc
     def _init(self, ptr: cython.pointer[cython.const[lib.AVIndexEntry]]) -> cython.void:
-        self.ptr = ptr
+        self.entry = ptr[0]  # Copied, not referenced
 
     def __repr__(self):
         return (
@@ -38,37 +39,37 @@ def __repr__(self):
 
     @property
     def pos(self):
-        return self.ptr.pos
+        return self.entry.pos
 
     @property
     def timestamp(self):
-        return self.ptr.timestamp
+        return self.entry.timestamp
 
     @property
     def flags(self):
-        return self.ptr.flags
+        return self.entry.flags
 
     @property
     def is_keyframe(self):
-        return bool(self.ptr.flags & lib.AVINDEX_KEYFRAME)
+        return bool(self.entry.flags & lib.AVINDEX_KEYFRAME)
 
     @property
     def is_discard(self):
-        return bool(self.ptr.flags & lib.AVINDEX_DISCARD_FRAME)
+        return bool(self.entry.flags & lib.AVINDEX_DISCARD_FRAME)
 
     @property
     def size(self):
-        return self.ptr.size
+        return self.entry.size
 
     @property
     def min_distance(self):
-        return self.ptr.min_distance
+        return self.entry.min_distance
 
 
 @cython.cfunc
-def wrap_index_entries(ptr: cython.pointer[lib.AVStream]) -> IndexEntries:
+def wrap_index_entries(stream: Stream) -> IndexEntries:
     obj: IndexEntries = IndexEntries(_cinit_bypass_sentinel)
-    obj._init(ptr)
+    obj._init(stream)
     return obj
 
 
@@ -90,21 +91,25 @@ def __cinit__(self, sentinel):
         raise RuntimeError("cannot manually instantiate IndexEntries")
 
     @cython.cfunc
-    def _init(self, ptr: cython.pointer[lib.AVStream]) -> cython.void:
-        self.stream_ptr = ptr
+    def _init(self, stream: Stream) -> cython.void:
+        self.stream = stream
 
     def __repr__(self):
+        if not self.stream._is_open():
+            return "<av.IndexEntries (container closed)>"
         return f"<av.IndexEntries[{len(self)}]>"
 
     def __len__(self) -> int:
+        self.stream._assert_open()
         with cython.nogil:
-            return lib.avformat_index_get_entries_count(self.stream_ptr)
+            return lib.avformat_index_get_entries_count(self.stream.ptr)
 
     def __iter__(self):
         for i in range(len(self)):
             yield self[i]
 
     def __getitem__(self, index):
+        self.stream._assert_open()
         if isinstance(index, int):
             n = len(self)
             if index < 0:
@@ -115,7 +120,7 @@ def __getitem__(self, index):
             c_idx: cython.int = index
             entry: cython.pointer[cython.const[lib.AVIndexEntry]]
             with cython.nogil:
-                entry = lib.avformat_index_get_entry(self.stream_ptr, c_idx)
+                entry = lib.avformat_index_get_entry(self.stream.ptr, c_idx)
             if entry == cython.NULL:
                 raise IndexError("index entry not found")
 
@@ -135,6 +140,7 @@ def search_timestamp(
 
         Returns an index into this object, or ``-1`` if no match is found.
         """
+        self.stream._assert_open()
         c_timestamp: int64_t = timestamp
         flags: cython.int = 0
 
@@ -144,6 +150,6 @@ def search_timestamp(
             flags |= lib.AVSEEK_FLAG_ANY
 
         with cython.nogil:
-            idx = lib.av_index_search_timestamp(self.stream_ptr, c_timestamp, flags)
+            idx = lib.av_index_search_timestamp(self.stream.ptr, c_timestamp, flags)
 
         return idx

av/opaque.pxd

@@ -1,8 +1,10 @@
 cimport libav as lib
+from libc.stdint cimport uint64_t
 
 
 cdef class OpaqueContainer:
     cdef dict _objects
+    cdef uint64_t _next_key
     cdef lib.AVBufferRef *add(self, object v)
     cdef object get(self, char *name)
     cdef object pop(self, char *name)

av/opaque.py

@@ -2,7 +2,7 @@
 import cython
 import cython.cimports.libav as lib
 from cython import NULL, sizeof
-from cython.cimports.libc.stdint import uint8_t, uintptr_t
+from cython.cimports.libc.stdint import uint8_t, uint64_t
 from cython.cimports.libc.string import memcpy
 
 u8ptr = cython.typedef(cython.pointer[uint8_t])
@@ -23,35 +23,42 @@ def key_free(opaque: cython.p_void, data: u8ptr) -> cython.void:
 class OpaqueContainer:
     def __cinit__(self):
         self._objects = {}
+        self._next_key = 0
 
     @cython.cfunc
     def add(self, v: object) -> cython.pointer[lib.AVBufferRef]:
-        # Use object's memory address as key
-        key: uintptr_t = cython.cast(uintptr_t, id(v))
-        self._objects[key] = v
+        # A fresh key per buffer. Keying on id(v) instead would give the same key
+        # to one object held by two frames, and freeing either would drop the
+        # entry out from under the other.
+        key: uint64_t = self._next_key
 
-        data: u8ptr = cython.cast(u8ptr, lib.av_malloc(sizeof(uintptr_t)))
+        data: u8ptr = cython.cast(u8ptr, lib.av_malloc(sizeof(uint64_t)))
         if data == NULL:
             raise MemoryError("Failed to allocate memory for key")
 
-        memcpy(data, cython.address(key), sizeof(uintptr_t))
+        memcpy(data, cython.address(key), sizeof(uint64_t))
 
         # Create the buffer with our free callback
         buffer_ref: cython.pointer[lib.AVBufferRef] = lib.av_buffer_create(
-            data, sizeof(uintptr_t), key_free, NULL, 0
+            data, sizeof(uint64_t), key_free, NULL, 0
         )
 
         if buffer_ref == NULL:
+            # av_buffer_create() leaves the data to us when it fails.
+            lib.av_free(data)
             raise MemoryError("Failed to create AVBufferRef")
 
+        # Register only once key_free() is in place to unregister it again.
+        self._objects[key] = v
+        self._next_key += 1
         return buffer_ref
 
     def get(self, name) -> object:
-        key: uintptr_t = cython.cast(cython.pointer[uintptr_t], name)[0]
+        key: uint64_t = cython.cast(cython.pointer[uint64_t], name)[0]
         return self._objects.get(key)
 
     def pop(self, name) -> object:
-        key: uintptr_t = cython.cast(cython.pointer[uintptr_t], name)[0]
+        key: uint64_t = cython.cast(cython.pointer[uint64_t], name)[0]
         return self._objects.pop(key, None)
 
 

av/stream.pxd

@@ -21,6 +21,8 @@ cdef class Stream:
 
     # Private API.
     cdef void _init(self, Container, lib.AVStream*, CodecContext)
+    cdef bint _is_open(self)
+    cdef void _assert_open(self)
     cdef void _assert_has_codec_context(self, int err=*)
     cdef void _finalize_for_output(self)
     cdef void _set_id(self, value)

av/stream.py

@@ -116,7 +116,7 @@ def _init(
     ) -> cython.void:
         self.container = container
         self.ptr = stream
-        self.index_entries = wrap_index_entries(self.ptr)
+        self.index_entries = wrap_index_entries(self)
 
         self.codec_context = codec_context
 
@@ -126,22 +126,37 @@ def _init(
             errors=self.container.metadata_errors,
         )
 
+    @cython.cfunc
+    def _is_open(self) -> cython.bint:
+        return self.container is not None and self.container.ptr != cython.NULL
+
+    @cython.cfunc
+    def _assert_open(self) -> cython.void:
+        if self.container is None or self.container.ptr == cython.NULL:
+            raise AssertionError("Container is not open")
+
     @cython.cfunc
     def _assert_has_codec_context(
         self, err: cython.int = lib.AVERROR_DECODER_NOT_FOUND
     ) -> cython.void:
-        # Calling into a NULL codec_context is a segfault, not an AttributeError.
         if self.codec_context is None:
             err_check(err)
 
     def __repr__(self):
+        if not self._is_open():
+            return (
+                f"<av.{self.__class__.__name__} (container closed) at 0x{id(self):x}>"
+            )
+
         name = getattr(self, "name", None)
         return (
             f"<av.{self.__class__.__name__} #{self.index} {self.type or '<notype>'}/"
             f"{name or '<nocodec>'} at 0x{id(self):x}>"
         )
 
     def __setattr__(self, name, value):
+        if name in ("id", "disposition", "discard", "time_base"):
+            self._assert_open()
         if name == "id":
             self._set_id(value)
             return
@@ -189,6 +204,7 @@ def id(self):
         :type: int
 
         """
+        self._assert_open()
         return self.ptr.id
 
     @cython.cfunc
@@ -229,6 +245,7 @@ def index(self):
 
         :type: int
         """
+        self._assert_open()
         return self.ptr.index
 
     @property
@@ -239,6 +256,7 @@ def time_base(self):
         :type: AVRational
 
         """
+        self._assert_open()
         return from_avrational(self.ptr.time_base)
 
     @property
@@ -249,6 +267,7 @@ def start_time(self):
 
         :type: int | None
         """
+        self._assert_open()
         if self.ptr.start_time != lib.AV_NOPTS_VALUE:
             return self.ptr.start_time
 
@@ -260,6 +279,7 @@ def duration(self):
         :type: int | None
 
         """
+        self._assert_open()
         if self.ptr.duration != lib.AV_NOPTS_VALUE:
             return self.ptr.duration
 
@@ -272,6 +292,7 @@ def frames(self):
 
         :type: int
         """
+        self._assert_open()
         return self.ptr.nb_frames
 
     @property
@@ -285,6 +306,7 @@ def language(self):
 
     @property
     def disposition(self):
+        self._assert_open()
         return Disposition(self.ptr.disposition)
 
     @property
@@ -298,6 +320,7 @@ def discard(self):
 
         :type: Discard
         """
+        self._assert_open()
         return Discard(self.ptr.discard)
 
     @property
@@ -307,6 +330,7 @@ def type(self):
 
         :type: Literal["audio", "video", "subtitle", "data", "attachment"]
         """
+        self._assert_open()
         media_type = lib.av_get_media_type_string(self.ptr.codecpar.codec_type)
         return "unknown" if media_type == cython.NULL else media_type
 
@@ -315,13 +339,19 @@ def type(self):
 @cython.cclass
 class DataStream(Stream):
     def __repr__(self):
+        if not self._is_open():
+            return (
+                f"<av.{self.__class__.__name__} (container closed) at 0x{id(self):x}>"
+            )
+
         return (
             f"<av.{self.__class__.__name__} #{self.index} data/"
             f"{self.name or '<nocodec>'} at 0x{id(self):x}>"
         )
 
     @property
     def name(self):
+        self._assert_open()
         desc: cython.pointer[cython.const[lib.AVCodecDescriptor]] = (
             lib.avcodec_descriptor_get(self.ptr.codecpar.codec_id)
         )
@@ -359,6 +389,7 @@ def mimetype(self):
     @property
     def data(self):
         """Return the raw attachment payload as bytes."""
+        self._assert_open()
         extradata: cython.p_uchar = self.ptr.codecpar.extradata
         size: cython.Py_ssize_t = self.ptr.codecpar.extradata_size
         if extradata == cython.NULL or size <= 0: