diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ba8faea92..9321d82fa 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -36,6 +36,7 @@ Major: - Drop support for Python 3.11. Binary wheels are now built for Python 3.12 and later. - Remove the undocumented ``CodecContext.hwaccel`` attribute. It held the ``HWAccel`` settings object passed in, not the live device context; use ``CodecContext.is_hwaccel`` to check whether hardware acceleration is in use. - Rational attributes (``time_base``, ``average_rate``, ``base_rate``, ``guessed_rate``, ``framerate``, ``rate``, ``sample_aspect_ratio``, and ``display_aspect_ratio``) now return :class:`av.AVRational` rather than ``fractions.Fraction``, and are never ``None``: an unset value is the falsy ``AVRational(0, 1)``. Test them with ``if not stream.time_base:`` instead of ``is None``. Setters still accept a ``fractions.Fraction``. +- Closing an :class:`.OutputContainer` now frees its context, so using one afterwards raises ``Container is not open`` instead of continuing against a finished file. Muxing has no ``avformat_close_input()`` to do this for it, so ``add_stream()``, ``mux()``, ``start_encoding()`` and the rest kept working after ``close()``, and streams held from it stayed readable. - Remove ``Capabilities.hwaccel``, ``Capabilities.hwaccel_vdpau``, and ``Capabilities.neg_linesizes``, none of which FFmpeg defines any more. - Remove the ``metadata_encoding`` and ``metadata_errors`` arguments to :func:`av.open`, and the matching attributes. Metadata is now always read and written as UTF-8 with ``surrogateescape``, which is byte exact: a tag in another encoding survives as surrogates and is recovered per key with ``value.encode("utf-8", "surrogateescape").decode("cp1251")``. Previously one encoding had to be chosen for a whole container, so a file mixing encodings across tags could not be represented at all. - Remove the ``stream_options`` argument to :func:`av.open` and the matching attribute. They only ever reached ``avformat_find_stream_info()``, and only for formats that expose their streams before it runs, so they raised for MPEG and friends; output containers rejected them outright. Pass ``options`` for every stream, set ``stream.codec_context.options`` for one, and ``Container.add_stream(..., options={})`` when writing. @@ -48,6 +49,8 @@ Features: Fixes: +- A rejected ``add_stream()`` or ``add_mux_stream()`` no longer breaks the container. +- ``InputContainer.size`` returns ``None`` when the size cannot be determined rather than the negative ``AVERROR`` it was passing through, which read as a plausible byte count. A non-seekable input, such as a pipe, reported ``-78``. - ``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. - ``FilterLink.input`` and ``FilterLink.output`` now follow the filters the graph auto-inserts while configuring. They cached the pad they first resolved, so reading one before ``Graph.configure()`` reported the filter the link no longer pointed at. - Fix a segfault when a ``FilterLink`` outlives its ``Graph``. It held the graph by weak reference and dereferenced ``AVFilterLink`` before consulting it, so ``link.input`` and ``link.output`` read freed memory. It now holds the graph, as a ``FilterContext`` already did. diff --git a/av/container/core.pyi b/av/container/core.pyi index 19e199508..5f25e7030 100644 --- a/av/container/core.pyi +++ b/av/container/core.pyi @@ -91,6 +91,7 @@ class Container: read_timeout: Real | None flags: int video_codec_id: int + def dumps_format(self) -> str: ... def __enter__(self) -> Self: ... def __exit__( self, diff --git a/av/container/input.py b/av/container/input.py index 745076fa9..c3070c58f 100644 --- a/av/container/input.py +++ b/av/container/input.py @@ -2,7 +2,6 @@ from cython.cimports.av.codec.context import CodecContext, wrap_codec_context from cython.cimports.av.container.streams import StreamContainer from cython.cimports.av.dictionary import Dictionary -from cython.cimports.av.error import err_check from cython.cimports.av.packet import Packet from cython.cimports.av.stream import Stream, wrap_stream from cython.cimports.av.utils import avdict_to_dict @@ -29,6 +28,7 @@ def __cinit__(self, *args, **kwargs): stream: cython.pointer[lib.AVStream] codec: cython.pointer[cython.const[lib.AVCodec]] codec_context: cython.pointer[lib.AVCodecContext] + ret: cython.int # Hand `options` to every stream that is already known. Only allocate # c_options when they are: some formats (e.g. MPEG) do not expose their @@ -42,6 +42,8 @@ def __cinit__(self, *args, **kwargs): cython.pointer[cython.pointer[lib.AVDictionary]], malloc(nb_streams_before * cython.sizeof(cython.p_void)), ) + if c_options == cython.NULL: + raise MemoryError() for i in range(nb_streams_before): c_options[i] = cython.NULL lib.av_dict_copy(cython.address(c_options[i]), base_dict.ptr, 0) @@ -51,13 +53,14 @@ def __cinit__(self, *args, **kwargs): with cython.nogil: ret = lib.avformat_find_stream_info(self.ptr, c_options) self.set_timeout(None) - self.err_check(ret) if c_options: for i in range(nb_streams_before): lib.av_dict_free(cython.address(c_options[i])) free(c_options) + self.err_check(ret) + at_least_one_accelerated_context = False self.streams = StreamContainer() @@ -66,9 +69,12 @@ def __cinit__(self, *args, **kwargs): codec = lib.avcodec_find_decoder(stream.codecpar.codec_id) if codec: codec_context = lib.avcodec_alloc_context3(codec) - err_check( - lib.avcodec_parameters_to_context(codec_context, stream.codecpar) - ) + if codec_context == cython.NULL: + raise MemoryError() + ret = lib.avcodec_parameters_to_context(codec_context, stream.codecpar) + if ret < 0: + lib.avcodec_free_context(cython.address(codec_context)) + self.err_check(ret) codec_context.pkt_timebase = stream.time_base py_codec_context = wrap_codec_context( codec_context, codec, self.hwaccel @@ -127,8 +133,17 @@ def bit_rate(self): @property def size(self): + """Size of the input in bytes, or ``None`` if it cannot be determined. + + A non-seekable input, such as a pipe or a file-like object without + ``seek``, has no size to report. + + Wraps :ffmpeg:`avio_size`. + """ self._assert_open() - return lib.avio_size(self.ptr.pb) + size: int64_t = lib.avio_size(self.ptr.pb) + if size >= 0: + return size def close(self): close_input(self) @@ -152,11 +167,12 @@ def demux(self, *args, **kwargs): self._assert_open() streams: list[Stream] = self.streams.get(*args, **kwargs) - if self.ptr.nb_streams == 0: + nb_streams: cython.uint = self.ptr.nb_streams + if nb_streams == 0: return include_stream: cython.pointer[uint8_t] = cython.cast( cython.pointer[uint8_t], - malloc(self.ptr.nb_streams * cython.sizeof(uint8_t)), + malloc(nb_streams * cython.sizeof(uint8_t)), ) if include_stream == cython.NULL: raise MemoryError() @@ -168,11 +184,11 @@ def demux(self, *args, **kwargs): self.set_timeout(self.read_timeout) try: - for i in range(self.ptr.nb_streams): + for i in range(nb_streams): include_stream[i] = 0 for stream in streams: i = stream.index - if i >= self.ptr.nb_streams: + if i >= nb_streams: raise ValueError(f"stream index {i} out of range") include_stream[i] = 1 @@ -194,11 +210,14 @@ def demux(self, *args, **kwargs): except EOFError: break - if include_stream[read_packet.stream_index]: - # If AVFMTCTX_NOHEADER is set in ctx_flags, then new streams - # may also appear in av_read_frame(). - # http://ffmpeg.org/doxygen/trunk/structAVFormatContext.html - # TODO: find better way to handle this + # If AVFMTCTX_NOHEADER is set in ctx_flags, then new streams + # may also appear in av_read_frame(). They are past the end of + # include_stream, and nothing selected them anyway. + # http://ffmpeg.org/doxygen/trunk/structAVFormatContext.html + if ( + read_packet.stream_index < nb_streams + and include_stream[read_packet.stream_index] + ): if read_packet.stream_index < len(self.streams): # Move the encoded data out of the read buffer into a # fresh Packet for the caller. @@ -211,7 +230,7 @@ def demux(self, *args, **kwargs): yield packet # Flush! - for i in range(self.ptr.nb_streams): + for i in range(nb_streams): if include_stream[i]: packet = Packet() packet._stream = self.streams[i] @@ -300,7 +319,7 @@ def seek( stream_index: cython.int = stream.index if stream else -1 with cython.nogil: ret = lib.av_seek_frame(self.ptr, stream_index, c_offset, flags) - err_check(ret) + self.err_check(ret) self.flush_buffers() diff --git a/av/container/input.pyi b/av/container/input.pyi index caea60b94..2ec10193c 100644 --- a/av/container/input.pyi +++ b/av/container/input.pyi @@ -17,7 +17,7 @@ class InputContainer(Container): start_time_realtime: int | None duration: int | None bit_rate: int - size: int + size: int | None @overload def demux(self, video_stream: VideoStream) -> Iterator[Packet[VideoStream]]: ... diff --git a/av/container/output.py b/av/container/output.py index b10e189f7..c540ec80b 100644 --- a/av/container/output.py +++ b/av/container/output.py @@ -38,6 +38,9 @@ def _set_codecpar_extradata( @cython.cfunc def close_output(self: OutputContainer) -> cython.void: + if self.ptr == cython.NULL: + return # Already closed. + if self.packet_ptr != cython.NULL and self._buffered_packets: buffered: list[Packet] = self._buffered_packets self._buffered_packets = [] @@ -46,21 +49,33 @@ def close_output(self: OutputContainer) -> cython.void: self._mux_one(packet) self.streams = StreamContainer() - if self._myflag & 12 == 4: # enum.started and not enum.done - # If the underlying Python IO file was already closed (e.g. during GC - # finalization where cycle ordering is undefined), skip the trailer. - if self.file is not None and getattr(self.file.file, "closed", False): - self._myflag |= 8 # enum.done = True - return - # We must only ever call av_write_trailer *once*, otherwise we get a - # segmentation fault. Therefore no matter whether it succeeds or not - # we must absolutely set enum.done. - try: - self.err_check(lib.av_write_trailer(self.ptr)) - finally: - if self.file is None and not (self.ptr.oformat.flags & lib.AVFMT_NOFILE): - lib.avio_closep(cython.address(self.ptr.pb)) - self._myflag |= 8 # enum.done = True + try: + if self._myflag & 12 == 4: # enum.started and not enum.done + # If the underlying Python IO file was already closed (e.g. during + # GC finalization where cycle ordering is undefined), skip the + # trailer. + if self.file is not None and getattr(self.file.file, "closed", False): + self._myflag |= 8 # enum.done = True + return + # We must only ever call av_write_trailer *once*, otherwise we get a + # segmentation fault. Therefore no matter whether it succeeds or not + # we must absolutely set enum.done. + try: + self.err_check(lib.av_write_trailer(self.ptr)) + finally: + if self.file is None and not ( + self.ptr.oformat.flags & lib.AVFMT_NOFILE + ): + lib.avio_closep(cython.address(self.ptr.pb)) + self._myflag |= 8 # enum.done = True + finally: + # Drop the context so a closed output reports itself as closed: + # Container._assert_open() tests for a NULL ptr, which demuxing gets + # for free from avformat_close_input(). Muxing has no such call, and + # without this every accessor kept working on a finished file. + with cython.nogil: + lib.avformat_free_context(self.ptr) + self.ptr = cython.NULL @cython.final @@ -111,6 +126,8 @@ def add_stream( """ + self._assert_open() + codec_obj: Codec = Codec(codec_name, "w") codec: cython.pointer[cython.const[lib.AVCodec]] = codec_obj.ptr @@ -122,22 +139,38 @@ def add_stream( f"{self.format.name!r} format does not support {codec_obj.name!r} codec" ) + c_time_base: lib.AVRational + c_framerate: lib.AVRational + has_time_base: cython.bint = "time_base" in kwargs + if has_time_base: + to_avrational(kwargs.pop("time_base"), cython.address(c_time_base)) + if codec.type == lib.AVMEDIA_TYPE_VIDEO: + to_avrational(rate or 24, cython.address(c_framerate)) + elif codec.type == lib.AVMEDIA_TYPE_AUDIO and not ( + rate is None or type(rate) is int + ): + raise TypeError("audio stream `rate` must be: int | None") + # Create new stream in the AVFormatContext, set AVCodecContext values. - stream: cython.pointer[lib.AVStream] = lib.avformat_new_stream(self.ptr, codec) ctx: cython.pointer[lib.AVCodecContext] = lib.avcodec_alloc_context3(codec) + if ctx == cython.NULL: + raise MemoryError("Could not allocate codec context") + stream: cython.pointer[lib.AVStream] = lib.avformat_new_stream(self.ptr, codec) + if stream == cython.NULL: + lib.avcodec_free_context(cython.address(ctx)) + raise MemoryError("Could not allocate stream") - # Now lets set some more sane video defaults + if has_time_base: + ctx.time_base = c_time_base + + # Now let's set some more sane video defaults if codec.type == lib.AVMEDIA_TYPE_VIDEO: ctx.pix_fmt = lib.AV_PIX_FMT_YUV420P ctx.width = kwargs.pop("width", 640) ctx.height = kwargs.pop("height", 480) ctx.bit_rate = kwargs.pop("bit_rate", 0) ctx.bit_rate_tolerance = kwargs.pop("bit_rate_tolerance", 128000) - try: - to_avrational(kwargs.pop("time_base"), cython.address(ctx.time_base)) - except KeyError: - pass - to_avrational(rate or 24, cython.address(ctx.framerate)) + ctx.framerate = c_framerate stream.avg_frame_rate = ctx.framerate stream.time_base = ctx.time_base @@ -157,17 +190,7 @@ def add_stream( ctx.sample_fmt = cython.cast(cython.pointer[lib.AVSampleFormat], out)[0] ctx.bit_rate = kwargs.pop("bit_rate", 0) ctx.bit_rate_tolerance = kwargs.pop("bit_rate_tolerance", 32000) - try: - to_avrational(kwargs.pop("time_base"), cython.address(ctx.time_base)) - except KeyError: - pass - - if rate is None: - ctx.sample_rate = 48000 - elif type(rate) is int: - ctx.sample_rate = rate - else: - raise TypeError("audio stream `rate` must be: int | None") + ctx.sample_rate = 48000 if rate is None else rate stream.time_base = ctx.time_base lib.av_channel_layout_default(cython.address(ctx.ch_layout), 2) @@ -208,6 +231,8 @@ def add_mux_stream(self, codec_name: str, rate=None, **kwargs) -> Stream: :rtype: The new :class:`~av.stream.Stream`. """ + self._assert_open() + # Find the codec to get its id and type (try encoder first, then decoder). codec_name_bytes: bytes = codec_name.encode() codec: cython.pointer[cython.const[lib.AVCodec]] = ( @@ -240,6 +265,13 @@ def add_mux_stream(self, codec_name: str, rate=None, **kwargs) -> Stream: f"{self.format.name!r} format does not support {codec_name!r} codec" ) + c_rate: lib.AVRational + if rate is not None: + if codec_type == lib.AVMEDIA_TYPE_VIDEO: + to_avrational(rate, cython.address(c_rate)) + elif codec_type == lib.AVMEDIA_TYPE_AUDIO and type(rate) is not int: + raise TypeError("audio stream `rate` must be: int | None") + # Create stream with no codec context. stream: cython.pointer[lib.AVStream] = lib.avformat_new_stream( self.ptr, cython.NULL @@ -254,13 +286,9 @@ def add_mux_stream(self, codec_name: str, rate=None, **kwargs) -> Stream: stream.codecpar.width = kwargs.pop("width", 0) stream.codecpar.height = kwargs.pop("height", 0) if rate is not None: - to_avrational(rate, cython.address(stream.avg_frame_rate)) - elif codec_type == lib.AVMEDIA_TYPE_AUDIO: - if rate is not None: - if type(rate) is int: - stream.codecpar.sample_rate = rate - else: - raise TypeError("audio stream `rate` must be: int | None") + stream.avg_frame_rate = c_rate + elif codec_type == lib.AVMEDIA_TYPE_AUDIO and rate is not None: + stream.codecpar.sample_rate = rate # Construct the user-land stream (no codec context). py_stream: Stream = wrap_stream(self, stream, None) @@ -282,6 +310,7 @@ def add_stream_from_template( :param \\**kwargs: Set attributes for the stream. :rtype: The new :class:`~av.stream.Stream`. """ + self._assert_open() template.container._assert_open() if opaque is None: @@ -382,6 +411,8 @@ def add_attachment(self, name: str, mimetype: str, data: bytes): - Only supported by formats that support attachments (e.g. Matroska). - No per-packet muxing is required; attachments are written at header time. """ + self._assert_open() + # Create stream with no codec (attachments are codec-less). stream: cython.pointer[lib.AVStream] = lib.avformat_new_stream( self.ptr, cython.NULL @@ -425,6 +456,8 @@ def add_data_stream(self, codec_name=None, options: dict | None = None): :param dict options: Stream options. :rtype: The new :class:`~av.data.stream.DataStream`. """ + self._assert_open() + codec: cython.pointer[cython.const[lib.AVCodec]] = cython.NULL codec_descriptor: cython.pointer[cython.const[lib.AVCodecDescriptor]] = ( cython.NULL @@ -489,6 +522,7 @@ def add_data_stream(self, codec_name=None, options: dict | None = None): @cython.ccall def start_encoding(self): """Write the file header! Called automatically.""" + self._assert_open() if self._myflag & 4: # started return @@ -551,6 +585,8 @@ def supported_codecs(self): """ Returns a set of all codecs this format supports. """ + self._assert_open() + result: set[str] = set() codec: cython.pointer[cython.const[lib.AVCodec]] = cython.NULL opaque: cython.p_void = cython.NULL @@ -604,6 +640,7 @@ def mux(self, packets): self.mux_one(packet) def mux_one(self, packet: Packet): + self._assert_open() if not (self._myflag & 4) and self._buffer_for_extradata(packet): return diff --git a/tests/test_encode.py b/tests/test_encode.py index d4232e108..f9fedc8b6 100644 --- a/tests/test_encode.py +++ b/tests/test_encode.py @@ -690,3 +690,29 @@ def test_metadata_survives_non_utf8_bytes(tmp_path) -> None: assert title.encode("utf-8", "surrogateescape") == raw assert note.encode("utf-8", "surrogateescape") == raw assert title.encode("utf-8", "surrogateescape").decode("latin-1") == "café" + + +@pytest.mark.parametrize("method", ["add_stream", "add_mux_stream"]) +def test_rejected_stream_leaves_container_usable(tmp_path, method: str) -> None: + # A stream cannot be removed from an AVFormatContext, so anything that can + # raise has to be checked before one is created. Otherwise the orphan + # desynchronises container.streams and the next valid call fails. + with av.open(str(tmp_path / "out.mkv"), "w") as output: + add = getattr(output, method) + with pytest.raises(TypeError): + add("aac", rate=44100.5) + assert len(output.streams) == 0 + + stream = add("aac", rate=44100) + assert stream.index == 0 + assert len(output.streams) == 1 + + +def test_rejected_time_base_leaves_container_usable(tmp_path) -> None: + with av.open(str(tmp_path / "out.mkv"), "w") as output: + with pytest.raises(AttributeError): + output.add_stream("mpeg4", rate=24, time_base="not-a-rational") + assert len(output.streams) == 0 + + stream = output.add_stream("mpeg4", rate=24) + assert stream.index == 0 diff --git a/tests/test_open.py b/tests/test_open.py index 60cb60072..534b1e19f 100644 --- a/tests/test_open.py +++ b/tests/test_open.py @@ -2,6 +2,8 @@ import io from pathlib import Path +import pytest + import av from .common import fate_suite @@ -53,3 +55,31 @@ def test_container_no_close() -> None: # Do not close so that container is freed through GC. _container_no_close() gc.collect() + + +def test_output_container_is_closed_after_close(tmp_path) -> None: + path = str(tmp_path / "out.mp4") + container = av.open(path, "w") + stream = container.add_stream("mpeg4", rate=24) + stream.width = stream.height = 64 + stream.pix_fmt = "yuv420p" + container.mux(stream.encode(av.VideoFrame(64, 64, "yuv420p"))) + container.mux(stream.encode(None)) + container.close() + + # Muxing has no avformat_close_input() to null the context for it, so + # everything below used to keep working on a finished file. + for call in ( + lambda: container.add_stream("mpeg4", rate=24), + lambda: container.add_mux_stream("h264"), + lambda: container.add_data_stream(), + lambda: container.add_attachment("a", "text/plain", b"b"), + lambda: container.supported_codecs, + lambda: container.mux(av.Packet(4)), + lambda: container.start_encoding(), + lambda: container.dumps_format(), + ): + with pytest.raises(AssertionError, match="Container is not open"): + call() + + container.close() # idempotent diff --git a/tests/test_python_io.py b/tests/test_python_io.py index a56196bde..293031a2e 100644 --- a/tests/test_python_io.py +++ b/tests/test_python_io.py @@ -116,8 +116,7 @@ def read( assert container.format.name == "mpegts" assert container.format.long_name == "MPEG-TS (MPEG-2 Transport Stream)" assert len(container.streams) == 1 - if seekable: - assert container.size == 800000 + assert container.size == (800000 if seekable else None) assert container.metadata == {} # Check method calls. diff --git a/tests/test_streams.py b/tests/test_streams.py index 0d8b9ba95..187f37241 100644 --- a/tests/test_streams.py +++ b/tests/test_streams.py @@ -229,6 +229,11 @@ def test_data_stream(self) -> None: packet.pts = i packet.stream = data_stream container1.mux(packet) + + # Test string representation, while the container is still open. + repr = f"{data_stream}" + assert repr.startswith("") + container1.close() # Test reading back the data stream @@ -248,10 +253,6 @@ def test_data_stream(self) -> None: for read_packet, original_data in zip(packets, test_data): assert bytes(read_packet) == original_data - # Test string representation - repr = f"{data_stream}" - assert repr.startswith("") - container.close() def test_data_stream_from_template(self) -> None: