casaroli commented on code in PR #3840:
URL: https://github.com/apache/nuttx-apps/pull/3840#discussion_r4435138508
##########
system/nxplayer/nxplayer.c:
##########
@@ -1898,14 +1931,74 @@ static int nxplayer_playinternal(FAR struct nxplayer_s
*pplayer,
(unsigned long)&caps) == caps.ac_len)
{
min_channels = caps.ac_channels >> 4;
+ max_channels = caps.ac_channels & 0x0f;
if (min_channels != 0 && nchannels < min_channels)
{
ret = -EINVAL;
goto err_out;
}
+
+#if defined(CONFIG_AUDIO_FORMAT_PCM) && \
+ defined(CONFIG_NXPLAYER_FMT_FROM_EXT)
+ if (pplayer->wav_playback &&
+ ((max_channels != 0 && nchannels > max_channels) ||
+ (caps.ac_controls.hw[0] & nxplayer_wav_rate_cap(samprate)) == 0))
+ {
+ ret = -ENOTSUP;
+ auderr("ERROR: WAV format is not supported by audio device\n");
+ goto err_out;
+ }
+#endif
}
+#if defined(CONFIG_AUDIO_FORMAT_PCM) && \
+ defined(CONFIG_NXPLAYER_FMT_FROM_EXT)
+ else if (pplayer->wav_playback)
+ {
+ ret = -ENOTSUP;
+ auderr("ERROR: Unable to query audio device capabilities for WAV\n");
+ goto err_out;
+ }
+
+ if (pplayer->wav_playback)
+ {
+ caps.ac_len = sizeof(caps);
+ caps.ac_type = AUDIO_TYPE_QUERY;
+ caps.ac_subtype = filefmt;
+ caps.ac_format.b[0] = 0;
+ supported = false;
+ if (ioctl(pplayer->dev_fd, AUDIOIOC_GETCAPS,
+ (unsigned long)&caps) != caps.ac_len)
+ {
+ ret = -ENOTSUP;
+ auderr("ERROR: Unable to query WAV PCM formats\n");
+ goto err_out;
+ }
+
+ for (index = 0; index < sizeof(caps.ac_controls.b); index++)
+ {
+ if (caps.ac_controls.b[index] == subfmt)
+ {
+ supported = true;
+ break;
+ }
+
+ if (caps.ac_controls.b[index] == AUDIO_SUBFMT_END)
+ {
+ break;
+ }
+ }
+
+ if (!supported)
+ {
+ ret = -ENOTSUP;
+ auderr("ERROR: Audio device does not support 16-bit PCM WAV\n");
Review Comment:
Most audio drivers do not list AUDIO_SUBFMT_PCM_S16_LE for an AUDIO_FMT_PCM
query: drivers/audio/cs4344.c returns AUDIO_SUBFMT_END, and only sim_alsa.c,
audio_fake.c, virtio-snd.c and sam_classd.c report it. On the boards that wrap
their codec with pcm_decode_initialize() (esp32 i2s, cs4344, wm8904, wm8994,
rp2040 i2s and others), `play file.wav` works on master and fails with ENOTSUP
after this PR.
Fix: keep the old path (send the whole file) for devices that do not report
AUDIO_SUBFMT_PCM_S16_LE.
##########
system/nxplayer/nxplayer.c:
##########
@@ -1860,6 +1881,16 @@ static int nxplayer_playinternal(FAR struct nxplayer_s
*pplayer,
struct ap_buffer_info_s buf_info;
struct audio_caps_s caps;
int min_channels;
+ int max_channels;
Review Comment:
max_channels is set at line 1934 but used only inside `#if
defined(CONFIG_AUDIO_FORMAT_PCM) && defined(CONFIG_NXPLAYER_FMT_FROM_EXT)`, so
with either option off -Wall reports it as set but not used.
Fix: declare it inside the same #if.
##########
system/nxplayer/nxplayer_wav.c:
##########
@@ -0,0 +1,388 @@
+/****************************************************************************
+ * apps/system/nxplayer/nxplayer_wav.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <inttypes.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <string.h>
+#include <strings.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include <nuttx/audio/audio.h>
+#include <nuttx/debug.h>
+
+#include "system/nxplayer.h"
+
+#if defined(CONFIG_AUDIO_FORMAT_PCM) && \
+ defined(CONFIG_NXPLAYER_FMT_FROM_EXT)
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+static uint16_t nxplayer_wav_le16(FAR const uint8_t *data)
+{
+ return (uint16_t)data[0] | ((uint16_t)data[1] << 8);
+}
+
+static uint32_t nxplayer_wav_le32(FAR const uint8_t *data)
+{
+ return (uint32_t)data[0] | ((uint32_t)data[1] << 8) |
+ ((uint32_t)data[2] << 16) | ((uint32_t)data[3] << 24);
+}
+
+static int nxplayer_wav_read_exact(int fd, FAR uint8_t *buffer,
+ size_t length)
+{
+ size_t offset = 0;
+
+ while (offset < length)
+ {
+ ssize_t ret = read(fd, buffer + offset, length - offset);
+
+ if (ret < 0)
+ {
+ if (errno == EINTR)
+ {
+ continue;
+ }
+
+ return -errno;
+ }
+
+ if (ret == 0)
+ {
+ return -EINVAL;
+ }
+
+ offset += ret;
+ }
+
+ return OK;
+}
+
+static int nxplayer_parse_wav_data(int fd, FAR uint32_t *samprate,
+ FAR uint8_t *nchannels,
+ FAR uint8_t *bpsamp,
+ FAR uint32_t *data_length)
+{
+ uint8_t header[12];
+ uint8_t chunk[8];
+ uint8_t format[16];
+ off_t file_end;
+ uint64_t riff_end;
+ uint64_t position = sizeof(header);
+ uint64_t data_offset = 0;
+ uint32_t data_size = 0;
+ uint32_t rate = 0;
+ uint32_t byte_rate = 0;
+ uint16_t channels = 0;
+ uint16_t block_align = 0;
+ uint16_t bits = 0;
+ bool have_format = false;
+ bool have_data = false;
+ int ret;
+
+ file_end = lseek(fd, 0, SEEK_END);
+ if (file_end < 0)
+ {
+ return -errno;
+ }
+
+ if (file_end < (off_t)sizeof(header) ||
+ lseek(fd, 0, SEEK_SET) != 0)
+ {
+ return -EINVAL;
+ }
+
+ ret = nxplayer_wav_read_exact(fd, header, sizeof(header));
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ if (memcmp(header, "RIFF", 4) != 0 ||
+ memcmp(header + 8, "WAVE", 4) != 0)
+ {
+ return -EINVAL;
+ }
+
+ riff_end = (uint64_t)nxplayer_wav_le32(header + 4) + 8;
+ if (riff_end < sizeof(header) || riff_end > (uint64_t)file_end)
+ {
+ return -EINVAL;
+ }
+
+ while (position < riff_end)
+ {
+ uint32_t chunk_size;
+ uint64_t next;
+
+ if (riff_end - position < sizeof(chunk) ||
+ lseek(fd, (off_t)position, SEEK_SET) != (off_t)position)
+ {
+ return -EINVAL;
+ }
+
+ ret = nxplayer_wav_read_exact(fd, chunk, sizeof(chunk));
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ chunk_size = nxplayer_wav_le32(chunk + 4);
+ next = position + sizeof(chunk) + chunk_size + (chunk_size & 1);
+ if (next > riff_end || next > (uint64_t)file_end)
+ {
+ return -EINVAL;
+ }
+
+ if (memcmp(chunk, "fmt ", 4) == 0)
+ {
+ uint64_t expected_rate;
+
+ if (have_format || chunk_size < sizeof(format))
+ {
+ return -EINVAL;
+ }
+
+ ret = nxplayer_wav_read_exact(fd, format, sizeof(format));
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ if (nxplayer_wav_le16(format) != 1)
+ {
+ return -ENOTSUP;
+ }
+
+ channels = nxplayer_wav_le16(format + 2);
+ rate = nxplayer_wav_le32(format + 4);
+ byte_rate = nxplayer_wav_le32(format + 8);
+ block_align = nxplayer_wav_le16(format + 12);
+ bits = nxplayer_wav_le16(format + 14);
+ expected_rate = (uint64_t)rate * channels * bits / 8;
+
+ if ((channels != 1 && channels != 2) || rate == 0 ||
+ bits != 16 || block_align != channels * 2 ||
+ expected_rate > UINT32_MAX || byte_rate != expected_rate)
+ {
+ return -EINVAL;
+ }
+
+ have_format = true;
+ }
+ else if (memcmp(chunk, "data", 4) == 0)
+ {
+ if (have_data)
+ {
+ return -EINVAL;
+ }
+
+ data_offset = position + sizeof(chunk);
+ data_size = chunk_size;
+ have_data = true;
+ }
+
+ position = next;
+ }
+
+ if (!have_format || !have_data || data_size == 0 ||
+ data_size % block_align != 0)
+ {
+ return -EINVAL;
+ }
+
+ if (lseek(fd, (off_t)data_offset, SEEK_SET) != (off_t)data_offset)
+ {
+ return errno > 0 ? -errno : -EIO;
+ }
+
+ *samprate = rate;
+ *nchannels = (uint8_t)channels;
+ *bpsamp = (uint8_t)bits;
+ *data_length = data_size;
+ return OK;
+}
+
+static int nxplayer_parse_wav(FAR struct nxplayer_s *pplayer, int fd,
+ FAR uint32_t *samplerate,
+ FAR uint8_t *nchannels, FAR uint8_t *bpsamp,
+ FAR int *subfmt)
+{
+ int ret;
+
+ ret = nxplayer_parse_wav_data(fd, samplerate, nchannels, bpsamp,
+ &pplayer->wav_data_remaining);
+ if (ret < 0)
+ {
+ auderr("ERROR: Invalid or unsupported WAV file: %d\n", ret);
+ return ret;
+ }
+
+ if (nxplayer_wav_rate_cap(*samplerate) == 0)
+ {
+ auderr("ERROR: Unsupported WAV sample rate: %" PRIu32 "\n",
+ *samplerate);
+ return -ENOTSUP;
+ }
+
+ pplayer->wav_block_align = *nchannels * 2;
+ pplayer->wav_playback = true;
+ *bpsamp = 16;
+ *subfmt = AUDIO_SUBFMT_PCM_S16_LE;
+ return OK;
+}
+
+static int nxplayer_fill_wav(FAR struct nxplayer_s *pplayer, int fd,
Review Comment:
audio/pcm_decode.c parses the WAV header from the first buffer and returns
-EINVAL ("Invalid PCM WAV file") without one, unless CONFIG_AUDIO_FORMAT_RAW=y.
This decoder sends only the data chunk, so pcm_decode devices cannot play it
even when the capability check passes.
Fix: send the header to pcm_decode devices too, and test on one pcm_decode
board (for example stm32f4discovery with cs43l22, as acassis suggested).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
