Output word timings #1974
Output word timings #1974
Conversation
No Taskcluster jobs started for this pull requestThe `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request. |
|
Ok, done: #1976 |
Sorry, I was just documenting for posterity, not asking you to do it :) |
|
I still can't get my branch to rebase so let me know when it's ready to be committed to master and I'll create a new branch and PR. |
|
No need to rebase, extra commits fixing review comments are fine. We just need @kdavis-mozilla's review now. |
| { | ||
| std::vector<meta_word> word_list; | ||
|
|
||
| std::string word = ""; |
There was a problem hiding this comment.
Is this a UTF-8 string? If so it should start with a u8.
If not, then I'm wondering how general non-ASCII strings are represented in word.
| word_list.push_back(w); | ||
|
|
||
| // Reset | ||
| word = ""; |
There was a problem hiding this comment.
Same question here about UTF-8 vs non-UTF-8 string.
| { | ||
| vector<Output> out = ModelState::decode_raw(logits); | ||
|
|
||
| return strdup(alphabet->LabelsToString(out[0].tokens).c_str()); |
There was a problem hiding this comment.
I know strdup() was used previously, but I worry that passing this over the ABI boundary and asking the client to free it will cause crashes if the client uses a different C runtime library.
There was a problem hiding this comment.
Fixing this requires more changes to the API, so could it be left for a follow up PR? I filed #1979.
|
I've made the changes requested. Let me know if you find anything else. |
|
Running PR with the latest branch |
|
@dabinat I know you got issues with rebasing and squashing in the past, is that still the case on this PR ? 12 commits, it makes it a bit complicated, if you could rebase into one or two commits it'd be perfect. |
|
I told them there's no need to rebase as it's insignificant to our workflow, and we can always use GitHub's squash and merge/rebase and merge functionality in the PR anyway. |
|
I fixed those two nits. If rebasing is a problem I could create a new branch and PR, but I would want to be certain there aren't any extra changes needed before I do so. |
|
No worries, I'll run a last taskcluster check and merge 😊 |
|
All green, thanks for the hard work @dabinat ! Now that this gets merged, you can improve our existing bindings (Python, JavaScript, Java/Android) as well as the rust ones https://github.com/RustAudio/deepspeech-rs and go ones https://github.com/asticode/go-astideepspeech by exposing the new feature :) |
|
It depends how quickly you need them. I can take on Python and JavaScript but won’t be able to for a couple of weeks. |
|
No deadline, just an idea I was sharing :) |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Letter timings are exposed on the API and the client then converts these into word-level timings. The client outputs CSV when used with the -e command-line argument.