Few questions about PA API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for the patch. Overall I think this is good new functionality.

Here's a review, which is mostly about details:

On 2014-04-15 02:14, Lukasz Marek wrote:
>
>  From ab9e95ab62602ed107b5d46642149de7ead15c10 Mon Sep 17 00:00:00 2001
> From: Lukasz Marek<lukasz.m.luki2 at gmail.com>
> Date: Tue, 15 Apr 2014 02:08:23 +0200
> Subject: [PATCH] Add pa_stream_write_ext_free() function.

Missing prefix, maybe "Client API:" would be good here, so the subject 
becomes:
"Client API: Add Add pa_stream_write_ext_free function"


> New function allows to pass data pointer that is a member
> of the outer structure that need to be freed too when data
> is not needed anymore.
>
> Signed-off-by: Lukasz Marek<lukasz.m.luki2 at gmail.com>
> ---
>   src/map-file             |  1 +
>   src/pulse/stream.c       | 20 ++++++++++++++++----
>   src/pulse/stream.h       | 11 +++++++++++
>   src/pulsecore/memblock.c | 14 ++++++++++++--
>   src/pulsecore/memblock.h |  2 +-
>   5 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/src/map-file b/src/map-file
> index dc36fdc..5159829 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -330,6 +330,7 @@ pa_stream_update_sample_rate;
>   pa_stream_update_timing_info;
>   pa_stream_writable_size;
>   pa_stream_write;
> +pa_stream_write_ext_free;
>   pa_strerror;
>   pa_sw_cvolume_divide;
>   pa_sw_cvolume_divide_scalar;
> diff --git a/src/pulse/stream.c b/src/pulse/stream.c
> index 8e35c29..2c9a777 100644
> --- a/src/pulse/stream.c
> +++ b/src/pulse/stream.c
> @@ -1464,13 +1464,14 @@ int pa_stream_cancel_write(
>       return 0;
>   }
>
> -int pa_stream_write(
> +int pa_stream_write_ext_free(
>           pa_stream *s,
>           const void *data,
>           size_t length,
>           pa_free_cb_t free_cb,
>           int64_t offset,
> -        pa_seek_mode_t seek) {
> +        pa_seek_mode_t seek,
> +        const void *free_cb_data) {

I think it would be wise to switch order here, i e, to have the free_cb 
just before free_cb_data.

Also, because "userdata" usually is "void *" (not "const void *"), maybe 
it would make more sense to have free_cb_data as "void *" too?

>
>       pa_assert(s);
>       pa_assert(PA_REFCNT_VALUE(s) >= 1);
> @@ -1519,7 +1520,7 @@ int pa_stream_write(
>               chunk.index = 0;
>
>               if (free_cb && !pa_pstream_get_shm(s->context->pstream)) {
> -                chunk.memblock = pa_memblock_new_user(s->context->mempool, (void*) t_data, t_length, free_cb, 1);
> +                chunk.memblock = pa_memblock_new_user(s->context->mempool, (void*) t_data, t_length, free_cb, 1, (void*) free_cb_data);
>                   chunk.length = t_length;
>               } else {
>                   void *d;
> @@ -1544,7 +1545,7 @@ int pa_stream_write(
>           }
>
>           if (free_cb && pa_pstream_get_shm(s->context->pstream))
> -            free_cb((void*) data);
> +            free_cb((void*) free_cb_data);
>       }
>
>       /* This is obviously wrong since we ignore the seeking index . But
> @@ -1591,6 +1592,17 @@ int pa_stream_write(
>       return 0;
>   }
>
> +int pa_stream_write(
> +        pa_stream *s,
> +        const void *data,
> +        size_t length,
> +        pa_free_cb_t free_cb,
> +        int64_t offset,
> +        pa_seek_mode_t seek) {
> +
> +    return pa_stream_write_ext_free(s, data, length, free_cb, offset, seek, data);
> +}
> +
>   int pa_stream_peek(pa_stream *s, const void **data, size_t *length) {
>       pa_assert(s);
>       pa_assert(PA_REFCNT_VALUE(s) >= 1);
> diff --git a/src/pulse/stream.h b/src/pulse/stream.h
> index 7ceb569..6fbd663 100644
> --- a/src/pulse/stream.h
> +++ b/src/pulse/stream.h
> @@ -554,6 +554,17 @@ int pa_stream_write(
>           int64_t offset,          /**< Offset for seeking, must be 0 for upload streams */
>           pa_seek_mode_t seek      /**< Seek mode, must be PA_SEEK_RELATIVE for upload streams */);
>
> +/** Function does exactly the same as pa_stream_write() with the difference
> + *  that free_cb_data is passed to free_cb instead of data. \since 6.0 */
> +int pa_stream_write_ext_free(
> +        pa_stream *p             /**< The stream to use */,
> +        const void *data         /**< The data to write */,
> +        size_t nbytes            /**< The length of the data to write in bytes */,
> +        pa_free_cb_t free_cb     /**< A cleanup routine for the data or NULL to request an internal copy */,
> +        int64_t offset,          /**< Offset for seeking, must be 0 for upload streams */

Nitpick: Inconsistent comma placement. I saw it was there in 
pa_stream_write too, but let's fix it anyway :-)

> +        pa_seek_mode_t seek      /**< Seek mode, must be PA_SEEK_RELATIVE for upload streams */,
> +        const void *free_cb_data /**< Argument passed to free_cb function */);
> +
>   /** Read the next fragment from the buffer (for recording streams).
>    * If there is data at the current read index, \a data will point to
>    * the actual data and \a nbytes will contain the size of the data in
> diff --git a/src/pulsecore/memblock.c b/src/pulsecore/memblock.c
> index 9cc02c1..3d75d99 100644
> --- a/src/pulsecore/memblock.c
> +++ b/src/pulsecore/memblock.c
> @@ -83,6 +83,8 @@ struct pa_memblock {
>           struct {
>               /* If type == PA_MEMBLOCK_USER this points to a function for freeing this memory block */
>               pa_free_cb_t free_cb;
> +            /* If type == PA_MEMBLOCK_USER this is passed as free_cb argument */
> +            pa_atomic_ptr_t free_cb_data;

Hmm, I'm not sure why free_cb_data needs to be atomic. If it's only set 
once in the creation of the memblock, then why not keep it as "void *" 
consistently?

>           } user;
>
>           struct {
> @@ -387,7 +389,13 @@ pa_memblock *pa_memblock_new_fixed(pa_mempool *p, void *d, size_t length, bool r
>   }
>
>   /* No lock necessary */
> -pa_memblock *pa_memblock_new_user(pa_mempool *p, void *d, size_t length, pa_free_cb_t free_cb, bool read_only) {
> +pa_memblock *pa_memblock_new_user(
> +        pa_mempool *p,
> +        void *d,
> +        size_t length,
> +        pa_free_cb_t free_cb,
> +        bool read_only,

Same as previous comment; keep free_cb next to free_cb_data

> +        void *free_cb_data) {
>       pa_memblock *b;
>
>       pa_assert(p);
> @@ -410,6 +418,7 @@ pa_memblock *pa_memblock_new_user(pa_mempool *p, void *d, size_t length, pa_free
>       pa_atomic_store(&b->please_signal, 0);
>
>       b->per_type.user.free_cb = free_cb;
> +    pa_atomic_ptr_store(&b->per_type.user.free_cb_data, free_cb_data);
>
>       stat_add(b);
>       return b;
> @@ -513,7 +522,7 @@ static void memblock_free(pa_memblock *b) {
>       switch (b->type) {
>           case PA_MEMBLOCK_USER :
>               pa_assert(b->per_type.user.free_cb);
> -            b->per_type.user.free_cb(pa_atomic_ptr_load(&b->data));
> +            b->per_type.user.free_cb(pa_atomic_ptr_load(&b->per_type.user.free_cb_data));
>
>               /* Fall through */
>
> @@ -647,6 +656,7 @@ static void memblock_make_local(pa_memblock *b) {
>       /* Humm, not enough space in the pool, so lets allocate the memory with malloc() */
>       b->per_type.user.free_cb = pa_xfree;
>       pa_atomic_ptr_store(&b->data, pa_xmemdup(pa_atomic_ptr_load(&b->data), b->length));
> +    pa_atomic_ptr_store(&b->per_type.user.free_cb_data, pa_atomic_ptr_load(&b->data));
>
>       b->type = PA_MEMBLOCK_USER;
>       b->read_only = false;
> diff --git a/src/pulsecore/memblock.h b/src/pulsecore/memblock.h
> index 502f207..0bfdc92 100644
> --- a/src/pulsecore/memblock.h
> +++ b/src/pulsecore/memblock.h
> @@ -85,7 +85,7 @@ pa_memblock *pa_memblock_new(pa_mempool *, size_t length);
>   pa_memblock *pa_memblock_new_pool(pa_mempool *, size_t length);
>
>   /* Allocate a new memory block of type PA_MEMBLOCK_USER */
> -pa_memblock *pa_memblock_new_user(pa_mempool *, void *data, size_t length, pa_free_cb_t free_cb, bool read_only);
> +pa_memblock *pa_memblock_new_user(pa_mempool *, void *data, size_t length, pa_free_cb_t free_cb, bool read_only, void *free_cb_data);
>
>   /* A special case of pa_memblock_new_user: take a memory buffer previously allocated with pa_xmalloc()  */
>   #define pa_memblock_new_malloced(p,data,length) pa_memblock_new_user(p, data, length, pa_xfree, 0)

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux