Repository navigation
system/nxplayer: Parse WAV headers and retain PCM fallback - #3840
imeghar2408-max wants to merge 3 commits into
Conversation
|
I've pushed the refactor in commit WAV playback now uses Validation completed:
Could you please take another look? |
665cd07 to
243b4a4
Compare
|
Hi @xiaoxiang781216 , thanks for the feedback! I've updated the implementation by moving the WAV-specific RIFF/WAVE parsing, format validation, and PCM buffer-filling logic into a dedicated The existing MP3/SBC decoder paths and raw PCM playback path have been kept separate from the WAV-specific changes. I've also rebuilt and tested the SIM ALSA configuration, including valid WAV playback with odd-sized chunks, raw PCM playback, malformed WAV rejection, and mid-playback truncation handling. Please have another look when you get a chance. Thanks! |
bb8605f to
7a2c7e3
Compare
| CODE int (*pre_parse)(int fd, FAR uint32_t *samplerate, | ||
| FAR uint8_t *chans, FAR uint8_t *bps); | ||
| CODE int (*fill_data)(int fd, FAR struct ap_buffer_s *apb); | ||
| CODE int (*pre_parse)(FAR struct nxplayer_s *pplayer, int fd, |
There was a problem hiding this comment.
why need change the prototype? if the change is really needed, please split the common change to the separated patch, and wave decoder in one and only one patch.
There was a problem hiding this comment.
The prototype change is needed to pass the player context to decoder callbacks and allow pre_parse to return the selected subformat. The WAV decoder needs this to maintain per-player parsing state and select the appropriate PCM subformat.
I'll separate the generic callback-interface change into its own commit, using adapters to preserve the existing MP3/SBC and common PCM behavior. All WAV-specific decoding and integration will be contained in one separate commit.
thanks!
There was a problem hiding this comment.
please remove the adapter and modify the mp3/sbc directly
There was a problem hiding this comment.
@xiaoxiang781216 I removed the adapters and modified the MP3/SBC callbacks directly to use the shared decoder interface.
I've also split the changes into two commits.. waiting for your review:)
7a2c7e3 to
ad73730
Compare
|
@imeghar2408-max I'm confuse, the nxplayer already have support to play wav files, in fact that was the first type of format supported before mp3, as you can see on this video tutorial from 8 years ago: Maybe the Copilot is making a mistake. I suggest you to get a STM32F4Discovery board and test it before applying your patch. |
Ohh, |
Ahhh ok, one limitation was the wav file had to be in an specific sampling rate otherwise it didn't work, maybe if this solution works with any format, it will be nice |
yeah, exactly! that's the limitation i was trying to address. In the rn nxplayer path, the WAV sample rate isn't extracted before playback, so it falls back to the default... (smth 48 khz ig), My change reads the sample rate from the WAV header and checks whether the audio device supports it before starting playback. |
| if (!supported) | ||
| { | ||
| ret = -ENOTSUP; | ||
| auderr("ERROR: Audio device does not support 16-bit PCM WAV\n"); |
There was a problem hiding this 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.
| return OK; | ||
| } | ||
|
|
||
| static int nxplayer_fill_wav(FAR struct nxplayer_s *pplayer, int fd, |
There was a problem hiding this 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).
| struct ap_buffer_info_s buf_info; | ||
| struct audio_caps_s caps; | ||
| int min_channels; | ||
| int max_channels; |
There was a problem hiding this 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.
|
@casaroli Thanks for the review! I've addressed the requested changes and pushed the updates... |
|
|
||
| if (pplayer->wav_playback && | ||
| ((max_channels != 0 && nchannels > max_channels) || | ||
| (caps.ac_controls.hw[0] & nxplayer_wav_rate_cap(samprate)) == 0)) |
There was a problem hiding this comment.
Some drivers report the sample rate mask in ac_controls.b[0] only: drivers/virtio/virtio-snd.c:735 stores it in b[0], and arch/arm/src/sama5/sam_classd.c:1517 casts it to uint8_t. AUDIO_SAMP_RATE_48K is 0x0100, so on these devices a 48 kHz WAV now fails with ENOTSUP (virtio-snd is in qemu-armv8a:netnsh and rv-virt:netnsh). On master the file plays.
Fix: remove the rate test here and rely on the error check of AUDIOIOC_CONFIGURE.
| { | ||
| ret = pplayer->ops->pre_parse(pplayer->fd, &samprate, | ||
| &nchannels, &bpsamp); | ||
| ret = pplayer->ops->pre_parse(pplayer, pplayer->fd, &samprate, |
There was a problem hiding this comment.
When nxplayer_parse_wav() fails, the playback fails, but master plays these files: an http:// URL with CONFIG_NXPLAYER_HTTP_STREAMING_SUPPORT (lseek(SEEK_END) fails on the socket), 8-bit samples (audio/pcm_decode.c accepts 8 and 16 bits), and a RIFF or data size larger than the file (pcm_decode.c does not check them).
Fix: when nxplayer_parse_wav() fails, use the PCM entry of g_dec_ops and send the whole file, as master does.
casaroli
left a comment
There was a problem hiding this comment.
Fix: squash ad649e5 into ad73730.
- The title and the body say that the PR adds WAV playback, but nxplayer plays WAV files on master. The body does not say that devices without
AUDIO_SUBFMT_PCM_S16_LEget the whole file, or that unsupported WAV files are now rejected.
Fix: describe the change against master (header parsing and the sample rate from the header on S16_LE devices, the whole file on other devices) and the tests of both paths.
| apb->curbyte = 0; | ||
| apb->flags = 0; | ||
|
|
||
| if (pplayer->wav_passthrough) |
There was a problem hiding this comment.
Every pcm_decode board uses this path, and it is not tested: the commit message says "Physical pcm_decode hardware validation remains outstanding", and sim ALSA reports AUDIO_SUBFMT_PCM_S16_LE, so the sim test does not use it.
Fix: play a .wav file on one pcm_decode board (for example stm32f4discovery:audio or esp32-lyrat:audio) and add the log to the PR body.
There was a problem hiding this comment.
Thanks for pointing this out. I currently don't own a PCM decoder-backed board, but I'm looking into purchasing a suitable board to perform the requested hardware validation.
I understand that SIM-ALSA does not cover this specific code path. If hardware validation is necessary before this PR can proceed, I'll continue working on it and share the playback logs once I have the board.
|
@imeghar2408-max please update your upstream and rebase your branch with it to fix the CI issue |
Pass the player context to decoder pre-parse and fill callbacks, and allow pre-parse callbacks to select the playback subformat. Update the existing PCM, MP3, and SBC callbacks to use the common interface. Assisted-by: ChatGPT:GPT-6 Signed-off-by: Megha Rajput <i.meghar.2408@gmail.com>
ad649e5 to
b389c3b
Compare
|
Hi @casaroli, thanks for the detailed review! |
b389c3b to
b3acb50
Compare
casaroli
left a comment
There was a problem hiding this comment.
Commit b3acb50 also has style changes in code it does not change (six int errcode = errno; blocks, an indent in nxplayer_playthread(), struct sockaddr_in) and the AUDIOIOC_RELEASE fix in the error path, which applies to every format.
Fix: move these to their own commit before the WAV commit.
The run on a pcm_decode board that I asked for on 11-01 is not necessary: pcm_decode over audio_null under QEMU plays stereo and mono files with this PR.
| max_channels = caps.ac_channels & 0x0f; | ||
|
|
||
| if (pplayer->wav_playback && | ||
| max_channels != 0 && nchannels > max_channels) |
There was a problem hiding this comment.
drivers/audio/es8311.c reports 1 channel for an AUDIO_TYPE_OUTPUT query, but es8311_configure() accepts 2 ("The codec can take stereo audio and play only one channel"). esp32s3-korvo-2:audio, esp32s3-lcd-ev:audio and esp32s2-kaluga-1:audio use it through pcm_decode. A stereo WAV plays on master and fails with ENOTSUP with this PR. Reproduced with pcm_decode over audio_null with the query reporting 1 channel: master plays the file, this PR prints Error playing file: 138.
Fix: remove the max_channels test and rely on the error check of AUDIOIOC_CONFIGURE, as for the sample rate.
| extern const struct nxplayer_dec_ops_s g_wav_dec_ops; | ||
|
|
||
| bool nxplayer_is_wav_file(FAR const char *pfilename); | ||
| uint32_t nxplayer_wav_rate_cap(uint32_t rate); |
There was a problem hiding this comment.
nxplayer_wav_rate_cap() is public, but its only caller is in nxplayer_wav.c, which only tests the result against 0.
Fix: make it a static function in nxplayer_wav.c, and remove it from the header.
b3acb50 to
6542ae7
Compare
Release the audio device on setup failures only when the reserve operation succeeded. Keep generic playback formatting changes separate from WAV support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Megha Rajput <i.meghar.2408@gmail.com>
Parse RIFF/WAVE headers to obtain sample rate, channel count, and sample format for PCM S16_LE playback. Preserve whole-file PCM fallback for devices that parse WAV headers themselves, and rewind seekable input when parsing fails. Assisted-by: ChatGPT:GPT-6 Signed-off-by: Megha Rajput <i.meghar.2408@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
6542ae7 to
6c38233
Compare
casaroli
left a comment
There was a problem hiding this comment.
Commit 6c38233 still removes the blank line after DEBUGASSERT(errcode > 0); in nxplayer_enqueuebuffer(), nxplayer_fforward() and nxplayer_rewind() of system/nxplayer/nxplayer.c; b143f1f keeps it there.
Fix: drop these three hunks from 6c38233.
The message of b143f1f has Co-authored-by: Copilot and no Assisted-by:. CONTRIBUTING 1.5 requires Assisted-by: AGENT_NAME:MODEL_VERSION above the signature on every commit that used such a tool.
Fix: add the Assisted-by: line above Signed-off-by in b143f1f.
system/nxplayer: Parse WAV headers and retain PCM fallback
Improve the existing WAV playback implementation in
system/nxplayerby parsing RIFF/WAVE headers and using the audio parameters specified in supported WAV files.The implementation supports PCM 16-bit mono and stereo WAV files through the existing audio playback path. It validates RIFF chunks, format parameters, chunk boundaries, and data lengths before playback.
It also preserves compatibility with devices that use
pcm_decodeto parse WAV headers internally, retains the existing raw PCM playback path, and reports read errors during WAV playback cleanup.Summary
Update the existing WAV playback path to handle WAV files with RIFF chunks and use the sample rate and channel information specified in their headers.
For devices reporting
AUDIO_SUBFMT_PCM_S16_LE, nxplayer parses the WAV header, extracts the audio parameters, and configures playback accordingly.For devices that do not report
AUDIO_SUBFMT_PCM_S16_LE, the existing whole-file PCM decoder path is preserved so thatpcm_decodecan parse the WAV header itself.When WAV parsing fails, nxplayer falls back to the existing PCM decoder when the original input can be safely replayed.
Changes
AUDIOIOC_CONFIGUREto report unsupported configurations.AUDIOIOC_CONFIGUREfor channel configuration errors.nxplayer_wav_rate_cap()local tonxplayer_wav.c.Commit structure
The changes are split into three commits:
system/nxplayer: Refactor decoder callback interface.system/nxplayer: Fix reserve cleanup and formattingsystem/nxplayer: Parse WAV headers and retain PCM fallbackValidation
1. SIM-ALSA build
Build command:
Relevant build output:
Result: Build, linking, and packaging completed successfully with the updated nxplayer sources.
2. Valid WAV playback
Test command:
Result: Audio playback was audible.
3. WAV playback with odd-sized RIFF chunks
Test command:
The WAV file contains an odd-sized
JUNKchunk.Result: Audio playback was audible.
4. Malformed WAV files and PCM fallback
Truncated WAV file
Result: No audible playback.
Incomplete EOF
Result: No audible playback.
Invalid RIFF length
Result: Audio playback was audible through the existing PCM fallback path.
Playback recovery after fallback
Result: Audio playback was audible, confirming that normal playback worked after the fallback test.
5. Raw PCM playback
Test command:
Result: Audio playback was audible. The existing raw PCM playback path remains functional.
6. Read error during WAV playback
During a playback test, the WAV file was truncated while playback was in progress.
Observed output:
After entering
q, the application returned to the NSH prompt.Result: The read error was reported and playback exited cleanly.
7. Configuration checks
The following combinations of
CONFIG_AUDIO_FORMAT_PCMandCONFIG_NXPLAYER_FMT_FROM_EXTpassed syntax checks:8. Checkpatch, Nxstyle, and whitespace checks
The changed files were checked using:
Both modified C files also passed
nxstyle.For the final commit series, the following checks were run:
Result: All checks passed. No whitespace errors were reported.
Review follow-up — 4 November 2026
The following review feedback has been addressed:
max_channelsrejection. Channel configuration is now determined by the result ofAUDIOIOC_CONFIGURE.nxplayer_wav_rate_cap()a file-local static function innxplayer_wav.cand removed its declaration frominclude/system/nxplayer.h.PCM decoder validation
The reviewer confirmed that
pcm_decodeoveraudio_nullunder QEMU successfully plays both mono and stereo WAV files with this PR.The reviewer also confirmed that separate physical-board validation is not necessary.
Note: This QEMU validation was confirmed by the reviewer; it was not independently reproduced as part of my local SIM-ALSA tests.
Assisted-by: OpenAI:GPT-6