jaruba / wcjs-player

Node Player made for WebChimera.js (libVLC wrapper)
http://webchimera.org/
GNU Lesser General Public License v2.1
178 stars 46 forks source link

Player does not emit playing event after buffering event #75

Open dragantl opened 7 years ago

dragantl commented 7 years ago

I get the following sequence of events:

However, I would expect playing to be emitted after the last buffering event, assuming that the two events are reflections of waiting (http://www.w3schools.com/tags/av_event_waiting.asp) and playing (http://www.w3schools.com/tags/av_event_playing.asp) events of HTML5 video.

I remember we had this conversation a while ago. It was mentioned that VLC lib emits a huge number of events and they are hard to sync/report. However, wcjs-player already tracks and syncs end of buffering event. That is seen by it being able to remove the Buffering % UI element. What needs to happen is it should also emit playing event at that time (I do not think buffering state can be entered while in paused state).

dragantl commented 7 years ago

Looks like this issue can be resolved by addition of the following snippet to onBuffering function:

if (event == 100) {
    vlcs[i].vlc.onPlaying(i);
}

With the final result:

vlcs[newid].vlc.onBuffering = function(i) {
    return function(event) {
        if (vlcs[i].lastState != "buffering") {
            vlcs[i].lastState = "buffering";
            vlcs[i].events.emit('StateChanged','buffering');
            vlcs[i].events.emit('StateChangedInt',2);
        }
        isBuffering.call(players[i],event);
        if (event == 100) {
            vlcs[i].vlc.onPlaying(i);
        }
    }
}(newid);

If you guys are too busy, maybe you could review the change and I will make a PR?

jaruba commented 7 years ago

I'm trying to remember the solution I recommended before.. But it wasn't in github issues, it was on gitter... and a long time ago..

dragantl commented 7 years ago

Why doesn't the above work? I've applied it to my local wcjs-player package and things seem to be working fine. The code also seems to handle things properly.

jaruba commented 7 years ago

I'm not saying it doesn't work, just that I can't predict all the possible implications of it. For example, keep in mind that the 'playing' event is not the one that's wrong here. The 'buffering' event is wrong. Because in order to prevent what's commonly known as event flood, the 'buffering' events are being queued one after the other. So 'playing' is sent properly, while 'buffering' is being pushed further into the timeline. What you did is correct in fixing it for your requirements, and the perfect / simplest way of avoiding a similar issue for others. But in reality, the 'playing' event should count as 'buffering 100', not the other way around. :)

jaruba commented 7 years ago

Besides, it would require removing the actual 'playing' event for a fake one. If (for whatever reason) there is no 'buffering 100' on a video, which might happen due to a different libVLC version being used and some silly video type / loading circumstances, etc. Then there would be no more 'playing' event at all with this code.. While with the reverse logic, overwriting 'buffering 100' (as absurd as it may be to code) can't create any such issues.

jaruba commented 7 years ago

I'm OK with this solution being added as a parameter to '.addPlayer()' something like 'appendPlaying: true', but definitely not as a permanent solution as it doesn't fix the actual problem.