[PATCH] alsa-lib: issues in snd_seq_change_bit and some enhancements

[PATCH] alsa-lib: issues in snd_seq_change_bit and some enhancements

[Aldrin Martoq]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

Hello hackers,

I found two issues in sequencer part of alsa-lib while working with the
python binding:
1) snd_seq_change_bit is not working as expected.
2) There is no sane way to clear the event_filter of
snd_seq_client_info_t structure. Because the structure is opaque, we
cannot memset to zero because the user doesn't know the size of the
bitmap.

The first attached patch is a demo of both issues. For 1) please apply
snd_seq_change_bit.txt patch. For 2) I propose to clear the bitmap if
NULL is passed, see snd_seq_client_info_set_event_filter.txt patch.



Also, I think it makes no sense that the bitmap is returned or expected
to be given in both snd_seq_client_info_{set,get}_event_filter; the
client_info structure is opaque and the alsa-lib user can't/shouldn't
known the bitmap size. The only sane way is to use the middle interface
function snd_seq_set_client_event_filter(), but it only can be used for
an opened sequencer's client_info.

So, the last patch is a proposal for changing this:
- mark as deprecated both snd_seq_client_info_{set,get}_event_filter
- snd_seq_client_info_event_filter_clear() clears the event_filter
- snd_seq_client_info_event_filter_add() add an event to the event
filter
- snd_seq_client_info_event_filter_del() removes an event from the event
filter
- snd_seq_client_info_event_filter_check() test if an event is present
in the filtering

Any comments? Thanks!



-- 
Aldrin Martoq <amartoq@dcc.uchile.cl>

[-- Attachment #2: event_filter.txt --]
[-- Type: text/x-patch, Size: 2927 bytes --]

diff --git a/test/Makefile.am b/test/Makefile.am
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -1,6 +1,6 @@ check_PROGRAMS=control pcm pcm_min laten
 check_PROGRAMS=control pcm pcm_min latency seq \
 	       playmidi1 timer rawmidi midiloop \
-	       oldapi queue_timer namehint
+	       oldapi queue_timer namehint event_filter
 
 control_LDADD=../src/libasound.la
 pcm_LDADD=../src/libasound.la
@@ -15,6 +15,7 @@ queue_timer_LDADD=../src/libasound.la
 queue_timer_LDADD=../src/libasound.la
 namehint_LDADD=../src/libasound.la
 code_CFLAGS=-Wall -pipe -g -O2
+event_filter_LDADD=../src/libasound.la
 
 INCLUDES=-I$(top_srcdir)/include
 AM_CFLAGS=-Wall -pipe -g
diff --git a/test/event_filter.c b/test/event_filter.c
new file mode 100644
--- /dev/null
+++ b/test/event_filter.c
@@ -0,0 +1,49 @@
+#include <alsa/asoundlib.h>
+
+int main(void) {
+	snd_seq_t *seq;
+	snd_seq_client_info_t *client_info;
+	const unsigned char *event_filter;
+	int isnote;
+	int ispgmc;
+	int oldnote;
+
+	snd_seq_client_info_alloca(&client_info);
+	snd_seq_open(&seq, "default", SND_SEQ_OPEN_DUPLEX, 0);
+
+	snd_seq_get_client_info(seq, client_info);
+	event_filter = snd_seq_client_info_get_event_filter(client_info);
+	printf("Initial event_filter=0x%p\n", event_filter);
+
+	snd_seq_set_client_event_filter(seq, SND_SEQ_EVENT_NOTEON);
+	snd_seq_get_client_info(seq, client_info);
+	event_filter = snd_seq_client_info_get_event_filter(client_info);
+	isnote = snd_seq_get_bit(SND_SEQ_EVENT_NOTEON, (void *) event_filter);
+	printf("after snd_seq_set_client_event (EVENT_NOTEON)    : "
+		"event_filter=0x%p isnote=%d\n",
+		event_filter, isnote);
+	oldnote = snd_seq_change_bit(SND_SEQ_EVENT_NOTEON, (void *) event_filter);
+	isnote = snd_seq_get_bit(SND_SEQ_EVENT_NOTEON, (void *) event_filter);
+	printf("after snd_seq_change_bit                         : "
+		"event_filter=0x%p isnote=%d oldnote=%d\n ==> %s %s\n",
+		event_filter, isnote, oldnote, "snd_seq_change_bit",
+		(isnote == 0 ? "changed" : "ERROR: NOT CHANGED"));
+
+	snd_seq_get_client_info(seq, client_info);
+	snd_seq_client_info_set_event_filter(client_info, NULL);
+	snd_seq_set_client_info(seq, client_info);
+	event_filter = snd_seq_client_info_get_event_filter(client_info);
+	printf("after snd_seq_client_info_set_event_filter(NULL) : "
+		"event_filter=0x%p\n",
+		event_filter);
+	
+	snd_seq_set_client_event_filter(seq, SND_SEQ_EVENT_PGMCHANGE);
+	snd_seq_get_client_info(seq, client_info);
+	event_filter = snd_seq_client_info_get_event_filter(client_info);
+	isnote = snd_seq_get_bit(SND_SEQ_EVENT_NOTEON, (void *) event_filter);
+	ispgmc = snd_seq_get_bit(SND_SEQ_EVENT_PGMCHANGE, (void *) event_filter);
+	printf("after snd_seq_set_client_event (EVENT_PGMCHANGE) : "
+		"event_filter=0x%p isnote=%d ispgmc=%d\n ==> %s %s\n",
+		event_filter, isnote, ispgmc, "snd_seq_client_info_set_event_filter(NULL)",
+		(isnote == 0 ? "cleared" : "ERROR: NOT CLEARED"));
+}

[-- Attachment #3: snd_seq_change_bit.txt --]
[-- Type: text/x-patch, Size: 374 bytes --]

diff --git a/src/seq/seq.c b/src/seq/seq.c
--- a/src/seq/seq.c
+++ b/src/seq/seq.c
@@ -4670,7 +4670,7 @@ int snd_seq_change_bit(int nr, void *arr
 	int result;
 
 	result = ((((unsigned int *)array)[nr >> 5]) & (1UL << (nr & 31))) ? 1 : 0;
-	((unsigned int *)array)[nr >> 5] |= 1UL << (nr & 31);
+	((unsigned int *)array)[nr >> 5] ^= 1UL << (nr & 31);
 	return result;
 }
 

[-- Attachment #4: snd_seq_client_info_set_event_filter.txt --]
[-- Type: text/x-patch, Size: 543 bytes --]

diff --git a/src/seq/seq.c b/src/seq/seq.c
--- a/src/seq/seq.c
+++ b/src/seq/seq.c
@@ -1633,9 +1633,10 @@ void snd_seq_client_info_set_event_filte
 void snd_seq_client_info_set_event_filter(snd_seq_client_info_t *info, unsigned char *filter)
 {
 	assert(info);
-	if (! filter)
+	if (! filter) {
 		info->filter &= ~SNDRV_SEQ_FILTER_USE_EVENT;
-	else {
+		memset(info->event_filter, 0, sizeof(info->event_filter));
+	} else {
 		info->filter |= SNDRV_SEQ_FILTER_USE_EVENT;
 		memcpy(info->event_filter, filter, sizeof(info->event_filter));
 	}

[-- Attachment #5: proposed.txt --]
[-- Type: text/x-patch, Size: 11584 bytes --]

diff --git a/include/seq.h b/include/seq.h
--- a/include/seq.h
+++ b/include/seq.h
@@ -152,6 +152,11 @@ void snd_seq_client_info_set_broadcast_f
 void snd_seq_client_info_set_broadcast_filter(snd_seq_client_info_t *info, int val);
 void snd_seq_client_info_set_error_bounce(snd_seq_client_info_t *info, int val);
 void snd_seq_client_info_set_event_filter(snd_seq_client_info_t *info, unsigned char *filter);
+
+void snd_seq_client_info_event_filter_clear(snd_seq_client_info_t *info);
+void snd_seq_client_info_event_filter_add(snd_seq_client_info_t *info, int event_type);
+void snd_seq_client_info_event_filter_del(snd_seq_client_info_t *info, int event_type);
+int snd_seq_client_info_event_filter_check(snd_seq_client_info_t *info, int event_type);
 
 int snd_seq_get_client_info(snd_seq_t *handle, snd_seq_client_info_t *info);
 int snd_seq_get_any_client_info(snd_seq_t *handle, int client, snd_seq_client_info_t *info);
@@ -575,6 +580,7 @@ int snd_seq_remove_events(snd_seq_t *han
  */
 
 void snd_seq_set_bit(int nr, void *array);
+void snd_seq_unset_bit(int nr, void *array);
 int snd_seq_change_bit(int nr, void *array);
 int snd_seq_get_bit(int nr, void *array);
 
diff --git a/src/seq/seq.c b/src/seq/seq.c
--- a/src/seq/seq.c
+++ b/src/seq/seq.c
@@ -1522,11 +1522,16 @@ int snd_seq_client_info_get_error_bounce
 }
 
 /**
- * \brief Get the event filter bitmap of a client_info container
+ * \brief (DEPRECATED) Get the event filter bitmap of a client_info container
  * \param info client_info container
  * \return NULL if no event filter, or pointer to event filter bitmap
  *
- * \sa snd_seq_get_client_info(), snd_seq_client_info_set_event_filter()
+ * \sa snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_client_info_event_filter_clear(),
+ *     snd_seq_get_client_info(),
+ *     snd_seq_client_info_set_event_filter()
  */
 const unsigned char *snd_seq_client_info_get_event_filter(const snd_seq_client_info_t *info)
 {
@@ -1623,23 +1628,110 @@ void snd_seq_client_info_set_error_bounc
 }
 
 /**
- * \brief Set the event filter bitmap of a client_info container
+ * \brief (DEPRECATED) Set the event filter bitmap of a client_info container
  * \param info client_info container
- * \param filter event filter bitmap
- *
- * \sa snd_seq_get_client_info(), snd_seq_client_info_get_event_filger(),
+ * \param filter event filter bitmap, pass NULL for clearing the bitmap and disable event filtering
+ *
+ * \sa snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_client_info_event_filter_clear(),
+ *     snd_seq_get_client_info(),
+ *     snd_seq_client_info_get_event_filter(),
  *     snd_seq_set_client_event_filter()
  */
 void snd_seq_client_info_set_event_filter(snd_seq_client_info_t *info, unsigned char *filter)
 {
 	assert(info);
-	if (! filter)
+	if (! filter) {
 		info->filter &= ~SNDRV_SEQ_FILTER_USE_EVENT;
-	else {
+		memset(info->event_filter, 0, sizeof(info->event_filter));
+	} else {
 		info->filter |= SNDRV_SEQ_FILTER_USE_EVENT;
 		memcpy(info->event_filter, filter, sizeof(info->event_filter));
 	}
 }
+
+/**
+ * \brief Disable event filtering of a client_info container
+ * \param info client_info container
+ *
+ * Remove all event types added with #snd_seq_client_info_event_filter_add and clear
+ * the event filtering flag of this client_info container.
+ * 
+ * \sa snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_get_client_info(),
+ *     snd_seq_set_client_info()
+ */
+void snd_seq_client_info_event_filter_clear(snd_seq_client_info_t *info)
+{
+	assert(info);
+	info->filter &= ~SNDRV_SEQ_FILTER_USE_EVENT;
+	memset(info->event_filter, 0, sizeof(info->event_filter));
+}
+
+/**
+ * \brief Add an event type to the event filtering of a client_info container
+ * \param info client_info container
+ * \param event_type event type to be added
+ * 
+ * Set the event filtering flag of this client_info and add the specified event type to the 
+ * filter bitmap of this client_info container.
+ *
+ * \sa snd_seq_get_client_info(),
+ *     snd_seq_set_client_info(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_client_info_event_filter_clear()
+ */
+void snd_seq_client_info_event_filter_add(snd_seq_client_info_t *info, int event_type)
+{
+	assert(info);
+	info->filter |= SNDRV_SEQ_FILTER_USE_EVENT;
+	snd_seq_set_bit(event_type, info->event_filter);
+}
+
+/**
+ * \brief Remove an event type from the event filtering of a client_info container
+ * \param info client_info container
+ * \param event_type event type to be removed
+ *
+ * Removes the specified event from the filter bitmap of this client_info container. It will
+ * not clear the event filtering flag, for this see #snd_seq_client_info_event_filter_clear
+ *
+ * \sa snd_seq_get_client_info(),
+ *     snd_seq_set_client_info(),
+ *     snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_client_info_event_filter_clear()
+ */
+void snd_seq_client_info_event_filter_del(snd_seq_client_info_t *info, int event_type)
+{
+	assert(info);
+	snd_seq_unset_bit(event_type, info->event_filter);
+}
+
+/**
+ * \brief Check if an event type is present in the event filtering of a client_info container
+ * \param info client_info container
+ * \param event_type event_type to be checked
+ * \return 1 if the event type is present, 0 otherwise
+ *
+ * Test if the event type is in the filter bitamp of this client_info container.
+ *
+ * \sa snd_seq_get_client_info(),
+ *     snd_seq_set_client_info(),
+ *     snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_clear()
+ */
+int snd_seq_client_info_event_filter_check(snd_seq_client_info_t *info, int event_type)
+{
+	assert(info);
+	return snd_seq_get_bit(event_type, info->event_filter);
+} 
 
 
 /**
@@ -4663,6 +4755,14 @@ void snd_seq_set_bit(int nr, void *array
 }
 
 /**
+ * \brief unset a bit flag
+ */
+void snd_seq_unset_bit(int nr, void *array)
+{
+	((unsigned int *)array)[nr >> 5] &= ~(1UL << (nr & 31));
+}
+
+/**
  * \brief change a bit flag
  */
 int snd_seq_change_bit(int nr, void *array)
@@ -4670,7 +4770,7 @@ int snd_seq_change_bit(int nr, void *arr
 	int result;
 
 	result = ((((unsigned int *)array)[nr >> 5]) & (1UL << (nr & 31))) ? 1 : 0;
-	((unsigned int *)array)[nr >> 5] |= 1UL << (nr & 31);
+	((unsigned int *)array)[nr >> 5] ^= 1UL << (nr & 31);
 	return result;
 }
 
diff --git a/src/seq/seqmid.c b/src/seq/seqmid.c
--- a/src/seq/seqmid.c
+++ b/src/seq/seqmid.c
@@ -251,8 +251,7 @@ int snd_seq_set_client_event_filter(snd_
 
 	if ((err = snd_seq_get_client_info(seq, &info)) < 0)
 		return err;
-	info.filter |= SNDRV_SEQ_FILTER_USE_EVENT;
-	snd_seq_set_bit(event_type, info.event_filter);
+	snd_seq_client_info_event_filter_add(&info, event_type);
 	return snd_seq_set_client_info(seq, &info);
 }
 
diff --git a/test/Makefile.am b/test/Makefile.am
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -1,6 +1,6 @@ check_PROGRAMS=control pcm pcm_min laten
 check_PROGRAMS=control pcm pcm_min latency seq \
 	       playmidi1 timer rawmidi midiloop \
-	       oldapi queue_timer namehint
+	       oldapi queue_timer namehint event_filter
 
 control_LDADD=../src/libasound.la
 pcm_LDADD=../src/libasound.la
@@ -15,6 +15,7 @@ queue_timer_LDADD=../src/libasound.la
 queue_timer_LDADD=../src/libasound.la
 namehint_LDADD=../src/libasound.la
 code_CFLAGS=-Wall -pipe -g -O2
+event_filter_LDADD=../src/libasound.la
 
 INCLUDES=-I$(top_srcdir)/include
 AM_CFLAGS=-Wall -pipe -g
diff --git a/test/event_filter.c b/test/event_filter.c
new file mode 100644
--- /dev/null
+++ b/test/event_filter.c
@@ -0,0 +1,86 @@
+#include <alsa/asoundlib.h>
+
+void dump_event_filter(snd_seq_client_info_t *client_info) {
+	int i, b;
+
+	for (i = 0; i <= 255;) {
+		b = snd_seq_client_info_event_filter_check(client_info, i);
+		i++;
+		printf("%c%s%s", (b ? 'X' : '.'),
+			(i % 8 == 0 ? " " : ""),
+			(i % 32 == 0 ? "\n" : ""));
+	}
+	printf("\n");
+}
+
+int main(void) {
+	snd_seq_t *seq;
+	snd_seq_client_info_t *client_info;
+	const unsigned char *event_filter;
+	int isnote;
+	int ispgmc;
+	int oldnote;
+
+	snd_seq_client_info_alloca(&client_info);
+	snd_seq_open(&seq, "default", SND_SEQ_OPEN_DUPLEX, 0);
+
+	snd_seq_get_client_info(seq, client_info);
+	event_filter = snd_seq_client_info_get_event_filter(client_info);
+	printf("Initial event_filter=0x%p\n", event_filter);
+
+	snd_seq_set_client_event_filter(seq, SND_SEQ_EVENT_NOTEON);
+	snd_seq_get_client_info(seq, client_info);
+	event_filter = snd_seq_client_info_get_event_filter(client_info);
+	isnote = snd_seq_get_bit(SND_SEQ_EVENT_NOTEON, (void *) event_filter);
+	printf("after snd_seq_set_client_event (EVENT_NOTEON)    : "
+		"event_filter=0x%p isnote=%d\n",
+		event_filter, isnote);
+	oldnote = snd_seq_change_bit(SND_SEQ_EVENT_NOTEON, (void *) event_filter);
+	isnote = snd_seq_get_bit(SND_SEQ_EVENT_NOTEON, (void *) event_filter);
+	printf("after snd_seq_change_bit                         : "
+		"event_filter=0x%p isnote=%d oldnote=%d\n ==> %s %s\n",
+		event_filter, isnote, oldnote, "snd_seq_change_bit",
+		(isnote == 0 ? "changed" : "ERROR: NOT CHANGED"));
+
+	snd_seq_get_client_info(seq, client_info);
+	snd_seq_client_info_set_event_filter(client_info, NULL);
+	snd_seq_set_client_info(seq, client_info);
+	event_filter = snd_seq_client_info_get_event_filter(client_info);
+	printf("after snd_seq_client_info_set_event_filter(NULL) : "
+		"event_filter=0x%p\n",
+		event_filter);
+	
+	snd_seq_set_client_event_filter(seq, SND_SEQ_EVENT_PGMCHANGE);
+	snd_seq_get_client_info(seq, client_info);
+	event_filter = snd_seq_client_info_get_event_filter(client_info);
+	isnote = snd_seq_get_bit(SND_SEQ_EVENT_NOTEON, (void *) event_filter);
+	ispgmc = snd_seq_get_bit(SND_SEQ_EVENT_PGMCHANGE, (void *) event_filter);
+	printf("after snd_seq_set_client_event (EVENT_PGMCHANGE) : "
+		"event_filter=0x%p isnote=%d ispgmc=%d\n ==> %s %s\n",
+		event_filter, isnote, ispgmc, "snd_seq_client_info_set_event_filter(NULL)",
+		(isnote == 0 ? "cleared" : "ERROR: NOT CLEARED"));
+
+	snd_seq_client_info_event_filter_clear(client_info);
+	printf("after snd_seq_client_info_event_filter_clear(client_info);\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_add(client_info, SND_SEQ_EVENT_NOTEON);
+	printf("after snd_seq_client_info_event_filter_add(client_info, SND_SEQ_EVENT_NOTEON);\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_add(client_info, SND_SEQ_EVENT_PGMCHANGE);
+	printf("after snd_seq_client_info_event_filter_add(client_info, SND_SEQ_EVENT_PGMCHANGE);\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_del(client_info, SND_SEQ_EVENT_NOTEON);
+	printf("after snd_seq_client_info_event_filter_del(client_info, SND_SEQ_EVENT_NOTEON);\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_clear(client_info);
+	printf("after snd_seq_client_info_event_filter_clear(client_info);\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_add(client_info, SND_SEQ_EVENT_NOTEON);
+	printf("after snd_seq_client_info_event_filter_add(client_info, SND_SEQ_EVENT_NOTEON);\n");
+	dump_event_filter(client_info);
+}

[-- Attachment #6: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] alsa-lib: issues in snd_seq_change_bit and some enhancements

[Clemens Ladisch]

Aldrin Martoq wrote:
> 1) snd_seq_change_bit is not working as expected.

Indeed.

> 2) There is no sane way to clear the event_filter of
> snd_seq_client_info_t structure. Because the structure is opaque, we
> cannot memset to zero because the user doesn't know the size of the
> bitmap.

The event type is an 8-bit quantity; this is part of the sequencer API.
The bitmap has exactly 256 bits, i.e., 32 bytes.

It would be nice if this were actually documented ...


Regards,
Clemens

Re: [PATCH] alsa-lib: issues in snd_seq_change_bit and some enhancements

[Aldrin Martoq]

On Thu, Feb 21, 2008 at 4:59 AM, Clemens Ladisch <clemens@ladisch.de> wrote:
> Aldrin Martoq wrote:
>  > 2) There is no sane way to clear the event_filter of
>  > snd_seq_client_info_t structure. Because the structure is opaque, we
>  > cannot memset to zero because the user doesn't know the size of the
>  > bitmap.
>  The event type is an 8-bit quantity; this is part of the sequencer API.
>  The bitmap has exactly 256 bits, i.e., 32 bytes.
>  It would be nice if this were actually documented ...

Yup, I traced it to the sndrv_seq_client_info definition, but...

I think Takashi explicitly undocumented that stuff. The client_info
and many other types/structures are hidden, the explanation is in the
following link:
typedef struct _snd_seq_client_info snd_seq_client_info_t;
http://www.alsa-project.org/~tiwai/alsa.html#NewSeqAPI

So, my proposal follows the same spirit of hidding internal
structures, which is kind of boring in C. But it's not boring if you
are using some higher language (like python alsaseq, which I'm
currently working on).


Another way to "resolve" this is to create a type instead of the raw
pointer "unsigned char *". Something like:
typedef struct snd_seq_client_info_event_filter {
  unsigned char d[32];
} snd_seq_client_info_event_filter_t;

and use the snd_seq_*_bit functions (btw, I added snd_seq_unset_bit);
but in my opinion, creating functions instead of structures is more in
the spirit of encapsulating the API for future extensions.


What do you think?

-- 
Aldrin Martoq

Re: [PATCH] alsa-lib: issues in snd_seq_change_bit and some enhancements

[Takashi Iwai]

At Thu, 21 Feb 2008 06:12:19 -0300,
Aldrin Martoq wrote:
> 
> On Thu, Feb 21, 2008 at 4:59 AM, Clemens Ladisch <clemens@ladisch.de> wrote:
> > Aldrin Martoq wrote:
> >  > 2) There is no sane way to clear the event_filter of
> >  > snd_seq_client_info_t structure. Because the structure is opaque, we
> >  > cannot memset to zero because the user doesn't know the size of the
> >  > bitmap.
> >  The event type is an 8-bit quantity; this is part of the sequencer API.
> >  The bitmap has exactly 256 bits, i.e., 32 bytes.
> >  It would be nice if this were actually documented ...
> 
> Yup, I traced it to the sndrv_seq_client_info definition, but...
> 
> I think Takashi explicitly undocumented that stuff. The client_info
> and many other types/structures are hidden, the explanation is in the
> following link:
> typedef struct _snd_seq_client_info snd_seq_client_info_t;
> http://www.alsa-project.org/~tiwai/alsa.html#NewSeqAPI
> 
> So, my proposal follows the same spirit of hidding internal
> structures, which is kind of boring in C. But it's not boring if you
> are using some higher language (like python alsaseq, which I'm
> currently working on).

Yes, that looks nice to me.

> Another way to "resolve" this is to create a type instead of the raw
> pointer "unsigned char *". Something like:
> typedef struct snd_seq_client_info_event_filter {
>   unsigned char d[32];
> } snd_seq_client_info_event_filter_t;
> 
> and use the snd_seq_*_bit functions (btw, I added snd_seq_unset_bit);
> but in my opinion, creating functions instead of structures is more in
> the spirit of encapsulating the API for future extensions.

It's better to add new APIs rather than re-defining the type.

Could you split the patches?  I applied the fix for
snd_seq_change_bit() to HG tree now, as it's an obvious bug.
The others are rather extensions.

- a patch to add snd_seq_unset_bit()
- a patch to add new *_event_filter_*() APIs
- a patch to add new test code

Don't forget to give a proper changelog for each.


Thanks,

Takashi

Re: [PATCH] alsa-lib: issues in snd_seq_change_bit and some enhancements

[Aldrin Martoq]

On Thu, 2008-02-21 at 12:36 +0100, Takashi Iwai wrote:
> Could you split the patches?
Hi Takashi, glad to do.

> I applied the fix for
> snd_seq_change_bit() to HG tree now, as it's an obvious bug.
> The others are rather extensions.
> 
> - a patch to add snd_seq_unset_bit()
> - a patch to add new *_event_filter_*() APIs
> - a patch to add new test code
> 
> Don't forget to give a proper changelog for each.

By proper changelog is sufficient the comments for each patch?

So, here we come. Each patch must be applied in the following order.

Patch 1
=======

Added snd_seq_unset_bit() to alsa sequencer API

Signed-off-by: Aldrin Martoq <amartoq@dcc.uchile.cl>
---
diff --git a/include/seq.h b/include/seq.h
--- a/include/seq.h
+++ b/include/seq.h
@@ -575,6 +575,7 @@ int snd_seq_remove_events(snd_seq_t *han
  */
 
 void snd_seq_set_bit(int nr, void *array);
+void snd_seq_unset_bit(int nr, void *array);
 int snd_seq_change_bit(int nr, void *array);
 int snd_seq_get_bit(int nr, void *array);
 
diff --git a/src/seq/seq.c b/src/seq/seq.c
--- a/src/seq/seq.c
+++ b/src/seq/seq.c
@@ -4663,6 +4663,14 @@ void snd_seq_set_bit(int nr, void *array
 }
 
 /**
+ * \brief unset a bit flag
+ */
+void snd_seq_unset_bit(int nr, void *array)
+{
+       ((unsigned int *)array)[nr >> 5] &= ~(1UL << (nr & 31));
+}
+
+/**
  * \brief change a bit flag
  */
 int snd_seq_change_bit(int nr, void *array)



Patch 2
=======

Added snd_seq_client_info_event_filter_{clear,add,del,check} to alsa
sequencer API

Signed-off-by: Aldrin Martoq <amartoq@dcc.uchile.cl>
---
diff -r a002b0270947 include/seq.h
--- a/include/seq.h	Fri Feb 22 01:48:15 2008 -0300
+++ b/include/seq.h	Fri Feb 22 01:59:28 2008 -0300
@@ -152,6 +152,11 @@ void snd_seq_client_info_set_broadcast_f
 void snd_seq_client_info_set_broadcast_filter(snd_seq_client_info_t
*info, int val);
 void snd_seq_client_info_set_error_bounce(snd_seq_client_info_t *info,
int val);
 void snd_seq_client_info_set_event_filter(snd_seq_client_info_t *info,
unsigned char *filter);
+
+void snd_seq_client_info_event_filter_clear(snd_seq_client_info_t
*info);
+void snd_seq_client_info_event_filter_add(snd_seq_client_info_t *info,
int event_type);
+void snd_seq_client_info_event_filter_del(snd_seq_client_info_t *info,
int event_type);
+int snd_seq_client_info_event_filter_check(snd_seq_client_info_t *info,
int event_type);
 
 int snd_seq_get_client_info(snd_seq_t *handle, snd_seq_client_info_t
*info);
 int snd_seq_get_any_client_info(snd_seq_t *handle, int client,
snd_seq_client_info_t *info);
diff -r a002b0270947 src/seq/seq.c
--- a/src/seq/seq.c	Fri Feb 22 01:48:15 2008 -0300
+++ b/src/seq/seq.c	Fri Feb 22 01:59:28 2008 -0300
@@ -1538,6 +1538,87 @@ const unsigned char *snd_seq_client_info
 }
 
 /**
+ * \brief Disable event filtering of a client_info container
+ * \param info client_info container
+ *
+ * Remove all event types added with
#snd_seq_client_info_event_filter_add and clear
+ * the event filtering flag of this client_info container.
+ * 
+ * \sa snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_get_client_info(),
+ *     snd_seq_set_client_info()
+ */
+void snd_seq_client_info_event_filter_clear(snd_seq_client_info_t
*info)
+{
+       assert(info);
+       info->filter &= ~SNDRV_SEQ_FILTER_USE_EVENT;
+       memset(info->event_filter, 0, sizeof(info->event_filter));
+}
+
+/**
+ * \brief Add an event type to the event filtering of a client_info
container
+ * \param info client_info container
+ * \param event_type event type to be added
+ * 
+ * Set the event filtering flag of this client_info and add the
specified event type to the 
+ * filter bitmap of this client_info container.
+ *
+ * \sa snd_seq_get_client_info(),
+ *     snd_seq_set_client_info(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_client_info_event_filter_clear()
+ */
+void snd_seq_client_info_event_filter_add(snd_seq_client_info_t *info,
int event_type)
+{
+       assert(info);
+       info->filter |= SNDRV_SEQ_FILTER_USE_EVENT;
+       snd_seq_set_bit(event_type, info->event_filter);
+}
+
+/**
+ * \brief Remove an event type from the event filtering of a
client_info container
+ * \param info client_info container
+ * \param event_type event type to be removed
+ *
+ * Removes the specified event from the filter bitmap of this
client_info container. It will
+ * not clear the event filtering flag, use
#snd_seq_client_info_event_filter_clear instead.
+ *
+ * \sa snd_seq_get_client_info(),
+ *     snd_seq_set_client_info(),
+ *     snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_client_info_event_filter_clear()
+ */
+void snd_seq_client_info_event_filter_del(snd_seq_client_info_t *info,
int event_type)
+{
+       assert(info);
+       snd_seq_unset_bit(event_type, info->event_filter);
+}
+
+/**
+ * \brief Check if an event type is present in the event filtering of a
client_info container
+ * \param info client_info container
+ * \param event_type event type to be checked
+ * \return 1 if the event type is present, 0 otherwise
+ *
+ * Test if the event type is in the filter bitamp of this client_info
container.
+ *
+ * \sa snd_seq_get_client_info(),
+ *     snd_seq_set_client_info(),
+ *     snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_clear()
+ */
+int snd_seq_client_info_event_filter_check(snd_seq_client_info_t *info,
int event_type)
+{
+       assert(info);
+       return snd_seq_get_bit(event_type, info->event_filter);
+} 
+
+/**
  * \brief Get the number of opened ports of a client_info container
  * \param info client_info container
  * \return number of opened ports



PATCH 3
=======

Added test code for
snd_seq_client_info_event_filter_{clear,add,del,check}

Signed-off-by: Aldrin Martoq <amartoq@dcc.uchile.cl>
---
diff -r d6d0af1de86d test/Makefile.am
--- a/test/Makefile.am	Fri Feb 22 02:01:06 2008 -0300
+++ b/test/Makefile.am	Fri Feb 22 02:10:43 2008 -0300
@@ -1,6 +1,6 @@ check_PROGRAMS=control pcm pcm_min laten
 check_PROGRAMS=control pcm pcm_min latency seq \
 	       playmidi1 timer rawmidi midiloop \
-	       oldapi queue_timer namehint
+	       oldapi queue_timer namehint client_event_filter
 
 control_LDADD=../src/libasound.la
 pcm_LDADD=../src/libasound.la
@@ -14,6 +14,7 @@ oldapi_LDADD=../src/libasound.la
 oldapi_LDADD=../src/libasound.la
 queue_timer_LDADD=../src/libasound.la
 namehint_LDADD=../src/libasound.la
+client_event_filter_LDADD=../src/libasound.la
 code_CFLAGS=-Wall -pipe -g -O2
 
 INCLUDES=-I$(top_srcdir)/include
diff -r d6d0af1de86d test/client_event_filter.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/client_event_filter.c	Fri Feb 22 02:10:43 2008 -0300
@@ -0,0 +1,46 @@
+#include <alsa/asoundlib.h>
+
+void dump_event_filter(snd_seq_client_info_t *client_info) {
+	int i, b;
+
+	for (i = 0; i <= 255;) {
+		b = snd_seq_client_info_event_filter_check(client_info, i);
+		i++;
+		printf("%c%s%s", (b ? 'X' : '.'),
+			(i % 8 == 0 ? " " : ""),
+			(i % 32 == 0 ? "\n" : ""));
+	}
+	printf("\n");
+}
+
+int main(void) {
+	snd_seq_client_info_t *client_info;
+
+	snd_seq_client_info_alloca(&client_info);
+
+	printf("first client_info_event_filter                   :\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_add(client_info,
SND_SEQ_EVENT_NOTEON);
+	printf("after snd_seq_client_info_event_filter_add(client_info,
SND_SEQ_EVENT_NOTEON);\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_add(client_info,
SND_SEQ_EVENT_PGMCHANGE);
+	printf("after snd_seq_client_info_event_filter_add(client_info,
SND_SEQ_EVENT_PGMCHANGE);\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_del(client_info,
SND_SEQ_EVENT_NOTEON);
+	printf("after snd_seq_client_info_event_filter_del(client_info,
SND_SEQ_EVENT_NOTEON);\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_clear(client_info);
+	printf("after snd_seq_client_info_event_filter_clear(client_info);
\n");
+	dump_event_filter(client_info);
+
+	snd_seq_client_info_event_filter_add(client_info,
SND_SEQ_EVENT_NOTEON);
+	printf("after snd_seq_client_info_event_filter_add(client_info,
SND_SEQ_EVENT_NOTEON);\n");
+	dump_event_filter(client_info);
+
+	return 0;
+}
+


PATCH 4
=======

Change snd_seq_set_client_event_filter to use the new
snd_seq_client_info_event_filter_* API

Signed-off-by: Aldrin Martoq <amartoq@dcc.uchile.cl>
---
diff -r a6718b9edd30 src/seq/seqmid.c
--- a/src/seq/seqmid.c	Fri Feb 22 02:12:53 2008 -0300
+++ b/src/seq/seqmid.c	Fri Feb 22 02:15:00 2008 -0300
@@ -251,8 +251,7 @@ int snd_seq_set_client_event_filter(snd_
 
 	if ((err = snd_seq_get_client_info(seq, &info)) < 0)
 		return err;
-	info.filter |= SNDRV_SEQ_FILTER_USE_EVENT;
-	snd_seq_set_bit(event_type, info.event_filter);
+	snd_seq_client_info_event_filter_add(&info, event_type);
 	return snd_seq_set_client_info(seq, &info);
 }



PATCH 5
=======

Mark snd_seq_client_info_{get,set}_event_filter deprecated

Signed-off-by: Aldrin Martoq <amartoq@dcc.uchile.cl>
---
diff -r 442154337bdf src/seq/seq.c
--- a/src/seq/seq.c	Fri Feb 22 02:15:58 2008 -0300
+++ b/src/seq/seq.c	Fri Feb 22 02:22:53 2008 -0300
@@ -1522,11 +1522,17 @@ int snd_seq_client_info_get_error_bounce
 }
 
 /**
- * \brief Get the event filter bitmap of a client_info container
+ * \brief (DEPRECATED) Get the event filter bitmap of a client_info
container
  * \param info client_info container
  * \return NULL if no event filter, or pointer to event filter bitmap
  *
- * \sa snd_seq_get_client_info(),
snd_seq_client_info_set_event_filter()
+ * Use #snd_seq_client_info_event_filter_check() instead.
+ *
+ * \sa snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_client_info_event_filter_clear(),
+ *     snd_seq_get_client_info()
  */
 const unsigned char *snd_seq_client_info_get_event_filter(const
snd_seq_client_info_t *info)
 {
@@ -1704,12 +1710,17 @@ void snd_seq_client_info_set_error_bounc
 }
 
 /**
- * \brief Set the event filter bitmap of a client_info container
+ * \brief (DEPRECATED) Set the event filter bitmap of a client_info
container
  * \param info client_info container
- * \param filter event filter bitmap
- *
- * \sa snd_seq_get_client_info(),
snd_seq_client_info_get_event_filger(),
- *     snd_seq_set_client_event_filter()
+ * \param filter event filter bitmap, pass NULL for no event filtering
+ *
+ * Use #snd_seq_client_info_event_filter_add instead.
+ *
+ * \sa snd_seq_client_info_event_filter_add(),
+ *     snd_seq_client_info_event_filter_del(),
+ *     snd_seq_client_info_event_filter_check(),
+ *     snd_seq_client_info_event_filter_clear(),
+ *     snd_seq_set_client_info()
  */
 void snd_seq_client_info_set_event_filter(snd_seq_client_info_t *info,
unsigned char *filter)
 {




Regards,

-- 
Aldrin Martoq <amartoq@dcc.uchile.cl>

Re: [PATCH] alsa-lib: issues in snd_seq_change_bit and some enhancements

[Takashi Iwai]

At Fri, 22 Feb 2008 02:25:06 -0300,
Aldrin Martoq wrote:
> 
> On Thu, 2008-02-21 at 12:36 +0100, Takashi Iwai wrote:
> > Could you split the patches?
> Hi Takashi, glad to do.
> 
> > I applied the fix for
> > snd_seq_change_bit() to HG tree now, as it's an obvious bug.
> > The others are rather extensions.
> > 
> > - a patch to add snd_seq_unset_bit()
> > - a patch to add new *_event_filter_*() APIs
> > - a patch to add new test code
> > 
> > Don't forget to give a proper changelog for each.
> 
> By proper changelog is sufficient the comments for each patch?

That's OK... but the embedded patches are broken due to line-break.
I had to fix manually.  Please use attachments at the next time if
it's not fixable.

> So, here we come. Each patch must be applied in the following order.

If you send multiple patches, send one patch in a mail.

Anyway, all the patches are now on ALSA HG tree.  Thanks!


Takashi