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

New player 1.6.1 breaking seek #528

Open
jseward311 opened this issue Jul 9, 2019 · 9 comments · Fixed by velochy/audio-tempo-changer.js#3
Open

New player 1.6.1 breaking seek #528

jseward311 opened this issue Jul 9, 2019 · 9 comments · Fixed by velochy/audio-tempo-changer.js#3

Comments

@jseward311
Copy link
Contributor

jseward311 commented Jul 9, 2019

After pulling in the new player I am receiving the following error which breaks seek.

TypeError: B[r] is undefined
TypeError: B[t] is undefined

flush
http://localhost:81/Scripts/Lib/ogvjs-uptivity/ogv.js:56:12911
AudioFeeder.prototype.flush
http://localhost:81/Scripts/Lib/ogvjs-uptivity/ogv.js:41:35794
prepForSeek
http://localhost:81/Scripts/Lib/ogvjs-uptivity/ogv.js:18:30985
_seek
http://localhost:81/Scripts/Lib/ogvjs-uptivity/ogv.js:18:31093
setCurrentTime
http://localhost:81/Scripts/Lib/ogvjs-uptivity/ogv.js:18:19523

private.player.currentTime seems to now be causing some issue.

Last release the media source was changed to except a stream of media.
Not sure if this is the reason for the break here or not yet.
Still looking into this.

Second issue that I believe also has to do with Audio Feeder Flush:

Uncaught TypeError: Cannot read property 'out_time' of undefined
at Object.flush (ogv.js:56)
at AudioFeeder.flush (ogv.js:41)

@samfoucart
Copy link

samfoucart commented Jul 9, 2019

When we do
player.currentTime = floatingPointNumber
or
player.fastSeek(floatingPointNumber)

We are getting the error:

TypeError: Cannot read property 'out_time' of undefined
at Object.flush (d:\html5playerparea\webplayerjs\scripts\lib\ogvjs-uptivity\ogv.js:56:12920)
at AudioFeeder.flush (d:\html5playerparea\webplayerjs\scripts\lib\ogvjs-uptivity\ogv.js:41:35814)
at prepForSeek (d:\html5playerparea\webplayerjs\scripts\lib\ogvjs-uptivity\ogv.js:18:31689)
at HTMLUnknownElement._seek (d:\html5playerparea\webplayerjs\scripts\lib\ogvjs-uptivity\ogv.js:18:31782)
at HTMLUnknownElement.setCurrentTime (d:\html5playerparea\webplayerjs\scripts\lib\ogvjs-uptivity\ogv.js:18:19949)
at d:\html5playerparea\webplayerjs\scripts\app\viewmodels\playerviewmodel.js:50:37
at PlayerViewModel.props.set (d:\html5playerparea\webplayerjs\scripts\app\viewmodels\viewmodel.js:28:22)
at HTMLDivElement.private.waveSlider.onMouseDrag (d:\html5playerparea\webplayerjs\webplayer.js:190:50)
at Object.private.mouseSetPosition (d:\html5playerparea\webplayerjs\scripts\app\views\wavesliderview.js:622:25)
at HTMLCanvasElement. (d:\html5playerparea\webplayerjs\scripts\app\views\wavesliderview.js:63:22)

It's referenced here in the ogv.js:
image

I think the issue comes from audio-tempo-changer.js on line 112.
This line says:
while(out_time<=changes[ci].out_time && ci>=0) { changes.pop(); ci--; }

While setting Pause on caught exceptions on, I was able to see that ci = -1 for some reason.

image

I suspect that if we change that line to:
while(ci >= 0 && out_time <= changes[ci].out_time) { changes.pop(); ci--; }
We might be able to fix this with just short circuit evaluation.

@bvibber
Copy link
Owner

bvibber commented Jul 9, 2019

Hmm, I'm not able to reproduce this so far. Do you have a sample file and a reliable way to reproduce it in code or through UI?

For instance this does not present any problems for me, entered as one line in console (using an audio webm file):

q = new OGVPlayer();
q.src = 'https://upload.wikimedia.org/wikipedia/commons/4/41/Zarna.webm';
q.load();
q.currentTime = 75.234;
q.play();

It seeks and plays without error.

@bvibber
Copy link
Owner

bvibber commented Jul 9, 2019

@velochy I haven't been able to repro this issue yet but I think it's caused when the AudioFeeder is initialized inside OGVPlayer but has nothing queued up yet when we flush things out for a seek. Does the suggested fix in above comments sound right to you (changing order to short-circuit the ci >= 0) or should it be slightly different?

@bvibber
Copy link
Owner

bvibber commented Jul 9, 2019

(I think the 0 entry in changes should be left in place, and it should actually be ci > 0 here.)

@samfoucart
Copy link

I just tried modifying the ogv.js file and:
image

and:
image

both fixed our issue.

So in the tempo changer,
ci >= 0 && out_time <= changes[ci].out_time
and
out_time <= changes[ci].out_time && ci > 0
should both work.

@velochy
Copy link
Contributor

velochy commented Jul 10, 2019

@samfoucart can you make it into a PR (and bump the minor version with it)? Im on a holiday without my work laptop, but i should be able to approve and merge the PR and update npm from my phone

@velochy
Copy link
Contributor

velochy commented Jul 12, 2019

@samfoucart Thank you for the fix.
Unfortunately, npm publish can not be done from phone as i hoped
@Brion I gave you access to the npm package. can you maybe publish it?

@andreyrd
Copy link

Can this update get published in 1.6.2 or something? Or will 1.7.x be released soon?

@bvibber
Copy link
Owner

bvibber commented Dec 12, 2019

Sorry, forgot about this -- I have a 1.7 release coming up, just got caught up on some last-minute threading issues.

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

Successfully merging a pull request may close this issue.

5 participants