From 4d0bab799cfbb567a3f96cc04f8b8826c00581ff Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Fri, 17 Nov 2017 13:34:42 +0100 Subject: [PATCH] link: don't allocate shared meta in shared mem We can't allocate the shared meta in shared mem because then clients can damage it for other clients. Place it instead right after the buffer metadata array. Filter out the shared metadata for a client, we send it as part of the client_buffer structure. Remove pointer metadata, it's not so useful. Document the layout of the allocated buffers and the shared memory. Work on metadata to define control parameters --- spa/include/spa/buffer/meta.h | 102 ++++++++++-------- spa/include/spa/node/node.h | 4 +- spa/lib/debug.c | 6 -- src/modules/module-client-node/client-node.c | 8 +- src/pipewire/link.c | 107 ++++++++++++++++--- 5 files changed, 158 insertions(+), 69 deletions(-) diff --git a/spa/include/spa/buffer/meta.h b/spa/include/spa/buffer/meta.h index 53768b95a..9dc91ac9e 100644 --- a/spa/include/spa/buffer/meta.h +++ b/spa/include/spa/buffer/meta.h @@ -36,13 +36,68 @@ extern "C" { #define SPA_TYPE_META_BASE SPA_TYPE__Meta ":" #define SPA_TYPE_META__Header SPA_TYPE_META_BASE "Header" -#define SPA_TYPE_META__Pointer SPA_TYPE_META_BASE "Pointer" #define SPA_TYPE_META__VideoCrop SPA_TYPE_META_BASE "VideoCrop" #define SPA_TYPE_META__Shared SPA_TYPE_META_BASE "Shared" +/** + * A metadata element. + * + * This structure is available on the buffer structure and contains + * the type of the metadata and a pointer/size to the actual metadata + * itself. + */ +struct spa_meta { + uint32_t type; /**< metadata type */ + void *data; /**< pointer to metadata */ + uint32_t size; /**< size of metadata */ +}; + +/** + * Describes essential buffer header metadata such as flags and + * timestamps. + */ +struct spa_meta_header { +#define SPA_META_HEADER_FLAG_DISCONT (1 << 0) /**< data is not continous with previous buffer */ +#define SPA_META_HEADER_FLAG_CORRUPTED (1 << 1) /**< data might be corrupted */ +#define SPA_META_HEADER_FLAG_MARKER (1 << 2) /**< media specific marker */ +#define SPA_META_HEADER_FLAG_HEADER (1 << 3) /**< data contains a codec specific header */ +#define SPA_META_HEADER_FLAG_GAP (1 << 4) /**< data contains media neutral data */ +#define SPA_META_HEADER_FLAG_DELTA_UNIT (1 << 5) /**< cannot be decoded independently */ + uint32_t flags; /**< flags */ + uint32_t seq; /**< sequence number, increments with a + * media specific frequency */ + int64_t pts; /**< presentation timestamp */ + int64_t dts_offset; /**< decoding timestamp and a difference with pts */ +}; + +/** + * Video cropping metadata + * a */ +struct spa_meta_video_crop { + int32_t x, y; /**< x and y offsets */ + int32_t width, height; /**< width and height */ +}; + +/** + * Describes the shared memory that holds buffer meta/chunk/data + */ +struct spa_meta_shared { + uint32_t flags; /**< flags */ + int fd; /**< file descriptor of memory */ + uint32_t offset; /**< offset in memory */ + uint32_t size; /**< size of memory */ +}; + +/** + * Describes a control location in the buffer. + */ +struct spa_meta_control { + uint32_t id; /**< control id */ + uint32_t offset; /**< offset in buffer memory */ +}; + struct spa_type_meta { uint32_t Header; - uint32_t Pointer; uint32_t VideoCrop; uint32_t Shared; }; @@ -51,54 +106,11 @@ static inline void spa_type_meta_map(struct spa_type_map *map, struct spa_type_m { if (type->Header == 0) { type->Header = spa_type_map_get_id(map, SPA_TYPE_META__Header); - type->Pointer = spa_type_map_get_id(map, SPA_TYPE_META__Pointer); type->VideoCrop = spa_type_map_get_id(map, SPA_TYPE_META__VideoCrop); type->Shared = spa_type_map_get_id(map, SPA_TYPE_META__Shared); } } -/** Describes essential buffer header metadata */ -struct spa_meta_header { -#define SPA_META_HEADER_FLAG_DISCONT (1 << 0) /**< data is not continous with previous buffer */ -#define SPA_META_HEADER_FLAG_CORRUPTED (1 << 1) /**< data might be corrupted */ -#define SPA_META_HEADER_FLAG_MARKER (1 << 2) /**< media specific marker */ -#define SPA_META_HEADER_FLAG_HEADER (1 << 3) /**< data contains a codec specific header */ -#define SPA_META_HEADER_FLAG_GAP (1 << 4) /**< data contains media neutral data */ -#define SPA_META_HEADER_FLAG_DELTA_UNIT (1 << 5) /**< cannot be decoded independently */ - uint32_t flags; /**< flags */ - uint32_t seq; /**< sequence number, increments with a - * media specific frequency */ - int64_t pts; /**< presentation timestamp */ - int64_t dts_offset; /**< decoding timestamp and a difference with pts */ -}; - -/** Pointer metadata */ -struct spa_meta_pointer { - uint32_t type; /**< the pointer type */ - void *ptr; /**< the pointer */ -}; - -/** Video cropping metadata */ -struct spa_meta_video_crop { - int32_t x, y; /**< x and y offsets */ - int32_t width, height; /**< width and height */ -}; - -/** Describes the shared memory of a buffer is stored */ -struct spa_meta_shared { - int32_t flags; /**< flags */ - int fd; /**< file descriptor of memory */ - int32_t offset; /**< offset in memory */ - uint32_t size; /**< size of memory */ -}; - -/** A metadata element */ -struct spa_meta { - uint32_t type; /**< metadata type */ - void *data; /**< pointer to metadata */ - uint32_t size; /**< size of metadata */ -}; - #ifdef __cplusplus } /* extern "C" */ #endif diff --git a/spa/include/spa/node/node.h b/spa/include/spa/node/node.h index 9692a5b2c..139b3f9cf 100644 --- a/spa/include/spa/node/node.h +++ b/spa/include/spa/node/node.h @@ -405,8 +405,8 @@ struct spa_node { * Tell the port to allocate memory for \a buffers. * * \a buffers should contain an array of pointers to buffers. The data - * in the buffers should point to an array of at least 1 SPA_DATA_TYPE_INVALID - * data pointers that will be filled by this function. + * in the buffers should point to an array of at least 1 data entry + * with a 0 type that will be filled by this function. * * For input ports, the buffers will be dequeued and ready to be filled * and pushed into the port. A notify should be configured so that you can diff --git a/spa/lib/debug.c b/spa/lib/debug.c index 1c0af308d..0abcc3735 100644 --- a/spa/lib/debug.c +++ b/spa/lib/debug.c @@ -72,12 +72,6 @@ int spa_debug_buffer(const struct spa_buffer *buffer) fprintf(stderr, " seq: %u\n", h->seq); fprintf(stderr, " pts: %" PRIi64 "\n", h->pts); fprintf(stderr, " dts_offset: %" PRIi64 "\n", h->dts_offset); - } else if (!strcmp(type_name, SPA_TYPE_META__Pointer)) { - struct spa_meta_pointer *h = m->data; - fprintf(stderr, " struct spa_meta_pointer:\n"); - fprintf(stderr, " type: %s\n", - spa_type_map_get_type(map, h->type)); - fprintf(stderr, " ptr: %p\n", h->ptr); } else if (!strcmp(type_name, SPA_TYPE_META__VideoCrop)) { struct spa_meta_video_crop *h = m->data; fprintf(stderr, " struct spa_meta_video_crop:\n"); diff --git a/src/modules/module-client-node/client-node.c b/src/modules/module-client-node/client-node.c index 6e5d2cca3..a9b2666fe 100644 --- a/src/modules/module-client-node/client-node.c +++ b/src/modules/module-client-node/client-node.c @@ -540,7 +540,7 @@ spa_proxy_node_port_use_buffers(struct spa_node *node, struct proxy *this; struct impl *impl; struct proxy_port *port; - uint32_t i, j; + uint32_t i, j, k; size_t n_mem; struct pw_client_node_buffer *mb; struct spa_meta_shared *msh; @@ -600,9 +600,11 @@ spa_proxy_node_port_use_buffers(struct spa_node *node, t->data.MemFd, msh->fd, msh->flags, msh->offset, msh->size); - for (j = 0; j < buffers[i]->n_metas; j++) { - memcpy(&b->buffer.metas[j], &buffers[i]->metas[j], sizeof(struct spa_meta)); + for (j = 0, k = 0; j < buffers[i]->n_metas; j++) { + if (buffers[i]->metas[j].type != t->meta.Shared) + memcpy(&b->buffer.metas[k++], &buffers[i]->metas[j], sizeof(struct spa_meta)); } + b->buffer.n_metas = k; for (j = 0; j < buffers[i]->n_datas; j++) { struct spa_data *d = &buffers[i]->datas[j]; diff --git a/src/pipewire/link.c b/src/pipewire/link.c index 1cf87df06..079f5d5df 100644 --- a/src/pipewire/link.c +++ b/src/pipewire/link.c @@ -246,6 +246,78 @@ static struct spa_pod *find_param(struct spa_pod **params, int n_params, uint32_ return NULL; } +/* Allocate an array of buffers that can be shared. + * + * All information will be allocated in \a mem. A pointer to a + * newly allocated memory with an array of buffer pointers is + * returned. + * + * \return an array of freshly allocated buffer pointers. free() + * after usage. + * + * Array of allocated buffers: + * + * +==============================+ + * +-| struct spa_buffer * | array of n_buffers of pointers + * | | ... | + * | +==============================+ + * +>| struct spa_buffer | + * | uint32_t id | id of buffer + * | uint32_t n_metas | number of metas + * +-| struct spa_meta *metas | pointer to array of metas + * | | uint32_t n_datas | number of datas + * +|-| struct spa_data *datas | pointer to array of datas + * || +------------------------------+ + * |+>| struct spa_meta | + * | | uint32_t type | Shared meta type, first one always shared + * |+-| void *data | pointer to inlined shared metadata + * || | uint32_t size | sizeof(struct spa_meta_shared) + * || | struct spa_meta | + * || | uint32_t type | more metadata + * +||-| void *data | pointer to mmaped metadata in shared memory + * ||| | uint32_t size | size of metadata + * ||| | ... | more metas follow + * ||| +------------------------------+ + * ||+>| struct spa_meta_shared | inlined shared metadata + * || | uint32_t flags | + * || | int fd | fd of the shared memory block + * || | uint32_t offset | offset of meta/chunk/data for this buffer + * || | uint32_t size | size of meta/chunk/data for this buffer + * || +------------------------------+ + * |+->| struct spa_data | + * | | uint32_t type | memory type, either MemFd or INVALID + * | | uint32_t flags | + * | | int fd | fd of shared memory block + * | | uint32_t mapoffset | offset in shared memory of data + * | | uint32_t maxsize | size of data block + * | +-| void *data | pointer to mmaped data in shared memory + * |+|-| struct spa_chunk *chunk | pointer to mmaped chunk in shared memory + * ||| | ... | more datas follow + * ||| +==============================+ + * ||| | ... | more buffer/meta/data definitions follow + * ||| +==============================+ + * ||| + * ||| + * ||| shared memory block: + * ||| + * ||| +==============================+ + * +-->| meta data memory | metadata memory + * || | ... | + * || +------------------------------+ + * +->| struct spa_chunk | memory for n_datas chunks + * | | struct spa_ringbuffer area | + * | | int32_t stride | + * | | ... chunks | + * | +------------------------------+ + * +>| data | memory for n_datas data + * | ... blocks | + * +==============================+ + * | ... | repeated for each buffer + * +==============================+ + * + * The shared memory block should not contain any types or structure, + * just the actual metadata contents. + */ static struct spa_buffer **alloc_buffers(struct pw_link *this, uint32_t n_buffers, uint32_t n_params, @@ -265,17 +337,19 @@ static struct spa_buffer **alloc_buffers(struct pw_link *this, n_metas = data_size = meta_size = 0; - /* each buffer */ + /* each buffer has 1 meta shared */ skel_size = sizeof(struct spa_buffer); + skel_size += sizeof(struct spa_meta); + skel_size += sizeof(struct spa_meta_shared); metas = alloca(sizeof(struct spa_meta) * (n_params + 1)); - /* add shared metadata */ + /* add shared metadata this should not go into shared + * memory or else a client can damage it so we inline it after + * the array of spa_meta. */ metas[n_metas].type = this->core->type.meta.Shared; metas[n_metas].size = sizeof(struct spa_meta_shared); - meta_size += metas[n_metas].size; n_metas++; - skel_size += sizeof(struct spa_meta); /* collect metadata */ for (i = 0; i < n_params; i++) { @@ -316,31 +390,35 @@ static struct spa_buffer **alloc_buffers(struct pw_link *this, for (i = 0; i < n_buffers; i++) { int j; struct spa_buffer *b; + struct spa_meta_shared *msh; void *p; buffers[i] = b = SPA_MEMBER(bp, skel_size * i, struct spa_buffer); p = SPA_MEMBER(mem->ptr, data_size * i, void); + msh = SPA_MEMBER(b, sizeof(struct spa_buffer), struct spa_meta_shared); + msh->flags = 0; + msh->fd = mem->fd; + msh->offset = data_size * i; + msh->size = data_size; + b->id = i; b->n_metas = n_metas; - b->metas = SPA_MEMBER(b, sizeof(struct spa_buffer), struct spa_meta); + b->metas = SPA_MEMBER(msh, sizeof(struct spa_meta_shared), struct spa_meta); for (j = 0; j < n_metas; j++) { struct spa_meta *m = &b->metas[j]; m->type = metas[j].type; - m->data = p; m->size = metas[j].size; if (m->type == this->core->type.meta.Shared) { - struct spa_meta_shared *msh = p; - - msh->flags = 0; - msh->fd = mem->fd; - msh->offset = data_size * i; - msh->size = data_size; + m->data = msh; + } + else { + m->data = p; + p += m->size; } - p += m->size; } /* pointer to data structure */ b->n_datas = n_datas; @@ -364,6 +442,7 @@ static struct spa_buffer **alloc_buffers(struct pw_link *this, d->chunk->stride = data_strides[j]; ddp += data_sizes[j]; } else { + /* needs to be allocated by a node */ d->type = SPA_ID_INVALID; d->data = NULL; } @@ -535,6 +614,8 @@ static int do_allocation(struct pw_link *this, uint32_t in_state, uint32_t out_s minsize = 1024; } + /* when one of the ports can allocate buffer memory, set the minsize to + * 0 to make sure we don't allocate memory in the shared memory */ if ((in_flags & SPA_PORT_INFO_FLAG_CAN_ALLOC_BUFFERS) || (out_flags & SPA_PORT_INFO_FLAG_CAN_ALLOC_BUFFERS)) minsize = 0;