> On 2014-11-05 00:26, Peter Meerwald wrote:
> > From: Peter Meerwald <p.meerwald at bct-electronic.com>
> >
> > item_info has per-type fields which should be within a union to
> > save space
>
> I think this is mostly a bikeshed patch, because the memory saved can't
> be significant, can it?
you are right, the patch's comment should not mention memory savings...
> If the memory saved is insignificant I'd say that just longer code
> (added "per_type.memblock_info" here and there) is a slight drawback and
> hence this patch could be skipped. But it's not a strong opinion.
probably the better argument is to codify which attributes are used by
which packet type as is done in several other places in PA
> > Signed-off-by: Peter Meerwald <pmeerw at pmeerw.net>
> > ---
> > src/pulsecore/pstream.c | 117
> +++++++++++++++++++++++++++---------------------
> > 1 file changed, 65 insertions(+), 52 deletions(-)
> >
> > diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
> > index 96ee247..f8217b3 100644
> > --- a/src/pulsecore/pstream.c
> > +++ b/src/pulsecore/pstream.c
> > @@ -92,21 +92,26 @@ struct item_info {
> > PA_PSTREAM_ITEM_SHMREVOKE
> > } type;
> >
> > - /* packet info */
> > - pa_packet *packet;
> > + union {
> > + /* packet info */
> > + pa_packet *packet;
> > +
> > + /* release/revoke info */
> > + uint32_t block_id;
> > +
> > + /* memblock info */
> > + struct {
> > + pa_memchunk chunk;
> > + uint32_t channel;
> > + int64_t offset;
> > + pa_seek_mode_t seek_mode;
> > + } memblock_info;
> > + } per_type;
> > +
> > #ifdef HAVE_CREDS
> > bool with_ancil_data;
> > pa_cmsg_ancil_data ancil_data;
> > #endif
> > -
> > - /* memblock info */
> > - pa_memchunk chunk;
> > - uint32_t channel;
> > - int64_t offset;
> > - pa_seek_mode_t seek_mode;
> > -
> > - /* release/revoke info */
> > - uint32_t block_id;
> > };
> >
> > struct pstream_read {
> > @@ -285,11 +290,11 @@ static void item_free(void *item) {
> > pa_assert(i);
> >
> > if (i->type == PA_PSTREAM_ITEM_MEMBLOCK) {
> > - pa_assert(i->chunk.memblock);
> > - pa_memblock_unref(i->chunk.memblock);
> > + pa_assert(i->per_type.memblock_info.chunk.memblock);
> > + pa_memblock_unref(i->per_type.memblock_info.chunk.memblock);
> > } else if (i->type == PA_PSTREAM_ITEM_PACKET) {
> > - pa_assert(i->packet);
> > - pa_packet_unref(i->packet);
> > + pa_assert(i->per_type.packet);
> > + pa_packet_unref(i->per_type.packet);
> > }
> >
> > if (pa_flist_push(PA_STATIC_FLIST_GET(items), i) < 0)
> > @@ -324,25 +329,19 @@ static void pstream_free(pa_pstream *p) {
> > pa_xfree(p);
> > }
> >
> > -void pa_pstream_send_packet(pa_pstream*p, pa_packet *packet, const
> pa_cmsg_ancil_data *ancil_data) {
> > - struct item_info *i;
> > -
> > +static void pa_pstream_send_item(pa_pstream*p, struct item_info *item,
> const pa_cmsg_ancil_data *ancil_data) {
>
> It looks like pa_pstream_send_item is only used for
> pa_pstream_send_packet and not for pa_pstream_send_memblock (etc), how come?
>
> > pa_assert(p);
> > pa_assert(PA_REFCNT_VALUE(p) > 0);
> > - pa_assert(packet);
> > + pa_assert(item);
> >
> > - if (p->dead)
> > + if (p->dead) {
> > + item_free(item);
> > return;
> > -
> > - if (!(i = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> > - i = pa_xnew(struct item_info, 1);
> > -
> > - i->type = PA_PSTREAM_ITEM_PACKET;
> > - i->packet = pa_packet_ref(packet);
> > + }
> >
> > #ifdef HAVE_CREDS
> > - if ((i->with_ancil_data = !!ancil_data)) {
> > - i->ancil_data = *ancil_data;
> > + if ((item->with_ancil_data = !!ancil_data)) {
> > + item->ancil_data = *ancil_data;
> > if (ancil_data->creds_valid)
> > pa_assert(ancil_data->nfd == 0);
> > else
> > @@ -350,11 +349,25 @@ void pa_pstream_send_packet(pa_pstream*p, pa_packet
> *packet, const pa_cmsg_ancil
> > }
> > #endif
> >
> > - pa_queue_push(p->send_queue, i);
> > + pa_queue_push(p->send_queue, item);
> >
> > p->mainloop->defer_enable(p->defer_event, 1);
> > }
> >
> > +void pa_pstream_send_packet(pa_pstream*p, pa_packet *packet, const
> pa_cmsg_ancil_data *ancil_data) {
> > + struct item_info *i;
> > +
> > + pa_assert(packet);
> > +
> > + if (!(i = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> > + i = pa_xnew(struct item_info, 1);
> > +
> > + i->type = PA_PSTREAM_ITEM_PACKET;
> > + i->per_type.packet = pa_packet_ref(packet);
> > +
> > + pa_pstream_send_item(p, i, ancil_data);
> > +}
> > +
> > void pa_pstream_send_memblock(pa_pstream*p, uint32_t channel, int64_t
> offset, pa_seek_mode_t seek_mode, const pa_memchunk *chunk) {
> > size_t length, idx;
> > size_t bsm;
> > @@ -381,13 +394,13 @@ void pa_pstream_send_memblock(pa_pstream*p, uint32_t
> channel, int64_t offset, pa
> > i->type = PA_PSTREAM_ITEM_MEMBLOCK;
> >
> > n = PA_MIN(length, bsm);
> > - i->chunk.index = chunk->index + idx;
> > - i->chunk.length = n;
> > - i->chunk.memblock = pa_memblock_ref(chunk->memblock);
> > + i->per_type.memblock_info.chunk.index = chunk->index + idx;
> > + i->per_type.memblock_info.chunk.length = n;
> > + i->per_type.memblock_info.chunk.memblock =
> pa_memblock_ref(chunk->memblock);
> >
> > - i->channel = channel;
> > - i->offset = offset;
> > - i->seek_mode = seek_mode;
> > + i->per_type.memblock_info.channel = channel;
> > + i->per_type.memblock_info.offset = offset;
> > + i->per_type.memblock_info.seek_mode = seek_mode;
> > #ifdef HAVE_CREDS
> > i->with_ancil_data = false;
> > #endif
> > @@ -414,7 +427,7 @@ void pa_pstream_send_release(pa_pstream *p, uint32_t
> block_id) {
> > if (!(item = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> > item = pa_xnew(struct item_info, 1);
> > item->type = PA_PSTREAM_ITEM_SHMRELEASE;
> > - item->block_id = block_id;
> > + item->per_type.block_id = block_id;
> > #ifdef HAVE_CREDS
> > item->with_ancil_data = false;
> > #endif
> > @@ -451,7 +464,7 @@ void pa_pstream_send_revoke(pa_pstream *p, uint32_t
> block_id) {
> > if (!(item = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> > item = pa_xnew(struct item_info, 1);
> > item->type = PA_PSTREAM_ITEM_SHMREVOKE;
> > - item->block_id = block_id;
> > + item->per_type.block_id = block_id;
> > #ifdef HAVE_CREDS
> > item->with_ancil_data = false;
> > #endif
> > @@ -495,9 +508,9 @@ static void prepare_next_write_item(pa_pstream *p) {
> > if (p->write.current->type == PA_PSTREAM_ITEM_PACKET) {
> > size_t plen;
> >
> > - pa_assert(p->write.current->packet);
> > + pa_assert(p->write.current->per_type.packet);
> >
> > - p->write.data = (void *) pa_packet_data(p->write.current->packet,
> &plen);
> > + p->write.data = (void *)
> pa_packet_data(p->write.current->per_type.packet, &plen);
> > p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] =
> htonl((uint32_t) plen);
> >
> > if (plen <= MINIBUF_SIZE - PA_PSTREAM_DESCRIPTOR_SIZE) {
> > @@ -508,32 +521,32 @@ static void prepare_next_write_item(pa_pstream *p) {
> > } else if (p->write.current->type == PA_PSTREAM_ITEM_SHMRELEASE) {
> >
> > p->write.descriptor[PA_PSTREAM_DESCRIPTOR_FLAGS] =
> htonl(PA_FLAG_SHMRELEASE);
> > - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl(p->write.current->block_id);
> > + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl(p->write.current->per_type.block_id);
> >
> > } else if (p->write.current->type == PA_PSTREAM_ITEM_SHMREVOKE) {
> >
> > p->write.descriptor[PA_PSTREAM_DESCRIPTOR_FLAGS] =
> htonl(PA_FLAG_SHMREVOKE);
> > - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl(p->write.current->block_id);
> > + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl(p->write.current->per_type.block_id);
> >
> > } else {
> > uint32_t flags;
> > bool send_payload = true;
> >
> > pa_assert(p->write.current->type == PA_PSTREAM_ITEM_MEMBLOCK);
> > - pa_assert(p->write.current->chunk.memblock);
> > + pa_assert(p->write.current->per_type.memblock_info.chunk.memblock);
> >
> > - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_CHANNEL] =
> htonl(p->write.current->channel);
> > - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl((uint32_t) (((uint64_t) p->write.current->offset) >> 32));
> > - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_LO] =
> htonl((uint32_t) ((uint64_t) p->write.current->offset));
> > + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_CHANNEL] =
> htonl(p->write.current->per_type.memblock_info.channel);
> > + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl((uint32_t) (((uint64_t) p->write.current->per_type.memblock_info.offset)
> >> 32));
> > + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_LO] =
> htonl((uint32_t) ((uint64_t)
> p->write.current->per_type.memblock_info.offset));
> >
> > - flags = (uint32_t) (p->write.current->seek_mode &
> PA_FLAG_SEEKMASK);
> > + flags = (uint32_t)
> (p->write.current->per_type.memblock_info.seek_mode & PA_FLAG_SEEKMASK);
> >
> > if (p->use_shm) {
> > uint32_t block_id, shm_id;
> > size_t offset, length;
> > uint32_t *shm_info = (uint32_t *)
> &p->write.minibuf[PA_PSTREAM_DESCRIPTOR_SIZE];
> > size_t shm_size = sizeof(uint32_t) * PA_PSTREAM_SHM_MAX;
> > - pa_mempool *current_pool =
> pa_memblock_get_pool(p->write.current->chunk.memblock);
> > + pa_mempool *current_pool =
> pa_memblock_get_pool(p->write.current->per_type.memblock_info.chunk.memblock);
> > pa_memexport *current_export;
> >
> > if (p->mempool == current_pool)
> > @@ -542,7 +555,7 @@ static void prepare_next_write_item(pa_pstream *p) {
> > pa_assert_se(current_export =
> pa_memexport_new(current_pool, memexport_revoke_cb, p));
> >
> > if (pa_memexport_put(current_export,
> > - p->write.current->chunk.memblock,
> > +
> p->write.current->per_type.memblock_info.chunk.memblock,
> > &block_id,
> > &shm_id,
> > &offset,
> > @@ -555,8 +568,8 @@ static void prepare_next_write_item(pa_pstream *p) {
> >
> > shm_info[PA_PSTREAM_SHM_BLOCKID] = htonl(block_id);
> > shm_info[PA_PSTREAM_SHM_SHMID] = htonl(shm_id);
> > - shm_info[PA_PSTREAM_SHM_INDEX] = htonl((uint32_t) (offset +
> p->write.current->chunk.index));
> > - shm_info[PA_PSTREAM_SHM_LENGTH] = htonl((uint32_t)
> p->write.current->chunk.length);
> > + shm_info[PA_PSTREAM_SHM_INDEX] = htonl((uint32_t) (offset +
> p->write.current->per_type.memblock_info.chunk.index));
> > + shm_info[PA_PSTREAM_SHM_LENGTH] = htonl((uint32_t)
> p->write.current->per_type.memblock_info.chunk.length);
> >
> > p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] =
> htonl(shm_size);
> > p->write.minibuf_validsize = PA_PSTREAM_DESCRIPTOR_SIZE +
> shm_size;
> > @@ -569,8 +582,8 @@ static void prepare_next_write_item(pa_pstream *p) {
> > }
> >
> > if (send_payload) {
> > - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] =
> htonl((uint32_t) p->write.current->chunk.length);
> > - p->write.memchunk = p->write.current->chunk;
> > + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] =
> htonl((uint32_t) p->write.current->per_type.memblock_info.chunk.length);
> > + p->write.memchunk =
> p->write.current->per_type.memblock_info.chunk;
> > pa_memblock_ref(p->write.memchunk.memblock);
> > p->write.data = NULL;
> > }
> >
>
>
--
Peter Meerwald
+43-664-2444418 (mobile)
