-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix sampling rate for reading opus files #158
Conversation
Reviewer's Guide by SourceryThis PR fixes sampling rate handling for non-SND files by introducing sampling rate parameters to audio conversion functions. The implementation extracts the sampling rate using mediainfo before conversion and passes it to both sox and ffmpeg converters. The changes ensure correct sampling rate preservation during audio file conversion. Sequence diagram for audio file conversion with sampling ratesequenceDiagram
participant User
participant AudioFile
participant MediaInfo
participant Sox
participant FFmpeg
participant SoundFile
User->>AudioFile: Call read(file)
AudioFile->>MediaInfo: Get sampling rate
MediaInfo-->>AudioFile: Return sampling rate
AudioFile->>Sox: Try convert(file, tmpfile, offset, duration, sampling_rate)
alt Sox conversion fails
AudioFile->>FFmpeg: Try convert(file, tmpfile, offset, duration, sampling_rate)
alt FFmpeg conversion fails
AudioFile->>User: Raise binary_missing_error("ffmpeg")
else FFmpeg conversion succeeds
FFmpeg-->>AudioFile: Conversion complete
end
else Sox conversion succeeds
Sox-->>AudioFile: Conversion complete
end
AudioFile->>SoundFile: Read tmpfile
SoundFile-->>AudioFile: Return signal, sampling_rate
AudioFile-->>User: Return signal, sampling_rate
Updated class diagram for audio conversion functionsclassDiagram
class AudioFile {
+read(file)
}
class MediaInfo {
+get_sampling_rate(file)
}
class Sox {
+run_sox(infile, outfile, offset, duration, sampling_rate)
}
class FFmpeg {
+run_ffmpeg(infile, outfile, offset, duration, sampling_rate)
}
class Convert {
+convert(infile, outfile, offset, duration, sampling_rate)
}
AudioFile --> MediaInfo : uses
AudioFile --> Sox : uses
AudioFile --> FFmpeg : uses
AudioFile --> Convert : uses
Convert --> Sox : calls
Convert --> FFmpeg : calls
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hagenw - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be curious to understand what I could do as a review.
Code-wise there is not much to say, one could possibly describe the
unusual behavior of opus in one sentence somewhere (don't even know where)
The unittests are all passing. My question whether it should entail testing against further external audio data. If not I would continue with approval.
I think you addressed the important points in your review, and I tried to improve on them. There is not really a change how we test the opus file, as the test file was broken before, and the test was hence passing for the
If you think we should expand on this, please open an issue. |
5edab36
to
ffe0389
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There had been minor comments that have been processed in separate threads that are resolved by now. It is therefore justified to approve here.
Closes #157
To add a failing test for #157 we first convert the
gs-16b-1c-44100hz.opus
test file (which had anyway a sampling rate of 48000 Hz) togs-16b-1c-16000hz.opus
with a sampling rate of 16000 Hz. This raises an error for the currentmain
branch as the sampling_rate returned byaudiofile/tests/test_audiofile.py
Line 525 in f32a612
does not match the sampling rate compared to at
audiofile/tests/test_audiofile.py
Line 527 in f32a612
To fix the issue, the conversion code is updated and
sox
andffmpeg
are now both getting asampling_rate
argument, which is extracted withmediainfo
insideaudiofile.read()
for non-snd files (everything besides wav, flac, mp3, ogg).As a side effect, we can no longer easily test the error message for a missing
ffmpeg
binary. In the tests we can only hide the generalPATH
variable, which meansffmpeg
andmediainfo
binaries are missing at the same time, which will always first raise an error formediainfo
now as this is used to access the sampling rate first, before callingffmpeg
.I also checked that the proposal introduced here does not slow down reading of MP4 files by using our benchmark.
Summary by Sourcery
Fix the sampling rate handling for non-SND files by introducing a sampling rate parameter in the audio conversion functions and updating the tests accordingly.
Bug Fixes:
Enhancements:
Tests: