Skip to content

Task Library: Added an offset param to the LoadAudioBufferFromFile C++ function for flexibility #851

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.

Sign up for GitHub

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

Jump to bottom
Merged
copybara-service merged 7 commits into from
Jul 14, 2022
Merged

Task Library: Added an offset param to the LoadAudioBufferFromFile C++ function for flexibility #851

copybara-service merged 7 commits into from
Jul 14, 2022

Conversation

@kinarr
Copy link
Contributor

@kinarr kinarr commented Jul 8, 2022

  • Updated some C tests
  • Added an offset param to create_from_wav_file in the Python Task API

@kinarr kinarr mentioned this pull request Jul 8, 2022
Open
khanhlvg
khanhlvg approved these changes Jul 12, 2022
View reviewed changes
tensorflow_lite_support/cc/task/audio/utils/wav_io.cc Outdated

absl::Status DecodeLin16WaveAsFloatVector(const std::string& wav_string,
std::vector<float>* float_values,
int offset,
Copy link
Member

@khanhlvg khanhlvg Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can you use uint32_t* here to align with the type of sample_count?

Copy link
Contributor Author

@kinarr kinarr Jul 12, 2022

Choose a reason for hiding this comment

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

The original data type for the offset was int though.

tflite-support/tensorflow_lite_support/cc/task/audio/utils/wav_io.cc

Line 119 in 72b4ca6

int offset = 0;

Copy link
Member

@khanhlvg khanhlvg Jul 12, 2022

Choose a reason for hiding this comment

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

Could you change it to uint32_t? It's better than int because offset can't be negative.

I think it'll require you to change the datatype for offset of underlying methods but I think it makes more sense to align the datatype of offset with that of sample_count through out the codebase.

Copy link
Contributor Author

@kinarr kinarr Jul 12, 2022

Choose a reason for hiding this comment

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

Alrighty, I'll make those changes.

@kinarr
Copy link
Contributor Author

kinarr commented Jul 12, 2022

@khanhlvg I've made some changes now. PTAL and let me know if it looks good.

khanhlvg
khanhlvg reviewed Jul 12, 2022
View reviewed changes
khanhlvg
khanhlvg reviewed Jul 12, 2022
View reviewed changes
@kinarr
Copy link
Contributor Author

kinarr commented Jul 12, 2022

@khanhlvg I've fixed and addressed some of the feedback you've left FYI. Oh and I've also tested the 3 audio classifier files (C/C++/Python demo + test code) to ensure it runs fine now.

khanhlvg
khanhlvg approved these changes Jul 12, 2022
View reviewed changes
Copy link
Member

@khanhlvg khanhlvg left a comment

Choose a reason for hiding this comment

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

Thanks Kinar!

@copybara-service copybara-service bot merged commit f260fa9 into tensorflow:master Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@khanhlvg khanhlvg khanhlvg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kinarr @khanhlvg