-
Notifications
You must be signed in to change notification settings - Fork 3
add new audio parse script with timestamp decoding #29
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
Conversation
Era-Dorta
left a comment
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.
Nice work!
| if current_pos + buffer_frames > len(audio_data): | ||
| buffer_frames = len(audio_data) - current_pos | ||
|
|
||
| if buffer_frames <= 0: |
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.
Can you explain when is this condition true? I don't quite understand it.
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.
Rather than adding decoding the audio file here. Can you remove the audio decoding function and import the metadata functions into the existing audio parser file and add an if statement that if a ".d" file exists then it also does the timestamp decoding and otherwise it doesn't? Otherwise, we have code duplication.
|
|
||
| # Convert timestamps to datetime for readability | ||
| base_time = datetime.fromtimestamp(timestamps[0]) | ||
| df['datetime'] = [base_time + timedelta(seconds=(t - timestamps[0])) for t in timestamps] |
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 think this is wrong, why are you turning them into time deltas? The time stamps already have the full date and time.
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 wrote it before the changed to the midge code were finalised. Now, I'm just waiting for #30 to go through before I fix this.
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.
Do you mind fixing it with python 2? VSCode doesn't work with python2 but pycharm does. So you can code it there. The update in #30 is turned out more complicated that I thought, somehow the update is breaking the connection as I wrote in #30 (comment)
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.
Alright. I'll close this PR for now then
Fix #25