From 8c841af9461eba6252690c38bc3e726f2fd09e8b Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Wed, 11 Dec 2019 22:04:10 -0600 Subject: MP3File: Avoid crash in get_buffer when deinitted When a playing mp3 is deinitted, it's possible to reach get_buffer, but all the internal pointers are NULL. This would lead to a hard fault. Avoid it by returning GET_BUFFER_ERROR instead. --- shared-module/audiomp3/MP3File.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'shared-module') diff --git a/shared-module/audiomp3/MP3File.c b/shared-module/audiomp3/MP3File.c index cabb46105..f0c4adc26 100644 --- a/shared-module/audiomp3/MP3File.c +++ b/shared-module/audiomp3/MP3File.c @@ -227,6 +227,9 @@ audioio_get_buffer_result_t audiomp3_mp3file_get_buffer(audiomp3_mp3file_obj_t* uint8_t channel, uint8_t** bufptr, uint32_t* buffer_length) { + if (!self->inbuf) { + return GET_BUFFER_ERROR; + } if (!single_channel) { channel = 0; } -- cgit v1.2.3 From 1cdac3f16a9244c992e8494a7246ab643cdb47c0 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Thu, 12 Dec 2019 08:44:15 -0600 Subject: MP3File: Fix stereo playback on samd AudioOut There were several problems with the way this worked -- the read_count approach was too complicated and I made a mistake "simplifying" it from WaveFile. And when the right channel was returned, it was off by 1 byte, making it into static. Instead, directly track which is the "other" channel that has data available, and by using the right data type make the "+ channel" arithmetic give the right result. This requires a double cast (int16_t*)(void*) due to an alignment warning; the alignment is now ensured manually, but the compiler doesn't make the necessary inference that the low address bit must be clear. --- shared-module/audiomp3/MP3File.c | 46 +++++++++++++++++++++++----------------- shared-module/audiomp3/MP3File.h | 6 +++--- 2 files changed, 30 insertions(+), 22 deletions(-) (limited to 'shared-module') diff --git a/shared-module/audiomp3/MP3File.c b/shared-module/audiomp3/MP3File.c index f0c4adc26..f4861da36 100644 --- a/shared-module/audiomp3/MP3File.c +++ b/shared-module/audiomp3/MP3File.c @@ -151,10 +151,13 @@ void common_hal_audiomp3_mp3file_construct(audiomp3_mp3file_obj_t* self, self->channel_count = fi.nChans; self->frame_buffer_size = fi.outputSamps*sizeof(int16_t); + if ((intptr_t)buffer & 1) { + buffer += 1; buffer_size -= 1; + } if (buffer_size >= 2 * self->frame_buffer_size) { self->len = buffer_size / 2 / self->frame_buffer_size * self->frame_buffer_size; - self->buffers[0] = buffer; - self->buffers[1] = buffer + self->len; + self->buffers[0] = (int16_t*)(void*)buffer; + self->buffers[1] = (int16_t*)(void*)buffer + self->len; } else { self->len = 2 * self->frame_buffer_size; self->buffers[0] = m_malloc(self->len, false); @@ -218,6 +221,7 @@ void audiomp3_mp3file_reset_buffer(audiomp3_mp3file_obj_t* self, f_lseek(&self->file->fp, 0); self->inbuf_offset = self->inbuf_length; self->eof = 0; + self->other_channel = -1; mp3file_update_inbuf(self); mp3file_find_sync_word(self); } @@ -234,26 +238,30 @@ audioio_get_buffer_result_t audiomp3_mp3file_get_buffer(audiomp3_mp3file_obj_t* channel = 0; } - uint16_t channel_read_count = self->channel_read_count[channel]++; - bool need_more_data = self->read_count++ == channel_read_count; - - *bufptr = self->buffers[self->buffer_index] + channel; + *bufptr = (uint8_t*)(self->buffers[self->buffer_index] + channel); *buffer_length = self->frame_buffer_size; - if (need_more_data) { - self->buffer_index = !self->buffer_index; - int16_t *buffer = (int16_t *)(void *)self->buffers[self->buffer_index]; + if (channel == self->other_channel) { + *bufptr = (uint8_t*)(self->buffers[self->other_buffer_index] + channel); + self->other_channel = -1; + return GET_BUFFER_MORE_DATA; + } - if (!mp3file_find_sync_word(self)) { - return self->eof ? GET_BUFFER_DONE : GET_BUFFER_ERROR; - } - int bytes_left = BYTES_LEFT(self); - uint8_t *inbuf = READ_PTR(self); - int err = MP3Decode(self->decoder, &inbuf, &bytes_left, buffer, 0); - CONSUME(self, BYTES_LEFT(self) - bytes_left); - if (err) { - return GET_BUFFER_DONE; - } + self->other_channel = 1-channel; + self->other_buffer_index = self->buffer_index; + + self->buffer_index = !self->buffer_index; + int16_t *buffer = (int16_t *)(void *)self->buffers[self->buffer_index]; + + if (!mp3file_find_sync_word(self)) { + return self->eof ? GET_BUFFER_DONE : GET_BUFFER_ERROR; + } + int bytes_left = BYTES_LEFT(self); + uint8_t *inbuf = READ_PTR(self); + int err = MP3Decode(self->decoder, &inbuf, &bytes_left, buffer, 0); + CONSUME(self, BYTES_LEFT(self) - bytes_left); + if (err) { + return GET_BUFFER_DONE; } return GET_BUFFER_MORE_DATA; diff --git a/shared-module/audiomp3/MP3File.h b/shared-module/audiomp3/MP3File.h index 12649ac1a..9d99e8d1f 100644 --- a/shared-module/audiomp3/MP3File.h +++ b/shared-module/audiomp3/MP3File.h @@ -39,7 +39,7 @@ typedef struct { uint8_t* inbuf; uint32_t inbuf_length; uint32_t inbuf_offset; - uint8_t* buffers[2]; + int16_t* buffers[2]; uint32_t len; uint32_t frame_buffer_size; @@ -50,8 +50,8 @@ typedef struct { uint8_t channel_count; bool eof; - uint16_t read_count; - uint16_t channel_read_count[2]; + int8_t other_channel; + int8_t other_buffer_index; } audiomp3_mp3file_obj_t; // These are not available from Python because it may be called in an interrupt. -- cgit v1.2.3 From a854a76819e4b871f9d0c4e059ca5c03f05dca76 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Fri, 13 Dec 2019 14:05:56 -0600 Subject: audiocore: The arguments to reset_buffer went missing In conversion I missed these arguments were being passed, but noticed it when an implausible value for audio_channel was sent to mp3's reset_buffer method. It's not clear whether there was any negative impact to this, but it should be fixed! --- shared-module/audiocore/__init__.c | 2 +- shared-module/audiocore/__init__.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) (limited to 'shared-module') diff --git a/shared-module/audiocore/__init__.c b/shared-module/audiocore/__init__.c index 1553dbe96..ac9d41b60 100644 --- a/shared-module/audiocore/__init__.c +++ b/shared-module/audiocore/__init__.c @@ -52,7 +52,7 @@ uint8_t audiosample_channel_count(mp_obj_t sample_obj) { void audiosample_reset_buffer(mp_obj_t sample_obj, bool single_channel, uint8_t audio_channel) { const audiosample_p_t *proto = mp_proto_get_or_throw(MP_QSTR_protocol_audiosample, sample_obj); - proto->reset_buffer(MP_OBJ_TO_PTR(sample_obj)); + proto->reset_buffer(MP_OBJ_TO_PTR(sample_obj), single_channel, audio_channel); } audioio_get_buffer_result_t audiosample_get_buffer(mp_obj_t sample_obj, diff --git a/shared-module/audiocore/__init__.h b/shared-module/audiocore/__init__.h index 38c1f5aeb..7a56454b8 100644 --- a/shared-module/audiocore/__init__.h +++ b/shared-module/audiocore/__init__.h @@ -42,7 +42,8 @@ typedef enum { typedef uint32_t (*audiosample_sample_rate_fun)(mp_obj_t); typedef uint8_t (*audiosample_bits_per_sample_fun)(mp_obj_t); typedef uint8_t (*audiosample_channel_count_fun)(mp_obj_t); -typedef void (*audiosample_reset_buffer_fun)(mp_obj_t); +typedef void (*audiosample_reset_buffer_fun)(mp_obj_t, + bool single_channel, uint8_t audio_channel); typedef audioio_get_buffer_result_t (*audiosample_get_buffer_fun)(mp_obj_t, bool single_channel, uint8_t channel, uint8_t** buffer, uint32_t* buffer_length); -- cgit v1.2.3 From bf9b7e7ece6186b2fb7716323fba8ef24315cb47 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Fri, 13 Dec 2019 14:07:00 -0600 Subject: MP3: skip ID3V2 metadata This was one reason that the "bartlebeats" mp3s would not play. --- shared-module/audiomp3/MP3File.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) (limited to 'shared-module') diff --git a/shared-module/audiomp3/MP3File.c b/shared-module/audiomp3/MP3File.c index f4861da36..789d75e0f 100644 --- a/shared-module/audiomp3/MP3File.c +++ b/shared-module/audiomp3/MP3File.c @@ -88,6 +88,39 @@ STATIC bool mp3file_update_inbuf(audiomp3_mp3file_obj_t* self) { #define BYTES_LEFT(self) (self->inbuf_length - self->inbuf_offset) #define CONSUME(self, n) (self->inbuf_offset += n) +// http://id3.org/d3v2.3.0 +// http://id3.org/id3v2.3.0 +STATIC void mp3file_skip_id3v2(audiomp3_mp3file_obj_t* self) { + mp3file_update_inbuf(self); + if (BYTES_LEFT(self) < 10) { + return; + } + uint8_t *data = READ_PTR(self); + if (!( + data[0] == 'I' && + data[1] == 'D' && + data[2] == '3' && + data[3] != 0xff && + data[4] != 0xff && + (data[5] & 0x1f) == 0 && + (data[6] & 0x80) == 0 && + (data[7] & 0x80) == 0 && + (data[8] & 0x80) == 0 && + (data[9] & 0x80) == 0)) { + return; + } + uint32_t size = (data[6] << 21) | (data[7] << 14) | (data[8] << 7) | (data[9]); + size += 10; // size excludes the "header" (but not the "extended header") + // First, deduct from size whatever is left in buffer + uint32_t to_consume = MIN(size, BYTES_LEFT(self)); + CONSUME(self, to_consume); + size -= to_consume; + + // Next, seek in the file after the header + f_lseek(&self->file->fp, f_tell(&self->file->fp) + size); + return; +} + /* If a sync word can be found, advance to it and return true. Otherwise, * return false. */ @@ -223,6 +256,7 @@ void audiomp3_mp3file_reset_buffer(audiomp3_mp3file_obj_t* self, self->eof = 0; self->other_channel = -1; mp3file_update_inbuf(self); + mp3file_skip_id3v2(self); mp3file_find_sync_word(self); } @@ -253,6 +287,7 @@ audioio_get_buffer_result_t audiomp3_mp3file_get_buffer(audiomp3_mp3file_obj_t* self->buffer_index = !self->buffer_index; int16_t *buffer = (int16_t *)(void *)self->buffers[self->buffer_index]; + mp3file_skip_id3v2(self); if (!mp3file_find_sync_word(self)) { return self->eof ? GET_BUFFER_DONE : GET_BUFFER_ERROR; } -- cgit v1.2.3 From 91a1706160aa653cb00c7910b0930df679bcdd21 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Fri, 13 Dec 2019 15:20:48 -0600 Subject: MP3: look harder for frame info Apparently sometimes, a proper "frame info" block is not found after a "sync word". Keep looking for one as needed, instead of giving up after one try. This was one reason that the "bartlebeats" mp3s would not play. --- shared-module/audiomp3/MP3File.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'shared-module') diff --git a/shared-module/audiomp3/MP3File.c b/shared-module/audiomp3/MP3File.c index 789d75e0f..0aa4f24a6 100644 --- a/shared-module/audiomp3/MP3File.c +++ b/shared-module/audiomp3/MP3File.c @@ -139,7 +139,15 @@ STATIC bool mp3file_find_sync_word(audiomp3_mp3file_obj_t* self) { } STATIC bool mp3file_get_next_frame_info(audiomp3_mp3file_obj_t* self, MP3FrameInfo* fi) { - int err = MP3GetNextFrameInfo(self->decoder, fi, READ_PTR(self)); + int err; + do { + err = MP3GetNextFrameInfo(self->decoder, fi, READ_PTR(self)); + if (err == ERR_MP3_NONE) { + break; + } + CONSUME(self, 1); + mp3file_find_sync_word(self); + } while (!self->eof); return err == ERR_MP3_NONE; } -- cgit v1.2.3