pause on slide #2041

olypros · 2017-01-30T13:23:11Z

No description provided.

coveralls · 2017-01-30T13:25:21Z

Coverage remained the same at 60.982% when pulling f7337c5 on ricking06:master into e4bc8f8 on johndyer:master.

rafa8626 · 2017-01-30T13:30:27Z

Thanks for the PR; however, you shouldn't work on the build files, but in the src files; please do the modifications ONLY on that folder so it can be reviewed. Thanks

olypros · 2017-01-30T13:36:09Z

Pros:
its smoother when sliding

cons :
you wont be able to see different frames while sliding
( but this can be solved by making thumbnails of video using ffmpeg and custom made plugin(a mejs__layer ) to show those thumbnails while seeking, but its out of the topic )

rafa8626 · 2017-01-30T13:39:08Z

Thanks for the extra notes. Let me know when you have the changes on the src folder and reverted any changes in the build folder so I can check and merge your changes, please

coveralls · 2017-01-30T13:49:10Z

Coverage remained the same at 60.982% when pulling 5dc8978 on ricking06:master into e4bc8f8 on johndyer:master.

olypros · 2017-01-30T13:55:07Z

i have reverted the build and made changes in src/features/progress.js

coveralls · 2017-01-30T13:55:19Z

Coverage remained the same at 60.982% when pulling 71d9818 on ricking06:master into e4bc8f8 on johndyer:master.

rafa8626 · 2017-01-30T13:56:39Z

Perfect. I'll check them in a minute; just for curiosity, which OS and browsers did you test? We are trying to get the experience the same as possible for mobile/desktop/tablet

olypros · 2017-01-30T15:01:48Z

chrome win/ android chrome. it should work in all browsers.

rafa8626 · 2017-01-30T15:19:16Z

Perfect. I'll check it today

rafa8626 · 2017-01-31T20:53:40Z

OK I have reviewed it and I think it's good to go; the only comment I can make is (after recoding some pieces to fit into ES6) is: would you mind if I set media.setCurrentTime(t.newTime) in the handleMouseMove method?

if (mouseIsDown && t.newTime.toFixed(4) !== media.currentTime.toFixed(4)) {
	media.setCurrentTime(t.newTime);
	t.setCurrentRailHandle(t.newTime);
	t.updateCurrent(t.newTime); 
}

Reason being is because the behavior sliding of the progress handle the way it was is the standard across many other players. Other than that I'm OK with the changes. Let me know ASAP to merge this. You don't need to do the change I can do it, I just need your permission to add this.

rafa8626 · 2017-01-31T22:10:41Z

src/js/features/progress.js

@@ -11,7 +11,20 @@ import {secondsToTimeCode} from '../utils/time';
 *
 * This feature creates a progress bar with a slider in the control bar, and updates it based on native events.
 */
-
+	function setCurrentRailMain(t,fakeTime){ 


This should be inside the Object.assign(MediaElementPlayer.prototype like the setCurrentRailHandle method

yeah it should be inside

rafa8626 · 2017-01-31T22:11:23Z

src/js/features/progress.js

+			if (t.total && t.handle) {
+				var newWidth = Math.round(t.total.width() * nTime / t.media.duration),
+				    handlePos = newWidth - Math.round(t.handle.outerWidth(true) / 2); 
+				newWidth = fakeTime / t.media.duration * 100;


It should be nTime instead of fakeTime; otherwise, the rail never gets updated

yeah thats right i missed it while editing github repos.

rafa8626 · 2017-01-31T22:12:11Z

src/js/features/progress.js

@@ -312,7 +346,9 @@ Object.assign(MediaElementPlayer.prototype, {
 		media.addEventListener('timeupdate', (e) => {
 			if (media.duration !== Infinity ) {
 				player.setProgressRail(e);
-				player.setCurrentRail(e);
+				if(!t.forcedHandlePause){


Is this conditional necessary? If it is, make sure you do the same in the progress listener

yeah it is necesary when media.setCurrentTime(t.newTime); is not added to handleMouseMove function

so if you add this option t.option.lowMemoryOptimize then condition should change to

if(!t.forcedHandlePause || t.option.lowMemoryOptimize){
...
}

Same for progress event

.

See below for t.option.lowMemoryOptimize

rafa8626 · 2017-01-31T22:12:37Z

src/js/features/progress.js

-	}
+	setCurrentRailHandle: function setCurrentRailHandle(fakeTime) {
+		var t = this;
+		setCurrentRailMain(t,fakeTime);  


When you move method, change this to t. setCurrentRailMain

rafa8626 · 2017-01-31T22:12:50Z

src/js/features/progress.js

+	},
+	setCurrentRail: function setCurrentRail() {
+		var t = this;
+		setCurrentRailMain(t); 


When you move method, change this to t.setCurrentRailMain

rafa8626 · 2017-01-31T22:18:20Z

I can fix all the issues I found for you if it's your desire. Just let me know

rafa8626 · 2017-02-01T01:52:21Z

src/js/features/progress.js

-					}
+					// fake seek to where the mouse is 
+					if (mouseIsDown && t.newTime.toFixed(4) !== media.currentTime.toFixed(4)) { 
+						t.setCurrentRailHandle(t.newTime);


Add media.setCurrentTime(t.newTime); to allow a better behavior

No is should not be added , thats the whole point of doing the changes.

In average/low memory pc when the player is added along with huge amounts of UI elements there is a (lag/delay OR no response) of t.current/t.handle when calling handleMouseMove because of the media.setCurrentTime(t.newTime); because mousemove is expensive event.

If you look at famous players like youtube/vimeo etc they dont setTIme of video on mousemove rather on mouseup.

Maybe you can add a t.option.lowMemoryOptimize

and do

if(!t.option.lowMemoryOptimize){
media.setCurrentTime(t.newTime);
}

on handleMouseMove

I see. Well I'll add that option, fix your changes and merge into master. Thanks

rafa8626 · 2017-02-01T19:40:18Z

Thanks for this PR. Merged now with fixes and I decided not to include the option, since I saw that Vimeo and YouTube actually handle that behavior in the way you described it. I based this mostly on other open source players like VideoJS.

olypros · 2017-02-01T21:20:05Z

yeah thats cool

olypros added 9 commits January 29, 2017 23:53

added forcedHandlePause

2130999

Update mediaelement-and-player.js

2369610

updated sliding lag fix

b92695e

Update mediaelement-and-player.js

e5b8914

Update mediaelement-and-player.js

c8e661a

Update mediaelement-and-player.js

d11d2e2

Update mediaelement-and-player.js

44062c7

Update mediaelement-and-player.js

dedfb26

Update mediaelement-and-player.js

f7337c5

Update progress.js

5dc8978

Update mediaelement-and-player.js

71d9818

rafa8626 reviewed Jan 31, 2017

View reviewed changes

rafa8626 reviewed Feb 1, 2017

View reviewed changes

rafa8626 merged commit 71d9818 into mediaelement:master Feb 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pause on slide #2041

pause on slide #2041

olypros commented Jan 30, 2017

coveralls commented Jan 30, 2017

rafa8626 commented Jan 30, 2017

olypros commented Jan 30, 2017

rafa8626 commented Jan 30, 2017

coveralls commented Jan 30, 2017

olypros commented Jan 30, 2017

coveralls commented Jan 30, 2017

rafa8626 commented Jan 30, 2017

olypros commented Jan 30, 2017 •

edited

Loading

rafa8626 commented Jan 30, 2017

rafa8626 commented Jan 31, 2017 •

edited

Loading

rafa8626 Jan 31, 2017

olypros Feb 1, 2017

rafa8626 Jan 31, 2017

olypros Feb 1, 2017

rafa8626 Jan 31, 2017

olypros Feb 1, 2017

olypros Feb 1, 2017

rafa8626 Jan 31, 2017

rafa8626 Jan 31, 2017

rafa8626 commented Jan 31, 2017

rafa8626 Feb 1, 2017

olypros Feb 1, 2017 •

edited

Loading

rafa8626 Feb 1, 2017

rafa8626 commented Feb 1, 2017

olypros commented Feb 1, 2017

pause on slide #2041

pause on slide #2041

Conversation

olypros commented Jan 30, 2017

coveralls commented Jan 30, 2017

rafa8626 commented Jan 30, 2017

olypros commented Jan 30, 2017

rafa8626 commented Jan 30, 2017

coveralls commented Jan 30, 2017

olypros commented Jan 30, 2017

coveralls commented Jan 30, 2017

rafa8626 commented Jan 30, 2017

olypros commented Jan 30, 2017 • edited Loading

rafa8626 commented Jan 30, 2017

rafa8626 commented Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafa8626 commented Jan 31, 2017

Choose a reason for hiding this comment

olypros Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafa8626 commented Feb 1, 2017

olypros commented Feb 1, 2017

olypros commented Jan 30, 2017 •

edited

Loading

rafa8626 commented Jan 31, 2017 •

edited

Loading

olypros Feb 1, 2017 •

edited

Loading