(#692) Fix duration mismatch between audio and metadata #818

demophoon · 2022-02-06T23:59:28Z

When an audio file is played and the duration from the passed properties
does not match the duration of the actual audio file, e.g. the duration
was rounded, the progress bar drifts outside of the total width of the
VWaveform dimensions.

On smaller width screens or larger tracks it is less noticeable but when
the audio track is small and the screen width is large that percentage
adds up to 10s or more of pixels which is where the bug crops up.

This commit fixes this issue by by using the audio context's duration
parameter and falling back to less specific sources for duration.

Fixes

Fixes #692 by @sarayourfriend

Description

Testing Instructions

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.
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
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 · 2022-02-07T00:04:28Z

src/components/VAudioTrack/VAudioTrack.vue

@@ -285,7 +285,9 @@ export default defineComponent({

    /* Timekeeping */

-    const duration = computed(() => (props.audio?.duration ?? 0) / 1e3) // seconds
+    const duration = computed(
+      () => (localAudio?.duration ?? props.audio?.duration ?? 0) / 1e3 // seconds


This looks great! There's also another component that will need this update: VGlobalAudioTrack.

You can see this component on the audio search results page when your viewport is small, like on a mobile device (or just a narrow desktop browser window).

Updated in f3aa9f2.

Hope its okay I added this to the single commit. Figured it was the same exact fix.

When an audio file is played and the duration from the passed properties does not match the duration of the actual audio file, e.g. the duration was rounded, the progress bar drifts outside of the total width of the VWaveform dimensions. On smaller width screens or larger tracks it is less noticeable but when the audio track is small and the screen width is large that percentage adds up to 10s or more of pixels which is where the bug crops up. This commit fixes this issue by by using the audio context's duration parameter and falling back to less specific sources for duration.

demophoon · 2022-02-07T00:35:16Z

Hmm.. this actually needs more investigation before being merged. I'm still seeing the bar fall off the end..

sarayourfriend · 2022-02-07T01:30:46Z

I think you have the right approach, but because the localAudio object isn't a ref, it's not going to recalculate.

One potential solution I can think of is to create a duration ref to store it in and use the props.audio.duration as the default value. Then we can add a load event listener to the localAudio object to retrieve the duration of the file once it's loaded.

What do you think @demophoon?

zackkrida · 2022-02-18T17:14:17Z

@demophoon do you think you'll have an opportunity to revisit this PR? If not, we can get another contributor to look at it, totally up to you. Thank you for your work so far!

zackkrida · 2022-02-18T17:14:53Z

@dhruvkb's expertise here would be useful as well, as the primary author of the original audio track components.

demophoon · 2022-02-18T18:22:45Z

@zackkrida Oh yes! It totally slipped out of my mind after the conference. I'm looking at this again this morning.

@sarayourfriend That makes total sense to me since the duration that I initially used is set before the browser has even had a chance to decode the audio track to determine the real duration.

demophoon · 2022-02-18T19:52:59Z

That change appears to have worked!

If there is a better way to notify a computed property of this change please let me know. I'm still a new to vue so I might not be aware of all the features.

Screenshots below of fix!

sarayourfriend · 2022-02-18T20:19:54Z

Looking good! Seems to work well on the single audio results, but for the global player there's a subtle bug.

First, to activate the global audio play, open a very narrow browser window on the audio search results page, something like this:

When you play an audio file, the global audio player will appear floating at the bottom of the screen. Make sure to narrow the results to just Jamendo provider (for example /search/audio?q=birds&source=jamendo).

Next, play one audio file that you know is shorter than another. Then play another audio file. Confirm that the duration was updated for the global player. Next, play the first audio again.

Notice that the duration will not be updated in the global player's context.

This is because the durationchanged event isn't firing, the audio objects have just been swapped out.

I think in the global audio track component we need to use the durationchanged event as well as grabbing the duration when the activeTrack ref changes, similar to how we grab the currentTime of the active audio on this line:

openverse-frontend/src/components/VAudioTrack/VGlobalAudioTrack.vue

Line 157 in fc35e75

currentTime.value = audio.currentTime

In fact, I think just adding another line below that one that does this would work:

audioDuration.value = audio.duration

I tried that locally and it seemed to work 👍

Before this commit the duration would be set based on the audio context state as the page was loaded. Since the audio file is never loaded by the time we check for the duration of the audio object, it is always null thus defaulting to the metadata for audio duration always. This commit adds a `durationchange` listener while initializing the audio object and sets the duration in a new `audioDuration` property which the computed `duration` property watches.

demophoon · 2022-02-18T20:52:38Z

Oh, great catch. I didn't think about the object still being around on a second play through especially with respect to the global audio component.

Updated in 9f228640.

sarayourfriend

This is great! Thanks so much for the contribution 🎉

zackkrida

So nice to see this fixed. The new code for durationchange listening and storage in a ref seems very logical!

demophoon requested a review from a team as a code owner February 6, 2022 23:59

demophoon requested review from krysal and obulat February 6, 2022 23:59

sarayourfriend reviewed Feb 7, 2022

View reviewed changes

demophoon force-pushed the issue-692/main/audio-waveform-bar branch from a3242fd to f3aa9f2 Compare February 7, 2022 00:13

demophoon force-pushed the issue-692/main/audio-waveform-bar branch from 1087aa7 to fc35e75 Compare February 18, 2022 19:58

demophoon force-pushed the issue-692/main/audio-waveform-bar branch from fc35e75 to 9f22864 Compare February 18, 2022 20:52

sarayourfriend approved these changes Feb 19, 2022

View reviewed changes

zackkrida approved these changes Feb 19, 2022

View reviewed changes

sarayourfriend merged commit 99a8035 into WordPress:main Feb 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#692) Fix duration mismatch between audio and metadata #818

(#692) Fix duration mismatch between audio and metadata #818

demophoon commented Feb 6, 2022 •

edited

Loading

sarayourfriend Feb 7, 2022

demophoon Feb 7, 2022

demophoon commented Feb 7, 2022

sarayourfriend commented Feb 7, 2022 •

edited

Loading

zackkrida commented Feb 18, 2022

zackkrida commented Feb 18, 2022

demophoon commented Feb 18, 2022

demophoon commented Feb 18, 2022

sarayourfriend commented Feb 18, 2022

demophoon commented Feb 18, 2022

sarayourfriend left a comment

zackkrida left a comment

(#692) Fix duration mismatch between audio and metadata #818

(#692) Fix duration mismatch between audio and metadata #818

Conversation

demophoon commented Feb 6, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend Feb 7, 2022

Choose a reason for hiding this comment

demophoon Feb 7, 2022

Choose a reason for hiding this comment

demophoon commented Feb 7, 2022

sarayourfriend commented Feb 7, 2022 • edited Loading

zackkrida commented Feb 18, 2022

zackkrida commented Feb 18, 2022

demophoon commented Feb 18, 2022

demophoon commented Feb 18, 2022

sarayourfriend commented Feb 18, 2022

demophoon commented Feb 18, 2022

sarayourfriend left a comment

Choose a reason for hiding this comment

zackkrida left a comment

Choose a reason for hiding this comment

demophoon commented Feb 6, 2022 •

edited

Loading

sarayourfriend commented Feb 7, 2022 •

edited

Loading