Skip to content
This repository has been archived by the owner on Sep 30, 2021. It is now read-only.

finish event fired more than once #242

Closed
phloxic opened this issue Feb 2, 2013 · 15 comments
Closed

finish event fired more than once #242

phloxic opened this issue Feb 2, 2013 · 15 comments
Assignees

Comments

@phloxic
Copy link
Contributor

phloxic commented Feb 2, 2013

Easy to detect:

flowplayer(function (api) {
  api.bind("finish", function () {
    console.log("finish event fired");
  });
});

See: http://flowplayer.org/forum/#!/moot/7204
Can be worked around with a more global var, but still unexpected.

@ghost ghost assigned tipiirai Feb 2, 2013
@tcooc
Copy link

tcooc commented Feb 20, 2013

This is a major issue, since videos are stopping before they finish. Even more so if playlists are involved.
Possible fix: http://stackoverflow.com/questions/2134450/anyone-run-into-this-bug-rtmp-streaming-videos-ending-3-4-secs-too-early

@phloxic
Copy link
Contributor Author

phloxic commented Mar 7, 2013

Workaround in global api setup:

flowplayer(function (api) {
  var finished = false;
  api.bind("resume", function () {
    finished = false;
  }).bind("finish", function (e, api) {
    if (!finished) {
      console.log("finished");
      finished = true;
    }
  });
});

@phloxic
Copy link
Contributor Author

phloxic commented Mar 7, 2013

@nnarhinen
Copy link
Contributor

For html5 the cause is clear.

In lib/engine/html5.js we are mapping the event "ended" from video tag to flowplayer event "finish". However, we are also firing the "finish" event from "progress" event if currentTime reaches duration.

However, the fix isn't obvious.

The "progress" event hack must be there for a reason, so I dont' dare to remove it.

Are there cases where player.video.duration wouldn't be set but the video tag still would emit ended event?

My initial fix would be to just remove the ended -> finish mapping as we are handling this special case already but I really wouldn't want to break anything with this fix..

@nnarhinen
Copy link
Contributor

The currentTime == duration check was introduced by @tipiirai in 06f6b11 in an attempt to fix #121 (reopened).

@tipiirai can you recall the reasoning in there?

@nnarhinen
Copy link
Contributor

@blacktrash if a video tag is looping, should the finish event fire? It seems that atleast in some browsers when video tag loops it doesn't fire the "ended" event (which in flowplayer would get translated to finish event)

So in my tests I could remove the check for current time vs. duration and remove the additional finish event firing IF we don't need finish event firing for looping videos.

@phloxic
Copy link
Contributor Author

phloxic commented Apr 11, 2013

Short answer: No finish event for a looping clip. Simply because that's the truth: the clip does not finish, it loops.

Longer answer/thoughts:

Question:
This looks like the second - or rather the first - finish event something like a beforefinish event? If yes, should we make it public? We certainly want any additional events if at all possible, but if it's there anyway, we could think about it.

Now for the problem:
video tag loop (HTML) vs playlist loop (JS).
Possible practical use case: JS config advance: false, but want to loop single clips. - Currently not possible because loop is not available in JS config for single items - or is the video attr loop inherited for all playlist items?
(Note: if a user shoots herself in the foot by configuring loop for the video tag and advance or loop for the playlist I vote for the playlist config having precedence)

Already:
All playlist items' "finish" ("onbeforefinish"?) must get special treatment if JS config loop: true. With only advance: true, all playlist items except the last.

This is how I have implemented it for the FP Flash JS playlist plugin:https://github.com/flowplayer/flash/blob/master/js-plugins/playlist/flowplayer.playlist.js#L162 ff. - Note that for the FP Flash playlist plugin the default behaviour corresponds to advance: false for plugin backwards compatibility. Note also that for FP Flash you achieve looping of a single clip by returning false in the onBeforeFinish event, so the logic of the approach differs, but the problem, or rather the desired behavior, is the same.

@phloxic
Copy link
Contributor Author

phloxic commented Apr 11, 2013

@nnarhinen - I've implemented "manual" looping using the "first" finish event here: http://demos.flowplayer.org/playlist/scrollable-jquerytools.html - but @bbbo told me that it breaks in IE9, so IE9 needs thorough testing in this context.
@bbbo - can you confirm that the demo still does not work in IE9?

@bbbo
Copy link
Member

bbbo commented Apr 11, 2013

yes, plays first clip only in IE9 (with both letting the playlist go
through or manual selection)

@nnarhinen
Copy link
Contributor

@blacktrash @bbbo http://demos.339f.flowplayer.me/ shouldn't double-fire finish event. Please test.

@phloxic
Copy link
Contributor Author

phloxic commented Apr 11, 2013

@nnarhinen - uggh http://demos.339f.flowplayer.me/bug/274.html stops working

nnarhinen added a commit that referenced this issue Apr 11, 2013
Remove additional finish event triggering fixes #242
@maheshkajaleinfobeans
Copy link

maheshkajaleinfobeans commented Jan 1, 2018

I am using Flowplayer 7.2, using API to get events like pause, next, finish.
When song completes then I expect finish event only but before that pause event gets triggered then finish event.
How to prevent that, please suggest.
Also on pause, I required pause event also in another scenario.

Thanks,
Mahesh

@nnarhinen
Copy link
Contributor

Please ask usage related questions in the forums.

Having the pause event triggered before finish is expected - technically the player pauses when it reaches the end of the video. You need to check api.video.duration on your pause handler to see if the video is reaching end. Or then delay the pause handler so long that api.finished is updated reliably.

@pranabVadakkath
Copy link

flowplayer api resume event called more than once each time when reinitializing the flow player after stopping it.

@bbbo
Copy link
Member

bbbo commented Feb 3, 2020

Flowplayer 7 is deprecated, there won't be any further fixes.

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

No branches or pull requests

7 participants