Fix fast-forward at end of episode #6074

terminalmage · 2022-09-13T04:04:17Z

When using variable speed, skipping back and forth introduces some uncertainty to the current position, causing skip-forward to try to skip to an invalid position when very near the end of the episode. This change fixes this by skipping the current episode if the desired skip-forward position exceeds the duration.

Fixes #5974.

Please let me know if there is a more elegant solution, but this seems to work.

loucasal · 2022-09-13T08:08:30Z

@terminalmage any chance this also addresses #5166?

terminalmage · 2022-09-13T12:10:59Z

@loucasal It seems like it might, though I have never encountered the exact issue you reported. But both bugs seem to be triggered under similar circumstances.

ByteHamster · 2022-09-13T17:36:53Z

This will have unwanted side effects.

Users not realizing their episode is almost done will not expect it to skip
When the file is encoded with VBR and does not have a duration in its metadata, the best AntennaPod can do to get the duration is make an educated guess. This means that sometimes the position is actually more than the duration (depending on the file encoding and duration, this might be as much as ~2 minutes). Your PR will break fast-forward in these cases.

terminalmage · 2022-09-13T17:42:06Z

I had a feeling my first crack at fixing this would not be the best 😆

What do you suggest?

ByteHamster · 2022-09-13T20:02:00Z

What do you suggest?

I don't really know

terminalmage · 2022-09-13T20:33:10Z

Perhaps this can be targeted more narrowly. For example, in the bug that I reported, the code as-is will attempt to skip forward but just.... won't do that. Is there a good way to detect when the skip-forward is unsuccessful, and simply skip the current episode in these cases? Maybe the position could be checked after the seek. If the position is not >= the desired seek position, then the seek was unsuccessful, and we can assume the end-of-episode has been reached. What do you think?

EDIT: I tried this, and it looks like the seek happens asynchronously, so you can't reliably check the position immediately after calling seekTo.

terminalmage · 2023-07-22T16:39:49Z

@keunes @ByteHamster As discussed in the most recent community call, I did eventually get a chance to see how another open-source player (Pocket Casts) handles skip-forward at the end of the file. Here is the relevant function: https://github.com/Automattic/pocket-casts-android/blob/09c5634/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt#L710-L732

They seem to be taking the same approach that I did independently when I opened this PR. If the new position is >= to the episode length, the episode is considered completed.

@ByteHamster had concerns with this approach, because the episode length in milliseconds is an estimate when the audio file is encoded with VBR. However, in my experience, skip forward always worked until I got to the last 10-15 seconds of an episode. Once you get near the end, skipping forward fails because we are telling exoplayer to skip to a position that doesn't actually exist.

I believe that people using skip forward at the end of an episode are typically doing so aware of the fact that they're at the end of the episode, and the expected behavior when you press the skip-forward button with <30 seconds remaining is that the episode is finished. I have rebased and slightly altered this pull request, as it wasn't building with changes made to AudioPlayerFragment in the time since I initially opened the PR.

ByteHamster · 2023-09-21T19:51:14Z

Sorry for the late reply. Shouldn't the skipping decision be made in the media player instead of the UI? Eg if you fast-forward from the notification, the widget, the player screen, headphones, etc, it should behave the same, I think.

antennapod-bot · 2023-09-21T21:23:20Z

This issue has been mentioned on AntennaPod Forum. There might be relevant details there:

https://forum.antennapod.org/t/monthly-community-call/1869/53

terminalmage · 2023-09-22T11:47:44Z

Sorry for the late reply. Shouldn't the skipping decision be made in the media player instead of the UI? Eg if you fast-forward from the notification, the widget, the player screen, headphones, etc, it should behave the same, I think.

That makes sense. I only found that one place where skip-forward was already being implemented (there could definitely be others I missed), and made my changes there. I'm not 100% sure where you're suggesting the code is moved, though.

ByteHamster · 2023-09-22T12:35:55Z

My idea (not having looked at the code) was to do it in LocalPSMP

ByteHamster · 2023-10-17T19:02:41Z

Hi @terminalmage, are you still interested in this PR? I would say let's give it a try if you move the code over to LocalPSMP.

terminalmage · 2023-10-17T19:41:23Z

Yeah, I'm interested. I should have some time to work on this later in the week. I'll let you know if I have any questions.

When using variable speed, skipping back and forth introduces some uncertainty to the current position, causing skip-forward to try to skip to an invalid position when very near the end of the episode. This change fixes this by skipping the current episode if the desired skip-forward position exceeds the duration. Fixes AntennaPod#5974.

terminalmage · 2023-10-23T13:50:49Z

@ByteHamster I have the code moved into LocalPSMP, but it doesn't work quite right. If you look at the updated diff, There's a call there to setPlayerStatus(), but I don't think it's the correct thing to do before invoking skip(). With the code as it currently is, playback doesn't start for the next episode in queue. However, if I remove that call, a couple things don't work correctly:

The current position is not reset. This means that when the next feed item in the queue is loaded, it is resumed at the same position from the previous episode, instead of the start of the episode. If that position is greater than the length of the new episode, it is also marked as played. Playback is never started for that episode.

If there is nothing in the queue, a traceback occurs:

Environment

Android version: 14
OS version: 5.10.157-android13-4-00001-g5c7ff5dc7aac-ab10381520
AntennaPod version: 3.1.0-beta2
Model: Pixel 7
Device: panther
Product: panther

Crash info

Time: 23-10-2023 08:38:58
AntennaPod version: 3.1.0-beta2

StackTrace

java.lang.NullPointerException: Attempt to invoke virtual method 'void android.support.v4.media.session.MediaSessionCompat.setQueue(java.util.List)' on a null object reference
        at de.danoeh.antennapod.core.service.playback.PlaybackService.lambda$loadQueueForMediaSession$1$de-danoeh-antennapod-core-service-playback-PlaybackService(PlaybackService.java:350)
        at de.danoeh.antennapod.core.service.playback.PlaybackService$$ExternalSyntheticLambda3.accept(Unknown Source:4)
        at io.reactivex.internal.observers.ConsumerSingleObserver.onSuccess(ConsumerSingleObserver.java:62)
        at io.reactivex.internal.operators.single.SingleObserveOn$ObserveOnSingleObserver.run(SingleObserveOn.java:81)
        at io.reactivex.android.schedulers.HandlerScheduler$ScheduledRunnable.run(HandlerScheduler.java:124)
        at android.os.Handler.handleCallback(Handler.java:958)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loopOnce(Looper.java:205)
        at android.os.Looper.loop(Looper.java:294)
        at android.app.ActivityThread.main(ActivityThread.java:8177)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971)

ByteHamster · 2023-10-28T17:22:59Z

Shouldn't you return after calling skip()? Otherwise it also goes through all the fast-forward code, which could cause the problems you are describing. I don't think a call to setPlayerStatus should be necessary because your old code just called skip() as well (indirectly), without any status change.

terminalmage · 2023-10-28T21:13:53Z

@ByteHamster Yeah that worked, thanks. This should be ready for review and merge now.

core/src/main/java/de/danoeh/antennapod/core/service/playback/LocalPSMP.java

…LocalPSMP.java

ByteHamster · 2023-10-29T08:44:25Z

Thanks. Will be released in 3.2.0. Let's hope that this does not cause problems when the media is VBR encoded.

When using variable speed, skipping back and forth introduces some uncertainty to the current position, causing skip-forward to try to skip to an invalid position when very near the end of the episode. This change fixes this by skipping the current episode if the desired skip-forward position exceeds the duration.

Merging AntennaPod#6074 has caused a new edge case for VBR audio files, in which using the seek bar to seek to the end of an episode sometimes hits the new code path, and the `skip()` function is called. Because `skip()` invokes `endPlayback()` with `hasEnded` set to `false`, post-processing tasks are not executed unless the pre-seek position falls within the "Smart mark as played" range. If "Smart mark as played" is set to `Disabled`, or the pre-seek position is outside that range, then the episode is not marked as played, and not removed from queue. This commit fixes that edge case by replacing `skip()` with a direct call to `endPlayback()`, with `hasEnded` set to `true`.

Merging #6074 has caused a new edge case for VBR audio files, in which using the seek bar to seek to the end of an episode sometimes hits the new code path, and the `skip()` function is called. Because `skip()` invokes `endPlayback()` with `hasEnded` set to `false`, post-processing tasks are not executed unless the pre-seek position falls within the "Smart mark as played" range. If "Smart mark as played" is set to `Disabled`, or the pre-seek position is outside that range, then the episode is not marked as played, and not removed from queue. This commit fixes that edge case by replacing `skip()` with a direct call to `endPlayback()`, with `hasEnded` set to `true`.

terminalmage force-pushed the issue5974 branch from 780b653 to cd7bb93 Compare July 22, 2023 16:29

terminalmage added 2 commits October 23, 2023 07:29

Move end-of-episode skip logic to LocalPSMP

4c9fa3b

terminalmage force-pushed the issue5974 branch from cd7bb93 to 4c9fa3b Compare October 23, 2023 13:17

Add return statement when episode is being skipped

e88692d

ByteHamster reviewed Oct 29, 2023

View reviewed changes

core/src/main/java/de/danoeh/antennapod/core/service/playback/LocalPSMP.java Outdated Show resolved Hide resolved

Update core/src/main/java/de/danoeh/antennapod/core/service/playback/…

6286b6c

…LocalPSMP.java

ByteHamster merged commit 8a011ba into AntennaPod:develop Oct 29, 2023
7 checks passed

This was referenced Nov 6, 2023

Fast forwarding past the end of a podcast leaves the player in a paused state #5166

Closed

Redraw the queue after the end of an episode without waiting for user interaction #6443

Open

Crash when reaching the end of an episode #6748

Closed

terminalmage mentioned this pull request Nov 14, 2023

[3.2.0-beta1] Fix seeking to end using seek bar #6763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fast-forward at end of episode #6074

Fix fast-forward at end of episode #6074

terminalmage commented Sep 13, 2022

loucasal commented Sep 13, 2022

terminalmage commented Sep 13, 2022

ByteHamster commented Sep 13, 2022 •

edited

terminalmage commented Sep 13, 2022

ByteHamster commented Sep 13, 2022

terminalmage commented Sep 13, 2022 •

edited

terminalmage commented Jul 22, 2023 •

edited

ByteHamster commented Sep 21, 2023

antennapod-bot commented Sep 21, 2023

terminalmage commented Sep 22, 2023 •

edited

ByteHamster commented Sep 22, 2023

ByteHamster commented Oct 17, 2023

terminalmage commented Oct 17, 2023

terminalmage commented Oct 23, 2023 •

edited

Environment

Crash info

StackTrace

ByteHamster commented Oct 28, 2023 •

edited

terminalmage commented Oct 28, 2023

ByteHamster commented Oct 29, 2023

Fix fast-forward at end of episode #6074

Fix fast-forward at end of episode #6074

Conversation

terminalmage commented Sep 13, 2022

loucasal commented Sep 13, 2022

terminalmage commented Sep 13, 2022

ByteHamster commented Sep 13, 2022 • edited

terminalmage commented Sep 13, 2022

ByteHamster commented Sep 13, 2022

terminalmage commented Sep 13, 2022 • edited

terminalmage commented Jul 22, 2023 • edited

ByteHamster commented Sep 21, 2023

antennapod-bot commented Sep 21, 2023

terminalmage commented Sep 22, 2023 • edited

ByteHamster commented Sep 22, 2023

ByteHamster commented Oct 17, 2023

terminalmage commented Oct 17, 2023

terminalmage commented Oct 23, 2023 • edited

Environment

Crash info

StackTrace

ByteHamster commented Oct 28, 2023 • edited

terminalmage commented Oct 28, 2023

ByteHamster commented Oct 29, 2023

ByteHamster commented Sep 13, 2022 •

edited

terminalmage commented Sep 13, 2022 •

edited

terminalmage commented Jul 22, 2023 •

edited

terminalmage commented Sep 22, 2023 •

edited

terminalmage commented Oct 23, 2023 •

edited

ByteHamster commented Oct 28, 2023 •

edited