RESOLVED FIXED 138717
[Media] Timeline scrubber not updating as the video plays
https://bugs.webkit.org/show_bug.cgi?id=138717
Summary [Media] Timeline scrubber not updating as the video plays
Dean Jackson
Reported 2014-11-13 16:28:07 PST
[Media] Timeline scrubber not updating as the video plays
Attachments
Patch (4.98 KB, patch)
2014-11-13 16:45 PST, Dean Jackson
no flags
Patch (5.92 KB, patch)
2014-11-14 11:27 PST, Dean Jackson
no flags
Patch (6.14 KB, patch)
2014-11-14 11:47 PST, Dean Jackson
no flags
Dean Jackson
Comment 1 2014-11-13 16:45:21 PST
Jer Noble
Comment 2 2014-11-13 19:48:30 PST
Dino, I don't think this is the right fix. The problem is that the controls aren't updating when they go from hidden to showing; instead, all the deferred updates need to be performed at that time.
Dean Jackson
Comment 3 2014-11-14 03:13:43 PST
(In reply to comment #2) > Dino, I don't think this is the right fix. The problem is that the controls > aren't updating when they go from hidden to showing; instead, all the > deferred updates need to be performed at that time. The problem is that there are five states: - hidden - show - paused - :hover - no CSS class (which is kind-of a hidden, but not with display none) (Note: this is fairly confusing, not only because the tense changes in the state names :), but because the hover state is not able to be queried from script easily) I think we can simplify this into a single "visible" state (although I'd probably call it "hidden"). I'm not sure we need the existing "hidden" - the display: none doesn't buy us much if we're smart about not updating the controls all the time. You're right that the controls don't update when they become visible. I think we should add that in showControls(), although I think it would need to go in the deferred call to timeline metrics update (or that should also update the controls). The reason this current patch passes the test is that it gets an event from the media which triggers the scrubber update.
Dean Jackson
Comment 4 2014-11-14 11:27:18 PST
Jer Noble
Comment 5 2014-11-14 11:35:04 PST
Comment on attachment 241602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241602&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1038 > + this.updateTime(); You need to call this.updateProgress() here too. > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1053 > - return !this.isAudio() && (!this.controls.panel.classList.contains(this.ClassNames.show) || this.controls.panel.classList.contains(this.ClassNames.hidden)); > + return !this.isAudio() && this.controls.panel.classList.contains(this.ClassNames.hidden); With this change, the controls will continue to update when they are hidden due to inactivity during playback. Is this what you want?
Dean Jackson
Comment 6 2014-11-14 11:46:42 PST
(In reply to comment #5) > Comment on attachment 241602 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241602&action=review > > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1038 > > + this.updateTime(); > > You need to call this.updateProgress() here too. Yeah. > > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1053 > > - return !this.isAudio() && (!this.controls.panel.classList.contains(this.ClassNames.show) || this.controls.panel.classList.contains(this.ClassNames.hidden)); > > + return !this.isAudio() && this.controls.panel.classList.contains(this.ClassNames.hidden); > > With this change, the controls will continue to update when they are hidden > due to inactivity during playback. Is this what you want? That's probably ok, since the updates would be fairly rare. e.g. we're not getting the playback tick events.
Dean Jackson
Comment 7 2014-11-14 11:47:31 PST
Jer Noble
Comment 8 2014-11-14 12:12:06 PST
(In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 241602 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=241602&action=review > > > > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1038 > > > + this.updateTime(); > > > > You need to call this.updateProgress() here too. > > Yeah. > > > > > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1053 > > > - return !this.isAudio() && (!this.controls.panel.classList.contains(this.ClassNames.show) || this.controls.panel.classList.contains(this.ClassNames.hidden)); > > > + return !this.isAudio() && this.controls.panel.classList.contains(this.ClassNames.hidden); > > > > With this change, the controls will continue to update when they are hidden > > due to inactivity during playback. Is this what you want? > > That's probably ok, since the updates would be fairly rare. e.g. we're not > getting the playback tick events. No, the opposite. The controls are hidden during playback, so you'd be getting many, many playback tick events.
Dean Jackson
Comment 9 2014-11-14 12:58:23 PST
(In reply to comment #8) > > > With this change, the controls will continue to update when they are hidden > > > due to inactivity during playback. Is this what you want? > > > > That's probably ok, since the updates would be fairly rare. e.g. we're not > > getting the playback tick events. > > No, the opposite. The controls are hidden during playback, so you'd be > getting many, many playback tick events. Sorry, I misunderstood your comment. I'm pretty sure they don't, for a couple of reasons: - I turned on repaint counters and didn't see painting while the controls were hidden - As we transition to the hidden state, the opacity transition causes handlePanelTransitionEnd, which adds the hidden class. controlsAreHidden() tests for hidden. So we'll update as we are fading out (which would be correct) but not once we've actually disappeared.
Jer Noble
Comment 10 2014-11-14 13:07:56 PST
Comment on attachment 241608 [details] Patch (In reply to comment #9) > (In reply to comment #8) > > > > > With this change, the controls will continue to update when they are hidden > > > > due to inactivity during playback. Is this what you want? > > > > > > That's probably ok, since the updates would be fairly rare. e.g. we're not > > > getting the playback tick events. > > > > No, the opposite. The controls are hidden during playback, so you'd be > > getting many, many playback tick events. > > Sorry, I misunderstood your comment. > > I'm pretty sure they don't, for a couple of reasons: > > - I turned on repaint counters and didn't see painting while the controls > were hidden > > - As we transition to the hidden state, the opacity transition causes > handlePanelTransitionEnd, which adds the hidden class. controlsAreHidden() > tests for hidden. > > So we'll update as we are fading out (which would be correct) but not once > we've actually disappeared. Okay, r=me.
Dean Jackson
Comment 11 2014-11-14 13:56:05 PST
Note You need to log in before you can comment on or make changes to this bug.