Audio and playlist components

chmorgan commented 2 years ago

Refactor audio player and playlist out of mp3_demo and make use of both in mp3_demo and factory examples.

@TDA-2030 I'd like to get the audio_player up on the components catalog at some point as I could make use of this on another project. Its not ready quite yet and lacks WAV file support (which I can add soon) but wanted to get your initial review and feedback to start moving in that direction.

chmorgan commented 2 years ago

I've got some fixes and cleanups that I missed and I'm adding wav support now. I'll msg here when its pushed and ready for review.

chmorgan commented 2 years ago

@TDA-2030 any reason that esp-adf wasn't used initially? I was unaware that esp-adf was so developed when I started on this a few weeks ago. Looking at it looks like the player and playlist stuff could be swapped out with esp-adf. One downside is that esp-adf doesn't provide codec source code, which is odd no?. It was slightly disappointing that I wasn't aware of esp-adf until now, I should have done my research, and perhaps a little work was wasted although it was fun none the less.

Otherwise I was tempted to complete what I've got, which is mp3 + wav file support, some fixes to play the end of the file (today we actually trim the end of the file off), and slightly better buffer support for mp3 playback so we aren't fread() + memmove() on each loop.

Thoughts? Should I switch to integrate esp-adf instead? I was hoping to get your feedback.

TDA-2030 commented 2 years ago

esp-box is not primarily used for audio applications, and esp-adf is not a lightweight library, so it is not integrated into the esp-box. If you use a lot of audio-related functionality, integrating esp-adf into your project will really help you get things done faster and better.

chmorgan commented 2 years ago

@TDA-2030 makes sense, appreciate the info. I'm getting close to having the audio_component ready. It's simple and fully open source, so it should have some utility for some people that are looking to avoid perhaps the overhead of esp-adf and I'm going to at least get the pr ready here before digging into trying out esp-adf.

chmorgan commented 2 years ago

@TDA-2030 alright, here is the next rev and I think ready for review. For my other use case I'm going to integrate another audio codec, a TI one, and that may drive some improvements to the audio component, making it more generic or whatever. But this is I think a good point to review.

TDA-2030 commented 2 years ago

It seems to be a great improvement. Thank you for your contribution!

TDA-2030 commented 2 years ago

For my other use case I'm going to integrate another audio codec, a TI one

Can you share some information about the audio codec you mentioned? Is it open source and free?

chmorgan commented 2 years ago

For my other use case I'm going to integrate another audio codec, a TI one

Can you share some information about the audio codec you mentioned? Is it open source and free?

Oh I mean hardware codec. It's a TI codec but I don't recall the part number at the moment. We tried to source the evergreen es8311 but had trouble finding a US source for that component. If you've got one, or know of a source for that component that can converse in English I think we'd consider using the 8311.

There isn't any way this audio component can and should aim to really overlap with the esp-adf, that codebase is massively more developed. I guess I could see someone interested in ogg decoding etc but yeah if you are going with BT streaming etc you might as well use esp-adf :-)

chmorgan commented 2 years ago

@TDA-2030 I should probably list out some of the improvements in the audio side.

mp3 playback finishes the song now, previous implementation I think would stop and not play the last few frames when EOF was hit.
mp3 playback reads in chunks, filling the buffer and then decoding from the buffer. Results in overall fewer filesystem reads although from a predictability standpoint it does mean cpu usage is slightly bursty due to the period reads to fill the buffer. It seemed worth the tradeoff vs. reading each loop.
wave decoding, I needed this for some audio files from google's test to speech system for audio prompts
maintains stereo / mono conversion, I think this should be an input parameter at some point as to whether you need the output in mono format
still trying to address a slight tick when audio is changing songs. Attempt was through the mute function so you can turn off the codec when no audio is playing.
Cleaned up events to better indicate state transitions, in particular playing -> playing without stopping. There is an ascii diagram at the top of audio_player.h to help the reader.
Tried to clean up the language around 'samples' vs. 'frames', I think its consistent and mostly correct at this point.
Refactored the playlist management out. Then noticed esp-adf has basically identical code, I guess great minds think alike? :)
Added layers of logging, this approach was borrowed maybe from the BT component? Where each log level produces more verbose output for debugging, configurable via kconfig.
Moved allocation to audio_player_init(), if you don't call this a number of buffers aren't allocated. We could statically allocate some of these, or ask the user to pass them in if the user would like to decide where/how to allocate this memory. Thoughts? There is presently no deinit function and no mechanism to have more than one audio component instance although this would be possible if we wrapped things up into an instance struct that the caller had to pass in for each call. Thoughts?

I think this pretty much summarizes it. Sorry for the delay. I also appreciate it overlaps with some esp-adf stuff but like your earlier comment the project I'm working on has no need for the full esp-adf framework and its hard to know how the esp-adf internals work or how much overhead there is.

One thing could be to look at the api there more closely to see if this code can be adjusted to better match it, or if its just silly duplicating this functionality here and if esp-adf can be used in a lightweight manner. It was reasonably fun to write this stuff and its scope is limited. Anyway, if we don't need two of the same thing then why have it? But it sounds like this is a lot more compact so....

Looking forward to your other thoughts and what you'd like to see to get it ready for merge.

chmorgan commented 2 years ago

Hi @TDA-2030 any chance to take a look at this PR? I was hoping to get it moving along the review and merging process.

chmorgan commented 2 years ago

Ping on review and feedback.

ESP-Mars commented 2 years ago

Hi @TDA-2030 any chance to take a look at this PR? I was hoping to get it moving along the review and merging process.

Ping on review and feedback.

Sorry, TDA-2030 has left Espressif, will arrange another guy to tackle the problems.

chmorgan commented 2 years ago

@ESP-Mars I'd be willing to be added as a maintainer to this repo. We do need another reviewer for this PR in any case. I have at least one other cleanup PR I'll be pushing up in the near future as well.

chmorgan commented 2 years ago

@tore-espressif updated PR coming in the next few days. Lots of cleanups and improvements, working on getting tests now although I ended up symlinking into the esp-idf/ install folder, couldn't figure out a better way to get unit-test-app to work correctly.

For this to be a component should I push it to a repo on my account or is that something we discuss later?

tore-espressif commented 2 years ago

updated PR coming in the next few day

Looking forward to it!

couldn't figure out a better way to get unit-test-app to work correctly.

Yeah, we are just finishing improvements that would make this easier for external users

For this to be a component should I push it to a repo on my account

Since you are the author and maintainer, I'd suggest keeping this under your namespace (and license) on GitHub. Then we can grant you write access to our components registry that would allow you to push (and then reuse) these components in any esp-idf projects. Does that sound reasonable?

chmorgan commented 2 years ago

@tore-espressif alright, pushed an updated branch with a ton of changes, plus unity tests, and a README.md. Next step after your next round of review is to move this out into its own repo and pull it in as a component, but thought we should continue review here until its ready. If you could take another look I'd appreciate it!

chmorgan commented 2 years ago

@tore-espressif very good review feedback, I appreciate it a ton. I should have an updated PR coming soon but also plan to split things into separate repos at this point, and also noticed libhelix-mp3 has a ton of duplicated files and needs to be its own component repo with a submodule, which I'll put in place as a part of the next round as well.

espressif / esp-box

Audio and playlist components #30