Closed
Bug 492213
Opened 16 years ago
Closed 15 years ago
seeking in video sometimes causes infinite busy spinner
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: blizzard, Assigned: roc)
References
(Depends on 1 open bug, )
Details
(Keywords: fixed1.9.1)
Attachments
(2 files, 6 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mconnor
:
review+
roc
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
1. Start playing the video and let it buffer for a few minutes.
2. Get distracted by something shiny.
3. Try and back up 30 seconds or a minute.
4. The spinner comes up and doesn't go away while the video plays behind it.
Comment 1•16 years ago
|
||
I can reproduce this on my current trunk build too.
With logging on, I see that the controls get the "seeked" event, and then playback continues with "timeupdate" events. No "canplaythough" event, but that's expected:
434 case "seeked":
435 // Normally we'd expect canplaythough to fire, but if we already
436 // have the data cached it shouldn't fire again.
437 if (this.video.readyState == this.video.HAVE_ENOUGH_DATA)
438 this.startFadeOut(this.statusFader);
Oddly, readyState is 4 (HAVE_ENOUGH_DATA) when this happens, so the throbber should get faded out. Not sure where it goes wrong, yet, but seems to be a bug in the controls.
Assignee: nobody → dolske
Flags: blocking1.9.1?
Comment 2•16 years ago
|
||
Wait, no. I cut'n'pasted the constant into my log message, not .readyState. *sigh*
When the controls get the "seeked" event, .readyState is actually *2* (HAVE_CURRENT_DATA). So I think the backend is wrong here: either it needs to be firing a "canplaythough" event, or the readyState here should actually be HAVE_ENOUGH_DATA. Probably the latter, since we're seeking backwards into cached data.
Dunno if it's related, but the video in the URL (http://people.xiph.org/~tterribe/pubs/lca2008/mel8-171.ogg) seems to stop buffering around the halfway mark (is that us, or the server?). It seems like this bug only occurs after buffering has stopped, but I didn't test it extensively.
Assignee: dolske → nobody
Assignee | ||
Updated•16 years ago
|
Flags: blocking1.9.1? → wanted1.9.1+
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → roc
Assignee | ||
Comment 3•16 years ago
|
||
I'm not seeing this bug.
You will see the buffering stop around the halfway mark, because the cache gets full. The cache's default size is 50MB and the whole video is 118MB.
Assignee | ||
Comment 4•16 years ago
|
||
I've tried setting my cache size to 5MB, but it hasn't helped me reproduce the bug.
// Normally we'd expect canplaythough to fire, but if we already
// have the data cached it shouldn't fire again.
if (this.video.readyState == this.video.HAVE_ENOUGH_DATA)
this.startFadeOut(this.statusFader);
break;
I'm not sure why you're doing this. We could easily be in state HAVE_FUTURE_DATA at this point, and ready to play.
The event-handling code seems a little dangerous in the way it mixes event handling with checks of readyState. One unfortunate issue with the video API is that events are dispatched asynchronously but reading readyState is synchronous, so by the time your event handler runs, the state of the video element may be completely different to what you expect.
I suggest you define exactly what states you want to show the statusFader, for example "this.video.seeking || (this.video.paused ? this.video.readyState < HAVE_CURRENT_DATA : this.video.readyState <= HAVE_CURRENT_DATA)". Then I'd just have the event handlers for the events that are relevant to that predicate all do the same thing: check whether the predicate is true and set up statusFader accordingly. Does that make sense?
Reporter | ||
Comment 5•16 years ago
|
||
I was able to reproduce this in about 15 seconds. Load the URL in the test case, let it start buffering and then just start seeking around backwards and forwards in 30 second chunks. You don't have to go beyond the edge of the media cache or let it finish buffering to see it.
Assignee | ||
Comment 6•16 years ago
|
||
OK, I see it now, thanks. The key is to start new seeks before the existing ones finish.
Assignee | ||
Comment 7•16 years ago
|
||
This seems to fix it. It changes when we display the status fader in some cases, which means it's a little scary, but I'm sure this is overall the right approach.
Attachment #377005 -
Flags: review?(dolske)
Assignee | ||
Comment 8•16 years ago
|
||
(We really need a JS animation API! Between the animated image, the fades, and the video playing, we must be wasting an enormous amount of work with excessive painting. I'm on it!)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 9•16 years ago
|
||
That patch was broken, sorry.
Attachment #377052 -
Flags: review?(dolske)
Updated•16 years ago
|
Attachment #377005 -
Attachment is obsolete: true
Attachment #377005 -
Flags: review?(dolske)
Assignee | ||
Comment 10•16 years ago
|
||
The previous patch displayed the statusFader when video playback ended. This patch fixes that.
Attachment #377052 -
Attachment is obsolete: true
Attachment #377082 -
Flags: review?(dolske)
Attachment #377052 -
Flags: review?(dolske)
Comment 11•16 years ago
|
||
Replying to this comment first, then looking at the suggested change/patch...
(In reply to comment #4)
> // Normally we'd expect canplaythough to fire, but if we already
> // have the data cached it shouldn't fire again.
> if (this.video.readyState == this.video.HAVE_ENOUGH_DATA)
> this.startFadeOut(this.statusFader);
>
> I'm not sure why you're doing this. We could easily be in state
> HAVE_FUTURE_DATA at this point, and ready to play.
When a video is playing and is being seeked to a new position, when does it resume playback? I would have expected it to need to wait until it has buffered enough data to ensure uninterrupted playback (just like |autoplay| does). Otherwise isn't it likely to stutter after the seek because it may immediately exhaust the available data? [I guess 4.8.10.10 in the spec isn't clear on this point.]
So, this code was trying to leave the throbber up after the seek until |canplaythrough| fires (at which point the throbber would be removed), and also checks readyState here lest we already be in the HAVE_ENOUGH_DATA state (in which case |canplaythough| isn't going to fire).
If playback is actually intended to resume in the HAVE_FUTURE_DATA state, I guess we should remove the throbber (and if the video eats through it's buffer, a |waiting| should fire just like any other time that happens). But there's still a backend bug here, because shouldn't |canplaythough| fire eventually? It doesn't seem to, which results in the infinite throbber.
> events are dispatched asynchronously but reading readyState is
> synchronous, so by the time your event handler runs, the state of the video
> element may be completely different to what you expect.
Hrm, yeah, I'll have to keep that in mind. But I don't think that should technically matter for this particular snippet of code -- the sync read of readyState should be enough to determine if canplaythough is going to fire in the future or not.
Assignee | ||
Comment 12•16 years ago
|
||
(In reply to comment #11)
> When a video is playing and is being seeked to a new position, when does it
> resume playback? I would have expected it to need to wait until it has
> buffered enough data to ensure uninterrupted playback (just like |autoplay|
> does). Otherwise isn't it likely to stutter after the seek because it may
> immediately exhaust the available data? [I guess 4.8.10.10 in the spec isn't
> clear on this point.]
We will start playing immediately and only buffer if you actually run out of data.
> So, this code was trying to leave the throbber up after the seek until
> |canplaythrough| fires (at which point the throbber would be removed), and
> also checks readyState here lest we already be in the HAVE_ENOUGH_DATA state
> (in which case |canplaythough| isn't going to fire).
I see.
> If playback is actually intended to resume in the HAVE_FUTURE_DATA state, I
> guess we should remove the throbber (and if the video eats through it's
> buffer, a |waiting| should fire just like any other time that happens). But
> there's still a backend bug here, because shouldn't |canplaythough| fire
> eventually? It doesn't seem to, which results in the infinite throbber.
I suspect we stay in HAVE_ENOUGH_DATA state throughout the seek operation, so canplaythrough is not required to fire again. I think this is allowed by the spec.
Comment 13•16 years ago
|
||
(In reply to comment #12)
> We will start playing immediately and only buffer if you actually run out of
> data.
Ok. So, per IRC discussion, the current videocontrol code is clearly wrong. It shouldn't be waiting for HAVE_ENOUGH_DATA / canplaythough after a seek. The throbber should still be going away (albeit a little too late) because canplaythrough ought to fire eventually, since it's not that's a core bug. I've spun that off to bug 493187.
Comment 14•16 years ago
|
||
Comment on attachment 377082 [details] [diff] [review]
fix v3
>--- a/toolkit/content/widgets/videocontrols.xml
...
>+ setupStatusFader : function() {
Add back support for an "immediate" argument here, and pass it to startFadeIn/startFadeOut. This was being used in 2 spots, to trigger an immediate fadein when in an error state [where startFadeIn(this.statusFader, true) was being used instead of just startFadeIn(this.statusFader)]
We want the error state to be indicated quickly. The throbber, OTOH, uses the delay to help mitigate briefly flashing UI for quick operations.
> this.firstFrameShown = true;
>- else
>- this.startFadeIn(this.statusFader);
>+ this.setupStatusFader();
Might as well move this down to where the statusIcon gets set, since it's a more logical grouping there now.
> if (this.video.error) {
> this.statusIcon.setAttribute("type", "error");
>- this.startFadeIn(this.statusFader, true);
> } else {
> this.statusIcon.setAttribute("type", "throbber");
> }
The braces can go away, since both blocks are 1 line now.
>+ case "canplay":
You'll need to add "canplay" to the list in Utils.videoEvents, otherwise there's nothing listing for it. :)
Unfortunately, I can still get an infinite throbber with this patch. After doing some seeking, it may get into a state where it's showing the throbber while the video's playing. Pause makes it go away, and then it comes back when clicking play.
I get log output like:
[did some seeking, then clicked pause, waited, now clicking play...]
videoctl: Showing status fader: seeking=false error=null readyState=2 paused=false ended=false
videoctl: Delaying status fade-in...
videoctl: Got media event ----> waiting
videoctl: Showing status fader: seeking=false error=null readyState=2 paused=false ended=false
videoctl: Got media event ----> timeupdate
videoctl: Got media event ----> timeupdate
videoctl: Got media event ----> timeupdate
videoctl: Got media event ----> timeupdate
The "showing status fader" state indicated seems odd, because I think those values should result in the throbber *not* being shown. So either something's changing between the if() conditional and the log message, or I'm missing something stupidly obvious because my brain's really tired and I should sleep.
I'm going to go with the latter; will look again in the morning.
Attachment #377082 -
Flags: review?(dolske) → review-
Comment 15•16 years ago
|
||
(In reply to comment #14)
> I'm going to go with the latter; will look again in the morning.
Hooray for sleep!
So, we're not paused and readyState == HAVE_CURRENT_DATA (bug 493187), so the last clause of the ternary operator in setupStatusFader() is true. Though I suppose one could argue the logic is wrong... Playing in the HAVE_CURRENT_DATA state need not cause a throbber; it's likely to immediately run out of buffer (because it's not in HAVE_FUTURE_DATA), but that's ok because then a |waiting| event should fire (and readyState should be HAVE_METADATA), and we'll put up a throbber for that.
Also, one more review comment:
Tthe |pause| event shouldn't call setupStatusFader() (and maybe not |play| either, but I don't think it hurts). The specific case this breaks is when we're throbbing and the user click the pause button, the throbber should remain (until some other event fires, say canplay). This also simplifies the test in setupStatusFader(), it can just look for .readyState < HAVE_CURRENT_DATA without checking the state of .paused.
Assignee | ||
Comment 16•16 years ago
|
||
If setupStatusFader depends on .paused, then we need to run it when that changes, which means hooking "pause" and "play".
When we're buffering, we will be in HAVE_CURRENT_DATA state --- we never go back to HAVE_METADATA. So I think the current logic is correct. So I think bug 493187 is the sole cause of the "infinite spinner" you're seeing and the logic in my patch is correct.
Comment 17•16 years ago
|
||
I'm saying setupStatusFader shouldn't depend on .paused, because we want to keep the spinner going even if the user pauses the video while it's in a throbbing state. So, the check can just be:
if (this.video.seeking || this.video.error ||
(!this.video.ended && this.video.readyState < this.video.HAVE_CURRENT_DATA)
[I guess .ended can actually go away too, then, since the spec says .readyState should be HAVE_CURRENT_DATA when the video ends.]
And, hmm, there's yet another behavioral change from this patch that should be fixed. When the video is initially loading, we want to throb until canplaythrough / HAVE_ENOUGH_DATA. Otherwise it's encouraging the user to click play when the media may quickly stall out.
Assignee | ||
Comment 18•16 years ago
|
||
(In reply to comment #17)
> I'm saying setupStatusFader shouldn't depend on .paused, because we want to
> keep the spinner going even if the user pauses the video while it's in a
> throbbing state. So, the check can just be:
>
> if (this.video.seeking || this.video.error ||
> (!this.video.ended && this.video.readyState < this.video.HAVE_CURRENT_DATA)
This wouldn't display the spinner while we're stopped due to buffering.
> And, hmm, there's yet another behavioral change from this patch that should be
> fixed. When the video is initially loading, we want to throb until
> canplaythrough / HAVE_ENOUGH_DATA. Otherwise it's encouraging the user to click
> play when the media may quickly stall out.
You mean if readyState is HAVE_FUTURE_DATA, we should display the spinner if the video is "initially loading", but not display the spinner if we're already in the middle of playback? I'm not sure how to precisely define "initially loading" here. currentTime == 0 and paused?
What about a video that's not "autobuffer", and hence not downloading at all? Should we display the spinner over it?
Assignee | ||
Comment 19•16 years ago
|
||
I guess it comes down to what the spinner "means".
I think of it as meaning "we're trying to do something, but we can't do it yet." Then if we're paused, all we're trying to do is display the current frame, hence checking readyState < HAVE_CURRENT_DATA. If we're not paused, we're trying to advance to future frames, hence checking readyState < HAVE_FUTURE_DATA.
Assignee | ||
Comment 20•16 years ago
|
||
(In reply to comment #17)
> I'm saying setupStatusFader shouldn't depend on .paused, because we want to
> keep the spinner going even if the user pauses the video while it's in a
> throbbing state.
So I don't really know what you mean by this. Can you be a bit more specific?
Assignee | ||
Comment 21•16 years ago
|
||
Updated for most of the review comments. I'm still not showing the spinner when we're paused and the state is HAVE_CURRENT_DATA, since I'm not yet convinced that makes sense. I have tweaked the condition slightly --- when we've ended, but the state is < HAVE_CURRENT_DATA, we should show the spinner.
Attachment #377082 -
Attachment is obsolete: true
Attachment #377977 -
Flags: review?(dolske)
Comment 22•16 years ago
|
||
(In reply to comment #18)
> You mean if readyState is HAVE_FUTURE_DATA, we should display the spinner if
> the video is "initially loading", but not display the spinner if we're already
> in the middle of playback?
Yes. The two specific examples I was thinking of:
1) Initial load with a non-autoplay video. The throbber should stay until the video buffers enough for uninterrupted playback (HAVE_ENOUGH_DATA / canplaythough). If the throbber goes away before that, it would be taken as a cue to click play now (which is likely to be disappointing if it just quickly stops and starts buffering again).
2) Initial load with a autoplay video. The above is also true, but more importantly the autoplay isn't supposed to begin until that point anyway. It would be odd to have 2 transitions in the UI (throbbing --> not throbbing, no playback --> playback starts) instead of 1 (throbbing -> playing).
Before comment 12 I would have said this is also true after a seek, but clearly if the backend is starting playback earlier there shouldn't be a throbber while playing.
> I'm not sure how to precisely define "initially
> loading" here. currentTime == 0 and paused?
Yeah, I think so, since the current spec doesn't have a way for currentTime to be anything else.
> What about a video that's not "autobuffer", and hence not downloading at all?
> Should we display the spinner over it?
No, I don't think so. That would end up being really distracting, because the throbbing wouldn't ever stop (until the user interacts with it).
So, this means the user may (will?) get a throbber as soon as they click play, which is kind of unfortunate UI... I'll chat with Boriss about it, maybe we need new UI to indicate this state without being as distracting as the throbber. Say, show the statusFader but with a non-animated icon.
(In reply to comment #19)
> I guess it comes down to what the spinner "means".
I think it needs to reflect more than just the technical internal state of the media. When it goes away, it should be a sign to the user that the video is ready for successful playback (ie, it's not going to immediately stop to buffer). Otherwise we're misleading the user about being ready for the task they want to perform (watching a video), and leaving them guessing as to when is actually a good time to click play and not be disappointed.
(In reply to comment #20)
> > I'm saying setupStatusFader shouldn't depend on .paused
>
> So I don't really know what you mean by this. Can you be a bit more specific?
Let me narrow that down... I don't think the transition from playing to paused should result in changing the throbber state. Whatever the video was trying to do when it was playing shouldn't appear to magically finish when pause is clicked. When I tried out the last patch, I recall hitting some busy state where clicking pause/play repeatedly made it go away and come back.
Assignee | ||
Comment 23•16 years ago
|
||
It feels to me like we're trying to use the throbber to mean two different things:
1) "waiting for the user's current action to happen"
2) "you probably don't want to press play yet since we might not be able to play through"
I feel like the throbber makes sense for #1 but for #2 we might be better off using different UI, maybe something associated with the play button. E.g. maybe the play button should be deemphasized somehow, although still enabled, when the state is < HAVE_ENOUGH_DATA and the video is loading.
> If the throbber goes away before that, it would be taken as a cue to click play
> now
Maybe you're right and there is no way to convey the difference to users.
> So, this means the user may (will?) get a throbber as soon as they click play,
> which is kind of unfortunate UI...
If we're paused and in HAVE_FUTURE_DATA state, and showing the throbber, then pressing play will start playback and make the throbber go away, which I think is also unfortunate UI.
Assignee | ||
Comment 24•16 years ago
|
||
If we want the throbber to be displayed in your two examples, I suggest we display it while nothing has played back yet (no timeupdate event has happened after the first one), the video is autobuffer or autoplay, and readyState < HAVE_ENOUGH_DATA. How does that sound?
That would mean the clicking play would magically make the throbber go away in some cases, but I think we'll have to live with behaviour like that unless we refactor the throbber UI into more fine-grained indicators.
Assignee | ||
Comment 25•16 years ago
|
||
Actually, a slightly better idea now that bug 479863 has landed would be to display the throbber while nothing has played back yet, readyState < HAVE_ENOUGH_DATA and the network state is NETWORK_LOADING.
Then if the download is suspended due to the cache being full or the media element not being 'autoplay'/'autobuffer', networkState will change to NETWORK_IDLE and we'll stop showing the throbber. Hiding the throbber when the cache is full means the user could click on 'play' and be disappointed, but if we show it, it's quite likely the cache will stay full and the throbber will never go away, which would be even worse.
When the status changes from LOADING to IDLE we get a 'suspend' event, and when we start receiving data again we'll get a 'progress' event, so we should hook those.
Assignee | ||
Comment 26•16 years ago
|
||
Here's that approach.
Attachment #377991 -
Flags: review?(dolske)
Assignee | ||
Comment 27•16 years ago
|
||
The seenPlaybackActivity flag in that patch was no good. Just counting the number of timeupdates we've seen is simpler and more robust.
Attachment #377991 -
Attachment is obsolete: true
Attachment #378036 -
Flags: review?(dolske)
Attachment #377991 -
Flags: review?(dolske)
Assignee | ||
Comment 28•16 years ago
|
||
I hate this language.
Attachment #378036 -
Attachment is obsolete: true
Attachment #378040 -
Flags: review?(dolske)
Attachment #378036 -
Flags: review?(dolske)
Comment 29•15 years ago
|
||
Comment on attachment 378040 [details] [diff] [review]
fix v7
I'm still not thrilled about having the throbber disappear in some cases when the user clicks pause. But I also think that's a somewhat subtle UI bug, and more serious issues are being fixed here... So rather than grinding away on that point, I'd rather see this land as-is to get it baking, file a followup bug, and consider fixing that one issue later.
>- * Set the initial state of the controls. The binding is normally created along
>+ * Set the initial state of tuihe controls. The binding is normally created along
Really? :)
>+ ++this.timeUpdateCount;
>+ if (this.timeUpdateCount <= 2) {
>+ this.setupStatusFader();
>+ }
Nit: no braces, and post-increment would be nicer to read.
I think it's work adding a brief comment here (or in setupStatusFader) as to what's going on, it's a somewhat indirect leap from counting timeupdate events to knowing if anything played.
Attachment #378040 -
Flags: review?(dolske) → review+
Updated•15 years ago
|
Attachment #377977 -
Flags: review?(dolske)
Comment 30•15 years ago
|
||
Also needs blessing from a toolkit peer... Enn is out for the week, so maybe mconnor?
Assignee | ||
Comment 31•15 years ago
|
||
Attachment #378040 -
Attachment is obsolete: true
Attachment #378286 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #378286 -
Flags: review? → review?(mconnor)
Assignee | ||
Updated•15 years ago
|
Attachment #378286 -
Flags: review+
Comment 32•15 years ago
|
||
Comment on attachment 378286 [details] [diff] [review]
fix v8
this looks good, though it's mostly me deferring to dolske's better domain knowledge here. ship it.
Attachment #378286 -
Flags: review?(mconnor) → review+
Assignee | ||
Comment 33•15 years ago
|
||
I'm not sure if we can ship it now, since the tree is locked down.
Whiteboard: [needs review] → [needs 191 landing]
Comment 34•15 years ago
|
||
Comment on attachment 378286 [details] [diff] [review]
fix v8
a191=beltzner once it lands on trunk and goes green
Attachment #378286 -
Flags: approval1.9.1+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 35•15 years ago
|
||
Assignee | ||
Comment 36•15 years ago
|
||
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
You need to log in
before you can comment on or make changes to this bug.
Description
•