Skip to content
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.

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

Playback Skipping: Custom Resource Loader with file cache #2247

Merged
merged 13 commits into from
Oct 17, 2024

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented Oct 9, 2024

| 📘 Part of: #44 |
|:---:|

Fixes #44

Implements a custom AVAssetResourceLoaderDelegate that as a memory buffer and a file cache. Use the custom delegate to to handle parallel stream and downloads of episode files.

The custom delegate is based on the code in this library, but adapted to our needs.

The PR is a bit large but as said above the majority of code comes from a separate library, and the core work is done on integrating it.

To test

  • Start the app
  • Ensure that you have the FF streamAndCachePlayingEpisode active

Scenario 1 - Start streaming then start download

  • Start playing an episode
  • While it's playing tap on the download button on episode detail
  • Check if the progress circle on episode detail starts showing progress detail for the download
  • Check that the download finishes correctly and you can see the final episode size
  • Check that playing continues correct

Scenario 2 - Start downloading then start playing the episode

  • Start download an episode, one with larger duration will make it easier to test
  • While it's downloading tap on the play button on episode detail
  • The progress circle should go back to 0% ( because we started a new download for the export session)
  • Check if the progress circle on episode detail continues to show progress detail for the download
  • Check that the download finishes correctly and you can see the final episode size
  • Check that playing continues correctly

Scenario 3 - Start downloading, the start playing, then cancel download

  • Start download an episode, one with larger duration will make it easier to test
  • While it's download tap on the play button on episode detail
  • Check if the progress circle on episode detail continues to show progress detail for the download
  • In the middle of the download stop the download on episode detail
  • Check that playing continues correctly

Scenario 4 - Start streaming on multiple episodes

  • Start playing an episode.
  • Start playing another episode before the first one ends
  • Resume playing the original episode
  • Check that all works correctly in terms of playing sound, and that you only see two DownloadManager export session: start exporting ... in the logs.

Scenario 5 - Auto-Downloads

  • Enable Auto-downloads for items added to UpNext
  • Add multiple items to UpNext
  • Play one of those items
  • See that the download continues ( it may go back in terms of progress, because we are starting a streaming download)
  • Wait to see if the download completes
  • Now tap play on another item for the podcast
  • Check that the player starts and at the same time download starts ( remember we have auto-downloads for all up next)
  • Tap play on another item on the podcast
  • Check the previous episode is queued and starts download again
  • Check that new item is playing and downloading

Scenario 6 - Auto-Downloads and AutoPlay

  • Enable Auto-downloads for UpNext and Auto-Play episode
  • Clean up your UpNext Queue
  • Start playing an episode on podcast that has more episodes to play after
  • Seek to the end of episode, less some seconds
  • Wait for the episode to finish
  • Check if the new episode starts and downloads goes correctly

Checklist

  • I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
  • I have considered adding unit tests for my changes.
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

@SergioEstevao SergioEstevao added the playback Issues related to playback label Oct 9, 2024
@SergioEstevao SergioEstevao added this to the 7.75 milestone Oct 9, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Oct 9, 2024

1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@SergioEstevao SergioEstevao modified the milestones: 7.75, 7.76 Oct 11, 2024
@SergioEstevao SergioEstevao marked this pull request as ready for review October 11, 2024 13:58
@SergioEstevao SergioEstevao requested a review from a team as a code owner October 11, 2024 13:58
@SergioEstevao SergioEstevao requested review from danielebogo and removed request for a team October 11, 2024 13:58
@danielebogo danielebogo requested a review from bjtitus October 11, 2024 14:06
@danielebogo
Copy link
Contributor

@SergioEstevao thanks for this PR!

I made the test covering all the scenarios. This is what I found (but perhaps is expected):

  1. While testing scenario 1 I got an error:
DownloadManager export session: start exporting 8f650b71-6263-4ce1-872a-7495d9b97c27
UpNextSyncTask [createUpNextSyncRequest]: Sync Reason? None
UpNextSyncTask: Syncing Up Next, sending 1 changes, modified time 1728992790718
UpNextSyncTask: server copy matches our copy, no action required
DownloadManager export session: finished with error -> Error Domain=AVFoundationErrorDomain Code=-11843 "Cannot write output file" UserInfo={NSLocalizedRecoverySuggestion=Change the output extension and try again., NSLocalizedDescription=Cannot write output file, NSUnderlyingError=0x600000da2970 {Error Domain=NSOSStatusErrorDomain Code=-16975 "(null)"}}
DownloadManager export session: failed
TracksService sendQueuedEvents completed. Sent 3 events.
Breaking Italy Night didn't need to be updated from cache server
Downloading episode Ep. 28 - Giuseppe Conte, autoDownloadStatus: notSpecified, previousDownloadFailed: false
TracksService sendQueuedEvents completed. Sent 3 events.
saving played up to 29.036933089 for episode Ep. 28 - Giuseppe Conte

not sure if it's an expected one. The episode continued to play.

  1. Testing scenario 1 and 2 showed that the player get paused while playing and the download finishes.
  2. Testing scenario 4 I often get this errors (same error as above)
DownloadManager export session: start exporting 90d2497d-f8e8-4f54-8534-8674d5148ac5
DownloadManager export session: finished with error -> Error Domain=AVFoundationErrorDomain Code=-11843 "Cannot write output file" UserInfo={NSLocalizedRecoverySuggestion=Change the output extension and try again., NSLocalizedDescription=Cannot write output file, NSUnderlyingError=0x600000d736c0 {Error Domain=NSOSStatusErrorDomain Code=-16975 "(null)"}}
DownloadManager export session: failed

@SergioEstevao
Copy link
Contributor Author

Testing scenario 1 and 2 showed that the player get paused while playing and the download finishes.

It's expected to detect a small pause on playback when download is complete, because we are switching players and opening the downloaded file.

Copy link
Contributor

@bjtitus bjtitus left a comment

Choose a reason for hiding this comment

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

✅ I tested streaming, downloading, and switching between episodes

@pocketcasts
Copy link
Contributor

Pocket Casts Prototype Build📲 You can test the changes from this Pull Request in Pocket Casts Prototype Build by scanning the QR code below to install the corresponding build.
App NamePocket Casts Prototype Build Pocket Casts Prototype Build
Build Numberpr2247-57e9a91
Version7.75
Bundle IDau.com.shiftyjelly.podcasts.prototype
Commit57e9a91
App Center BuildPocket Casts - Prototype Builds #36
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@SergioEstevao SergioEstevao merged commit 159985d into trunk Oct 17, 2024
4 of 6 checks passed
@SergioEstevao SergioEstevao deleted the playback_skipping/download_with_resource_loader branch October 17, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
playback Issues related to playback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Playback: audio skipped
5 participants