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

Audio file timestamp doesn't stop counting at the end of playback #21796

Open
luixxiul opened this issue Apr 14, 2022 · 28 comments
Open

Audio file timestamp doesn't stop counting at the end of playback #21796

luixxiul opened this issue Apr 14, 2022 · 28 comments
Labels
A-Media O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect

Comments

@luixxiul
Copy link

luixxiul commented Apr 14, 2022

Steps to reproduce

  1. Create an encrypted new room
  2. Upload you favorite 4-5 minute song
  3. Wait until the song finished by listening to it
  4. Check the player

Outcome

What did you expect?

Either (a) the song would be played again from the start or (b) the counter would stop counting at the end of the song. I am not sure which one is expected.

What happened instead?

The counter was reset to zero and started counting, but the song itself was not replayed.

player.mp4

Also, unless the room is reloaded, the audio file cannot be even replayed on the player.

Operating system

Debian

Browser information

Firefox

URL for webapp

localhost

Application version

develop

Homeserver

No response

Will you send logs?

No

@dbkr dbkr added S-Minor Impairs non-critical functionality or suitable workarounds exist O-Uncommon Most users are unlikely to come across this or unexpected workflow A-Media labels Apr 14, 2022
@dbkr
Copy link
Member

dbkr commented Apr 14, 2022

I suspect the correct behaviour is for the playhead & time to reset to zero and stay there once playback has reached the end (instead of going back to zero and then counting up again which is what it sounds like it happening). Perhaps this could be a good issue for a relatively new person to help with. src/components/views/messages/MAudioBody.tsx / src/components/views/audio_messages/AudioPlayerBase.tsx in react-sdk are probably the files to start with.

@dbkr dbkr added the Help Wanted Extra attention is needed label Apr 14, 2022
@luixxiul
Copy link
Author

I suspect the correct behaviour is for the playhead & time to reset to zero and stay there once playback has reached the end.

Also, the file should be able to be replayed, of course 😉

@robintown robintown changed the title Looped audio file not replayed Audio file timestamp doesn't stop counting at the end of playback Apr 18, 2022
@robintown robintown added the good first issue Good for newcomers label Apr 18, 2022
@htmlHxcker
Copy link

I noticed something else while drafting a PR for this issue. If you let the player keep counting after the song's finished, switch to another room and switch back it works as normal i.e Stoppping at zero and when the play button is clicked, it plays normally so this may be something to look out for.

@htmlHxcker
Copy link

I'd also like for this issue to be assigned to me but given the information at https://github.com/vector-im/element-meta/wiki/Guidance-on-assigning-issues I'll hold that off until I submit my draft PR. Thought I'd mention this here in case anyone else wants to work on this issue.

@luixxiul
Copy link
Author

The issue here is that the counter does not only stop at the end of playback, but also the audio cannot be replayed unless the page is manually reloaded.

@htmlHxcker
Copy link

After digging around for a while, it seems this is a bit above me and I will not be able to continue working on this, anyone who wants to work on it can pick it up.

@chandra1899
Copy link

can anyone please explain the problem clearly, i'm unable to get the issue.

@yoda-76
Copy link

yoda-76 commented Feb 12, 2023

I want to work on it. Please assign me this issue.

@weeman1337
Copy link
Contributor

Thank you for your interest in this issue @yoda-76 . We will normally not assign the issue to an external contributor until they have provided at least a draft PR which is taking the right direction. For further details, see our guidance on assigning issues.

If you have any technical questions about this issue, you can ask for help in #element-dev:matrix.org .

@ABHIXIT2
Copy link

ABHIXIT2 commented Mar 1, 2023

I am working on this issue and what i've noticed that the counter goes to zero but you want it to stay at the end right?!
And also I didn't find any problrm in playing it again.....Could anyone please confirm. @dbkr @luixxiul

@luixxiul
Copy link
Author

luixxiul commented Mar 1, 2023

@chandra1899 I edited the explanation for you so that you can understand the issue.

@luixxiul
Copy link
Author

luixxiul commented Mar 1, 2023

@ABHIXIT2 Do you mean that the issue is no longer reproduced?

I confirmed the issue still could be reproduced on my side.

@ABHIXIT2
Copy link

ABHIXIT2 commented Mar 1, 2023

I think it has something to do with the app because it wasn't happening but after installing app and playing audio there it started happening. I will try to solve the problem and if there anything i need to know just lemme know... @luixxiul

Edit : This is happening for above 5 min clips

@farhan-momin
Copy link

I am unable to find this file in the source code src/components/views/messages/MAudioBody.tsx
Where can i find it?

@farhan-momin
Copy link

farhan-momin commented Mar 17, 2023

@t3chguy I looked at https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/components/views/messages/MAudioBody.tsx. Can a reset Function be added to this react component?
I think this is a bit too complicated for my current level.

@vishalsingh2972
Copy link

would like to work on this, but new so would love to collaborate and work together on this issue if anyone is interested!

@Axxi3
Copy link

Axxi3 commented Jul 18, 2023

is the issue still open or is anyone working in it?

@rashmitpankhania
Copy link

is the issue still open

yes i can still reproduce the same

@rashmitpankhania
Copy link

rashmitpankhania commented Aug 30, 2023

Hi @t3chguy @luixxiul
I found the issue it related to "ended" event
after the audio (size greater than 5mb as mentioned in desc) gets completed the ended event callback doesnt get fired even though the attribute ended says true
I think the issue might be related to HTML5 cause I have tested on firefox, chrome & brave..its there

so a dirty work around would be comparing the currentTime and duration and firing the ended callback
any thoughts?

@Johennes Johennes added the Hacktoberfest Issues which are suitable for Hacktoberfest PRs: https://hacktoberfest.digitalocean.com/ label Oct 24, 2023
@nallapaneni-sreehari
Copy link

Is this issue still there, because I cannot reproduce this, I tried to reproduce in the Chrome, Brave, Firefox but I wasn't able to do so. Can anybody confirm ? Attaching the POC video for reference.

Element._.audio-file-timestamp-21796.-.Brave.2024-01-08.00-31-05.mp4

@rashmitpankhania
Copy link

i am still able to reproduce @nallapaneni-sreehari

www_screencapture_com_2024-1-8_12_45.mp4

@AdityaPrasad275
Copy link

Adding to this issue, I have managed to reproduce it + found a new issue where the play button for different voice note file send after the music file, doesn't work.
I was experimenting with voice note due to Issue #22183 saying voice note playback goes back to start sometimes and sometimes doesn't but I wasnt able to replicate that.
In the following video, I have made the first music file "exhaust" itself so that playback starts again and there's no music playing afterwards. And then I have tried clicking repetedly on the next voice note play button and it doesn't work (sorry the clicks arent visible, but I have clicked a ton)

2024-12-05.22-20-57.mp4

The play button not "clicking" is very random, sometimes works sometimes doesn't. I was able to replicate, all the other properties others have identified about the playback issue

I plan to work on this, I'll propose a draft PR in a bit, hopefully within a day. I am barely a beginner when it comes to this, so any help is greatly appreciated!

@AdityaPrasad275
Copy link

AdityaPrasad275 commented Dec 5, 2024

Testing and trying a bit more I am not sure abt why and when the play button decides to not respond. I dont understand the error of "Error downloading audio" in voice notes. I followed the steps of installing again and again, first i forgot abt config file, the next time I corrected that, the error is still there.
It's not exactly reproducible.

@AdityaPrasad275
Copy link

Lord this is tough, coming from knowing nothing of this audio and playback stuff. I have dug a bit and in Playback.ts I found this if else block in prepare method

// The point where we use an audio element is fairly arbitrary, though we don't want
        // it to be too low. As of writing, voice messages want to show a waveform but audio
        // messages do not. Using an audio element means we can't show a waveform preview, so
        // we try to target the difference between a voice message file and large audio file.
        // Overall, the point of this is to avoid memory-related issues due to storing a massive
        // audio buffer in memory, as that can balloon to far greater than the input buffer's
        // byte length.
        if (this.buf.byteLength > 5 * 1024 * 1024) {
            // 5mb
            logger.log("Audio file too large: processing through <audio /> element");
            this.element = document.createElement("AUDIO") as HTMLAudioElement;
            const deferred = defer<unknown>();
            this.element.onloadeddata = deferred.resolve;
            this.element.onerror = deferred.reject;
            this.element.src = URL.createObjectURL(new Blob([this.buf]));
            await deferred.promise; // make sure the audio element is ready for us
        } else {
        // Safari compat: promise API not supported on this function
        this.audioBuf = await new Promise((resolve, reject) => {
            this.context.decodeAudioData(
            // small file logic from here

and i thought, why not remove the if block and make every file go through the same decoding instead of resorting to <audio> for larger files. Well that worked in the sense that the audio files > 5mb now function as expected with no issues as described above but of course, because of bigger memory, they require larger load times and hence are unusable for a short while when you refresh the page. I don't exactly understand what is wrong with the if block but a bit of chatgpting (i am only slightly better than beginner here),

Why It Might Have Been Problematic
Event Binding Issues:

The logic relies on onloadeddata and onerror events for the <audio> element to ensure it is ready.
If these events don't fire correctly or are not handled robustly, playback could fail.
Mismatch in Behavior:

Large files processed through <audio> likely followed a different playback mechanism compared to smaller files using decodeAudioData. This could cause inconsistent behavior (e.g., no reset, clock issues).
Unnecessary Complexity:

Splitting logic for small and large files added complexity, potentially introducing edge cases where the playback flow diverged unintentionally.
Blob Creation and URL Lifecycle:

The line this.element.src = URL.createObjectURL(new Blob([this.buf])); creates a blob URL for the audio buffer. If this blob URL isn't properly cleaned up or has issues, it might fail to play back as expected.
Incomplete Error Handling:

The fallback logic for large files might not have handled all scenarios where onerror could trigger, leading to silent failures.

and I am not sure how exactly to fix these issues lol

@dbkr
Copy link
Member

dbkr commented Dec 6, 2024

Thanks for looking at this: seems it's not so trivial so I'll take the good first issue etc labels off.

@dbkr dbkr removed Help Wanted Extra attention is needed good first issue Good for newcomers Hacktoberfest Issues which are suitable for Hacktoberfest PRs: https://hacktoberfest.digitalocean.com/ labels Dec 6, 2024
@AdityaPrasad275
Copy link

Hey so I fixed the playback error slightly.
Now the time goes back to zero, the file "resets" and we can play it again, sure. But the seekbar isnt stopping. The seekbar keeps on moving even after end but the time doesn't and it stays at zero. Now if i hit the play button, the time jumps to wherever the seekbar is and then increments. The audio file starts from beginning tho, leading to a synch error between whats playing and whats being indicated by the seekbar and time. Then when the seekbar/time is at end but the file isn't, the seekbar/time resets back to 0 and keeps incrementing. Only when the file actually ends, the time resets back to zero but the seekbar, again doesn't.
Here's the changes I did in src/audio/Playback.ts
From line 157-167

 if (this.buf.byteLength > 5 * 1024 * 1024) {
            // 5mb
            logger.log("Audio file too large: processing through <audio /> element");
            this.element = document.createElement("AUDIO") as HTMLAudioElement;
            const deferred = defer<unknown>();
            this.element.onloadeddata = deferred.resolve;
            this.element.onerror = deferred.reject;
            this.element.src = URL.createObjectURL(new Blob([this.buf]));
+           this.element.onended = this.onPlaybackEnd; 
            await deferred.promise; // make sure the audio element is ready for us
        } else {

From line 216, rewrote playbackend method

 private onPlaybackEnd = async (): Promise<void> => {
+       if (this.element) {
+          this.element.pause();
+           this.element.currentTime = 0; // Reset the audio element
+       } else {
            await this.context.suspend();
+       }
         this.emit(PlaybackState.Stopped);
    };

@AdityaPrasad275
Copy link

eh it still doesnt work idk what i did :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Media O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect
Projects
None yet
Development

No branches or pull requests

17 participants