From b258e1f7eefa67006da72172d2a61389a58b7630 Mon Sep 17 00:00:00 2001 From: Robert Mader Date: Mon, 9 Oct 2023 22:47:38 +0200 Subject: [PATCH 1/2] screen-cast/stream-src: Various code cleanups No functional changes. Part-of: --- src/backends/meta-screen-cast-stream-src.c | 117 ++++++++++----------- 1 file changed, 57 insertions(+), 60 deletions(-) diff --git a/src/backends/meta-screen-cast-stream-src.c b/src/backends/meta-screen-cast-stream-src.c index 8d4e32ef60e..acb5e9a1da1 100644 --- a/src/backends/meta-screen-cast-stream-src.c +++ b/src/backends/meta-screen-cast-stream-src.c @@ -555,17 +555,16 @@ maybe_record_cursor (MetaScreenCastStreamSrc *src, } static gboolean -do_record_frame (MetaScreenCastStreamSrc *src, - MetaScreenCastRecordFlag flags, - struct spa_buffer *spa_buffer, - uint8_t *data, - GError **error) +do_record_frame (MetaScreenCastStreamSrc *src, + MetaScreenCastRecordFlag flags, + struct spa_buffer *spa_buffer, + GError **error) { MetaScreenCastStreamSrcPrivate *priv = meta_screen_cast_stream_src_get_instance_private (src); + struct spa_data *spa_data = &spa_buffer->datas[0]; - if (spa_buffer->datas[0].data || - spa_buffer->datas[0].type == SPA_DATA_MemFd) + if (spa_data->data || spa_data->type == SPA_DATA_MemFd) { int width = priv->video_format.size.width; int height = priv->video_format.size.height; @@ -575,14 +574,14 @@ do_record_frame (MetaScreenCastStreamSrc *src, width, height, stride, - data, + spa_data->data, error); } - else if (spa_buffer->datas[0].type == SPA_DATA_DmaBuf) + else if (spa_data->type == SPA_DATA_DmaBuf) { CoglDmaBufHandle *dmabuf_handle = g_hash_table_lookup (priv->dmabuf_handles, - GINT_TO_POINTER (spa_buffer->datas[0].fd)); + GINT_TO_POINTER (spa_data->fd)); CoglFramebuffer *dmabuf_fbo = cogl_dma_buf_handle_get_framebuffer (dmabuf_handle); @@ -592,7 +591,7 @@ do_record_frame (MetaScreenCastStreamSrc *src, } g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, - "Unknown SPA buffer type %u", spa_buffer->datas[0].type); + "Unknown SPA buffer type %u", spa_data->type); return FALSE; } @@ -745,12 +744,12 @@ meta_screen_cast_stream_src_maybe_record_frame_with_timestamp (MetaScreenCastStr MetaScreenCastStreamSrcPrivate *priv = meta_screen_cast_stream_src_get_instance_private (src); MetaScreenCastRecordResult record_result = - META_SCREEN_CAST_RECORD_RESULT_RECORDED_NOTHING; + META_SCREEN_CAST_RECORD_RESULT_RECORDED_NOTHING; MtkRectangle crop_rect; struct pw_buffer *buffer; struct spa_buffer *spa_buffer; struct spa_meta_header *header; - uint8_t *data = NULL; + struct spa_data *spa_data; /* Accumulate the damaged region since we might not schedule a frame capture * eventually but once we do, we should report all the previous damaged areas. @@ -817,13 +816,13 @@ meta_screen_cast_stream_src_maybe_record_frame_with_timestamp (MetaScreenCastStr } spa_buffer = buffer->buffer; - data = spa_buffer->datas[0].data; + spa_data = &spa_buffer->datas[0]; header = spa_buffer_find_meta_data (spa_buffer, SPA_META_Header, sizeof (*header)); - if (spa_buffer->datas[0].type != SPA_DATA_DmaBuf && !data) + if (spa_data->type != SPA_DATA_DmaBuf && !spa_data->data) { g_critical ("Invalid buffer data"); if (header) @@ -838,10 +837,9 @@ meta_screen_cast_stream_src_maybe_record_frame_with_timestamp (MetaScreenCastStr g_autoptr (GError) error = NULL; g_clear_handle_id (&priv->follow_up_frame_source_id, g_source_remove); - if (do_record_frame (src, flags, spa_buffer, data, &error)) + if (do_record_frame (src, flags, spa_buffer, &error)) { maybe_add_damaged_regions_metadata (src, spa_buffer); - struct spa_data *spa_data = &spa_buffer->datas[0]; struct spa_meta_region *spa_meta_video_crop; spa_data->chunk->size = spa_data->maxsize; @@ -879,14 +877,14 @@ meta_screen_cast_stream_src_maybe_record_frame_with_timestamp (MetaScreenCastStr { if (error) g_warning ("Failed to record screen cast frame: %s", error->message); - spa_buffer->datas[0].chunk->size = 0; - spa_buffer->datas[0].chunk->flags = SPA_CHUNK_FLAG_CORRUPTED; + spa_data->chunk->size = 0; + spa_data->chunk->flags = SPA_CHUNK_FLAG_CORRUPTED; } } else { - spa_buffer->datas[0].chunk->size = 0; - spa_buffer->datas[0].chunk->flags = SPA_CHUNK_FLAG_CORRUPTED; + spa_data->chunk->size = 0; + spa_data->chunk->flags = SPA_CHUNK_FLAG_CORRUPTED; } record_result |= maybe_record_cursor (src, spa_buffer); @@ -1091,7 +1089,7 @@ on_stream_add_buffer (void *data, meta_screen_cast_session_get_screen_cast (session); CoglDmaBufHandle *dmabuf_handle; struct spa_buffer *spa_buffer = buffer->buffer; - struct spa_data *spa_data = spa_buffer->datas; + struct spa_data *spa_data = &spa_buffer->datas[0]; const int bpp = 4; int stride; @@ -1099,11 +1097,11 @@ on_stream_add_buffer (void *data, stride = SPA_ROUND_UP_N (priv->video_format.size.width * bpp, 4); - spa_data[0].mapoffset = 0; - spa_data[0].maxsize = stride * priv->video_format.size.height; - spa_data[0].data = NULL; + spa_data->mapoffset = 0; + spa_data->maxsize = stride * priv->video_format.size.height; + spa_data->data = NULL; - if (spa_data[0].type & (1 << SPA_DATA_DmaBuf)) + if (spa_data->type & (1 << SPA_DATA_DmaBuf)) { CoglPixelFormat cogl_format; @@ -1141,19 +1139,19 @@ on_stream_add_buffer (void *data, "Allocating DMA buffer for pw_stream %u", pw_stream_get_node_id (priv->pipewire_stream)); - spa_data[0].type = SPA_DATA_DmaBuf; - spa_data[0].flags = SPA_DATA_FLAG_READWRITE; - spa_data[0].fd = cogl_dma_buf_handle_get_fd (dmabuf_handle); + spa_data->type = SPA_DATA_DmaBuf; + spa_data->flags = SPA_DATA_FLAG_READWRITE; + spa_data->fd = cogl_dma_buf_handle_get_fd (dmabuf_handle); g_hash_table_insert (priv->dmabuf_handles, - GINT_TO_POINTER (spa_data[0].fd), + GINT_TO_POINTER (spa_data->fd), dmabuf_handle); } else { unsigned int seals; - if (!(spa_data[0].type & (1 << SPA_DATA_MemFd))) + if (!(spa_data->type & (1 << SPA_DATA_MemFd))) { g_critical ("No supported PipeWire stream buffer data type could " "be negotiated"); @@ -1165,40 +1163,39 @@ on_stream_add_buffer (void *data, pw_stream_get_node_id (priv->pipewire_stream)); /* Fallback to a memfd buffer */ - spa_data[0].type = SPA_DATA_MemFd; - spa_data[0].flags = SPA_DATA_FLAG_READWRITE; - spa_data[0].fd = memfd_create ("mutter-screen-cast-memfd", - MFD_CLOEXEC | MFD_ALLOW_SEALING); - if (spa_data[0].fd == -1) + spa_data->type = SPA_DATA_MemFd; + spa_data->flags = SPA_DATA_FLAG_READWRITE; + spa_data->fd = memfd_create ("mutter-screen-cast-memfd", + MFD_CLOEXEC | MFD_ALLOW_SEALING); + if (spa_data->fd == -1) { g_critical ("Can't create memfd: %m"); return; } - spa_data[0].mapoffset = 0; - spa_data[0].maxsize = stride * priv->video_format.size.height; + spa_data->maxsize = stride * priv->video_format.size.height; - if (ftruncate (spa_data[0].fd, spa_data[0].maxsize) < 0) + if (ftruncate (spa_data->fd, spa_data->maxsize) < 0) { - close (spa_data[0].fd); - spa_data[0].fd = -1; - g_critical ("Can't truncate to %d: %m", spa_data[0].maxsize); + close (spa_data->fd); + spa_data->fd = -1; + g_critical ("Can't truncate to %d: %m", spa_data->maxsize); return; } seals = F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL; - if (fcntl (spa_data[0].fd, F_ADD_SEALS, seals) == -1) + if (fcntl (spa_data->fd, F_ADD_SEALS, seals) == -1) g_warning ("Failed to add seals: %m"); - spa_data[0].data = mmap (NULL, - spa_data[0].maxsize, - PROT_READ | PROT_WRITE, - MAP_SHARED, - spa_data[0].fd, - spa_data[0].mapoffset); - if (spa_data[0].data == MAP_FAILED) + spa_data->data = mmap (NULL, + spa_data->maxsize, + PROT_READ | PROT_WRITE, + MAP_SHARED, + spa_data->fd, + spa_data->mapoffset); + if (spa_data->data == MAP_FAILED) { - close (spa_data[0].fd); - spa_data[0].fd = -1; + close (spa_data->fd); + spa_data->fd = -1; g_critical ("Failed to mmap memory: %m"); return; } @@ -1219,23 +1216,23 @@ on_stream_remove_buffer (void *data, MetaScreenCastStreamSrcPrivate *priv = meta_screen_cast_stream_src_get_instance_private (src); struct spa_buffer *spa_buffer = buffer->buffer; - struct spa_data *spa_data = spa_buffer->datas; + struct spa_data *spa_data = &spa_buffer->datas[0]; priv->buffer_count--; - if (spa_data[0].type == SPA_DATA_DmaBuf) + if (spa_data->type == SPA_DATA_DmaBuf) { - if (!g_hash_table_remove (priv->dmabuf_handles, GINT_TO_POINTER (spa_data[0].fd))) + if (!g_hash_table_remove (priv->dmabuf_handles, GINT_TO_POINTER (spa_data->fd))) g_critical ("Failed to remove non-exported DMA buffer"); } - else if (spa_data[0].type == SPA_DATA_MemFd) + else if (spa_data->type == SPA_DATA_MemFd) { - g_warn_if_fail (spa_data[0].fd > 0 || !spa_data[0].data); + g_warn_if_fail (spa_data->fd > 0 || !spa_data->data); - if (spa_data[0].fd > 0) + if (spa_data->fd > 0) { - munmap (spa_data[0].data, spa_data[0].maxsize); - close (spa_data[0].fd); + munmap (spa_data->data, spa_data->maxsize); + close (spa_data->fd); } } } -- GitLab From 9b663f44e6044ece52c38b3ee23bbc2b55328b47 Mon Sep 17 00:00:00 2001 From: Robert Mader Date: Thu, 5 Oct 2023 08:36:31 +0200 Subject: [PATCH 2/2] screen-cast/strieam-src: Fix stride and max buffer size calculation 1. Centralize stride calculation in one function. 2. For dmabufs query the stride instead of assuming a certain value. 3. For system memory buffers use the pixel format to calculate the stride. 4. Stop negotiating `SPA_PARAM_BUFFERS_size` and `SPA_PARAM_BUFFERS_stride`. 2. fixes an actual bug where we reported wrong max buffer sizes, resulting in crashes in Gstreamer when doing area screencasts on AMD GPUs. The reasoning for 4. is that the values were possibly wrong for dmabufs as the negotiation happens before we create any buffers. Further more neither Mutter nor the common consumers required it. The later either ignore the values (OBS), always accept (gstpipewiresrc) them or calculate the exact same possibly wrong values (libwebrtc). Closes: https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/6747 Part-of: --- src/backends/meta-screen-cast-stream-src.c | 70 ++++++++++------------ 1 file changed, 33 insertions(+), 37 deletions(-) diff --git a/src/backends/meta-screen-cast-stream-src.c b/src/backends/meta-screen-cast-stream-src.c index acb5e9a1da1..cf198cf6472 100644 --- a/src/backends/meta-screen-cast-stream-src.c +++ b/src/backends/meta-screen-cast-stream-src.c @@ -102,7 +102,6 @@ typedef struct _MetaScreenCastStreamSrcPrivate uint32_t node_id; struct spa_video_info_raw video_format; - int video_stride; int64_t last_frame_timestamp_us; guint follow_up_frame_source_id; @@ -554,6 +553,33 @@ maybe_record_cursor (MetaScreenCastStreamSrc *src, g_assert_not_reached (); } +static int32_t +meta_screen_cast_stream_src_calculate_stride (MetaScreenCastStreamSrc *src, + struct spa_data *spa_data) +{ + MetaScreenCastStreamSrcPrivate *priv = + meta_screen_cast_stream_src_get_instance_private (src); + CoglPixelFormat cogl_format; + int bpp; + + if (spa_data->type == SPA_DATA_DmaBuf) + { + CoglDmaBufHandle *dmabuf_handle = NULL; + + dmabuf_handle = g_hash_table_lookup (priv->dmabuf_handles, + GINT_TO_POINTER (spa_data->fd)); + if (dmabuf_handle) + return cogl_dma_buf_handle_get_stride (dmabuf_handle); + } + + if (!cogl_pixel_format_from_spa_video_format (priv->video_format.format, + &cogl_format)) + g_assert_not_reached (); + + bpp = cogl_pixel_format_get_bytes_per_pixel (cogl_format, 0); + return SPA_ROUND_UP_N (priv->video_format.size.width * bpp, 4); +} + static gboolean do_record_frame (MetaScreenCastStreamSrc *src, MetaScreenCastRecordFlag flags, @@ -568,7 +594,7 @@ do_record_frame (MetaScreenCastStreamSrc *src, { int width = priv->video_format.size.width; int height = priv->video_format.size.height; - int stride = priv->video_stride; + int stride = meta_screen_cast_stream_src_calculate_stride (src, spa_data); return meta_screen_cast_stream_src_record_to_buffer (src, width, @@ -632,26 +658,6 @@ maybe_schedule_follow_up_frame (MetaScreenCastStreamSrc *src, src); } -static int32_t -meta_screen_cast_stream_src_calculate_stride (MetaScreenCastStreamSrc *src, - struct spa_data *spa_data) -{ - MetaScreenCastStreamSrcPrivate *priv = - meta_screen_cast_stream_src_get_instance_private (src); - CoglDmaBufHandle *dmabuf_handle = NULL; - - if (spa_data->type == SPA_DATA_DmaBuf) - { - dmabuf_handle = g_hash_table_lookup (priv->dmabuf_handles, - GINT_TO_POINTER (spa_data->fd)); - } - - if (dmabuf_handle) - return cogl_dma_buf_handle_get_stride (dmabuf_handle); - else - return priv->video_stride; -} - static void maybe_add_damaged_regions_metadata (MetaScreenCastStreamSrc *src, struct spa_buffer *spa_buffer) @@ -1014,11 +1020,9 @@ on_stream_param_changed (void *data, MetaScreenCastStreamSrcClass *klass = META_SCREEN_CAST_STREAM_SRC_GET_CLASS (src); uint8_t params_buffer[1024]; - int32_t width, height, stride, size; struct spa_pod_builder pod_builder; const struct spa_pod *params[5]; int n_params = 0; - const int bpp = 4; int buffer_types; if (!format || id != SPA_PARAM_Format) @@ -1027,13 +1031,6 @@ on_stream_param_changed (void *data, spa_format_video_raw_parse (format, &priv->video_format); - width = priv->video_format.size.width; - height = priv->video_format.size.height; - stride = SPA_ROUND_UP_N (width * bpp, 4); - size = height * stride; - - priv->video_stride = stride; - pod_builder = SPA_POD_BUILDER_INIT (params_buffer, sizeof (params_buffer)); buffer_types = 1 << SPA_DATA_MemFd; @@ -1045,8 +1042,6 @@ on_stream_param_changed (void *data, SPA_TYPE_OBJECT_ParamBuffers, SPA_PARAM_Buffers, SPA_PARAM_BUFFERS_buffers, SPA_POD_CHOICE_RANGE_Int (16, 2, 16), SPA_PARAM_BUFFERS_blocks, SPA_POD_Int (1), - SPA_PARAM_BUFFERS_size, SPA_POD_Int (size), - SPA_PARAM_BUFFERS_stride, SPA_POD_Int (stride), SPA_PARAM_BUFFERS_align, SPA_POD_Int (16), SPA_PARAM_BUFFERS_dataType, SPA_POD_CHOICE_FLAGS_Int (buffer_types)); @@ -1090,15 +1085,11 @@ on_stream_add_buffer (void *data, CoglDmaBufHandle *dmabuf_handle; struct spa_buffer *spa_buffer = buffer->buffer; struct spa_data *spa_data = &spa_buffer->datas[0]; - const int bpp = 4; int stride; priv->buffer_count++; - stride = SPA_ROUND_UP_N (priv->video_format.size.width * bpp, 4); - spa_data->mapoffset = 0; - spa_data->maxsize = stride * priv->video_format.size.height; spa_data->data = NULL; if (spa_data->type & (1 << SPA_DATA_DmaBuf)) @@ -1143,6 +1134,9 @@ on_stream_add_buffer (void *data, spa_data->flags = SPA_DATA_FLAG_READWRITE; spa_data->fd = cogl_dma_buf_handle_get_fd (dmabuf_handle); + stride = meta_screen_cast_stream_src_calculate_stride (src, spa_data); + spa_data->maxsize = stride * priv->video_format.size.height; + g_hash_table_insert (priv->dmabuf_handles, GINT_TO_POINTER (spa_data->fd), dmabuf_handle); @@ -1172,6 +1166,8 @@ on_stream_add_buffer (void *data, g_critical ("Can't create memfd: %m"); return; } + + stride = meta_screen_cast_stream_src_calculate_stride (src, spa_data); spa_data->maxsize = stride * priv->video_format.size.height; if (ftruncate (spa_data->fd, spa_data->maxsize) < 0) -- GitLab