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

Load peaks from file post v1.2.8 #1067

Closed
X-Raym opened this issue Apr 18, 2017 · 38 comments
Closed

Load peaks from file post v1.2.8 #1067

X-Raym opened this issue Apr 18, 2017 · 38 comments

Comments

@X-Raym
Copy link
Contributor

X-Raym commented Apr 18, 2017

Hi !

With v1.2.8,
To get peaks and create peaks file from it,
I use to use this:

peaks = object.backend.getPeaks(960);

This seems to be broken post v1.2.8 (like v1.4.0).

After quite some testing, I found that the following code produces the same json file than in v1.2.8

peaks = object.backend.getPeaks(960, 0, 959);

Note: I think there should have been something do about this to assure backward compatibility, as I spent hours on it, thinking the problem came from elsewhere.

Well I thought that it was good, but despite the files are now the same as before,
it seems that something have also change in the way they are render !

Here is my code:
wavesurfer.load(file_url, peaks, preload);

and here is what I got with v1.2.8

V1.2.8

And here is the same peaks files, but loaded with v1.4.0 (and v1.3.x)

v1.4.0

Why is there such difference ?
What is the new way to load peaks ?
Could backward compatibility be assured right within wavesurfer-js ?

Thanks for your assistance !

@X-Raym X-Raym changed the title Load peaks from file post v1.3.3 Load peaks from file post v1.2.8 Apr 19, 2017
@X-Raym
Copy link
Contributor Author

X-Raym commented Apr 19, 2017

@katspaugh I did a bit more careful testing, in fact the problem appears from v1.3.0. v1.2.8 works great, but not after that.

@entonbiba
Copy link
Contributor

entonbiba commented Apr 22, 2017

@X-Raym yea the getPeaks method should not break when using just getPeaks(960), I created a pull request to fix the back-end compatibility for this method. #1072

Not sure why the rendering it doing that for you. Can you provide the peaks and audio you're using? Also what are you setting as the preload?

@X-Raym
Copy link
Contributor Author

X-Raym commented Apr 23, 2017

@entonbiba Thanks for the getPeak fix !

For my case:

(of course, I only use the 'peaks' section of my peak file in the wavesurfer load method).

Preload is set to 'metadata'.

Thanks for your support :)

@X-Raym
Copy link
Contributor Author

X-Raym commented Apr 23, 2017

#1055
this issue seems to be related to my problem.

@entonbiba
Copy link
Contributor

@X-Raym can you set the MultiCanvas parameter on your code to true and see the results? I'll create a sample with the files you provided in a few days.

@X-Raym
Copy link
Contributor Author

X-Raym commented Apr 23, 2017

@entonbiba tested
MultiCanvas: true and renderer: 'MultiCanvas' with v1.4.0 without any successful result.

I also tried to remove the code snippet I use for having waveform redrawn when browser size is adjusted, but it wasn't successful either.

for reference, my code for width adjustment is:

	$j(window).resize(function() {
		wavesurfer[i].drawer.containerWidth = wavesurfer[i].drawer.container.clientWidth;
		wavesurfer[i].drawBuffer();
	});

Thanks again for your assistance :)

@entonbiba
Copy link
Contributor

@X-Raym for the resize event do it like so.

var responsiveWave = wavesurfer.util.debounce(function() {
  wavesurfer.empty();
  wavesurfer.drawBuffer();
}, 150);

window.addEventListener('resize', responsiveWave);

here are also two samples to look at that I have already created before,
http://codepen.io/entonbiba/pen/VPqvME (zoom and resize with peaks data)
http://codepen.io/entonbiba/pen/jyKrEz (responsive waveform)

@X-Raym
Copy link
Contributor Author

X-Raym commented Apr 26, 2017

@entonbiba Ok, thanks for the tip, I'll replace my code by yours as it seems more performance friendly.

Unfortunately, it doesn't fixed my peak loading problem. 🐈

Thanks again for the tip anyway :)

@entonbiba
Copy link
Contributor

@X-Raym yes, I noticed the issue happening when the window size is 992px and less. I'll have a look at the peaks code and see why it's doing that.

Here it is with your peaks data.
http://codepen.io/entonbiba/pen/PmpXqJ

@X-Raym
Copy link
Contributor Author

X-Raym commented Apr 29, 2017

@entonbiba
Thank you for you expertise :)
Good luck for the bug hunting !

@X-Raym
Copy link
Contributor Author

X-Raym commented May 5, 2017

@entonbiba Hi !
I think if the width key value is more 960px or less.
According to the fact that there is 1920 values (samples from a .getPeaks(960) )... maybe there is some kind of relation there ?

@Jamby93
Copy link

Jamby93 commented May 8, 2017

@X-Raym There is. The bug rises when you have more peak values than pixel due to erroneous scale. See #1055

@X-Raym
Copy link
Contributor Author

X-Raym commented May 8, 2017

@Jamby93 Nice catch, this is definitely the source of the problem.
I don't really how how it could be fixed (apart maybe restoring v1.2.8 loop points functions, but I guess that if it has been change, it is for a good reason).

@TrevorHinesley
Copy link

Ya this is a major problem in my app right now. Glad to submit a PR, are we against bringing back loop functions?

@X-Raym
Copy link
Contributor Author

X-Raym commented May 14, 2017

@TrevorHinesley I guess it would be better to fix the current method rather than to rollback, cause I guess it has been change to fix other issues.

@entonbiba Do you an idea about how to fix this ? :)

@entonbiba
Copy link
Contributor

@X-Raym yea the pr #1072 should provide backwards compatibility for the getPeaks method. Once it's pushed to cdnjs we can test it with my sample:

http://codepen.io/entonbiba/pen/PmpXqJ

@X-Raym
Copy link
Contributor Author

X-Raym commented May 16, 2017

@entonbiba Cool, can't wait to see that in action ! 👍

@X-Raym
Copy link
Contributor Author

X-Raym commented May 17, 2017

@entonbiba In which version this will be merged ? 1.4.1 or 1.6 ?

@Fiorello
Copy link

Hi, if it helps someone, I found a temporary fix
in drawBuffer function (line 321) the call to drawPeaks pass start and end arguments, instead i use:

// my fix @CUSTOM
this.drawer.drawPeaks(peaks, width, 0, peaks.length - 1);

// actual wavesurfer code
// this.drawer.drawPeaks(peaks, width, start, end);

@X-Raym
Copy link
Contributor Author

X-Raym commented May 25, 2017

@Fiorello Thanks, this can be useful, temporary !

If it was just this simple line of code to fix, I wonder how close it is to fix the actual problem. :)

@entonbiba
Copy link
Contributor

#1072 has already been merged, but cdnjs version is still 1.4 at the moment.

@X-Raym
Copy link
Contributor Author

X-Raym commented Jun 2, 2017

@entonbiba Thanks for the news, I hope it could be put in CDN soon so we can test it 👍

@taylorvc
Copy link

taylorvc commented Jun 2, 2017

Hi, I pulled from the master branch which has #1072 merged, however I am still experiencing this issue with the waveform not fully rendering when loading peaks.

image

@entonbiba
Copy link
Contributor

@taylorvc hmm can you post the code on codepen?

This is the example I created a while back, do you get the same result here?
https://codepen.io/entonbiba/pen/PmpXqJ

@taylorvc
Copy link

taylorvc commented Jun 5, 2017

@entonbiba https://codepen.io/taylorvc/pen/OgPzVG
This is using the wavesurfer.min.js file I pulled from the master branch, also to generate my peaks I used the following command:
audiowaveform -i '.$filepath. ' --pixels-per-second 10 -b 8 -o '.$json

Sometimes the waveform loads appropriately and sometimes it does not... Could this be related to a network connection issue where the last range of peaks are not getting loaded? I am confused why connection would influence the waveform drawing though, since the peaks are in the static defined array.

Here is the waveform not fully loaded:
waveform-error

Here is the waveform loading successfully:
waveform-test

@entonbiba
Copy link
Contributor

@taylorvc yes I now get the same result. When the window is full width of the screen it works correctly. Once it's smaller and you reload, it doesn't add the last part of the waveform. I'll see if I can create a quick pr fix in a couple days.

@entonbiba
Copy link
Contributor

pr created to fix this issue #1114
preview: https://codepen.io/entonbiba/pen/PmpXqJ

@X-Raym
Copy link
Contributor Author

X-Raym commented Jun 6, 2017

@entonbiba Peaks loading seems good, waveform resize seems to also works... I think you nailed it ! Many thanks for that ! Can't wait for the see the new official version with this fix 👍
Cheers !

@taylorvc
Copy link

taylorvc commented Jun 6, 2017

@entonbiba Thanks for the quick response! I tested it with the original file (which is ~3 min long) and it works great with the resize. However, I then tested it with a larger file ~20 min long and it still had the error. I looked at your fix and increased the width value from 9999 to 99999 on my local machine and it was able to render properly. Just thought I'd let you know, it seems to still be an issue if the 'end' value is not large enough for bigger files.

@X-Raym
Copy link
Contributor Author

X-Raym commented Jun 6, 2017

@taylorvc Thanks for debugging !
Maybe 99999 is not enough in certain circumstances... ? Maybe there is a more reliable way (without arbitrary limit) to set this number ?

@entonbiba
Copy link
Contributor

entonbiba commented Jun 6, 2017

@X-Raym @taylorvc changed to width*end which should always be enough to render properly.

@X-Raym
Copy link
Contributor Author

X-Raym commented Jun 6, 2017

@entonbiba ok this will help, some of my users have more than 1hour audio. 🐱 Thanks again for your support !

@entonbiba
Copy link
Contributor

@X-Raym @taylorvc May you guys test it with the files now.
https://codepen.io/entonbiba/pen/PmpXqJ

@taylorvc
Copy link

taylorvc commented Jun 6, 2017

@entonbiba The fix of setting the end value to 'end * length' works great! For rendering the peaks and resizing. Thank you so much for the quick help on this :)

@entonbiba
Copy link
Contributor

closing, fixed once pr #1114 is merged. re-open if issue persists after merge.

@X-Raym
Copy link
Contributor Author

X-Raym commented Jun 11, 2017

@taylorvc Did you see any error with the fix proposed by @agamemnus on the pull request page ? #1114 (comment)

@agamemnus
Copy link
Contributor

agamemnus commented Jun 11, 2017

Ah, so that's what it's about. It seems that @entonbiba's fix does fix that particular issue (and mine doesn't), but it seems like a weird way to solve it...

@taylorvc
Copy link

taylorvc commented Jun 12, 2017

@X-Raym @agamemnus Please refer to my comment on #1114 but the proposed fix end = width - 1 does not work for my use case.

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

7 participants