diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index 82a8a129e..8dcac71aa 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -72,7 +73,7 @@ static int osc_debug_enabled(void); #define OSC_DRM_FORMAT_MOD_INVALID 0x00ffffffffffffffULL /* - * Mapped dmabuf fds, keyed by fd. + * Mapped dmabuf planes, keyed by (fd, mapoffset). * * 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 @@ -85,11 +86,15 @@ static int osc_debug_enabled(void); * rather than trusting that. */ #define OSC_MAX_DMABUF_MAPS 32 +#define OSC_FRAME_DROP_REPORTS 5 +#define OSC_VIDEO_BYTES_PER_PIXEL 4 struct osc_dmabuf_map { int fd; + uint32_t mapoffset; void *ptr; size_t len; + size_t refs; }; /* @@ -118,7 +123,8 @@ struct osc_dmabuf_map { * unit test can assert instead of something a maintainer rediscovers. */ #define OSC_CURSOR_META_SIZE(w, h) \ - (sizeof(struct spa_meta_cursor) + sizeof(struct spa_meta_bitmap) + (size_t)(w) * (h) * 4) + (sizeof(struct spa_meta_cursor) + sizeof(struct spa_meta_bitmap) + \ + (size_t)(w) * (h) * OSC_VIDEO_BYTES_PER_PIXEL) /* * How many buffers to describe before going quiet. @@ -183,6 +189,8 @@ struct osc_pw_session { * decides whether buffers arrive as dmabuf fds or shared memory. */ int uses_dmabuf; struct osc_dmabuf_map dmabuf_maps[OSC_MAX_DMABUF_MAPS]; + int frame_drop_reports; + int capture_issue_reports; /* 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. */ @@ -651,6 +659,8 @@ static void osc_on_param_changed(void *userdata, uint32_t id, const struct spa_p * with them. Re-arm the reports so the instrumentation describes the buffer * set actually in use rather than a set that no longer exists. */ session->buffer_info_reports = 0; + session->frame_drop_reports = 0; + session->capture_issue_reports = 0; api.stream_update_params(session->stream, params, SPA_N_ELEMENTS(params)); } @@ -688,34 +698,50 @@ static int osc_debug_enabled(void) return cached; } -/* - * `why` receives a caller-reportable reason on failure. The three ways this can - * fail are not interchangeable, and conflating them sent the one real - * investigation of this path looking at the GPU driver for an hour. - */ -static void *osc_map_dmabuf(int fd, size_t *len, const char **why) +int osc_pw_dmabuf_mapped_len(size_t allocation_len, uint32_t mapoffset, size_t *mapped_len) +{ + if (mapped_len == NULL || (size_t)mapoffset >= allocation_len) { + return 0; + } + *mapped_len = allocation_len - (size_t)mapoffset; + return 1; +} + +static void *osc_map_dmabuf(int fd, uint32_t data_flags, uint32_t mapoffset, size_t *len, + const char **why) { void *ptr; + off_t probed; if (fd < 0) { *why = "the compositor handed us a DMA-BUF plane with no file descriptor"; return NULL; } /* - * A DmaBuf plane legitimately carries maxsize = 0: the size of a dmabuf is a - * property of the exporting buffer, not of the SPA descriptor, and wlroots - * leaves it unset. Every dmabuf fd is seekable to its own length, which is - * the documented way to recover it. Without this the mmap was never even - * attempted and the failure was reported as "this driver does not allow CPU - * mapping" — blaming the GPU for a size the producer simply had not filled in. + * A DmaBuf plane can carry an advisory maxsize rather than its allocation + * length. The allocation size is a property of the exporting fd — dma_buf's + * llseek returns exactly dmabuf->size — so when the fd answers, it is the + * authority and maxsize is not. + * + * It has to win in BOTH directions. Preferring it only when it is LARGER + * leaves an over-declared maxsize as the mmap length, and the kernel refuses + * a dmabuf mapping longer than the object (-EINVAL), so the import dies + * reporting a driver that will not map — which is precisely the misdiagnosis + * this code exists to stop, with the right answer already in hand. */ - if (*len == 0) { - off_t probed = lseek(fd, 0, SEEK_END); - if (probed > 0) { - *len = (size_t)probed; - if (osc_debug_enabled()) { - fprintf(stderr, "[osc-dmabuf] maxsize=0, recovered %zu bytes via lseek\n", *len); - } + probed = lseek(fd, 0, SEEK_END); + if (probed > 0 && (uintmax_t)probed <= SIZE_MAX) { + size_t advertised = *len; + + if (!osc_pw_dmabuf_mapped_len((size_t)probed, mapoffset, len)) { + *why = "the DMA-BUF plane's mapoffset is outside the fd allocation"; + return NULL; + } + if (advertised != *len && osc_debug_enabled()) { + fprintf(stderr, + "[osc-dmabuf] maxsize=%zu, fd reports %jd bytes, mapoffset=%u, " + "mapped_len=%zu\n", + advertised, (intmax_t)probed, mapoffset, *len); } } if (*len == 0) { @@ -725,33 +751,246 @@ static void *osc_map_dmabuf(int fd, size_t *len, const char **why) *why = "the DMA-BUF plane reports no size and its fd is not seekable"; return NULL; } - ptr = mmap(NULL, *len, PROT_READ, MAP_SHARED, fd, 0); + ptr = mmap(NULL, *len, PROT_READ, MAP_SHARED, fd, (off_t)mapoffset); if (osc_debug_enabled()) { if (ptr == MAP_FAILED) { - fprintf(stderr, "[osc-dmabuf] mmap fd=%d len=%zu FAILED errno=%d (%s)\n", fd, *len, - errno, strerror(errno)); + fprintf(stderr, + "[osc-dmabuf] mmap fd=%d mapoffset=%u len=%zu FAILED errno=%d (%s)\n", fd, + mapoffset, *len, errno, strerror(errno)); } else { - fprintf(stderr, "[osc-dmabuf] mmap fd=%d len=%zu ok\n", fd, *len); + fprintf(stderr, "[osc-dmabuf] mmap fd=%d mapoffset=%u len=%zu ok\n", fd, mapoffset, + *len); } } if (ptr == MAP_FAILED) { - *why = "this driver does not allow CPU mapping of the capture buffer"; + /* The producer already answered this question. SPA_DATA_FLAG_MAPPABLE + * exists because "some memory types are not simply mappable (DmaBuf) + * unless explicitly specified with this flag" — so when it is unset, + * blaming the GPU driver sends the reader somewhere else entirely. */ + *why = SPA_FLAG_IS_SET(data_flags, SPA_DATA_FLAG_MAPPABLE) + ? "this driver does not allow CPU mapping of the capture buffer" + : "the compositor did not mark this DMA-BUF plane mappable " + "(SPA_DATA_FLAG_MAPPABLE unset)"; } return ptr == MAP_FAILED ? NULL : ptr; } -static void *osc_find_dmabuf_map(struct osc_pw_session *session, int fd) +static struct osc_dmabuf_map *osc_find_dmabuf_map(struct osc_pw_session *session, int fd, + uint32_t mapoffset) { 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; + if (session->dmabuf_maps[i].ptr != NULL && session->dmabuf_maps[i].fd == fd && + session->dmabuf_maps[i].mapoffset == mapoffset) { + return &session->dmabuf_maps[i]; } } return NULL; } +static void osc_retain_dmabuf_map(struct osc_dmabuf_map *map) +{ + map->refs++; +} + +static int osc_release_dmabuf_map(struct osc_dmabuf_map *map) +{ + if (map->refs > 1) { + map->refs--; + return 0; + } + return 1; +} + +int osc_pw_dmabuf_map_lifecycle_valid(void) +{ + struct osc_pw_session session; + struct osc_dmabuf_map *map; + + memset(&session, 0, sizeof(session)); + map = &session.dmabuf_maps[0]; + map->fd = 7; + map->mapoffset = 4096; + map->ptr = (void *)(uintptr_t)1; + map->len = 8192; + map->refs = 1; + + if (osc_find_dmabuf_map(&session, 7, 4096) != map || + osc_find_dmabuf_map(&session, 7, 0) != NULL) { + return 0; + } + osc_retain_dmabuf_map(map); + if (map->refs != 2 || osc_release_dmabuf_map(map) != 0 || map->refs != 1) { + return 0; + } + return osc_release_dmabuf_map(map) == 1; +} + +enum osc_frame_bounds_error { + OSC_FRAME_BOUNDS_OK, + /* Not a failure: a buffer carrying metadata and no pixel rows. */ + OSC_FRAME_BOUNDS_METADATA_ONLY, + OSC_FRAME_BOUNDS_NO_CAPACITY, + OSC_FRAME_BOUNDS_OFFSET, + OSC_FRAME_BOUNDS_CORRUPTED, + OSC_FRAME_BOUNDS_GEOMETRY, + OSC_FRAME_BOUNDS_ROW_TOO_SHORT, + OSC_FRAME_BOUNDS_FRAME_TOO_LARGE, +}; + +static enum osc_frame_bounds_error osc_resolve_frame_bounds( + uint32_t data_type, uint32_t maxsize, size_t mapped_len, uint32_t chunk_offset, + uint32_t chunk_size, int32_t chunk_flags, int32_t stride, int32_t width, int32_t height, + size_t *available_out, size_t *offset_out, size_t *size_out) +{ + size_t available; + size_t offset; + size_t size; + uint64_t row_bytes; + uint64_t frame_bytes; + + available = data_type == SPA_DATA_DmaBuf ? mapped_len : (size_t)maxsize; + *available_out = available; + /* Unlike advisory chunk_size, stride distinguishes metadata-only buffers. */ + if (stride == 0) { + return OSC_FRAME_BOUNDS_METADATA_ONLY; + } + if (available == 0) { + return OSC_FRAME_BOUNDS_NO_CAPACITY; + } + offset = chunk_offset; + if (offset > available) { + return OSC_FRAME_BOUNDS_OFFSET; + } + if (SPA_FLAG_IS_SET(chunk_flags, SPA_CHUNK_FLAG_CORRUPTED)) { + return OSC_FRAME_BOUNDS_CORRUPTED; + } + if (stride < 0 || width <= 0 || height <= 0) { + return OSC_FRAME_BOUNDS_GEOMETRY; + } + row_bytes = (uint64_t)width * OSC_VIDEO_BYTES_PER_PIXEL; + if ((uint64_t)stride < row_bytes) { + return OSC_FRAME_BOUNDS_ROW_TOO_SHORT; + } + /* Widen before multiplying: both operands originate outside this process. */ + frame_bytes = (uint64_t)stride * (uint64_t)height; + /* DMA-BUF chunk_size is advisory; shared-memory chunk_size is a bound. */ + if (data_type == SPA_DATA_DmaBuf) { + size = available - offset; + } else { + /* Offset was validated above, so this subtraction cannot underflow. */ + size = SPA_MIN((size_t)chunk_size, available - offset); + } + if (frame_bytes > (uint64_t)size) { + return OSC_FRAME_BOUNDS_FRAME_TOO_LARGE; + } + *offset_out = offset; + *size_out = size; + return OSC_FRAME_BOUNDS_OK; +} + +static const char *osc_frame_bounds_error_name(enum osc_frame_bounds_error error) +{ + switch (error) { + case OSC_FRAME_BOUNDS_METADATA_ONLY: + return "metadata-only"; + case OSC_FRAME_BOUNDS_NO_CAPACITY: + return "buffer-length-zero"; + case OSC_FRAME_BOUNDS_OFFSET: + return "chunk-offset-out-of-bounds"; + case OSC_FRAME_BOUNDS_CORRUPTED: + return "producer-marked-frame-corrupted"; + case OSC_FRAME_BOUNDS_GEOMETRY: + return "invalid-frame-geometry"; + case OSC_FRAME_BOUNDS_ROW_TOO_SHORT: + return "stride-shorter-than-row"; + case OSC_FRAME_BOUNDS_FRAME_TOO_LARGE: + return "frame-bytes-exceed-available-chunk"; + case OSC_FRAME_BOUNDS_OK: + return "none"; + } + return "unknown"; +} + +const char *osc_pw_frame_bounds_reason(uint32_t data_type, uint32_t maxsize, size_t mapped_len, + uint32_t chunk_offset, uint32_t chunk_size, + int32_t chunk_flags, int32_t stride, int32_t width, + int32_t height) +{ + size_t available = 0; + size_t offset = 0; + size_t size = 0; + + return osc_frame_bounds_error_name(osc_resolve_frame_bounds( + data_type, maxsize, mapped_len, chunk_offset, chunk_size, chunk_flags, stride, width, + height, &available, &offset, &size)); +} + +static const char *osc_data_type_name(uint32_t data_type) +{ + switch (data_type) { + case SPA_DATA_MemPtr: + return "MemPtr"; + case SPA_DATA_MemFd: + return "MemFd"; + case SPA_DATA_DmaBuf: + return "DmaBuf"; + default: + return "Unknown"; + } +} + +/* + * The chunk fields arrive as values, not as a `chunk` pointer to re-read. This + * only ever runs when the producer has already misbehaved, which is exactly the + * moment it may be rewriting that struct; reading it a second time here would + * let the line describe a frame other than the one that was refused. + */ +static void osc_report_frame_drop(struct osc_pw_session *session, const struct spa_data *data, + size_t mapped_len, size_t available, uint32_t chunk_offset, + uint32_t chunk_size, int32_t chunk_flags, int32_t stride, + enum osc_frame_bounds_error error) +{ + const char *reason = osc_frame_bounds_error_name(error); + + if (session->frame_drop_reports >= OSC_FRAME_DROP_REPORTS) { + return; + } + session->frame_drop_reports++; + + /* + * On the event stream, not only in a debug log. Every reason this reports is + * decided by stride, geometry and the mapped length, all of which are fixed + * for a whole negotiation — so a buffer that fails once fails every time and + * the user gets a recording with no frames in it. Nothing downstream can + * name the cause: a refused frame never reaches the mailbox, so the + * frames-dropped counter stays at zero and the session still stops + * "successfully". Requiring OPENSCREEN_PIPEWIRE_DEBUG and a reproduction to + * learn why is the diagnosis gap this whole change set exists to close. + */ + if (session->callbacks.on_capture_issue != NULL) { + char detail[320]; + + snprintf(detail, sizeof(detail), + "the compositor's buffer failed validation (%s), so this frame was dropped: " + "dataType=%s stride=%d width=%u height=%u chunkOffset=%u chunkSize=%u " + "available=%zu", + reason, osc_data_type_name(data->type), stride, session->format.size.width, + session->format.size.height, chunk_offset, chunk_size, available); + session->callbacks.on_capture_issue(session->callbacks.user, "frame-dropped", detail); + } + if (osc_debug_enabled()) { + fprintf(stderr, + "[osc-frame-drop] reason=%s data_type=%s data_type_id=%u fd=%lld maxsize=%u " + "mapped_len=%zu available_len=%zu chunk_offset=%u chunk_size=%u " + "chunk_flags=%d stride=%d width=%u height=%u format=%u\n", + reason, osc_data_type_name(data->type), data->type, (long long)data->fd, + data->maxsize, mapped_len, available, chunk_offset, chunk_size, chunk_flags, stride, + session->format.size.width, session->format.size.height, session->format.format); + } +} + /* * 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 @@ -775,10 +1014,22 @@ static void osc_dmabuf_sync(int fd, int start) } } +static void osc_report_capture_issue(struct osc_pw_session *session, const char *code, + const char *detail) +{ + if (session->callbacks.on_capture_issue == NULL || + session->capture_issue_reports >= OSC_BUFFER_INFO_REPORTS) { + return; + } + session->capture_issue_reports++; + session->callbacks.on_capture_issue(session->callbacks.user, code, detail); +} + static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) { struct osc_pw_session *session = userdata; const char *why = "unknown reason"; + struct osc_dmabuf_map *existing; struct spa_data *data; size_t maplen; size_t i; @@ -790,45 +1041,55 @@ static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) if (data->type != SPA_DATA_DmaBuf) { return; } + /* References keep a shared plane mapped until its last buffer is removed. */ + existing = osc_find_dmabuf_map(session, (int)data->fd, data->mapoffset); + if (existing != NULL) { + osc_retain_dmabuf_map(existing); + return; + } for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { if (session->dmabuf_maps[i].ptr != NULL) { continue; } - /* `maxsize` is the producer's statement of how much of the fd belongs to - * this buffer, and mapping exactly that keeps the bounds checks in - * osc_read_frame meaningful. It is legitimately 0 for a DMA-BUF plane — - * wlroots leaves it unset — in which case osc_map_dmabuf recovers the - * real length from the fd and reports it back here. Storing the - * producer's 0 instead would leave every later bounds check comparing - * against an empty mapping. */ + /* `maxsize` is a starting point, not the answer: osc_map_dmabuf prefers + * the length the fd itself reports, because a DmaBuf plane can carry an + * advisory maxsize — wlroots leaves it at 0 — rather than its allocation + * length. It reports back the length actually mapped, and THAT is what + * every later bounds check measures against. */ maplen = data->maxsize; - session->dmabuf_maps[i].ptr = osc_map_dmabuf((int)data->fd, &maplen, &why); + session->dmabuf_maps[i].ptr = + osc_map_dmabuf((int)data->fd, data->flags, data->mapoffset, &maplen, &why); 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. + /* Reported once, on the event stream: a mapping failure here means + * no frames at all, and silence would read as a hang. * - * The reason is carried up rather than assumed: this used to say the + * The reason is carried up rather than assumed. This used to say the * driver refused CPU mapping no matter what actually went wrong, and * that message sent the one real investigation of this path looking * at the GPU for a size the compositor had simply left at 0. */ - if (session->callbacks.on_buffer_info != NULL && - session->buffer_info_reports < OSC_BUFFER_INFO_REPORTS) { - char detail[256]; - - snprintf(detail, sizeof(detail), "dmabuf import failed: %s; capture cannot proceed", - why); - session->buffer_info_reports++; - session->callbacks.on_buffer_info(session->callbacks.user, data->type, - pw_buf->buffer->n_datas, 0, 0, detail); - } + char detail[256]; + + snprintf(detail, sizeof(detail), "dmabuf import failed: %s; capture cannot proceed", + why); + osc_report_capture_issue(session, "dmabuf-import-failed", detail); return; } session->dmabuf_maps[i].fd = (int)data->fd; + session->dmabuf_maps[i].mapoffset = data->mapoffset; session->dmabuf_maps[i].len = maplen; + session->dmabuf_maps[i].refs = 1; return; } + /* + * Out of slots. Falling out of this loop used to be the one failure here + * that said nothing on any channel: osc_read_frame's lookup then misses for + * every frame of the session and returns before it can report anything, so + * the recording freezes with no explanation anywhere. + */ + osc_report_capture_issue(session, "dmabuf-map-table-full", + "the compositor allocated more DMA-BUF buffers than this helper can " + "map; capture cannot proceed"); } static void osc_on_remove_buffer(void *userdata, struct pw_buffer *pw_buf) @@ -841,15 +1102,24 @@ static void osc_on_remove_buffer(void *userdata, struct pw_buffer *pw_buf) 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 || - session->dmabuf_maps[i].fd != (int)data->fd) { + session->dmabuf_maps[i].fd != (int)data->fd || + session->dmabuf_maps[i].mapoffset != data->mapoffset) { continue; } + if (!osc_release_dmabuf_map(&session->dmabuf_maps[i])) { + return; + } 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].mapoffset = 0; session->dmabuf_maps[i].len = 0; + session->dmabuf_maps[i].refs = 0; return; } } @@ -865,7 +1135,9 @@ static void osc_unmap_all_dmabufs(struct osc_pw_session *session) 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].mapoffset = 0; session->dmabuf_maps[i].len = 0; + session->dmabuf_maps[i].refs = 0; } } @@ -969,21 +1241,37 @@ static int osc_read_cursor(const struct spa_buffer *buffer, struct osc_pw_cursor * Extracts the pixels of one buffer. Returns 1 when `out` describes a frame, 0 * when this buffer carries none. * - * The offset/size clamping against `maxsize` is the standard PipeWire consumer - * idiom and is not paranoia: `chunk` lives in memory the PRODUCER writes, so its - * fields are untrusted input from another process. A compositor bug — or a - * malicious one — that reports a size past the end of the mapping would - * otherwise be a read straight off the end of the shared memory. + * Every bound comes out of osc_resolve_frame_bounds, and none of it is + * paranoia: `chunk` lives in memory the PRODUCER writes, so its fields are + * untrusted input from another process. A compositor bug — or a malicious one — + * that reports a size past the end of the mapping would otherwise be a read + * straight off the end of the shared memory. + * + * The readable length is the MAPPING's length. For MemPtr/MemFd pw_stream maps + * the region and `maxsize` is that length; for DMA-BUF we map it ourselves and + * dmabuf_maps[] records what was actually mapped, because SPA's maxsize is + * advisory there and some producers leave a placeholder in it. */ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffer *buffer, struct osc_pw_frame *out) { struct spa_data *data; + struct osc_dmabuf_map *dmabuf_map = NULL; struct spa_meta_header *header; struct spa_meta_region *region; - uint32_t offset; - uint32_t size; + enum osc_frame_bounds_error bounds_error; + size_t available = 0; + size_t mapped_len; + /* Written by osc_resolve_frame_bounds only when it returns OK. Initialised + * so that reading them on any other path is a zero, never stack junk that + * would reach SPA_PTROFF as a pointer offset. */ + size_t offset = 0; + size_t size = 0; + uint32_t chunk_offset; + uint32_t chunk_size; + int32_t chunk_flags; int32_t stride; + int32_t width; int32_t height; const uint8_t *base; @@ -1006,10 +1294,12 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe * 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) { + dmabuf_map = osc_find_dmabuf_map(session, (int)data->fd, data->mapoffset); + if (dmabuf_map == NULL) { return 0; } + base = dmabuf_map->ptr; + mapped_len = dmabuf_map->len; } else if (data->data == NULL) { /* * NULL on a shared-memory buffer means it was never mapped, which is the @@ -1018,24 +1308,27 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe return 0; } else { base = data->data; + mapped_len = data->maxsize; } - /* A zero-sized chunk is how a compositor ships a cursor update with no new - * frame attached. Not an error, just not a frame. */ - if (data->chunk->size == 0) { - return 0; - } - - offset = SPA_MIN(data->chunk->offset, data->maxsize); - size = SPA_MIN(data->chunk->size, data->maxsize - offset); - + width = (int32_t)session->format.size.width; height = (int32_t)session->format.size.height; + /* Read each producer-written field ONCE, and decide and report from that + * same snapshot. Re-reading them for the diagnostic would let a misbehaving + * producer rewrite the numbers in between, so the report would describe a + * frame other than the one that was refused. */ + chunk_offset = data->chunk->offset; + chunk_size = data->chunk->size; + chunk_flags = data->chunk->flags; stride = data->chunk->stride; - if (stride <= 0 || height <= 0) { + bounds_error = + osc_resolve_frame_bounds(data->type, data->maxsize, mapped_len, chunk_offset, chunk_size, + chunk_flags, stride, width, height, &available, &offset, &size); + if (bounds_error == OSC_FRAME_BOUNDS_METADATA_ONLY) { return 0; } - /* One short row is one row of garbage in the recording; refuse the whole - * frame instead, and let the caller count it as dropped. */ - if ((uint64_t)stride * (uint64_t)height > (uint64_t)size) { + if (bounds_error != OSC_FRAME_BOUNDS_OK) { + osc_report_frame_drop(session, data, mapped_len, available, chunk_offset, chunk_size, + chunk_flags, stride, bounds_error); return 0; } @@ -1052,7 +1345,7 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe out->data = SPA_PTROFF(base, offset, const uint8_t); out->size = size; out->stride = stride; - out->width = (int32_t)session->format.size.width; + out->width = width; out->height = height; out->video_format = session->format.format; diff --git a/electron/native/pipewire-capture/csrc/pw_shim.h b/electron/native/pipewire-capture/csrc/pw_shim.h index ab6f78305..d4c5ec0ee 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.h +++ b/electron/native/pipewire-capture/csrc/pw_shim.h @@ -122,6 +122,18 @@ struct osc_pw_callbacks { void (*on_buffer_info)(void *user, uint32_t data_type, uint32_t n_datas, int32_t has_cursor_meta, uint32_t cursor_meta_size, const char *metas); + /* A condition that costs the user frames, reported on the event stream + * rather than to a debug log. + * + * This channel exists because the failures it carries are TERMINAL and + * SILENT: stride, geometry and the mapped length are constant for a whole + * negotiation, so a buffer that fails validation once fails every time, and + * the user gets a recording with no frames in it. Nothing downstream can + * infer the cause — a rejected frame never reaches the mailbox, so the + * frames-dropped counter stays at zero and the session still stops + * "successfully". `code` is a stable kebab-case identifier; `detail` is a + * borrowed sentence for a human. */ + void (*on_capture_issue)(void *user, const char *code, const char *detail); void (*on_state)(void *user, const char *state, const char *error); }; @@ -171,6 +183,30 @@ int osc_pw_cursor_meta_accepts_producer_size(uint32_t width, uint32_t height); */ int osc_pw_enum_format_accepts_dmabuf_producer(int with_modifier, int64_t producer_modifier); +/* + * Which bound, if any, rejects this frame? Returns the same stable reason string + * the frame-drop diagnostics carry ("stride-shorter-than-row", + * "chunk-offset-out-of-bounds", ...), or "none" when the frame is accepted. + * + * It returns the REASON rather than a yes/no because six distinct conditions + * reject a frame here, and a test asserting only "rejected" passes when the + * wrong one fires — a reordering of the checks, or a broken row computation, + * would keep such a suite green. Exposed so the bound can be asserted without a + * portal, a compositor, or a screen. + * + * The caller's pre-checks in osc_read_frame (n_datas, a NULL chunk, a missing + * dmabuf mapping) are deliberately NOT modelled here: this answers only the + * bounds question. + */ +const char *osc_pw_frame_bounds_reason(uint32_t data_type, uint32_t maxsize, size_t mapped_len, + uint32_t chunk_offset, uint32_t chunk_size, + int32_t chunk_flags, int32_t stride, int32_t width, + int32_t height); + +/* DMA-BUF mapoffset and shared-mapping lifecycle test helpers. */ +int osc_pw_dmabuf_mapped_len(size_t allocation_len, uint32_t mapoffset, size_t *mapped_len); +int osc_pw_dmabuf_map_lifecycle_valid(void); + struct osc_pw_session; /* diff --git a/electron/native/pipewire-capture/src/main.rs b/electron/native/pipewire-capture/src/main.rs index 8ec4078c2..92dd534e5 100644 --- a/electron/native/pipewire-capture/src/main.rs +++ b/electron/native/pipewire-capture/src/main.rs @@ -940,7 +940,7 @@ fn run( ("metas", metas.clone().into()), ]), }); - if !has_cursor_meta { + if !has_cursor_meta && config.cursor_mode.reports_cursor() { let _ = emitter.emit(&Event::Warning { code: "no-cursor-metadata".to_owned(), message: format!( @@ -953,6 +953,19 @@ fn run( } } + // Surfaced as a warning rather than logged, because these are the + // conditions that leave a recording empty while every counter still + // reads zero: a frame refused by the shim never reaches the mailbox, + // so `frames-dropped` stays at 0 and the session stops "cleanly". + // Without this the only honest symptom is a file with no frames in + // it and nothing anywhere saying why. + Ok(Message::Stream(StreamEvent::CaptureIssue { code, detail })) => { + let _ = emitter.emit(&Event::Warning { + code, + message: detail, + }); + } + Ok(Message::Stream(StreamEvent::State { state, error })) => { // Every transition, not just the failures. Without this a run // that ends in "target not found" is ambiguous: there is no way diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index 9d3a03ec7..7404b861c 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -65,6 +65,7 @@ struct RawCallbacks { on_cursor: extern "C" fn(*mut c_void, *const RawCursor), on_frame: extern "C" fn(*mut c_void, *const RawFrame), on_buffer_info: extern "C" fn(*mut c_void, u32, u32, i32, u32, *const c_char), + on_capture_issue: extern "C" fn(*mut c_void, *const c_char, *const c_char), on_state: extern "C" fn(*mut c_void, *const c_char, *const c_char), } @@ -102,6 +103,26 @@ extern "C" { with_modifier: i32, producer_modifier: i64, ) -> i32; + #[cfg(test)] + fn osc_pw_frame_bounds_reason( + data_type: u32, + maxsize: u32, + mapped_len: usize, + chunk_offset: u32, + chunk_size: u32, + chunk_flags: i32, + stride: i32, + width: i32, + height: i32, + ) -> *const c_char; + #[cfg(test)] + fn osc_pw_dmabuf_mapped_len( + allocation_len: usize, + mapoffset: u32, + mapped_len: *mut usize, + ) -> i32; + #[cfg(test)] + fn osc_pw_dmabuf_map_lifecycle_valid() -> i32; fn osc_pw_start( fd: i32, node_id: u32, @@ -131,6 +152,15 @@ pub enum StreamEvent { /// negotiation. Empty when the buffers carry none at all. metas: String, }, + /// Something cost the user frames and nothing downstream could work out + /// what. Raised by the shim for conditions that are terminal and otherwise + /// silent — a DMA-BUF import that failed, a buffer that fails validation — + /// where the recording comes out empty and every counter still reads zero. + CaptureIssue { + /// Stable kebab-case identifier, surfaced as the warning's `code`. + code: String, + detail: String, + }, Cursor(CursorEvent), /// A frame is waiting in the [`FrameMailbox`]. Carries no payload on /// purpose: an 8 MB frame per channel message would allocate and copy far @@ -287,6 +317,11 @@ impl FrameMailbox { } } +/// How much silence the ring will stand in for before it stops trying. +/// +/// This bounds the allocation after a long stall; drop accounting remains exact. +const MAX_SILENCE_SECONDS: usize = 30; + /// Interleaved samples waiting to be encoded. /// /// UNLIKE THE VIDEO MAILBOX, THIS IS A QUEUE. A dropped video frame costs one @@ -311,27 +346,6 @@ impl FrameMailbox { /// until someone drains, which is also the only thing that can stop it growing — /// and it stops being exact at [`MAX_SILENCE_SECONDS`], which is where a bounded /// allocation starts to matter more than sync nobody can still use. -/// How much silence the ring will stand in for before it stops trying. -/// -/// The debt costs a counter while it is owed and only becomes memory when -/// someone drains — 384 KB per second of it, at 48 kHz stereo f32. A drain runs -/// on every tick of the main loop, heartbeat included (`Capture::advance` from -/// the `RecvTimeoutError::Timeout` arm in main.rs), so a debt worth seconds -/// means the loop itself has stopped. There is no bound on how long a stopped -/// loop stays stopped, and one that comes back materialises the whole stall in a -/// single allocation — which is also the one place the ring's own two-second cap -/// does not reach. There is a second, quieter way to get there: nothing drains -/// before the first video frame either, so the debt grows for as long as the -/// portal picker is up. That normally ends in `clear` rather than a drain, but -/// not if staging that first frame fails, and the stop path flushes the ring. -/// -/// Thirty seconds is far past any stall a recording survives, and past it the -/// take has a hole half a minute wide — the cap trades sync that is already lost -/// for an allocation that stays bounded. `dropped_samples` keeps counting the -/// whole loss regardless, so the `audio-dropped` warning still reports what -/// really happened rather than what could be paid back. -const MAX_SILENCE_SECONDS: usize = 30; - #[derive(Debug)] pub struct AudioRing { inner: std::sync::Mutex, @@ -719,6 +733,93 @@ pub fn enum_format_accepts_dmabuf_producer(with_modifier: bool, producer_modifie unsafe { osc_pw_enum_format_accepts_dmabuf_producer(i32::from(with_modifier), producer_modifier) } } +/// The inputs `osc_resolve_frame_bounds` weighs, as named fields. +/// +/// The C entry point takes nine bare integers, four of which are plausible +/// neighbours (`maxsize`/`mapped_len`, `chunk_offset`/`chunk_size`) that a +/// transposition would not disturb. Building from a healthy baseline and +/// changing one field means each test says which input it is about. +#[cfg(test)] +#[derive(Clone, Copy)] +struct Bounds { + data_type: u32, + maxsize: u32, + mapped_len: usize, + chunk_offset: u32, + chunk_size: u32, + chunk_flags: i32, + stride: i32, + width: i32, + height: i32, +} + +#[cfg(test)] +impl Bounds { + /// A healthy 1920x1080 BGRx frame in the 8 MiB allocation a GPU hands back + /// for it — the shape a working DMA-BUF session delivers, with an honest + /// `chunk_size`. + fn dmabuf_1080p() -> Self { + let stride = 1920 * 4; + Self { + data_type: constants().data_dma_buf, + maxsize: 0, + mapped_len: 8 * 1024 * 1024, + chunk_offset: 0, + chunk_size: stride * 1080, + chunk_flags: 0, + stride: stride as i32, + width: 1920, + height: 1080, + } + } + + /// The same frame over shared memory, where `maxsize` really is the mapping + /// length because pw_stream mapped it. + fn memfd_1080p() -> Self { + let stride = 1920u32 * 4; + Self { + data_type: constants().data_mem_fd, + maxsize: stride * 1080, + mapped_len: (stride * 1080) as usize, + ..Self::dmabuf_1080p() + } + } + + /// Which bound rejects this frame, or `"none"` when it is accepted. + fn reason(self) -> String { + // SAFETY: the C helper performs arithmetic only and returns a pointer to + // one of its own string literals, which outlives this call. + let raw = unsafe { + osc_pw_frame_bounds_reason( + self.data_type, + self.maxsize, + self.mapped_len, + self.chunk_offset, + self.chunk_size, + self.chunk_flags, + self.stride, + self.width, + self.height, + ) + }; + assert!(!raw.is_null(), "the shim always names a reason"); + unsafe { CStr::from_ptr(raw) }.to_string_lossy().into_owned() + } + + fn is_accepted(self) -> bool { + self.reason() == "none" + } +} + +#[cfg(test)] +fn dmabuf_mapped_len(allocation_len: usize, mapoffset: u32) -> Option { + let mut mapped_len = 0; + // SAFETY: `mapped_len` is a live `usize` destination. The helper only + // performs checked arithmetic and writes through this pointer on success. + let valid = unsafe { osc_pw_dmabuf_mapped_len(allocation_len, mapoffset, &mut mapped_len) }; + (valid != 0).then_some(mapped_len) +} + /// SPA enum values as compiled from the vendored headers. pub fn constants() -> Constants { let mut out = Constants::default(); @@ -765,6 +866,7 @@ impl Session { on_cursor, on_frame, on_buffer_info, + on_capture_issue, on_state, }; @@ -855,9 +957,9 @@ extern "C" fn on_frame(user: *mut c_void, frame: *const RawFrame) { if rows > frame.size { return; } - // SAFETY: the shim clamped `size` against the mapping's `maxsize` before - // the callback, `rows <= size` was just checked, and the mapping stays - // live until this returns. + // SAFETY: the shim clamped `size` against the mapping length before the + // callback, `rows <= size` was just checked, and the mapping stays live + // until this returns. let pixels = unsafe { std::slice::from_raw_parts(frame.data, rows) }; mailbox.put(pixels, frame); (state.sink)(StreamEvent::FrameReady); @@ -929,6 +1031,24 @@ extern "C" fn on_cursor(user: *mut c_void, cursor: *const RawCursor) { }); } +extern "C" fn on_capture_issue(user: *mut c_void, code: *const c_char, detail: *const c_char) { + with_sink(user, |sink| { + let owned = |raw: *const c_char| { + if raw.is_null() { + String::new() + } else { + // SAFETY: the shim passes NUL-terminated buffers that outlive the + // callback. + unsafe { CStr::from_ptr(raw) }.to_string_lossy().into_owned() + } + }; + sink(StreamEvent::CaptureIssue { + code: owned(code), + detail: owned(detail), + }); + }); +} + extern "C" fn on_state(user: *mut c_void, state: *const c_char, error: *const c_char) { with_sink(user, |sink| { let to_string = |raw: *const c_char| { @@ -1037,6 +1157,241 @@ mod tests { ); } + #[test] + fn a_healthy_dmabuf_frame_is_accepted() { + assert_eq!(Bounds::dmabuf_1080p().reason(), "none"); + } + + #[test] + fn dmabuf_chunk_size_is_always_advisory() { + let healthy = Bounds::dmabuf_1080p(); + let row_bytes = healthy.width as u32 * 4; + for chunk_size in [ + 0, + 1, + 9, + row_bytes - 1, + row_bytes, + healthy.chunk_size / 2, + healthy.chunk_size, + u32::MAX, + ] { + let frame = Bounds { + chunk_size, + ..healthy + }; + assert_eq!( + frame.reason(), + "none", + "DMA-BUF chunk size {chunk_size} must not replace the fd allocation bound" + ); + } + } + + #[test] + fn dmabuf_mapping_length_accounts_for_mapoffset() { + let allocation_len = 8 * 1024 * 1024; + assert_eq!(dmabuf_mapped_len(allocation_len, 0), Some(allocation_len)); + assert_eq!( + dmabuf_mapped_len(allocation_len, 4096), + Some(allocation_len - 4096) + ); + assert_eq!( + dmabuf_mapped_len(allocation_len, allocation_len as u32), + None + ); + assert_eq!( + dmabuf_mapped_len(allocation_len, allocation_len as u32 + 4096), + None + ); + } + + #[test] + fn shared_dmabuf_mapping_is_reference_counted_by_exact_plane() { + // SAFETY: the helper owns its synthetic session and touches no external + // resources; it only exercises lookup and reference-count transitions. + assert_eq!(unsafe { osc_pw_dmabuf_map_lifecycle_valid() }, 1); + } + + /// `maxsize` is advisory on the DMA-BUF path, so it can be arbitrarily + /// large — and being large must not let a frame reach past what was mapped. + #[test] + fn advisory_sizes_cannot_stretch_a_frame_past_its_mapping() { + let frame = Bounds { + maxsize: u32::MAX, + mapped_len: 4096, + ..Bounds::dmabuf_1080p() + }; + assert_eq!(frame.reason(), "frame-bytes-exceed-available-chunk"); + } + + /// Every rejection, each reached by perturbing exactly one field of a frame + /// that is otherwise healthy, and each asserted by NAME. Asserting only + /// "rejected" would pass when the wrong bound fires — which is exactly what + /// a reordering of these checks would do. + #[test] + fn each_rejection_names_itself() { + let healthy = Bounds::dmabuf_1080p(); + let cases = [ + ( + "metadata-only", + Bounds { + stride: 0, + ..healthy + }, + ), + ( + "buffer-length-zero", + Bounds { + mapped_len: 0, + ..healthy + }, + ), + ( + "chunk-offset-out-of-bounds", + Bounds { + chunk_offset: healthy.mapped_len as u32 + 1, + ..healthy + }, + ), + ( + "producer-marked-frame-corrupted", + Bounds { + chunk_flags: 1, // SPA_CHUNK_FLAG_CORRUPTED + ..healthy + }, + ), + ( + "stride-shorter-than-row", + Bounds { + stride: healthy.width * 4 - 1, + ..healthy + }, + ), + ( + "frame-bytes-exceed-available-chunk", + Bounds { + mapped_len: 1024 * 1024, + ..healthy + }, + ), + ]; + for (expected, frame) in cases { + assert_eq!(frame.reason(), expected, "the {expected} case"); + } + } + + /// The geometry gate moved during the bounds refactor and its `width` term + /// is new, so every axis is pinned here. Nothing else in the suite reaches + /// this branch: a healthy frame's stride, width and height are all positive. + #[test] + fn geometry_is_rejected_on_every_axis() { + let healthy = Bounds::dmabuf_1080p(); + let broken = [ + ( + "negative stride", + Bounds { + stride: -1, + ..healthy + }, + ), + ( + "zero width", + Bounds { + width: 0, + ..healthy + }, + ), + ( + "negative width", + Bounds { + width: -1, + ..healthy + }, + ), + ( + "zero height", + Bounds { + height: 0, + ..healthy + }, + ), + ( + "negative height", + Bounds { + height: -1, + ..healthy + }, + ), + ]; + for (axis, frame) in broken { + assert_eq!(frame.reason(), "invalid-frame-geometry", "{axis}"); + } + } + + #[test] + fn a_zero_stride_buffer_is_metadata_only() { + for base in [Bounds::dmabuf_1080p(), Bounds::memfd_1080p()] { + for chunk_size in [0, 9, base.chunk_size] { + let metadata = Bounds { + stride: 0, + chunk_size, + ..base + }; + assert_eq!(metadata.reason(), "metadata-only"); + } + } + } + + /// The DMA-BUF leniency must not leak to shared memory, where `maxsize` is + /// the real mapping length and `chunk->size` is a real byte count. MemPtr + /// and MemFd are both advertised by the video path, so both are exercised. + #[test] + fn shared_memory_still_clamps_to_the_declared_chunk_size() { + let constants = constants(); + for data_type in [constants.data_mem_fd, constants.data_mem_ptr] { + let healthy = Bounds { + data_type, + ..Bounds::memfd_1080p() + }; + assert!(healthy.is_accepted(), "a healthy shared-memory frame"); + for chunk_size in [0, 1, 9, healthy.chunk_size / 2] { + let short = Bounds { + chunk_size, + ..healthy + }; + assert_eq!( + short.reason(), + "frame-bytes-exceed-available-chunk", + "{chunk_size} bytes is a real bound on shared memory" + ); + } + } + } + + #[test] + fn corrupted_dmabuf_with_zero_chunk_size_is_rejected() { + let frame = Bounds { + chunk_size: 0, + chunk_flags: 1, // SPA_CHUNK_FLAG_CORRUPTED + ..Bounds::dmabuf_1080p() + }; + assert_eq!(frame.reason(), "producer-marked-frame-corrupted"); + } + + /// stride * height is computed on widened operands because both come from + /// another process; multiplied as int32 these wrap to a small positive + /// number and the frame would be accepted. + #[test] + fn an_overflowing_frame_size_cannot_wrap_past_the_bound() { + let frame = Bounds { + stride: i32::MAX, + height: i32::MAX, + ..Bounds::dmabuf_1080p() + }; + assert_eq!(frame.reason(), "frame-bytes-exceed-available-chunk"); + } + /// End-to-end exercise of the PipeWire half with NO portal involved. /// /// `pw_context_connect_fd` accepts any socket already connected to a @@ -1132,6 +1487,12 @@ mod tests { Ok(StreamEvent::Cursor(cursor)) => { println!("[{:>5}ms] cursor {cursor:?}", stamp(std::time::Instant::now())); } + Ok(StreamEvent::CaptureIssue { code, detail }) => { + println!( + "[{:>5}ms] capture-issue {code}: {detail}", + stamp(std::time::Instant::now()) + ); + } // Unreachable: this session was started with no mailbox, so the // C side was never asked for frames. Matched rather than // wildcarded so that adding a variant is a compile error here