pschatzmann / arduino-audio-tools

Arduino Audio Tools (a powerful Audio library not only for Arduino)
GNU General Public License v3.0
1.43k stars 222 forks source link

Digital "farts" when transitionning between streams and when using player.stop() #581

Closed 0x0fe closed 1 year ago

0x0fe commented 1 year ago

So, the hardware setup is ESP32-S3FH4R2 which feeds a DAC (ES7148) which itself feeds a PA (LM4863).

I first noticed the issue when using the MP3 test files i made to debug the application logic, these are very short MP3 enumerating the directory and file name, they were initially at 48kbps but i had to change them to 192kbps to work with the library. These files do not exhibit any anomaly when played in VLC, but when played by the library a small digital "fart" can be heard at the transition between two files, when i check the log in verbose mode it seems to occur when the last "incomplete" (less than 1024 byte) chunk is read. I tested other files with the same result, so i am not sure it has anything to do with these specific files.

Later when implementing play/pause function with player.stop() i noticed exactly the same problem, whenever player.stop() is called a digital "fart" can be heard after the file stops, with varying amplitude.

I am not sure what causes this, but in the search for a solution i tried few things, the only one which seems to work was the following :

int pause_flag=0;

void play_pause(void){

  int l;
  pause_flag=1-pause_flag;
  if(pause_flag==0){  
    l=0;
    while(l<vol){l+=1; player.setVolume(l/100.0); }  
    cnt=0;
    player.play();
  }
  else{   
    l=vol;
    while(l>0){ l-=1; player.setVolume(l/100.0); }
    while(l<10){ l++; player.copy(); }
    player.stop();
  }
}

Essentially a short fadeout followed by running 10 times the player.copy before finally calling player.stop(). this function is what i call in the tact switch handler which is served by the main loop, it checks the keys flags and implements debouncing.

However, there is a dramatic side effect to this "fix", sometimes, with no reason that i can find, it will totally go wild and blast full distortion and remain in that state until next reset. The play pause still works, but instead of playing normally it will play an extremely overdriven audio (ear rape, i guess), to the point we cannot really even discern the original audio, i am not sure what can cause this yet. There is nothing special in the log.

I used the following file set for tests. There is one long file in MK0015 which is more convenient to test play/pause

MKI01.zip

pschatzmann commented 1 year ago

There are potentially 2 ways to address this:

I think this functionality should be integrated into the AudioPlayer.

I guess this is getting more complicated than I initially thought....

0x0fe commented 1 year ago

i see, so the root cause is stopping elsewhere than a zero cross, clear. I will see if i can find a fix for that, As a side note i want to precise it is not a single pop, rather a serie of pops with decreasing amplitude. i will try to record the thing at some point.

pschatzmann commented 1 year ago

What happens if you call i2s.writeSilence(1024) right after player.stop ? You can also try player.setSilenceOnInactive(true)

0x0fe commented 1 year ago

i just tested, player.setSilenceOnInactive(true) seems to improve a little bit, but there is still some residual farting going on. i2s.writeSilence(1024) after player.stop() does not have much effect, it seems be be about same as without, randomly some stop() will be clean but most have noise with varying amplitude.

0x0fe commented 1 year ago

interrestingly, if i add both player.setSilenceOnInactive(true) and i2s.writeSilence(1024) after player.stop() it seems to work, i dont get noise when using player.stop() anymore.

There is still the noise at the transition between file, it is in perfect sync with the last data chunk.

image

0x0fe commented 1 year ago

for the sake of it i verified the file tail, there is indeed no issue, amplitude is 0 or very near (this is normalized, the original file isnt)

image

0x0fe commented 1 year ago

So, on this issue of noise at the end of playback during transition, the exact place where the problem occurs in the code is still unclear but i am not convainced it is a zero cross issue since amplitude in the file tails is null.

I have been able to confirm it has something to do with the last chunk of data (chunk < buffer size) because i have implemented the mute line control directly inside audiocopy.h and it could "fix" the problem, unfortunately it also cuts the file short, so, not much of a fix.

Line 110 and 146

image

So this seems to prove the noise occurs somewhere after the last chunk is copied, but before the timeout because i tried to put the mute control inside the timeout function in audioplayer.h and the noise was back.

I dont really understand why the audio is cut too short because it seems I mute after the last chunk has been copied to I2S, but it can clearly be heard, especailly on the files where the last chunk is larger.

pschatzmann commented 1 year ago

I added plenty of new functionality in the Fade.h and tried to integrate it into the player. I am not sure however if it is better now...

Doing some tests, I came to the conclusion that with the current ESP32 version you will hear some noise when you stop to send data to I2S. This can be addressed by using setSilenceOnInactive(true);

0x0fe commented 1 year ago

okay, i will have a try too, as told earlier, what worked for me when using player.stop is to set silenceoninactive true and feed the i2s with a chuck of empty datas, just after player.stop, both at same time, if only one there was still some noise.

However that did not solved the issue of transition between files. I could only "solve" it for now by essentially cutting the output with mute on the last chunk. Other than cutting some audio it wont work for URL streams and possibly others since partial buffers do occur during the transfer, contrary to SD_MMC where it only occurs on the last chunk.

pschatzmann commented 1 year ago

I think that is a "feature" of the decoder. I added some logic to close and reopen the decoder and I think it improved a bit but did not resolve the issue entirely.

0x0fe commented 1 year ago

oh, nice, i will try that soon, i was busy with solving the index and file count issue (it doesnt play the last song of directories). I followed your advise and started from player callback i2s example, please note this example is not functional, when i implemented the two callback you shown in the example i couldnt get anything to play, apparently a third callback is required source.setCallbackSelectStream(selectCb); and i implemented its logic identical to nextCb but with the index as argument. Now it works but i have still to test the functions such as next() and previous() to see if they still work. But at least the sorting of index is now working and it reads all the files of all directories.

here is the (not final) implementation, which works but will be cleaned. I will probably implement some caching so that the directory is only re-indexed if it doesnt changes. Anyway because it is based on std::sort which is a lot more efficient than qsort due to inlining and defined type (i hate to admit, not a big fan of cpp).

char filePath[128];
char prefix_[2];
char suffix_[10];
int filesCount=0,fileIndex=0;

strVec index_dir(const char* path){

  strVec filenames;
  File root=SD_MMC.open(path);
  File file=root.openNextFile();
  while(file){
    char buf[48];
    strcpy(buf,file.name());
    filenames.emplace_back(buf); 
    file=root.openNextFile();
  }
  root.close();
  file.close();  
  filesCount=filenames.size();  
  return filenames;  
}
string get_fname(const char* prefix,const char* suffix,int index){

  string str;  
  str.append("/");
  str.append(prefix);  
  str.append(suffix);
  strVec filenames=index_dir(str.c_str());
  //for(string& name:filenames) printf("%s\n",name.c_str());
  std::sort(filenames.begin(), filenames.end(), [](string a, string b) {
    return strcmp(a.c_str(), b.c_str()) < 0;
  });
  //for(string& name:filenames) printf("%s\n",name.c_str()); 
  str.clear();
  str.append("/");
  str.append(prefix);
  str.append(suffix);
  str.append("/");
  str.append(filenames.at(index));
  strcpy(filePath,str.c_str());
  return str;
}
void set_index(int index){

  fileIndex=index;
}
void set_path(char* prefix,char* suffix){

  strcpy(prefix_,prefix);
  strcpy(suffix_,suffix);
}
string get_path(void){

  string s(get_fname(prefix_,suffix_,fileIndex));
  printf("%s\n",s.c_str());
  return s;
}
void initCb() {

  printf("init Cb\n");
  if(!sd_ready) init_sd();
  get_path();
  audioFile=SD_MMC.open(filePath);
}
Stream* selectCb(int id) {

  printf("select Cb\n");  
  if(fileIndex>=filesCount) return nullptr;
  set_index(id);
  get_path();
  audioFile = SD_MMC.open(filePath);
  lastFile.close();
  if(fileIndex<filesCount) fileIndex+=1;
  else fileIndex=0;
  return &audioFile;
}
Stream* nextCb() {

  printf("next Cb\n");
  if(fileIndex>=filesCount) return nullptr;
  get_path();
  audioFile = SD_MMC.open(filePath);
  lastFile.close();
  if(fileIndex<filesCount) fileIndex+=1;
  else fileIndex=0;
  return &audioFile;
}

that is how it is called in the key handler for example:

    set_path("K","0012");
    set_index(0);
    player.begin(); 
0x0fe commented 1 year ago

one problem i am facing with callback implementation, player.next() seems to work, it calls the "next" callback, but player.previous does not work, it seems to call the "next" callback, increasing the index, is there another callback i should assign for player.previous?

0x0fe commented 1 year ago

image

so i guess the problem is here, nextStream callback should have an "offset" argument, but for some reason setCallbackNextStream will only take a callback without argument. Thus it is impossible to know if it is a call to decrease or increase the stream from inside the callback function. I think there should be either an int offset argument on the nextStream callback, or a function to assign a prevStream callback. Besides, i dont really understand why the index tracking is not done in the class. If we could just set the maxIndex from outside then all the index tracking and bounding could be done internally, currently it seems it can only be done outside.

pschatzmann commented 1 year ago

Yes, there were some changes in the API of the AudioPlayer which might not be reflected in the callback API. It seems that some more cleanup is needed. In this case I suggest to create your own subclass and you get all you need

0x0fe commented 1 year ago

ok i see, i'll add a prevStream callback for now and make a whole new audiosource class in a second time.

pschatzmann commented 1 year ago

I just commited a correction which is extending the nextStreamCallback to provide the offset. A negative value is used for moving backwards...

pschatzmann commented 1 year ago

I simplified the code around the fading logic a bit and I think it is much better now...