Incorrect Implementation in WAV memory read

[kennethtang4]

The issue was when I tried to read the entire wav file with the following code, the program will crash when it is reaching the end:

q::wav_memory wav{ "test\wav" };
for (int i = 0; i < wav.length(); i++)
    auto temp = wav();

After a dig through the code, I found the issue was caused by an incorrect implementation in "q/q_io/include/q_io/audio_file.hpp", in the function "inline iterator_range<float const*> const wav_memory::operator()()".

The implementation was adding the number of channels that the audio WAV has for each call, which is correct but did not consider the case that the offset is incorrect when a new buffer is read. In that case, the pointer should equal buf.begin() rather than buf.begin() + num_channels().

So the resolve the issue, simply change the following code:

if ((_pos + num_channels()) >= _buff.end())
{
    auto read_len = read(_buff.data(), _buff.size());
     if (read_len == 0)
     {
        _buff.resize(num_channels());
        std::fill(_buff.begin(), _buff.end(), 0.0f);
     }
     else if (read_len != _buff.size())
     {
         std::fill(_buff.begin()+read_len, _buff.end(), 0.0f);
     }
     _pos = _buff.begin();
}
float const* p = &*_pos;
iterator_range<float const*> r{ p, p + num_channels() };
_pos +=  num_channels();
return r;

to the following code:

if ((_pos + num_channels()) >= _buff.end())
{
    auto read_len = read(_buff.data(), _buff.size());
     if (read_len == 0)
     {
        _buff.resize(num_channels());
        std::fill(_buff.begin(), _buff.end(), 0.0f);
     }
     else if (read_len != _buff.size())
     {
         std::fill(_buff.begin()+read_len, _buff.end(), 0.0f);
     }
     _pos = _buff.begin();
}
else
{
    _pos +=  num_channels();
}
float const* p = &*_pos;
iterator_range<float const*> r{ p, p + num_channels() };
return r;

[djowel]

Can you please make a PR for this? A PR would be great, so the commit will have you in the history.

[kennethtang4]

There is still one more flaw that I have found out when I was working with some WAV files recently. By comparing the data that is loaded by librosa (Python), I noticed there is one index shifted. Or precisely, num_channels() as I was working with mono track WAV file.

The problem was aroused due to the fact that for the initial read, the function:

inline wav_memory::wav_memory(char const* filename, std::size_t buff_size)
 : wav_reader(filename)
 , _buff(*this? buff_size * num_channels() : num_channels())
{
      if (*this)
         read(_buff.data(), _buff.size());
      else
         std::fill(_buff.begin(), _buff.end(), 0.0f);
      _pos = _buff.begin();
}

has set the variable _pos to _buff.begin() already. This causes when the code executed to the iterator_range function, it will add num_channels() instead of reading the file as the file has been read previously already. This problem can be solved by a numerous way but I am suggesting the following.

Add a bool protected variable in the declaration:

class wav_base
   {
   public:

      wav_base();
      wav_base(wav_base const&) = delete;
      ~wav_base();

      wav_base&      operator=(wav_base const&) = delete;
      explicit       operator bool() const;
      std::size_t    sps() const;
      std::size_t    num_channels() const;

   protected:

      wav_impl*      _wav;
      bool           first_read; // <<<<<<<<< Added Here
   };

Do not attempt to read the file during the initialization:

   inline wav_memory::wav_memory(char const* filename, std::size_t buff_size)
    : wav_reader(filename)
    , _buff(*this? buff_size * num_channels() : num_channels())
   {
      if (!(*this))
         std::fill(_buff.begin(), _buff.end(), 0.0f);
      first_read = false;
   }

And check if the file has been read before:

         if (!first_read || (_pos + num_channels()) >= _buff.end())
         {
            if (!first_read) first_read = true;
            auto read_len = read(_buff.data(), _buff.size());
            if (read_len == 0)
            {
               _buff.resize(num_channels());
               std::fill(_buff.begin(), _buff.end(), 0.0f);
            }
            else if (read_len != _buff.size())
            {
               std::fill(_buff.begin()+read_len, _buff.end(), 0.0f);
            }
            _pos = _buff.begin();
         }
         else
         {
            _pos += num_channels();
         }

This will then solve the issue. Please do tell me if you want to have a pull request on this as well.

[djowel]

This will then solve the issue. Please do tell me if you want to have a pull request on this as well.

Yes, please! That will be great!

[kennethtang4]

Also, there is another potential problem I have encountered when I was using libcurl with this library. The part:

using namespace std;

was causing the problem as it will cause ambiguity for byte (this is actually a known problem with C++17). Not sure if you want to remove that as well but it is not critical for most use case.

[djowel]

Also, there is another potential problem I have encountered when I was using libcurl with this library. The part:

using namespace std;

Could you tell me where you see that using namespace? That should not exist in the library.