Bug 138717 - [Media] Timeline scrubber not updating as the video plays
Summary: [Media] Timeline scrubber not updating as the video plays
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords:
Depends on:
Blocks: 138792
  Show dependency treegraph
 
Reported: 2014-11-13 16:28 PST by Dean Jackson
Modified: 2014-11-17 00:00 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.98 KB, patch)
2014-11-13 16:45 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (5.92 KB, patch)
2014-11-14 11:27 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (6.14 KB, patch)
2014-11-14 11:47 PST, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2014-11-13 16:28:07 PST
[Media] Timeline scrubber not updating as the video plays
Comment 1 Dean Jackson 2014-11-13 16:45:21 PST
Created attachment 241519 [details]
Patch
Comment 2 Jer Noble 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.
Comment 3 Dean Jackson 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.
Comment 4 Dean Jackson 2014-11-14 11:27:18 PST
Created attachment 241602 [details]
Patch
Comment 5 Jer Noble 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?
Comment 6 Dean Jackson 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.
Comment 7 Dean Jackson 2014-11-14 11:47:31 PST
Created attachment 241608 [details]
Patch
Comment 8 Jer Noble 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.
Comment 9 Dean Jackson 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.
Comment 10 Jer Noble 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.
Comment 11 Dean Jackson 2014-11-14 13:56:05 PST
Committed r176139: <http://trac.webkit.org/changeset/176139>