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

Add Support for Loading Audio Separate From Video #117

Open
dan24678 opened this issue Oct 5, 2020 · 25 comments
Open

Add Support for Loading Audio Separate From Video #117

dan24678 opened this issue Oct 5, 2020 · 25 comments

Comments

@dan24678
Copy link

dan24678 commented Oct 5, 2020

This is probably a very niche request but I need to use videojs-wavesurfer to display wavesurfer where the contents of the waveform are pulled from a different file/URL than is used for the video.

FWIW, I'll briefly explain why I need this. I want to be able to extract audio from a video file of two people speaking and process it to separate it into two channels: Person 1 on the left, and Person 2 on the right. I already have this part working.

I then want to be able to use videojs-wavesurfer to display the original video while showing wavesurfer with the results of the splitChannel audio file. It doesn't matter which version of the audio file is actually played back. I assume it would be the audio from the original non-split video, which is fine. But I need wavesurfer itself to show the splitChannel file.

I know I can do this by editing the original video to splice in the new audio, but I'm hoping to be able to avoid that approach.

I tried to use player.wavesurfer().surfer.load(url) after video.js loaded to swap in a new audio file. This does work but it seems to break the playback link between the video.js and wavesurfer so they are no longer in sync.

I looked at the code and it does seem like it might be a fairly minor change to support this, if you're willing to offer a "url" config option to support this.

It might look something like this where you could check for a "url" property on the wavesurfer config object and use that to override the "src" being used with the video:

node-app_–___src_node-app_ui_node_modules_videojs-wavesurfer_src_js_middleware_js

Note that I think this code should be above the switch case because it might apply to both backend types. In my case, I'm actually using MediaElement because of this bug in wavesurfer.

That being said, I'm happy if you can help provide a resolution to this for either backend type. So if there is some workaround that will work with the current code but not with MediaElement, that would be fine.

Any help or advice you can give would be appreciated. I'm happy to help with testing or a pull request if you can confirm there is no current workaround and that my suggested change would probably work. (Although to test it I'd need to figure out how to build this project)

Thanks,
Dan

@thijstriemstra
Copy link
Member

thijstriemstra commented Oct 5, 2020

I looked at the code and it does seem like it might be a fairly minor change to support this, if you're willing to offer a "url" config option to support this.
It might look something like this where you could check for a "url" property on the wavesurfer config object and use that to override the "src" being used with the video:

Sounds like a good enhancement. Can you maybe check what properties are/should be supported in video.js, besides src and peaks, so we can fix it properly in one go?

@thijstriemstra
Copy link
Member

In my case, I'm actually using MediaElement because of this bug in wavesurfer.

Could you add a one or two sentence summary of the bug in a new comment on that wavesurfer.js ticket? It'll make it a lot easier to digest the issue, the original report is quite large.

@dan24678
Copy link
Author

dan24678 commented Oct 5, 2020

@thijstriemstra -- The Wavesurfer bug is kind of unrelated but I did add a note there.

You're right about probably including peaks at the same time as src. I didn't see any other properties in that file or elsewhere that I'm aware of that should be included.

So maybe something like this above the switch statement?

            let src = this.player.wavesurfer().surfer.params.src || srcObj.src;
            let peaks = this.player.wavesurfer().surfer.params.peaks || srcObj.peaks;

            // if src was passed in through wavesurfer, call load() regardless of the backend
            if (this.player.wavesurfer().surfer.params.src) {
                this.player.wavesurfer().load(src);
                return;
            }

@dan24678
Copy link
Author

dan24678 commented Oct 7, 2020

It just occurred to me that I probably don't need to be using MediaElement here since the audio playback is done through video.js and not Wavesurfer. Since my only concern with the default backend pertains to improper playback of 3+ channel audio files, this isn't an issue when playback is handled by video.js. Thus, I can use the regular backend here and the simpler change would actually be fine for me as well:

            let src = this.player.wavesurfer().surfer.params.src || srcObj.src;
            let peaks = this.player.wavesurfer().surfer.params.peaks || srcObj.peaks;

But that assumes the audio I hear when I play a video comes from video.js and not wavesurfer.

@thijstriemstra
Copy link
Member

since the audio playback is done through video.js and not Wavesurfer.

It isn't, when using webaudio backend.

@thijstriemstra
Copy link
Member

this.player.wavesurfer().surfer.params.src

No, I don't want to use src param again, videojs-wavesurfer 3.x moved away from that.

@dan24678
Copy link
Author

dan24678 commented Oct 7, 2020

Oh. Then, yea, the longer fix would be better for me. But really the 3-channel thing is a minor annoyance. I'd be happy with the shorter fix, using WebAudio, and just dealing with that bug. It's not a deal breaker for me.

Can you just use a different param name? Even something namespaced for this use-case is fine. Really any way to manually inject a different URL to wavesurfer (or update it after the fact without breaking the sync) is all my use case requires.

@thijstriemstra
Copy link
Member

Really any way to manually inject a different URL to wavesurfer (or update it after the fact without breaking the sync) is all my use case requires.

The way to load a file or url is player.src() (see https://github.com/collab-project/videojs-wavesurfer/blob/master/examples/index.html#L61). What is 'breaking the sync'? Is there a bug?

@thijstriemstra
Copy link
Member

Can you just use a different param name? Even something namespaced for this use-case is fine.

A different name for what? A src param? As I said, you should use player.src(). In videojs-wavesurfer 2.x there was src param and videojs's player.src() which was confusing and didn't make much sense, so it was removed in favor of the videojs way of doing things.

@dan24678
Copy link
Author

dan24678 commented Oct 7, 2020

I was using player.load() and not player.src() so let me try that and see if it works. If it does, then I should be all set. I am using 3.x.

@dan24678
Copy link
Author

dan24678 commented Oct 7, 2020

No luck. I think I'm misunderstanding what you're getting at, or there is indeed a bug.

These are the two things I tried.

REPLACING WAVESURFER AUDIO VIA SURFER.LOAD()
code1

Behavior: The audio and video playback buttons get out of sync. Calling surfer.play() triggers the new audio but does not trigger video playback. Clicking the video play button does nothing.

REPLACING AUDIO WITH PLAYER.SRC
code2

Behavior: The video turns all black and won't play. Calling surfer.play() triggers the new audio but does not trigger video playback. Clicking the video play button triggers audio playback but the video does not play.

One thing that I don't think is related but I will mention is that the audio and video files I'm using to test his are different lengths. They won't be in the final product, this is just what I had handy. I can retest with files of identical lengths if you think that might be the cause of the bug.

Am I doing something wrong?

@thijstriemstra
Copy link
Member

can you test replacing different audio with audio, and video with video? we can take a look at audio for video after that.

@thijstriemstra
Copy link
Member

also you shouldn't use timeout, wait until the waveform is ready and then load:

player.on('waveReady', function(event) {
    player.src(/* etc */);
});

@dan24678
Copy link
Author

dan24678 commented Oct 7, 2020

Sure, I'll get the correct audio file that matches the video file in length as well, just so we can not have to worry about that difference. Gimme an hour or two.

@dan24678
Copy link
Author

dan24678 commented Oct 7, 2020

I'll report my tests one at a time. First, trying to swap in new video for existing video...

Result: Works as expected. The new video replaces the old one and all the playback works correctly with video.js and wavesurfer remaining synced.

There is one minor quirk. I think it probably qualifies as a wavesurfer bug, but maybe not. There are two events that I need to track: player.waveReady (the video player is ready) and wavesurfer.ready (per the docs: "When audio is loaded, decoded and the waveform drawn").

Technically, there is a race condition here. Wavesurfer doesn't exist yet before player.waveReady has fired so I cannot set its "ready" handler before starting the loading process. As it turns out, this actually is a problem and wavesurfer.ready never fires because it gets set AFTER the event occurred. It is also interesting to note that the event is not actually waiting for the waveform to be drawn. I'm using a big video file and there is a noticeable delay between when I see in my console output that wavesurfer.ready gets set and when the waveform actually appears. So it seems like the event is actually not linked to when the waveform is visible on the page. So I think maybe the docs are wrong about when the event is fired? Or something else may be going on.

At any rate (and this is interesting) when I swap in a new video with player.src(), the wavesurfer.ready event DOES fire this second time because the race condition does not apply to this second video, only to the first one.

For my purposes, I have an inline fix for the race condition and it is not a concern. Here is my code that I tested with:

node-app_–_WavesurferJs_js

@dan24678
Copy link
Author

dan24678 commented Oct 7, 2020

I finished testing replacing audio with audio via player.src(). It works as expected and has no issues.

BUT it shows the video player with a black pane, which makes sense since there is no video:

VoiceVibes_Evaluate_Recording

This explains the behavior I see when I try to use player.src() to swap in new audio for a video. What it is actually doing is unloading the video and replacing everything with the audio. That's actually the behavior I would expect and want it to do. So I think that approach is probably not valid for what I need to do, which is to get the wavesurfer to show audio that is NOT taken from the video file which is being displayed.

Even when the audio and video are identical length, when I use player.wavesurfer.surfer.load() to swap in a different audio file it does not give satisfactory results. Everything works okay EXCEPT when I click a button bound to wavesurfer.play() it does not play the video. And when I play the video or move the video search bar, it does not sync with wavesurfer. Here you can see the audio and video can be put in different positions after I called player.wavesurfer.surfer.load() and broke the sync:

VoiceVibes_Evaluate_Recording

Is there a way I can manually re-sync these? That would be an acceptable workaround for me. Alternately, being able to pass in "src" to wavesurfer also addresses the issue.

@thijstriemstra
Copy link
Member

thijstriemstra commented Oct 7, 2020

And when I play the video or move the video search bar, it does not sync with wavesurfer.

What wavesurfer backend are you using?

Also, there is a lot to digest here, can you break it down in few bullet points, hard to summarize like this.

You also haven't shared your videojs-wavesurfer config?

@dan24678
Copy link
Author

dan24678 commented Oct 7, 2020

I was using WebAudio. I just retested with MediaElement and the behavior is identical.

@dan24678
Copy link
Author

dan24678 commented Oct 7, 2020

Here is my config:

VoiceVibes_Evaluate_Recording

Forget about all the other issues I mentioned. I don't really care about any of them, except this:

  • Calling "player.wavesurfer.surfer.load()" to update wavesurfer breaks the synchronization between video-js and wavesurfer

Are you not able to replicate this? If that's the case, it might have something to do with me being inside a Vue.js component. I don't think this would matter but maybe it does.

@thijstriemstra
Copy link
Member

thijstriemstra commented Oct 7, 2020

can you make a codepen that reproduces the issue? also, try with a minimal setup, no ws plugins.

Have you tried https://collab-project.github.io/videojs-wavesurfer/demo/video.html btw?

@dan24678
Copy link
Author

dan24678 commented Oct 8, 2020

@thijstriemstra -- Thanks for the suggestion to use your demo page. That was much easier than having to make a code pen.

Here is a simple demonstration of the bug:

  1. Go to https://collab-project.github.io/videojs-wavesurfer/demo/video.html
  2. Paste the following into the dev console: player.wavesurfer().surfer.load('https://voicevibes.s3-us-west-2.amazonaws.com/static/video/downloaded.wav');
  3. Click on the video file slider to change the playback position. Observe that playback position of the wavesurfer waveform does not change.
  4. Paste into dev console: player.wavesurfer().surfer.play();
  5. Observe that the new audio plays but the video does not.

Technically, the new audio is much longer than the old audio it is replacing, but I know from my other testing that even with audio of identical length, the bug remains.

Do you think it is possible to fix this and make the two remain synced?


Also, I hacked away at the compiled JS file and tested the fix I was asking about originally (passing in a custom "src" argument). I had to specify to use the "WebAudio" backend to get it to load it. This has something to do with how that switch statement is constructed. Unfortunately, when I do that, it doesn't load the video. I thought this was because the switch statement didn't call next(null, srcObj); so I added that line in as well.

This gets me really close. The correct video and audio are now showing. But when I click a play button, BOTH audio files start playing.


I think it's still possible to get the behavior I want one way or another, but it doesn't seem as easy as I had initially hoped. I do want/need to get this working though, so any advice or next steps you can provide would be much appreciated.

@thijstriemstra
Copy link
Member

Here is a simple demonstration of the bug:

Thanks for those steps. Can you try the same steps with player.src() and report results? Using the wavesurfer instance directly, e.g. player.wavesurfer().surfer is not encouraged, but for testing/debugging, sure.

@dan24678
Copy link
Author

dan24678 commented Oct 8, 2020

Wavesurfer_Plugin_Video_Example

It throws an error because the player is in video mode and we are trying to load an audio file.

@thijstriemstra
Copy link
Member

@DrLongGhost that last screenshot, player.src() expects an object, try this instead:

player.src({src: 'url_of_your_file', type: 'audio/wav'});

@dan24678
Copy link
Author

Using player.src() to swap in a different audio file results in the Video playback window resetting its duration counter to 00:00/00:00 and no longer playing the video when my wavesurfer play button is clicked, so this isn't a working solution for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants