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

Add seeking to rawread and macca, and tests #73

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

Conversation

pconerly
Copy link

@pconerly pconerly commented Aug 23, 2018

@sampsyo

WIP towards #71 and #72 . On the path of adding seeking, I did a variety of things:

  • I added block_samples into audio_open. It works in macca and rawread and I'm sure it's broken (via Unexpected keyword arg) in the other backends.
  • Added unit tests for rawread and macca
  • Added audio file fixtures for those tests
  • Added a tox config to run in py27 and py36
  • Added seeking to rawread and macca

Moving block_samples to the init for the other backends, and adding seeking to them is still needed. But let me know how this looks!

Todos:

  • 1st round Code review fixes
  • ffmpeg
  • mad
  • gstreamer

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Looks great!! I'm excited to have some real unit tests here too. I've left a few comments inline.

@@ -323,6 +328,11 @@ def read_data(self, blocksize=4096):
blob = data[:size]
yield blob

def seek(self, pos):
"""Seeks to the position in the file"""
Copy link
Member

Choose a reason for hiding this comment

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

Style nitpicking: it's nice to write docstrings (a) in the imperative voice, and (b) as complete sentences, including a period. So I suggest:

Seek to a position in the file.

audioread/rawread.py Outdated Show resolved Hide resolved
audioread/rawread.py Outdated Show resolved Hide resolved
test/fixtures/make_test_wave.py Outdated Show resolved Hide resolved

#### wavetest.wav

Produced with `make_test_wave.py`
Copy link
Member

Choose a reason for hiding this comment

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

It's a little confusing that there are files called test.wav and wavtest.wav. Maybe it would be useful to describe what each one is for, and how they're different?

Copy link
Author

Choose a reason for hiding this comment

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

I'm using test.wav for the macca backend--- I'll find some better names for it. We also might not need both

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.

2 participants