On Wed, 20.08.08 01:32, Joe Marcus Clarke ([EMAIL PROTECTED]) wrote:
> Here is an OSS driver I did for libcanberra. It would work on any
> platform which supports OSS. The sample Freedesktop sound theme has
> been tested to work with this driver. The diff applies to libcanberra
> 0.7.
>
> http://www.marcuscom.com/downloads/libcanberra.diff
Here are a few comments:
+#ifdef __FreeBSD__
+static inline char *strndup (const char *s, size_t n)
Hmm, could you replace this by a proper HAVE_STRNDUP check? The
Solaris people need this too. As might others.
Also, please move the implementation to malloc.c and call it
ca_strdup. Newer GCC will refuse to inline this anyway, so please
don't even try to.
+{
+ size_t nAvail;
Hehe, this isn't Windows. ;-) "n_avail" sounds better, doesn't it?
+ char *p;
+
+ if (!s) {
+ return NULL;
+ }
+
+ if (memchr (s, '\0', n) != NULL) {
+ nAvail = strlen (s);
+ if (nAvail > n)
+ nAvail = n;
The strlen could be replaced by subtracting s from the return value of
memchr. WOuld be an optimization, but doesn't really matter.
+ } else {
+ nAvail = n;
+ }
+ mode = fcntl(out->pcm, F_GETFL);
+ mode &= ~O_NONBLOCK;
+ fcntl(out->pcm, F_SETFL, mode);
I'd prefer if we'd had some proper error checking here.
+ switch (ca_sound_file_get_sample_type(out->file)) {
+ case CA_SAMPLE_U8:
+ val = AFMT_U8;
+ break;
+ case CA_SAMPLE_S16NE:
+ val = AFMT_S16_NE;
+ break;
+ case CA_SAMPLE_S16RE:
+#if _BYTE_ORDER == _LITTLE_ENDIAN
+ val = AFMT_S16_BE;
+#else
+ val = AFMT_S16_LE;
+#endif
+ break;
+ }
+
+ if (ioctl(out->pcm, SNDCTL_DSP_SETFMT, &val) < 0)
+ goto finish;
This is not sufficient. SNDCTL_DSP_SETFMT asks the driver to enable the a
specific format. But if the hw cannot do this it will enable a
different one. You need to check for what the sound card actually
actviated. Also, if the file is in CA_SAMPLE_S16RE and your soundcard
can do only S16NE then *you* will have to do the byteswapping. OSS is
a very low level API and leaves all this conversion mess to the user,
sorry. (yes, this is one of the reasons why OSS is broken)
I think it is relatively safe to assume that the sound card can do
either S16NE or S16RE (sun hardware is this crazy). Thus I fear you
need to add code than byteswaps all PCM data if necessary and is
capable of converting U8 to either one.
+ val = ca_sound_file_get_nchannels(out->file) > 1 ? 1 : 0;
+ if (ioctl(out->pcm, SNDCTL_DSP_STEREO, &val) < 0)
+ goto finish;
Sound files can be multi-channel. This check is not sufficient. If
the sound file does surround (which might be good for login sounds as
one example) this check will break. Of course, since OSS doesn't
define any channel mapping when more than 2ch are used you probably
should return an error here if the file announces more than two
channels. Proper surround sound on OSS is not possible.
Again, SNDCTL_DSP_STEREO succeeding is no guarantee that the driver
actually can do stereo. You have to check directly if this
worked. Then, you probably want to use SNDCTL_DSP_CHANNELS.
Also, you need to do at least the conversion from mono to stereo
properly, since many (especially newer) sound devices can only do
stereo, but most event sound files are probably mono.
+ val = ca_sound_file_get_rate(out->file);
+ if (ioctl(out->pcm, SNDCTL_DSP_SPEED, &val) < 0)
+ goto finish;
Again, SNDCTL_DSP_SPEED is no guarantee that the freq you asked for is
actually available. You need to check if that worked. Many drivers
will return that they can do 44099 if you ask them for 44100. You
should thus assume that you got the right freq if the returned value
is between 0.95 and 1.05 times what you requested.
And again, newer sound cards can do only one fixed frequency, such as
48000. Most sound files will be in 441000, 22000 or even 8000Hz. You
need to do some basic resampling here.
Having said all this, I am happy to merge the patch, even if you don't
add the conversion code between sample types, channel maps, and
rates. If users complain, I will redirect them to you. But please,
before I merge this, add at least some checks that the cases are
caught where the hardware cannot do what you ask for, because
otherwise the user might end up hearing terrible noises, and I don't
want to take the blame for this. ;-)
If you could submit the strndup part and the actual driver as seperate
mails you would do me a favour because it makes it much easier for me
to make this two commits in git with you being attributed as the author.
Lennart
--
Lennart Poettering Red Hat, Inc.
lennart [at] poettering [dot] net ICQ# 11060553
http://0pointer.net/lennart/ GnuPG 0x1A015CC4
_______________________________________________
libcanberra-discuss mailing list
[email protected]
https://tango.0pointer.de/mailman/listinfo/libcanberra-discuss
