From a7742fba932c3469a1c3fdabb35bc2578ca3437f Mon Sep 17 00:00:00 2001 From: WyattBlue Date: Sun, 16 Aug 2026 11:36:29 -0400 Subject: [PATCH 1/2] Give each opaque buffer its own key OpaqueContainer keyed its entries on id(v), so one object attached to two frames produced a single entry shared by both buffers. Freeing either frame ran key_free, which popped that entry, and the other frame's opaque silently became None. Worse, once the object was released, a later object could land on the same address and be handed back through the stale key. Hand out a fresh uint64 per add() instead. Every buffer then owns its entry, holders are independent, and the object is released once the last one goes away. --- CHANGELOG.rst | 1 + av/opaque.pxd | 3 +++ av/opaque.py | 25 ++++++++++++++++--------- tests/test_videoframe.py | 24 ++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 64a8311f0..d475c81c8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -47,6 +47,7 @@ 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. +- 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``. diff --git a/av/opaque.pxd b/av/opaque.pxd index 998e13397..d442aee62 100644 --- a/av/opaque.pxd +++ b/av/opaque.pxd @@ -1,8 +1,11 @@ +from libc.stdint cimport uint64_t + cimport libav as lib 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) diff --git a/av/opaque.py b/av/opaque.py index 281033258..b7a33c316 100644 --- a/av/opaque.py +++ b/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) diff --git a/tests/test_videoframe.py b/tests/test_videoframe.py index 3ed417da7..a25873f7e 100644 --- a/tests/test_videoframe.py +++ b/tests/test_videoframe.py @@ -1,3 +1,5 @@ +import gc +import weakref from fractions import Fraction import numpy @@ -136,6 +138,28 @@ def test_opaque() -> None: assert frame.opaque is None +def test_opaque_shared_between_frames() -> None: + class Payload: + pass + + payload = Payload() + ref = weakref.ref(payload) + frames = [VideoFrame(16, 16, "yuv420p") for _ in range(3)] + for frame in frames: + frame.opaque = payload + + # Dropping one holder must not take the object away from the others. + while frames: + assert all(f.opaque is payload for f in frames) + frames.pop() + gc.collect() + + # ...and the last one going away must still release it. + del frame, payload + gc.collect() + assert ref() is None + + def test_interpolation() -> None: container = av.open(fate_png()) for _ in container.decode(video=0): From aa718412e71e0a42a586dc239725019b99714936 Mon Sep 17 00:00:00 2001 From: WyattBlue Date: Sun, 16 Aug 2026 11:48:15 -0400 Subject: [PATCH 2/2] Stop reading streams that the container has freed --- CHANGELOG.rst | 1 + av/audio/stream.py | 5 +++++ av/filter/loudnorm.py | 6 +++--- av/index.pxd | 10 ++++++---- av/index.py | 36 +++++++++++++++++++++--------------- av/opaque.pxd | 3 +-- av/stream.pxd | 2 ++ av/stream.py | 35 +++++++++++++++++++++++++++++++++-- av/video/stream.py | 9 +++++++++ tests/test_indexentries.py | 35 +++++++++++++++++++++++++++++++++++ tests/test_streams.py | 30 ++++++++++++++++++++++++++++++ 11 files changed, 146 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d475c81c8..ffb2d22e2 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -47,6 +47,7 @@ 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. diff --git a/av/audio/stream.py b/av/audio/stream.py index e7fc63560..0efde1a06 100644 --- a/av/audio/stream.py +++ b/av/audio/stream.py @@ -8,6 +8,11 @@ @cython.cclass class AudioStream(Stream): def __repr__(self): + if not self._is_open(): + return ( + f"" + ) + if self.codec_context is None: return f" at 0x{id(self):x}>" form = self.format.name if self.format else None diff --git a/av/filter/loudnorm.py b/av/filter/loudnorm.py index 0be991ab6..0be6d5bbf 100644 --- a/av/filter/loudnorm.py +++ b/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 diff --git a/av/index.pxd b/av/index.pxd index adca6ae00..ca19c169f 100644 --- a/av/index.pxd +++ b/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) diff --git a/av/index.py b/av/index.py index 27bdf1763..8f7b9e499 100644 --- a/av/index.py +++ b/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 "" return f"" 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 diff --git a/av/opaque.pxd b/av/opaque.pxd index d442aee62..4cd1c258e 100644 --- a/av/opaque.pxd +++ b/av/opaque.pxd @@ -1,6 +1,5 @@ -from libc.stdint cimport uint64_t - cimport libav as lib +from libc.stdint cimport uint64_t cdef class OpaqueContainer: diff --git a/av/stream.pxd b/av/stream.pxd index 66b94f199..755d194cb 100644 --- a/av/stream.pxd +++ b/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) diff --git a/av/stream.py b/av/stream.py index b696b9d98..cfd231860 100644 --- a/av/stream.py +++ b/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,15 +126,28 @@ 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"" + ) + name = getattr(self, "name", None) return ( f"'}/" @@ -142,6 +155,8 @@ def __repr__(self): ) 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,6 +339,11 @@ def type(self): @cython.cclass class DataStream(Stream): def __repr__(self): + if not self._is_open(): + return ( + f"" + ) + return ( f"'} at 0x{id(self):x}>" @@ -322,6 +351,7 @@ def __repr__(self): @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: diff --git a/av/video/stream.py b/av/video/stream.py index 5bf83c5a2..b4d0508cc 100644 --- a/av/video/stream.py +++ b/av/video/stream.py @@ -12,6 +12,11 @@ @cython.cclass class VideoStream(Stream): def __repr__(self): + if not self._is_open(): + return ( + f"" + ) + if self.codec_context is None: return f" at 0x{id(self):x}>" return ( @@ -128,6 +133,7 @@ def average_rate(self): :type: AVRational """ + self._assert_open() return from_avrational(self.ptr.avg_frame_rate) @property @@ -141,6 +147,7 @@ def base_rate(self): :type: AVRational """ + self._assert_open() return from_avrational(self.ptr.r_frame_rate) @property @@ -152,6 +159,7 @@ def guessed_rate(self): :type: AVRational """ + self._assert_open() val: lib.AVRational = lib.av_guess_frame_rate( cython.NULL, self.ptr, cython.NULL ) @@ -166,6 +174,7 @@ def sample_aspect_ratio(self): :type: AVRational """ + self._assert_open() sar: lib.AVRational = lib.av_guess_sample_aspect_ratio( self.container.ptr, self.ptr, cython.NULL ) diff --git a/tests/test_indexentries.py b/tests/test_indexentries.py index c0d5cd393..682b41ebd 100644 --- a/tests/test_indexentries.py +++ b/tests/test_indexentries.py @@ -77,3 +77,38 @@ def test_index_entries_slice(self) -> None: for i, j in zip(individual_indices, slice_indices) ] ) + + def test_index_entries_after_close(self) -> None: + container = av.open(fate_suite("h264/interlaced_crop.mp4")) + stream = container.streams.video[0] + entries = stream.index_entries + snapshot = entries[0] + pos, timestamp = snapshot.pos, snapshot.timestamp + + container.close() + + # The AVStream is gone, so the view must refuse rather than read it. + for call in ( + lambda: len(entries), + lambda: entries[0], + lambda: entries.search_timestamp(0), + ): + with self.assertRaises(AssertionError): + call() + + # An entry already handed out is a copy, so it stays readable. + assert (snapshot.pos, snapshot.timestamp) == (pos, timestamp) + assert "container closed" in repr(entries) + assert repr(snapshot) + + def test_index_entries_outlive_container(self) -> None: + import gc + + container = av.open(fate_suite("h264/interlaced_crop.mp4")) + entries = container.streams.video[0].index_entries + length = len(entries) + del container + gc.collect() + + # Holding the view keeps its stream, and so the container, alive. + assert len(entries) == length diff --git a/tests/test_streams.py b/tests/test_streams.py index 9e4941c3a..0d8b9ba95 100644 --- a/tests/test_streams.py +++ b/tests/test_streams.py @@ -407,3 +407,33 @@ def test_unknown_stream_type(self) -> None: stream = container.streams[0] assert stream.type == "unknown" assert type(stream) is av.stream.Stream + + def test_stream_after_close(self) -> None: + container = av.open(fate_suite("h264/interlaced_crop.mp4")) + stream = container.streams.video[0] + assert stream.time_base and stream.index == 0 + + container.close() + + for name in ( + "id", + "index", + "time_base", + "start_time", + "duration", + "frames", + "disposition", + "discard", + "type", + "average_rate", + "base_rate", + "guessed_rate", + ): + with pytest.raises(AssertionError): + getattr(stream, name) + + with pytest.raises(AssertionError): + stream.time_base = 1 + + # Still describable, so a traceback or debugger does not blow up. + assert "container closed" in repr(stream)