search

polimediaupv / paella-core

Paella Player core library
Educational Community License v2.0
20 stars 15 forks source link

Extra protection for setTime for end and start #362

Closed karendolan closed 4 months ago

karendolan commented 4 months ago

This pull helps protected on mutli-video with slightly different end times. Example: main video is slightly shorter than second, the seek to end on the main causes issues with the second going over the duration.

miesgre commented 4 months ago

What is approximately the time difference between the videos? I want to generate some videos to test it.

ferserc1 commented 4 months ago

With this solution the problem is only fixed for video formats derived from mp4. I just uploaded a commit that fixes this problem in the StreamProvider class, which makes it fixed in any video format plugin:

https://github.com/polimediaupv/paella-core/commit/1982049f0a09449d2ef917588d6169dccbf7c695

karendolan commented 4 months ago

With this solution the problem is only fixed for video formats derived from mp4. I just uploaded a commit that fixes this problem in the StreamProvider class, which makes it fixed in any video format plugin:

1982049

@ferserc1 Where in the code is there protection when the seek request is beyond the end of the video? How does onVideoEnded protect against that?

ferserc1 commented 4 months ago

In previous versions the length of the video was set by the length of the stream that had the mainAudio role, because it was assumed that the streams would have a very similar length. From this commit, the duration of the video is now set by the shortest stream, making it impossible to require an instant of time beyond the duration of the shortest stream.

I haven't seen how to implement a better solution: if video playback is allowed to continue when one of the streams has already finished, we will have unpredictable behaviour between browsers, even between different versions of the same browser. This also affects the synchronisation mechanisms between the videos and the trimming system, so the only safe option is to ensure that the total duration is that of the shorter stream.

karendolan commented 4 months ago

Ok, I read to the end of the pull and see that the Math.min gets duration to be the min length video. For our site, I will verify that all calls to setCurrent() time go through the protection (not shown in the pull) that ensures the seek is truncated to the video duration (now of the shortest video).

ferserc1 commented 4 months ago

When setCurrentTime is called, a video bounds check is done using the duration() function that is modified in this commit, so by code the only way to go outside the video bounds would be to call one of the video containers directly (which should never be done).

https://github.com/polimediaupv/paella-core/blob/19f5200b9732656ebfa934de1b4bcff56705e1d9/src/js/core/StreamProvider.js#L280-L287