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

Is this expected behavior for setcurrenttime()/play()? #1924

Closed
someonetookmyname opened this issue Nov 8, 2016 · 30 comments
Closed

Is this expected behavior for setcurrenttime()/play()? #1924

someonetookmyname opened this issue Nov 8, 2016 · 30 comments

Comments

@someonetookmyname
Copy link

Hi,

I am using 2.23.2.

If I first create a player wrapping some audio:

       var player = new MediaElementPlayer('#myaudio');

And imagine I would like to start the audio at an offset:

	player.setCurrentTime(10.0);
	player.play();

Under that usage setCurrentTime(10.0) fails and playback starts from 0.0. Results are the same for Ubuntu and iOS.

However, if I instead directly address the underlying audio (after creating the player):

	var aud = document.getElementById("myaudio");
        aud.setCurrentTime(10.0);
        aud.play();

.. then playback starts as desired at 10.0 on both Ubuntu and iOS.

Why does the address-the-audio approach work as expected, but not the address-the-player approach? I was expecting for either both approaches to work, or both to fail.

Is there a drawback to using the address-the-audio approach, even if the audio is wrapped by the player (I'd still like to use the player interface for other things)?

p.s. Based on referenced approaches I've found elsewhere, the following also works as expected:

	var aud = document.getElementById("myaudio");
	aud.one("progress", function() {
    		player.setCurrentTime(10.0);
	});
	player.play();

Thank you for any clarifications.
Best,
Andrew

@someonetookmyname someonetookmyname changed the title Is this expected behavior for setcurrenttime()/play() Is this expected behavior for setcurrenttime()/play()? Nov 8, 2016
@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

@someonetookmyname The player instantiates the whole MediaElement wrapper and setCurrentTime is a function only related to the tag itself. Have tried with player.media.setCurrentTime?

@someonetookmyname
Copy link
Author

Thank you for the quick reply.
player.media.setCurrentTime seems to work as desired so far! (but player.setCurrentTime also works fine except for setCurrentTime only before the first play() (?))
Does a document/description exist for best practices of these handlings? I never saw documentation anywhere with your suggestion to use media.setCurrentTime. That is, which calls should go through player? which calls should go through player.media etc?
Thank you very much,
Andrew

@someonetookmyname
Copy link
Author

Actually, I was mistaken .. your suggestion does not work.

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

I'll check this and also make sure that there's documentation about it

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

For now my focus is on 3.x-dev branch so any fixes will go to that branch given that we will release it soon

@someonetookmyname
Copy link
Author

Ok .. thanks very much for looking into it. I'm happy to work with the 3.x-dev branch. Please let me know if I can be of any help. Thanks again, Andrew

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

Of course If you test your issue I'm that branch and a test in general just to make sure we're solid I will appreciate it

@someonetookmyname
Copy link
Author

Absolutely .. I will put that together and test against the current 3.x-dev and let you know soon ... thanks.

@someonetookmyname
Copy link
Author

Hi,

I just made a discovery .. calling player.media.play() seems to be what determines if things work as expected (see below) .. though, that makes me more confused as to when I'm supposed to use the main wrapper functions etc.

Please shout if any questions about the example or if I can provide further help.

Thanks,

Andrew

Attached is an example .. it behaves the same against 2.23.2 and 3.x-dev.
Put both files from the zip into the demo/ folder of the media elements distribution, and then view startproblem.html (eg. Chrome on OS X 10.10 Desktop).
Since the problem only applies to the first call of play(), make sure to reload the page after each button-press/test.
The page shows 5 buttons below a player instance. Each button will call setCurrentTime() and play() a different way.

  1. calls setCurrentTime and play against the audio element .. works as expected (starts audio at 15-second offset)
  2. calls setCurrentTime and play against the player .. doesn't work as expected (starts audio at 0)
  3. calls setCurrentTime against player.media and play against player .. doesn't work as expected (starts audio at 0)
  4. calls setCurrentTime against player and play against player.media .. works as expected (starts audio at 15-second offset)
  5. calls setCurrentTime and play against player.media .. works as expected (starts audio at 15-second offset)

Archive.zip

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

@someonetookmyname I'll check this right now

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

@someonetookmyname I know why. play using player has a call to load() method. So it will load it first, so that causes to override the expected behavior. If you are using 3.x-dev branch, check mediaelement-and-player.js and comment line 1521. Let me know if that solves it so I can incorporate a fix in the core package.

@someonetookmyname
Copy link
Author

Hi, Thanks for taking a look .. can you give a few lines of context? .. I just downloaded a fresh mediaelement-3-2.x-dev/, but line 1521 in build/mediaelement-and-player.js doesn't seem like what you are mentioning (unless I'm looking in the wrong file?)

Thanks, Andrew

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

My apologies. Line 7369 in that file

@someonetookmyname
Copy link
Author

Hi,
Thanks for clarifying. Yes, that change resolves everything (assuming that what I'm hoping to see if the correct/preferred behavior ... smile). So the most robust way to do what I'm trying to do is via player.setCurrentTime and player.play .. is that right?
Thanks for your help.
Best,
Andrew

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

Exactly. I'll put the fix once I do more testing to make sure it won't affect anything

@someonetookmyname
Copy link
Author

Awesome .. thanks.

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

Can you do me a favor and test on iPhone please? I can't test it right now but if you can help me with that and in an iPad I'd appreciate it

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

I haven't put the fix yet but if you can use your files to test it would be great

@someonetookmyname
Copy link
Author

Hi,
Unfortunately I only have an iPad mini for an iOS device and I only use it to confirm functionality (I don't use it for general development) .. so I'm not even sure how to (or if I can) load the local test case onto the iPad (I was surprised this didn't seem trivial to do). However, when I swap the latest/tweaked mediaplayer (and when I use player.setCurrentTime and player.play) into the original served context where I found the problem, and then view that served web application on the iPad mini, things seem much improved from before .. I haven't yet seen any bad behaviors despite scrubbing the cache between loads etc., so that seems like very good news .. I'm just no expert on iOS devices, unfortunately. I hope this is at least some help (and some good news).
Best,
Andrew

@someonetookmyname
Copy link
Author

I just had 1 test out of ~25 start from 0 instead of the offset .. it was on OS X Desktop. Not sure how much to worry about that.

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

I'm not sure I understand the last comment, but to give you some context of the nature of the piece of code added as a part of the play method that is messing with setCurrentTime: in 2013 a PR was submitted since they stated that there were issues with iPad when playing the video only; so to ensure that it will play properly, the added the load() method inside the play. I haven't seen this as an issue in other places, but you basically tested this on an iPad without issues. So basically we are invalidating the fix since it seems not needed. Let me know if you have any more questions or comments about this before I perform the fix

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

I had an emulator and I tested your example on it and everything went fine BTW

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

If you wanna do one last test, just hit the play button, and try to test including a video and audio on the same page. And then try to set the current time for both to 25. If you don't see any issues I think it's safe to say that solves the issue

@someonetookmyname
Copy link
Author

Hi,

My last comment simply meant that I did ~25 tests trying to get some audio to start-for-the-first-time at sec 15 or so .. 24/25 of those tests worked great and started at 15 .. 1/25 started at 0.

That's great news about the emulator.

I'm certainly no expert on web audio/video .. in fact, I don't envy your job at all .. I'm amazed how finicky it is (and how stable your player has made it) .. and I don't know what is going on under the hood of mediaelement's methods, such that setCurrentTime->play could work fine for a small-ish audio file, but I don't have any feel for if things would start to break if you were searching all the way to the end of a big audio or video file on your way to play'ing (and without much delay) .. and then I also hear/see weirdnesses that seem to crop up in iOS that I don't hit anywhere else, which also makes me nervous.

I don't do much video, but I'll try to put together a few tests like you describe and be back in touch .. smile.

Thanks,

Andrew

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

OK I'll push in a minute the fix so you can test this; I'll do it in such a way that if the current time is 0 before playing I'd allow the loading; otherwise, only the play. I'll let you know when it's updated

rafa8626 pushed a commit that referenced this issue Nov 8, 2016
@rafa8626
Copy link
Contributor

rafa8626 commented Nov 8, 2016

OK it's ready. Download the latest version of 3.x-dev so you can test, please

@someonetookmyname
Copy link
Author

Hi,
I was able to do some video and audio testing, starting at both 0 and ~25. Has worked perfectly!
Thanks for all the help. When do you expect to release version 3?

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 9, 2016

We are currently working on getting a new website and I'm fixing minor things. We hope by the end of November but we'll try to release it as soon as possible.

@rafa8626
Copy link
Contributor

rafa8626 commented Nov 9, 2016

If nothing else I need to do, can you close this ticket, please?

@someonetookmyname
Copy link
Author

Great .. thanks.
Yes .. sorry .. didn't realize I was supposed to close it .. done.

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

No branches or pull requests

2 participants