This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Fix waveform misalignment for short audios #1476
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/1476 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
🕹 aspect: interface
|
Size Change: -1.79 kB (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
zackkrida
approved these changes
May 30, 2022
Comment on lines
191
to
197
| if (localAudio) | ||
| if (status.value === 'playing') { | ||
| currentTime.value = localAudio.currentTime | ||
| window.requestAnimationFrame(updateTimeLoop) | ||
| } else if (status.value === 'played') { | ||
| currentTime.value = localAudio.currentTime | ||
| } |
There was a problem hiding this comment.
I'm going to look into why later but I wanted to note it here so I don't forget. It looks like the final update to put the audio current time happens slightly later and causes a jump in the waveform progression at the end for short audio files 🤔 I want to try to find a fix for that too if you don't mind @obulat?
There was a problem hiding this comment.
@sarayourfriend, I haven't seen the jump. I wonder if it was caused by the audio sometimes ending with a paused status, and sometimes with played. I still don't understand why it happens, but I removed the check for status being played to update the currentTime on line 196 here.
sarayourfriend
approved these changes
Jun 7, 2022
There was a problem hiding this comment.
Approving this for now. I haven't had time to check out the issue I mentioned. @obulat if you don't get around to checking on this now can you open an issue to address it? I think it's very low priority, but this fix is better to have than to block on it for the minor jumpiness.
|
@sarayourfriend does this video look like what you're seeing? I haven't been able to reproduce the jump you mentioned. CleanShot.2022-06-08.at.09.44.40.mp4I have, however, @obulat, noticed interesting behavior with audio around 1 second long. Here's the 'end point' of a bunch of >1s audios:
They all finish in a different position! I wonder if the position is deterministic and related to their exact length. Perhaps for these we'd need to round up the progress bar to 1 second. I also wonder: should we alter the timestamp to show milliseconds for audio tracks less than one second long? We can make a separate issue for that if needed. |
|
@zackkrida it does not look like your video. Unfortunately I'm completely unable to play any audio files locally at the moment (no idea why, it's very strange) so I can't take a video to show what I was seeing. |
|
I can reproduce this every single time for the first time I play an audio: openverse-audio-end-jump.mp4And then it appears to happen every other time when replaying: openverse-audio-end-jump-replays.mp4 |
This is a really interesting bug! Sometimes I get the correct end position every time, sometimes it's incorrect on the first time, sometimes - on following replays. I'm going to look into it. |
# Conflicts: # src/locales/po-files/openverse.pot
|
I'm wondering if instead of relying on the animation loop we need to just set |
Update time for `paused` and `played` statuses to prevent uneven waveform at the end
It seems to be working now, after removing the check for played status. Do you think this solution is good enough, @sarayourfriend ? |
|
Looks better to me, nice 👍 |
|
I still see the jump at the end unfortunately 😢 openverse-jump-at-end-after-fix.mp4At least it is consistent now in that it does fill the progress bar fully every time. The milliseconds showing is pretty cool... however, because the API durations are not accurate to the actual durations of the files (:rage1:) there is a very noticeable change in the number of milliseconds for basically any file that shows them the first time you hit play (after the file is loaded and we start using the browser's reported duration for the file). It makes me wonder if we really should only show milliseconds for sub-second files, mostly to minimize the visibility of that frustrating data issue :grimacing: |
Ah, I only re-tested on a results page with all sub-second results 😅. I agree with this. |
This was an easy thing to fix :) |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #1304 by @sarayourfriend
Description
This PR fixes the waveform misalignment at the end of the waveform. It was especially noticeable for very short audios. We were only updating the
currentTimevalue when the audio was inplayingstatus, so the last update when the audio went fromplayingtoplayedstatus was never made, and the audio waveformprogressBarWidthwas not updated for the last time.There was also an error when we used a computed without calling its
.value.Testing Instructions
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin