Skip to content

Fix leading silence removal feature#17699

Merged
seanbudd merged 5 commits into
nvaccess:masterfrom
gexgd0419:fix-silence-detection
Feb 16, 2025
Merged

Fix leading silence removal feature#17699
seanbudd merged 5 commits into
nvaccess:masterfrom
gexgd0419:fix-silence-detection

Conversation

@gexgd0419

Copy link
Copy Markdown
Contributor

Link to issue number:

None. Fixes problems introduced by PR #17648.

Summary of the issue:

The leading silence trimming feature should only trim the silence part of the audio. However, a longer part of the speech audio ended up being chopped off, which makes distinguishing the consonants at the beginning more difficult.

In addition, trimming is incorrectly applied to non-speech audio, and applied even when the feature is turned off in Settings.

Description of user facing changes

The problem mentioned above should be fixed.

Description of development approach

Added missing check for _enableTrimmingLeadingSilence in _idleCheck.

Before sending the trimmed audio to the device, one silent sample will be inserted at the beginning. For some reason, if the first sample is not zero, the audio may be chopped off at the beginning. Although the exact cause is still unknown, prepending one silent sample seems to fix this issue. See this discussion.

Testing strategy:

Tested manually on my system. Now the output waveform seems normal.

Known issues with pull request:

None.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@zstanecic

Copy link
Copy Markdown
Contributor

@gexgd0419 Yess! This fixes the issue.
I have tested it even on espeak with two critical consonants. S and p were also chopped. So, it really does the job.

@gexgd0419 gexgd0419 marked this pull request as ready for review February 14, 2025 08:06
@gexgd0419 gexgd0419 requested a review from a team as a code owner February 14, 2025 08:06
@amirmahdifard

Copy link
Copy Markdown

hi.
onistly, while I really liked nvda's voices become responcive, however, I don't like how it's done actually. even if all voices and sinths has to be modifyed, i'd personally like this to be done same as nvgt project has done this. instead of adding a function to trim the silence, the developer took and modifyed all sappy voices and trimmed the silences him self and inclooded them in the project. they became very responcive, and there is no such problems as done hear because silences has bean trimmed by a human raather than a hardcoded programming function value, which in some cases will cause problems. so I suggest allways give people an option to turn this feature off as such feature can be unreliable at any time even after years, and also always give the ajustable value in advance settings so people can modify it in times they are using a voice and the hardcoded value will cause a problem instead of a hardcoded value. Or you can get the sappy voices from nvgt as it is open sorce, and do the same with other voices and use them in nvgt. For example, open a pr on espeack ng and trim all silences of voices, and then use it. I'm always using espeeck ng and it's really responcive, it's the only sinth i'm used to it and if ever it's not existed, i'd stop using any screenreader with out this nice sinth

@Danstiv

Danstiv commented Feb 14, 2025

Copy link
Copy Markdown
Contributor

Fine. Cannot reproduce beeps and speech chopping on the snapshot build.

@amirsol81

Copy link
Copy Markdown

Thanks for the fix! It takes care of all issues mentioned. @gexgd0419 Awesome job!

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 785946df68

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Works like a charm now, thanks for fixing!

@seanbudd seanbudd merged commit 9bac9eb into nvaccess:master Feb 16, 2025
@github-actions github-actions Bot added this to the 2025.1 milestone Feb 16, 2025
@gexgd0419 gexgd0419 deleted the fix-silence-detection branch February 17, 2025 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants