-
Notifications
You must be signed in to change notification settings - Fork 206
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
Streaming mode #185
Streaming mode #185
Conversation
43f64f7
to
96a916d
Compare
fe1bcd1
to
594413c
Compare
Will this make the existing algorithms work with RT audio input? |
Yes, exactly. We hope to get this done by end of the year. |
fea7c50
to
11f9f68
Compare
11f9f68
to
52629fa
Compare
6a53e3a
to
a7cb9d3
Compare
e14ad8b
to
6fa56fd
Compare
Besides the open TODOs below, this PR is more or less ready. As decided/merged in #244, As discussed in #246, an additional Open questions / TODOs:
Maybe these can remain open or don't need to be addressed (right now), as this PR handles only the common foundation for various online processing methods (e.g. #238 for online beat tracking). |
6fa56fd
to
bceab16
Compare
bceab16
to
3b72ece
Compare
One more thing to consider: at the moment there is no way to determine whether a processor is online-capable or not. A previous attempt to solve this used different process methods. E.g. There's no need to address this within this PR, since none of the added functionality affects this. We should just keep this in mind. |
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.
Great work!
Besides the minor comments, I think the documentation needs to be updated to reflect the possibilities and caveats when doing online processing.
EDIT
Oh, and of course we should create an issue with the open ToDos...
args = dict(frame_size=self.frame_size, hop_size=self.hop_size, | ||
fps=self.fps, origin=self.origin, end=self.end, | ||
num_frames=self.num_frames) | ||
args.update(kwargs) |
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.
Is there a reason for this change? I actually think the previous code was more direct and thus easier to understand.
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.
Previously it was not possible to overwrite the parameters, only to pass additional ones to FramedSignal
(which in turn got passed to Signal
).
This change was necessary in order to be able to pass arguments to process()
, which is needed to change the behaviour of processors, e.g. not reset stateful processors when being processed framewise.
madmom/audio/spectrogram.py
Outdated
@property | ||
def bin_frequencies(self): | ||
"""Bin frequencies.""" | ||
try: |
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.
Is this necessary? As far as I understand, the self.spectrogram
is a FilteredSpectrogram
(and thus has a filterbank), then self.spectrogram.bin_frequencies
will return the center_frequencies
of the filterbank anyways.
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.
You are right. I think you meant: "if self.spectrogram
is a FilteredSpectrogram
", though.
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 will update the branch to include the suggested corrections and add more documentation before merging.
args = dict(frame_size=self.frame_size, hop_size=self.hop_size, | ||
fps=self.fps, origin=self.origin, end=self.end, | ||
num_frames=self.num_frames) | ||
args.update(kwargs) |
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.
Previously it was not possible to overwrite the parameters, only to pass additional ones to FramedSignal
(which in turn got passed to Signal
).
This change was necessary in order to be able to pass arguments to process()
, which is needed to change the behaviour of processors, e.g. not reset stateful processors when being processed framewise.
madmom/audio/spectrogram.py
Outdated
@property | ||
def bin_frequencies(self): | ||
"""Bin frequencies.""" | ||
try: |
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.
You are right. I think you meant: "if self.spectrogram
is a FilteredSpectrogram
", though.
set online=None for all other modes
This fixes #33 and enables overwriting of processing parameters during runtime.
3b72ece
to
735d438
Compare
This is a stripped down version of @amaurydurand's work (PR #184).
TODO:
BufferProcessor
Filterbank
)bin_frequencies
as an propertyfloat32
instead ofint16
dtyperewrite all features as processors (mostly(separate issue/PR)madmom.features.onsets.*
)Stream
with the given argumentsif one is given check if the arguments match the ones of the processorhop_size
not possible, raise error? (see issue Allow float hop_size for Stream? #249)diff_ratio
right inSpectrogramDifference
, find a solution (maybe only allowdiff_frames
in online mode?)make(see separate issue Allow processing in blocks #250)block_size
configurable (e.g. it could be advantageous to process everything in blocks of 2048 frames instead of whole length pieces)rewrite(see separate issue Rewrite FilterbankProcessor #248)FilterbankProcessor
to process the input and create aFilterbank
when called the first time and cache it for consecutive calls (like a buffer)drop the combined spectrogram processors ((see separate issue Rewrite FilterbankProcessor #248)FilteredSpectrogramProcessor
,LogarithmicSpectrogramProcessor
andLogarithmicFilteredSpectrogramProcessor
) altogether?add support to select mono/stereo/left/right channel in(should be handled byStream
Signal
, see separate issue add channel selection mechanism to Signal #211)**kwargs
toprocess()
methods is needed (related: add **kwargs to process() #33)