Improve skipping silences. #12530
Improve skipping silences. #12530
Conversation
|
Looking forward to this! |
|
Do I need to fix/change anything? It's been over 2 months, which is a lot for a tiny change, and there is zero feedback |
AudricV
added
feature request
AudricV
left a comment
AudricV
left a comment
There was a problem hiding this comment.
Thank you for your pull request and sorry for the late reply.
Even if we don't accept new features (see the README), as the refactor branch will use NewPlayer with Media3 at some point, I think we can make an exception for this PR.
Code looks almost good to me, you should provide a way to reset the value set by the user.
| final SilenceSkippingAudioProcessor silenceSkippingAudioProcessor = | ||
| new SilenceSkippingAudioProcessor( | ||
| MILLISECONDS.toMicros(maxSilenceDurationMillis), | ||
| MILLISECONDS.toMicros(maxSilenceDurationMillis), | ||
| SilenceSkippingAudioProcessor.DEFAULT_SILENCE_THRESHOLD_LEVEL); |
There was a problem hiding this comment.
For reference for NewPlayer, this approach has been changed in Media3.
I would remove the duplicate call to MILLISECONDS.toMicros(maxSilenceDurationMillis) by saving the result in a variable.
Can you explain why you set the same value to minimumSilenceDurationUs and paddingSilenceUs?
There was a problem hiding this comment.
Moved it to a variable.
The duration is passed both as minimumSilenceDurationUs and paddingSilenceUs so that when the silence longer than maxSilenceDuration is detected, it is truncated to maxSilenceDuration. Silence detection length is controlled by minimumSilenceDurationUs and the duration which is left is controlled by paddingSilenceUs. So when we set them to the same value, we achieve this truncation behavior.
I think this behavior is reasonable and it also unclutters the settings by having only one parameter to control silence skipping.
If these variables are independent, the behavior could be different, e.g. if minimumSilenceDurationUs is 2s and paddingSilenceUs is 0.01s, it means that silences up to 2 seconds are kept as-is, but silences longer than 2 seconds get truncated to 0.01s. To me this behavior seems unnatural so I made these values the same.
app/src/main/java/org/schabi/newpipe/player/helper/CustomRenderersFactory.java
Show resolved
Hide resolved
Stypox
left a comment
Stypox
left a comment
There was a problem hiding this comment.
Thank you for this nice addition! Sorry for the late review from my side, too. I haven't had enough time to keep up with the various PRs lately.
I agree with @AudricV, I think this can be merged as it's rather small and shouldn't introduce issues.
| app:singleLineTitle="false" | ||
| app:iconSpaceReserved="false"/> | ||
|
|
||
| <SeekBarPreference |
There was a problem hiding this comment.
I wonder whether this should go to the playback parameters dialog, so it's easily accessible from the player, and it also does not clutter settings. And there it should be shown only when "Fast-forward during silence" is enabled, to further reduce clutter for users that don't enable "Fast-forward during silence". Let me know what you think.
There was a problem hiding this comment.
I'd be glad to add it in the menu, but the issue here is that applying the change to the value requires reinitializing the player. SilenceSkippingAudioProcessor doesn't support changing its parameters after it is created, so the only way to apply the change to the parameter is initializing the player from scratch. I am not really familiar with the codebase, if such behavior makes sense and if it is relatively easy to do it, I'd be happy to implement it, but if it is too complicated or it will lead to some weird artifacts (such as player freezing while it reinitializes), I'd prefer to leave it as it is now.
There was a problem hiding this comment.
This preference should go to ExoPlayer settings in my opinion.
There was a problem hiding this comment.
Moved it to ExoPlayer settings (also changed the default from 1000ms to 500ms)
ExoPlayer allows controlling silence skipping by adjusting the silence duration that is removed by `SilenceSkippingAudioProcessor`. The default behavior with the default parameters for this component makes it hard to listen to content because it leaves almost no pauses. After this commit it is possible to adjust silence length that remains and shorten only long pauses.
- Extract duplicated microsecond duration calculation - Update JavaDoc for CustomRenderersFactory
a-shtefan
force-pushed
the
improve-silence-skipping
branch
from
dd29f0e to
d55d6a1
Compare
4 participants
ExoPlayer allows controlling silence skipping by adjusting the silence duration that is removed by
SilenceSkippingAudioProcessor. The default behavior with the default parameters for this component makes it hard to listen to content because it leaves almost no pauses. After this commit it is possible to adjust silence length that remains and shorten only long pauses.What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.
Due diligence