Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Fix global audio syncing and seeking issues #654

Merged
merged 5 commits into from
Jan 21, 2022

Conversation

sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Jan 20, 2022

Fixes

Fixes #653 by @sarayourfriend
Closes #632 by @sarayourfriend — All audio tracks on the page buffer immediately so there's no longer a delay between playing them... do we need to prevent audio from loading until the "play" button is hit on it or it is seeked? Probably so.

Description

Switches to using a global ref for the currently active audio track and swapping between multiple audio elements (one per track on the page, aside from the global track).

Some notes/key concepts:

  1. We derive all state from the state of the audio object, whether the global "active audio" or the local audio. This means our state is updated when the local audio object emits the corresponding events. The only exception to this is the current time. I originally implemented this to always update the currentTime ref (which is used for displaying the play progress on the waveform) using the timeupdate event exclusively. But to achieve a smooth animation, when a track is playing, we'll update the currentTime ref in a requestAnimationFrame loop. timeupdate is still used to update the current time when the audio is not playing (for example if it is seeked while it is paused or ended). Otherwise the timeupdate event is ignored when the track is playing.
  2. The GlobalAudioTrack and AudioTrack components share much of the same logic. At a glance it does appear that some of this stuff could be abstracted and I did spend a short amount of time thinking about how to do this. However, most of the "simple" ideas for how to do this, were thwarted by various issues with syncing global and local state selectively. The current implementation works, and while there is some duplicated code, most of it is subtly different. The approaching deadline for the audio release means spending time on refactoring this PR right now is pretty low value. If we decide to spend more time on this in the future, it could alleviate the maintainability burden of these two components, as long as we're sure their implementations won't diverge in the future. Given the already existing subtle differences between the two, I'd make a pretty confident bet that trying to abstract these behaviors into a shared utility would present difficulties in the future.
  3. The local audio track has to be careful not to use the Audio object on the server. I tried to minimize the amount of code that was shifted into the onMounted hook, mostly because putting everything in there seems like an organizational misstep. If we did move everything in there, however, we could probably get rid of a lot of the optional chaining on the localAudio variable, but it seems like a small victory that could cause delays in mounting the component that are easily avoided (thanks to ergonomic features like optional chaining).
  4. I tried to add explanatory comments to any logic that was non-obvious or could be implemented in different ways but needed to be done the way I did. If there's anything else that warrants explanation, let me know and I am happy to add more comments.

Testing Instructions

Checkout this branch and run pnpm dev.

Visit an audio result page like http://localhost:8443/search/audio?q=birds and play around with seeking, playing, pausing, replaying tracks, both with and without the global audio player. Also make sure to test navigation between the search results and single result pages.

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the default branch of the repository (main) or a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • [Not possible yet] I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@sarayourfriend sarayourfriend added 🟧 priority: high Stalls work on the project or its dependents 🛠 goal: fix Bug fix 💻 aspect: code Concerns the software code in the repository labels Jan 20, 2022
@sarayourfriend sarayourfriend requested a review from a team as a code owner January 20, 2022 16:58
Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

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

This is amazing, and eliminates all browser inconsistencies and quirks I've encountered in the past.

As I was writing up this PR description I found one bug where if you play an audio track on the search results page but then visit a different single result page, the previous audio track will continue playing.

Regarding this, I noticed the track that is left playing is completely decoupled from any UI in that it can't be paused from it's original audio player or the global player. I think we can handle it in a separate PR but just wanted to note the behavior I'm seeing.

@sarayourfriend
Copy link
Contributor Author

@zackkrida the issue was pretty easy to fix, but did require some complicated changes to the initialization logic in VAudioTrack. However, as a side effect of this, I was able to delay loading audio until playback or seeking happens 👍

All page transitions should now be working as smoothly as reasonably possible. Hopefully I haven't made any incorrect assumptions about how the search results are stored and what's presented on the page!

@zackkrida zackkrida merged commit c13125e into main Jan 21, 2022
@zackkrida zackkrida deleted the fix/global-audio-syncing-issues branch January 21, 2022 05:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟧 priority: high Stalls work on the project or its dependents
Projects
None yet
2 participants