diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index 7646b8863..2f379719d 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -29,10 +29,13 @@ #include #include +#include #include #include #include #include +#include +#include #include #include @@ -47,6 +50,45 @@ #define OSC_PW_SONAME "libpipewire-0.3.so.0" +/* + * DRM format modifiers, spelled out rather than pulled from . + * + * They are ABI constants — LINEAR has been 0 and INVALID has been + * ((1ULL << 56) - 1) since the modifier API was introduced — and taking the + * header would put libdrm-dev in the build path of every contributor and CI + * runner for two integers. That is the same trade the dlopen above makes. + * + * These two are the ONLY modifiers this helper advertises, and the reason is + * osc_map_dmabuf(): a linear or implicit buffer can be read through a plain + * mmap of the dmabuf fd, while a tiled or compression-enabled one cannot — its + * bytes are not in raster order, so handing them to the encoder would produce a + * scrambled recording rather than an error. Anything else needs a real GPU + * import (EGL/gbm), which this helper deliberately does not link. + */ +#define OSC_DRM_FORMAT_MOD_LINEAR 0ULL +#define OSC_DRM_FORMAT_MOD_INVALID 0x00ffffffffffffffULL + +/* + * Mapped dmabuf fds, keyed by fd. + * + * PipeWire reuses a small, fixed buffer set for the life of a negotiation, so + * the mapping is established once per buffer in add_buffer and torn down in + * remove_buffer. Doing it per frame instead would mean an mmap and an munmap of + * a full framebuffer 60 times a second. + * + * The bound matches the ceiling we ask for in SPA_PARAM_BUFFERS_buffers + * (CHOICE_RANGE_Int(4, 2, 16)), with headroom; a compositor handing back more + * than it was offered is a protocol violation, but the table refuses to overflow + * rather than trusting that. + */ +#define OSC_MAX_DMABUF_MAPS 32 + +struct osc_dmabuf_map { + int fd; + void *ptr; + size_t len; +}; + /* * Cursor metadata budget: `struct spa_meta_cursor` + `struct spa_meta_bitmap` + * w*h*4 bytes of pixels. @@ -134,6 +176,14 @@ struct osc_pw_session { struct spa_video_info_raw format; int buffer_info_reports; int want_video; + /* Set from the negotiated format's SPA_VIDEO_FLAG_MODIFIER, which is what + * decides whether buffers arrive as dmabuf fds or shared memory. */ + int uses_dmabuf; + struct osc_dmabuf_map dmabuf_maps[OSC_MAX_DMABUF_MAPS]; + /* fd whose DMA_BUF_SYNC_START has not been closed by its END yet, or -1. + * The bracket has to span the on_frame callback, not just osc_read_frame, + * because the callback is where the pixels are actually read. */ + int dmabuf_sync_fd; }; struct osc_pw_audio_api osc_audio_api; @@ -310,6 +360,77 @@ static const struct spa_pod *osc_build_enum_format(struct spa_pod_builder *build &SPA_FRACTION(240, 1))); } +/* + * The same format, plus SPA_FORMAT_VIDEO_modifier — and the modifier property is + * the entire reason this second object exists. + * + * WHAT BREAKS WITHOUT IT. A compositor that can only produce DMA-BUF publishes + * its EnumFormat with `VideoModifier` carrying SPA_POD_PROP_FLAG_MANDATORY. + * spa_pod_filter treats a mandatory producer property that the consumer does not + * mention as fatal for the WHOLE object: + * + * else if ((p1->flags & SPA_POD_PROP_FLAG_MANDATORY) != 0) + * res = -EINVAL; (spa/pod/filter.h:352) + * + * so every one of its formats is filtered out and the link dies reporting + * "no more input formats" — issue #287, niri on Arch, reproduced against the + * vendored headers by osc_pw_enum_format_accepts_dmabuf_producer() below. + * mutter is not affected because it publishes shm pods too, several of them + * without any modifier at all. + * + * ORDER IS THE SAFETY PROPERTY. osc_pw_start sends the shm object FIRST and this + * one second, and pw_stream keeps that as a preference order. A compositor that + * can do shared memory therefore still negotiates shared memory, exactly as + * before this object existed — GNOME and KDE are bit-for-bit unchanged. Only a + * producer with nothing to offer but DMA-BUF reaches this fallback. + * + * NO SPA_POD_PROP_FLAG_DONT_FIXATE. That flag asks the producer to leave the + * modifier unresolved so the consumer can pick one after querying its GPU, and + * it obliges us to renegotiate with a fixated format. We advertise exactly two + * modifiers, both of which are readable through a plain mmap and neither of + * which needs a GPU query, so letting the producer fixate is both simpler and + * one fewer round trip that can go wrong. + */ +static const struct spa_pod *osc_build_enum_format_dmabuf(struct spa_pod_builder *builder) +{ + struct spa_pod_frame object_frame; + struct spa_pod_frame choice_frame; + + spa_pod_builder_push_object(builder, &object_frame, SPA_TYPE_OBJECT_Format, + SPA_PARAM_EnumFormat); + spa_pod_builder_add(builder, SPA_FORMAT_mediaType, SPA_POD_Id(SPA_MEDIA_TYPE_video), + SPA_FORMAT_mediaSubtype, SPA_POD_Id(SPA_MEDIA_SUBTYPE_raw), + SPA_FORMAT_VIDEO_format, + SPA_POD_CHOICE_ENUM_Id(5, SPA_VIDEO_FORMAT_BGRx, SPA_VIDEO_FORMAT_BGRx, + SPA_VIDEO_FORMAT_RGBx, SPA_VIDEO_FORMAT_BGRA, + SPA_VIDEO_FORMAT_RGBA), + 0); + + /* Built with the explicit prop/choice calls rather than the varargs macro + * because the macro has no way to set a property flag, and MANDATORY here is + * what tells the producer we genuinely handle modifiers rather than merely + * tolerating the key. */ + spa_pod_builder_prop(builder, SPA_FORMAT_VIDEO_modifier, SPA_POD_PROP_FLAG_MANDATORY); + spa_pod_builder_push_choice(builder, &choice_frame, SPA_CHOICE_Enum, 0); + /* Default first, then every alternative — the default is repeated, same + * idiom as SPA_POD_CHOICE_ENUM_Id above. */ + spa_pod_builder_long(builder, (int64_t)OSC_DRM_FORMAT_MOD_LINEAR); + spa_pod_builder_long(builder, (int64_t)OSC_DRM_FORMAT_MOD_LINEAR); + spa_pod_builder_long(builder, (int64_t)OSC_DRM_FORMAT_MOD_INVALID); + spa_pod_builder_pop(builder, &choice_frame); + + spa_pod_builder_add( + builder, SPA_FORMAT_VIDEO_size, + SPA_POD_CHOICE_RANGE_Rectangle(&SPA_RECTANGLE(1920, 1080), &SPA_RECTANGLE(1, 1), + &SPA_RECTANGLE(16384, 16384)), + SPA_FORMAT_VIDEO_framerate, + SPA_POD_CHOICE_RANGE_Fraction(&SPA_FRACTION(30, 1), &SPA_FRACTION(0, 1), + &SPA_FRACTION(240, 1)), + 0); + + return spa_pod_builder_pop(builder, &object_frame); +} + /* * The consumer side of the SPA_META_Cursor negotiation, in one place so the * bytes a unit test checks are literally the bytes sent on the wire. @@ -372,6 +493,62 @@ int osc_pw_cursor_meta_accepts_producer_size(uint32_t width, uint32_t height) return spa_pod_filter(&result, &filtered, producer, consumer) < 0 ? 0 : 1; } +/* + * Reproduces issue #287 offline: run spa_pod_filter against a producer object + * shaped the way a DMA-BUF-only compositor publishes one, and report whether our + * EnumFormat survives it. + * + * `with_modifier` selects which of our two objects to test, so the test can + * assert both halves of the contract — the shm object must still be rejected by + * such a producer (otherwise the second object would be pointless and the + * ordering argument in osc_build_enum_format_dmabuf would be untested), and the + * dmabuf object must be accepted. + * + * The producer's modifier property carries SPA_POD_PROP_FLAG_MANDATORY, which is + * what niri emits (src/screencasting/pw_utils.rs) and what turns a missing + * consumer property into -EINVAL for the entire object rather than a merely + * narrower intersection. + */ +int osc_pw_enum_format_accepts_dmabuf_producer(int with_modifier, int64_t producer_modifier) +{ + uint8_t ours_storage[1024]; + uint8_t theirs_storage[1024]; + uint8_t result_storage[2048]; + struct spa_pod_builder ours = SPA_POD_BUILDER_INIT(ours_storage, sizeof(ours_storage)); + struct spa_pod_builder theirs = SPA_POD_BUILDER_INIT(theirs_storage, sizeof(theirs_storage)); + struct spa_pod_builder result = SPA_POD_BUILDER_INIT(result_storage, sizeof(result_storage)); + struct spa_pod_frame object_frame; + struct spa_pod_frame choice_frame; + struct spa_pod *filtered = NULL; + const struct spa_pod *consumer; + const struct spa_pod *producer; + + consumer = with_modifier ? osc_build_enum_format_dmabuf(&ours) : osc_build_enum_format(&ours); + if (consumer == NULL) { + return -1; + } + + spa_pod_builder_push_object(&theirs, &object_frame, SPA_TYPE_OBJECT_Format, + SPA_PARAM_EnumFormat); + spa_pod_builder_add(&theirs, SPA_FORMAT_mediaType, SPA_POD_Id(SPA_MEDIA_TYPE_video), + SPA_FORMAT_mediaSubtype, SPA_POD_Id(SPA_MEDIA_SUBTYPE_raw), + SPA_FORMAT_VIDEO_format, SPA_POD_Id(SPA_VIDEO_FORMAT_BGRx), 0); + spa_pod_builder_prop(&theirs, SPA_FORMAT_VIDEO_modifier, SPA_POD_PROP_FLAG_MANDATORY); + spa_pod_builder_push_choice(&theirs, &choice_frame, SPA_CHOICE_Enum, 0); + spa_pod_builder_long(&theirs, producer_modifier); + spa_pod_builder_long(&theirs, producer_modifier); + spa_pod_builder_pop(&theirs, &choice_frame); + spa_pod_builder_add(&theirs, SPA_FORMAT_VIDEO_size, SPA_POD_Rectangle(&SPA_RECTANGLE(2560, 1080)), + SPA_FORMAT_VIDEO_framerate, SPA_POD_Fraction(&SPA_FRACTION(59978, 1000)), + 0); + producer = spa_pod_builder_pop(&theirs, &object_frame); + if (producer == NULL) { + return -1; + } + + return spa_pod_filter(&result, &filtered, producer, consumer) < 0 ? 0 : 1; +} + static void osc_on_param_changed(void *userdata, uint32_t id, const struct spa_pod *param) { struct osc_pw_session *session = userdata; @@ -403,27 +580,43 @@ static void osc_on_param_changed(void *userdata, uint32_t id, const struct spa_p session->callbacks.on_format(session->callbacks.user, &reported); } + /* + * The negotiated format tells us which kind of buffer to ask for. A format + * carrying SPA_VIDEO_FLAG_MODIFIER came from the DMA-BUF EnumFormat object, + * so the producer is going to hand out dmabuf fds and asking for shared + * memory would intersect to nothing. + */ + session->uses_dmabuf = (session->format.flags & SPA_VIDEO_FLAG_MODIFIER) != 0; + /* * No `size`/`stride` constraint is published: the compositor's own choice is * fine, and osc_read_frame validates whatever comes back. * * The dataType set differs by mode, and the difference is load-bearing. * Cursor-only advertises everything, so that on_buffer_info reports what the - * compositor would PREFER rather than what we forced it into. Video mode - * advertises shared memory only: pw_stream does not map DmaBuf even with - * PW_STREAM_FLAG_MAP_BUFFERS, so accepting one would leave `datas[0].data` - * NULL and produce a recording of nothing. Importing DmaBuf properly is its - * own piece of work; until then, not offering it is what makes the - * compositor fall back to memfd instead. + * compositor would PREFER rather than what we forced it into. + * + * Video mode follows the format that was just negotiated. It used to + * advertise shared memory unconditionally, on the reasoning that "not + * offering DmaBuf is what makes the compositor fall back to memfd" — true of + * mutter and KWin, false of niri and every other Smithay/wlroots compositor + * that has no memfd path at all. Against those, this is the second wall + * behind the EnumFormat modifier: fixing only the format would move the + * failure from "no more input formats" to an empty buffer intersection. + * + * pw_stream still does not map dmabuf itself even with + * PW_STREAM_FLAG_MAP_BUFFERS, so `datas[0].data` stays NULL and the mapping + * is ours to do — see osc_map_dmabuf and osc_on_add_buffer. */ params[0] = spa_pod_builder_add_object( &builder, SPA_TYPE_OBJECT_ParamBuffers, SPA_PARAM_Buffers, SPA_PARAM_BUFFERS_buffers, SPA_POD_CHOICE_RANGE_Int(4, 2, 16), SPA_PARAM_BUFFERS_blocks, SPA_POD_Int(1), SPA_PARAM_BUFFERS_dataType, - SPA_POD_CHOICE_FLAGS_Int(session->want_video - ? ((1 << SPA_DATA_MemPtr) | (1 << SPA_DATA_MemFd)) - : ((1 << SPA_DATA_MemPtr) | (1 << SPA_DATA_MemFd) | - (1 << SPA_DATA_DmaBuf)))); + SPA_POD_CHOICE_FLAGS_Int( + session->want_video + ? (session->uses_dmabuf ? (1 << SPA_DATA_DmaBuf) + : ((1 << SPA_DATA_MemPtr) | (1 << SPA_DATA_MemFd))) + : ((1 << SPA_DATA_MemPtr) | (1 << SPA_DATA_MemFd) | (1 << SPA_DATA_DmaBuf)))); params[1] = spa_pod_builder_add_object( &builder, SPA_TYPE_OBJECT_ParamMeta, SPA_PARAM_Meta, SPA_PARAM_META_type, @@ -448,6 +641,148 @@ static void osc_on_param_changed(void *userdata, uint32_t id, const struct spa_p api.stream_update_params(session->stream, params, SPA_N_ELEMENTS(params)); } +/* + * Map a dmabuf fd for CPU reads. + * + * This is the cheap import, and it is only correct because of what + * osc_build_enum_format_dmabuf advertises. A dmabuf whose modifier is LINEAR or + * INVALID is in raster order, so the bytes behind a plain mmap are the bytes the + * encoder wants. A tiled or DCC-compressed buffer is not, and reading one this + * way yields a scrambled image rather than a failure — which is exactly why + * those modifiers are never offered. The real alternative is an EGL/gbm import, + * a GPU context and two more link-time dependencies in a helper that + * deliberately dlopens everything. + * + * mmap on a dmabuf fd is optional for the exporter (it requires a .mmap in the + * dma_buf_ops), so this can legitimately fail on some drivers. It returns NULL + * and the caller turns that into a visible error rather than a black recording. + */ +static void *osc_map_dmabuf(int fd, size_t len) +{ + void *ptr; + + if (fd < 0 || len == 0) { + return NULL; + } + ptr = mmap(NULL, len, PROT_READ, MAP_SHARED, fd, 0); + return ptr == MAP_FAILED ? NULL : ptr; +} + +static void *osc_find_dmabuf_map(struct osc_pw_session *session, int fd) +{ + size_t i; + + for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { + if (session->dmabuf_maps[i].ptr != NULL && session->dmabuf_maps[i].fd == fd) { + return session->dmabuf_maps[i].ptr; + } + } + return NULL; +} + +/* + * CPU access to a dmabuf has to be bracketed by DMA_BUF_IOCTL_SYNC, or the + * driver is under no obligation to have flushed the GPU's writes into the + * mapping. Skipping it does not fail — it tears, intermittently, which is the + * worst way for this to be wrong. + * + * Best-effort by design: a driver that does not implement the ioctl returns + * ENOTTY, and refusing the frame over that would be worse than reading it. + */ +static void osc_dmabuf_sync(int fd, int start) +{ + struct dma_buf_sync sync; + + if (fd < 0) { + return; + } + memset(&sync, 0, sizeof(sync)); + sync.flags = (start ? DMA_BUF_SYNC_START : DMA_BUF_SYNC_END) | DMA_BUF_SYNC_READ; + while (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) == -1 && errno == EINTR) { + /* retry */ + } +} + +static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) +{ + struct osc_pw_session *session = userdata; + struct spa_data *data; + size_t i; + + if (pw_buf == NULL || pw_buf->buffer == NULL || pw_buf->buffer->n_datas < 1) { + return; + } + data = &pw_buf->buffer->datas[0]; + if (data->type != SPA_DATA_DmaBuf) { + return; + } + + for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { + if (session->dmabuf_maps[i].ptr != NULL) { + continue; + } + /* `maxsize` is the producer's own statement of how much of the fd + * belongs to this buffer, and mapping exactly that keeps the bounds + * checks in osc_read_frame meaningful. */ + session->dmabuf_maps[i].ptr = osc_map_dmabuf((int)data->fd, data->maxsize); + if (session->dmabuf_maps[i].ptr == NULL) { + /* Reported once, through the buffer-info channel that already exists + * for describing what the compositor handed us — a mapping failure + * here means no frames at all, and silence would read as a hang. */ + if (session->callbacks.on_buffer_info != NULL && + session->buffer_info_reports < OSC_BUFFER_INFO_REPORTS) { + session->buffer_info_reports++; + session->callbacks.on_buffer_info( + session->callbacks.user, data->type, pw_buf->buffer->n_datas, 0, 0, + "dmabuf mmap failed: this driver does not allow CPU mapping of the " + "capture buffer; capture cannot proceed"); + } + return; + } + session->dmabuf_maps[i].fd = (int)data->fd; + session->dmabuf_maps[i].len = data->maxsize; + return; + } +} + +static void osc_on_remove_buffer(void *userdata, struct pw_buffer *pw_buf) +{ + struct osc_pw_session *session = userdata; + struct spa_data *data; + size_t i; + + if (pw_buf == NULL || pw_buf->buffer == NULL || pw_buf->buffer->n_datas < 1) { + return; + } + data = &pw_buf->buffer->datas[0]; + for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { + if (session->dmabuf_maps[i].ptr == NULL || + session->dmabuf_maps[i].fd != (int)data->fd) { + continue; + } + munmap(session->dmabuf_maps[i].ptr, session->dmabuf_maps[i].len); + session->dmabuf_maps[i].ptr = NULL; + session->dmabuf_maps[i].fd = -1; + session->dmabuf_maps[i].len = 0; + return; + } +} + +static void osc_unmap_all_dmabufs(struct osc_pw_session *session) +{ + size_t i; + + for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { + if (session->dmabuf_maps[i].ptr == NULL) { + continue; + } + munmap(session->dmabuf_maps[i].ptr, session->dmabuf_maps[i].len); + session->dmabuf_maps[i].ptr = NULL; + session->dmabuf_maps[i].fd = -1; + session->dmabuf_maps[i].len = 0; + } +} + static void osc_on_state_changed(void *userdata, enum pw_stream_state old, enum pw_stream_state state, const char *error) { @@ -565,6 +900,8 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe int32_t stride; int32_t height; + const uint8_t *base; + memset(out, 0, sizeof(*out)); out->pts_ns = -1; @@ -572,15 +909,29 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe return 0; } data = &buffer->datas[0]; - /* - * NULL means the buffer was never mapped: either this is a cursor-only - * session (no PW_STREAM_FLAG_MAP_BUFFERS) or the compositor handed us a - * DmaBuf, which pw_stream does not map even with the flag. Neither is an - * error here — the format negotiation excludes DmaBuf when want_video is - * set, so in practice this is the cursor-only case. - */ - if (data->data == NULL || data->chunk == NULL) { + if (data->chunk == NULL) { + return 0; + } + + if (data->type == SPA_DATA_DmaBuf) { + /* + * pw_stream never populates `datas[0].data` for a dmabuf, so the base + * pointer comes from our own mapping, established once per buffer in + * osc_on_add_buffer. A miss means the mmap failed there — reported at + * that point — and there is nothing readable here. + */ + base = osc_find_dmabuf_map(session, (int)data->fd); + if (base == NULL) { + return 0; + } + } else if (data->data == NULL) { + /* + * NULL on a shared-memory buffer means it was never mapped, which is the + * cursor-only case: those sessions do not set PW_STREAM_FLAG_MAP_BUFFERS. + */ return 0; + } else { + base = data->data; } /* A zero-sized chunk is how a compositor ships a cursor update with no new * frame attached. Not an error, just not a frame. */ @@ -602,7 +953,17 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe return 0; } - out->data = SPA_PTROFF(data->data, offset, const uint8_t); + /* + * Open the CPU-access window on a dmabuf and leave it open: the pixels are + * read by the on_frame callback, not here, so the matching SYNC_END lives in + * osc_inspect_buffer once that callback has returned. + */ + if (data->type == SPA_DATA_DmaBuf) { + session->dmabuf_sync_fd = (int)data->fd; + osc_dmabuf_sync(session->dmabuf_sync_fd, 1); + } + + out->data = SPA_PTROFF(base, offset, const uint8_t); out->size = size; out->stride = stride; out->width = (int32_t)session->format.size.width; @@ -736,9 +1097,17 @@ static void osc_inspect_buffer(struct osc_pw_session *session, const struct spa_ if (session->want_video && session->callbacks.on_frame != NULL) { struct osc_pw_frame frame; + session->dmabuf_sync_fd = -1; if (osc_read_frame(session, buffer, &frame)) { session->callbacks.on_frame(session->callbacks.user, &frame); } + /* Closes the DMA_BUF_SYNC_START osc_read_frame opened, if any. Placed + * here rather than inside it because the callback above is what actually + * touches the pixels, and the window has to cover the read. */ + if (session->dmabuf_sync_fd >= 0) { + osc_dmabuf_sync(session->dmabuf_sync_fd, 0); + session->dmabuf_sync_fd = -1; + } } } @@ -778,6 +1147,8 @@ static const struct pw_stream_events osc_stream_events = { PW_VERSION_STREAM_EVENTS, .state_changed = osc_on_state_changed, .param_changed = osc_on_param_changed, + .add_buffer = osc_on_add_buffer, + .remove_buffer = osc_on_remove_buffer, .process = osc_on_process, }; @@ -786,9 +1157,11 @@ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, size_t err_len) { struct osc_pw_session *session; - uint8_t buffer[1024]; + /* Two EnumFormat objects now, and the DMA-BUF one carries an extra choice — + * sized so a builder overflow stays impossible rather than merely unlikely. */ + uint8_t buffer[2048]; struct spa_pod_builder builder = SPA_POD_BUILDER_INIT(buffer, sizeof(buffer)); - const struct spa_pod *params[1]; + const struct spa_pod *params[2]; int result; if (api_handle == NULL) { @@ -805,6 +1178,10 @@ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, } session->callbacks = *callbacks; session->want_video = want_video; + /* calloc zeroes these, and 0 is a legitimate fd — so the "nothing pending" + * sentinel has to be set explicitly. dmabuf_maps is keyed on ptr != NULL, + * which calloc does get right. */ + session->dmabuf_sync_fd = -1; session->loop = api.thread_loop_new("openscreen-pipewire", NULL); if (session->loop == NULL) { @@ -847,9 +1224,18 @@ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, api.stream_add_listener(session->stream, &session->stream_listener, &osc_stream_events, session); + /* + * Shared memory FIRST, DMA-BUF second, and the order is the compatibility + * guarantee: pw_stream keeps this as a preference list, so a compositor able + * to produce shm still picks shm and nothing changes on GNOME or KDE. The + * second object only ever wins against a producer that has no shm path — + * niri and the other Smithay/wlroots compositors of issue #287, which + * previously failed the whole negotiation with "no more input formats". + */ params[0] = osc_build_enum_format(&builder); - if (params[0] == NULL) { - osc_set_error(err, err_len, "the EnumFormat POD did not fit its builder"); + params[1] = osc_build_enum_format_dmabuf(&builder); + if (params[0] == NULL || params[1] == NULL) { + osc_set_error(err, err_len, "the EnumFormat PODs did not fit their builder"); goto fail; } @@ -890,7 +1276,7 @@ struct osc_pw_session *osc_pw_start(int fd, uint32_t node_id, int want_video, want_video ? (PW_STREAM_FLAG_AUTOCONNECT | PW_STREAM_FLAG_MAP_BUFFERS) : PW_STREAM_FLAG_AUTOCONNECT, - params, 1); + params, SPA_N_ELEMENTS(params)); if (result < 0) { osc_set_error(err, err_len, "pw_stream_connect failed: %s", spa_strerror(result)); goto fail; @@ -935,6 +1321,10 @@ void osc_pw_stop(struct osc_pw_session *session) api.stream_disconnect(session->stream); api.stream_destroy(session->stream); } + /* After stream_destroy: remove_buffer fires during teardown and unmaps most + * of these itself. This is the backstop for anything it did not reach, and + * it runs once the loop is joined so nothing can be mapping concurrently. */ + osc_unmap_all_dmabufs(session); if (session->core != NULL) { api.core_disconnect(session->core); } diff --git a/electron/native/pipewire-capture/csrc/pw_shim.h b/electron/native/pipewire-capture/csrc/pw_shim.h index cabaace3c..ab6f78305 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.h +++ b/electron/native/pipewire-capture/csrc/pw_shim.h @@ -158,6 +158,19 @@ void osc_pw_constants(struct osc_pw_constants *out); */ int osc_pw_cursor_meta_accepts_producer_size(uint32_t width, uint32_t height); +/* + * Would our EnumFormat survive negotiation against a DMA-BUF-only producer that + * declares `producer_modifier` as MANDATORY? 1 yes, 0 no, -1 if the PODs could + * not be built. + * + * `with_modifier` picks which of our two EnumFormat objects to test: 0 for the + * shared-memory one sent first, 1 for the DMA-BUF one sent as a fallback. Such a + * producer must reject the former and accept the latter — that asymmetry is the + * whole fix for issue #287, and this is how it is asserted without niri, a + * portal or a screen. + */ +int osc_pw_enum_format_accepts_dmabuf_producer(int with_modifier, int64_t producer_modifier); + struct osc_pw_session; /* diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index 4e8fecdb8..8a4806ca4 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -97,6 +97,11 @@ extern "C" { /// producer, but the unit tests do. The C side is always compiled. #[cfg(test)] fn osc_pw_cursor_meta_accepts_producer_size(width: u32, height: u32) -> i32; + #[cfg(test)] + fn osc_pw_enum_format_accepts_dmabuf_producer( + with_modifier: i32, + producer_modifier: i64, + ) -> i32; fn osc_pw_start( fd: i32, node_id: u32, @@ -590,6 +595,22 @@ pub fn cursor_meta_accepts_producer_size(width: u32, height: u32) -> i32 { unsafe { osc_pw_cursor_meta_accepts_producer_size(width, height) } } +/// DRM format modifiers, as spelled in `pw_shim.c`. +#[cfg(test)] +pub const DRM_FORMAT_MOD_LINEAR: i64 = 0; +#[cfg(test)] +pub const DRM_FORMAT_MOD_INVALID: i64 = 0x00ff_ffff_ffff_ffff; + +/// Would our EnumFormat survive negotiation against a DMA-BUF-only producer +/// declaring `producer_modifier` as MANDATORY? `with_modifier` selects which of +/// our two objects to test: `false` for the shared-memory one, `true` for the +/// DMA-BUF fallback. +#[cfg(test)] +pub fn enum_format_accepts_dmabuf_producer(with_modifier: bool, producer_modifier: i64) -> i32 { + // SAFETY: no arguments to validate; the shim builds and frees its own PODs. + unsafe { osc_pw_enum_format_accepts_dmabuf_producer(i32::from(with_modifier), producer_modifier) } +} + /// SPA enum values as compiled from the vendored headers. pub fn constants() -> Constants { let mut out = Constants::default(); @@ -865,6 +886,49 @@ mod tests { ); } + /// Issue #287, reproduced without niri, a portal or a screen. + /// + /// A DMA-BUF-only compositor publishes its EnumFormat with + /// `SPA_FORMAT_VIDEO_modifier` carrying `SPA_POD_PROP_FLAG_MANDATORY`. + /// `spa_pod_filter` (spa/pod/filter.h:352) turns a mandatory producer + /// property the consumer never mentions into `-EINVAL` for the WHOLE object, + /// so every format is filtered out and the link dies reporting "no more + /// input formats" — the exact string from the report, on Arch + niri, on + /// both 1.8.0 and 1.9.0-rc.3. + /// + /// The fix is a second EnumFormat object that does declare a modifier, sent + /// after the shared-memory one so compositors that can do shm are unaffected. + /// Both halves are asserted here, because "the new object is accepted" alone + /// would still hold if the old one had been silently made to match too — + /// and that would mean GNOME had quietly moved to the DMA-BUF path. + #[test] + fn enum_format_survives_a_dmabuf_only_producer() { + for modifier in [DRM_FORMAT_MOD_LINEAR, DRM_FORMAT_MOD_INVALID] { + assert_eq!( + enum_format_accepts_dmabuf_producer(false, modifier), + 0, + "the shm object must still be rejected by a mandatory-modifier producer \ + (modifier {modifier:#x}) — that rejection is why the second object exists" + ); + assert_eq!( + enum_format_accepts_dmabuf_producer(true, modifier), + 1, + "the dmabuf object must intersect a producer declaring modifier {modifier:#x}" + ); + } + + // The advertised modifier set is a real set, not a wildcard: a tiled or + // compressed buffer cannot be read through a plain mmap, so it must fail + // negotiation rather than be accepted and decoded into garbage. + // 0x0300000000000001 = a vendor (AMD) modifier, neither LINEAR nor INVALID. + assert_eq!( + enum_format_accepts_dmabuf_producer(true, 0x0300_0000_0000_0001), + 0, + "a modifier we cannot mmap must not intersect — accepting it would ship \ + a scrambled recording instead of an error" + ); + } + /// End-to-end exercise of the PipeWire half with NO portal involved. /// /// `pw_context_connect_fd` accepts any socket already connected to a