Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for Tranfer-Encoding: chunked streams #394

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yoav-klein
Copy link

Howdy Earl !

I first started using your great library just a while ago. I wanted to have a ESP8266 as a Radio player in my home since my radio reception in my home is not that good. So I did all the setup, but then I always got the lost synchronization.. error.

First googling, all I could find is "Try encrementing your buffer size", with no help.

After some research, I realized that the stream of my favorite radio station is using a Transfer-Encoding: chunked, and that what caused the loss of synchronization.

So I pulled up my sleeves and added support in the AudioFileSourceHTTPStream class to chunked Transfer-Encoding.

This could be also implemented in the ICY class as well, but for me it wasn't all that important and all the ICY thing was quite complex, so I didn't get to it.

Hope it will solve some other folks' same problem as well.

This is the radio stream I'm using if you want to check it out.

http://kanliveicy.media.kan.org.il/icy/749624_mp3

@yoav-klein
Copy link
Author

Hi @earlephilhower , did have the chance having a look at this?

Copy link
Owner

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Sorry for the delay, I've been quite busy this past week. A few changes would make it a very good addition, I think!

src/AudioFileSourceHTTPStream.cpp Outdated Show resolved Hide resolved
src/AudioFileSourceHTTPStream.cpp Outdated Show resolved Hide resolved
src/AudioFileSourceHTTPStream.cpp Outdated Show resolved Hide resolved
src/AudioFileSourceHTTPStream.cpp Outdated Show resolved Hide resolved
src/AudioFileSourceHTTPStream.cpp Outdated Show resolved Hide resolved
@@ -52,14 +56,42 @@ class AudioFileSourceHTTPStream : public AudioFileSource
enum { STATUS_HTTPFAIL=2, STATUS_DISCONNECTED, STATUS_RECONNECTING, STATUS_RECONNECTED, STATUS_NODATA };

private:
virtual uint32_t readInternal(void *data, uint32_t len, bool nonBlock);
bool is_chunked;
std::size_t next_chunk;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure of the reason for std::size_t vs. the well-known stdint.h size_t?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really matter?

src/AudioFileSourceHTTPStream.h Show resolved Hide resolved
WiFiClient client;
HTTPClient http;
int pos;
int size;
int reconnectTries;
int reconnectDelayMs;
char saveURL[128];
uint32_t (AudioFileSourceHTTPStream::*readImpl)(void *data, uint32_t len, bool nonBlock);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the AudioFileSourceHTTPStream:: is superfluous here. We're already in class AudioFileSourceHTTPStream...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I've checked, this is how it can be used. You're welcomed to further check it yourself.

src/AudioFileSourceHTTPStream.h Outdated Show resolved Hide resolved
@yoav-klein
Copy link
Author

Hi @earlephilhower, fixed all needed to be fixed, please review

@DeqingSun
Copy link

DeqingSun commented Feb 3, 2022

@yoav-klein Thanks for providing the improvement. Just report 2 bugs I found:

  1. The improvement will crash AudioFileSourceICYStream class, because the readImpl keeps uninitialized. I did some quick test to copy the open code to the AudioFileSourceICYStream. But the readInternal seems to be much more complicated and thrown a lot Couldn't read CRLF after chunk, something is wrong error. I did not fully investigate the issue. But it seems doing the icy in readInternal is quite easy to get conflict with the chunk. Maybe doing the icy in read and readNonBlock?

  2. In readChunked function, if the len parameter is big, it becomes a long blocking function. In my test the len was 16K and the WDT reset was triggered, after the function run for a few seconds. A quick and dirty way is to add if (len > next_chunk) len = next_chunk;

after some tweak, here is my improvement. No loop in the function.

uint32_t AudioFileSourceHTTPStream::readChunked(void *data, uint32_t len, bool nonBlock)
{
  uint32_t bytesRead = 0;
  uint32_t pos = 0;
  
  if(len > 0)
  {
    if(len >= next_chunk)
    {
      if (next_chunk)
      {
        bytesRead = readInternal(data + pos, next_chunk, nonBlock);
        next_chunk -= bytesRead;
        pos += bytesRead;
      }
      len -= pos;
      if (!next_chunk){
        if(!verifyCrlf())
        {
          audioLogger->printf(PSTR("Couldn't read CRLF after chunk, something is wrong !!\n"));
          return 0;
        }
        next_chunk = getChunkSize();
      }
    }
    else
    {
      bytesRead = readInternal(data + pos, len, nonBlock);
      next_chunk -= bytesRead;
      len -= bytesRead;
      pos += bytesRead;
    }
  }
  return pos;
}

@DeqingSun
Copy link

And here are my functional AudioFileSourceICYStream, it can deal with both chunked or regular stream.
AudioFileSource_ICY_HTTPS.zip

@sh-user
Copy link

sh-user commented Feb 3, 2022

@DeqingSun i tested your code on ESP8266, it works but cuts off the end of the word
https://translate.google.com/translate_tts?ie=UTF-8&q=Text%20to%20Speech&tl=en&client=tw-ob

@yoav-klein
Copy link
Author

yoav-klein commented Feb 4, 2022

HI @DeqingSun , thanks for the improvement!

I actually also experiencing some hiccups and also restarts every now and then. When I say now and then I mean it could go smooth for hours, but there are times that it's more frequent. I didn't get the chance to look deeply into why exactly this happens, but maybe it has something to do with what you're pointing at.

Although I find it hard to believe. It's not fresh in my memory, but as far as I remember, the reads in the original code are only one MP3 frame at a time, so how did you get to a point where the readChunked function received a len of 16K?

And I'll be pleased to know that - how do you know what function causes a reset? Would love if you could share with me some debugging skills so I could understand why I get all these hiccups and restarts.

@DeqingSun
Copy link

HI @yoav-klein,

For exceptions reset, ESP will print a Backtrace in 115200. And such info can be decoded by EspExceptionDecoder. However, it can not give useful info when you forget you return a value in function, WDT reset etc.

What I did was adding tons of print in every suspicious function, on the entry and exit part, and any point I feel suspicious. Also the print can carry info of the parameters and variables. With a lot of prints, you can see how the code flows, and see where it stuck.

If your code runs for hours before a bug, it might be helpful to get a serial logger like this one. I always want to get one but never get a chance to debug such a bug.

For the 16K problem, I think the read function in AudioFileSourceBuffer will try to fill the buffer in the first read. My code may break the read casually and return a significantly shorter response and may cause a longer delay at the beginning.

@DeqingSun
Copy link

@sh-user The key to solve the problem is to load the data as much as possible at the beginning and stop when the data stream stops. So some change to the read, the getChunkSize, needs to be done.

Try this version and use the AudioFileSourceICYStream class
AudioFileSource_ICY_HTTPS_20220205.zip
.

@sh-user
Copy link

sh-user commented Feb 6, 2022

checked, there is no longer a big delay after playback, but the last word is cut off. I noticed that if you send the text longer, then everything works fine
mp3

@DeqingSun
Copy link

@sh-user I tested on ESP32 with 16K buffer. That might be a reason.

schmurtzm added a commit to schmurtzm/ESP8266Audio that referenced this pull request Feb 21, 2022
Included changes from yoav-klein and DeqingSun to manage correctly Google TTS.
See here for details :
earlephilhower#394
schmurtzm added a commit to schmurtzm/MrDiy-Audio-Notifier that referenced this pull request Feb 22, 2022
Resolve Google TTS problems thanks to adding support for "Tranfer-Encoding: chunked streams"

Included changes from yoav-klein and DeqingSun to manage correctly Google TTS.
See here for details :
earlephilhower/ESP8266Audio#394
@schmurtzm
Copy link

@sh-user I tested on ESP32 with 16K buffer. That might be a reason.

I confirm this behavior with Google TTS :

  • Tested on ESP32 : it seems that the ESP32 plays the stream a little too long with a repeat of the last syllab during 1 second.
  • Tested on ESP8266 : it seems that the ESP8266 cut the stream about one seconde before the end.

Still a lot better than before (with the current dev version it hangs during few minutes!)
It helps a lot for the resolution of my issue ! #475 (comment)
So thanks you for the modifications !

@DeqingSun
Copy link

@yoav-klein Another thing worth improving: The getChunkSize() using readStringUntil which is blocking. In the following case it may corrupt a lot of data.

  1. The buffer is so large and it fetches all data from the wifi client
  2. The network is slow
  3. The Audio is decoding

When they all happens, the getChunkSize() may takes a long time to respond, and it will create a lot of hiccup and eventually kill the stream. I'll see if I can improve it.

@rebane2001
Copy link

Thank you @DeqingSun, your version seems to runs really well. I modified it to support https (through WiFiClientSecure) for a personal project and it still runs great on my ESP32!

@yoav-klein
Copy link
Author

Hi @rebane2001 , Will your support for HTTPS will also work on ESP8266?

@rebane2001
Copy link

I'm not sure as I don't have an ESP8266 to test on, but you can see what I did in #628 if you want to try it yourself.

@yoav-klein
Copy link
Author

@rebane2001 , can I pull your code from somewhere? Additionally, on what version of @DeqingSun were you talking about? Can you share the unified versions?

@rebane2001
Copy link

I used the version initially posted (AudioFileSource_ICY_HTTPS.zip), I didn't notice there was another one there. Here's my code at the moment:
with-https.zip

Fyi I've only tested/used this for http, not icy.

WuSiYu added a commit to WuSiYu/ESP8266Audio that referenced this pull request Jan 20, 2024
minor: add CMakeLists.txt for usage in ESP-idf; code changes to pass the compile
most changes from earlephilhower#394 (comment) by @DeqingSun
@sintrb
Copy link

sintrb commented May 27, 2024

I used the version initially posted (AudioFileSource_ICY_HTTPS.zip), I didn't notice there was another one there. Here's my code at the moment: with-https.zip

Fyi I've only tested/used this for http, not icy.

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants