Per spec, progress events are fired in two cases:

1. "While the load is not suspended (see below), every 350ms (±200ms) or for every byte received, whichever is least frequent, queue a task to fire a simple event named progress at the element."

2. "Once the entire media resource has been fetched (but potentially before any of it has been decoded): Fire a simple event named progress at the media element."

Case 2 is one of very few cases where an event is fired outside of a task for media elements. This is not great, because it means that for a fast resource (e.g. a data: URL) that progress event could be fired even before the loadstart event. This is probably not intentional.

Suggest queuing a task to fire the last progress event and then update networkState, which should both make the event order and networkState during the event deterministic.

This issue was noted while looking at a flaky test for networkState during the progress event: https://code.google.com/p/chromium/issues/detail?id=306249#c4

It's not outside a task, it's on the networking task queued by the 'fetch' algorithm. Many events are fired from those tasks, actually, like addtrack, error, suspend, etc.

That the 'loadstart' event happens on a different task source is an interesting point, though. Maybe I should move 'loadstart' to the network task source? Maybe the 'progress' one as well? Or maybe we should just move all the networking tasks queued for a media element to the media element's task source. That's probably the saner solution.

I think the right way to fix this is to have the tasks queued by the 'fetch' algorithm immediately just queue up a new task on the media element's task source to do all the work that they're currently doing. It's ugly in the spec, but the result is to put all the events into a defined order, which seems desireable.

If you agree with the change done here, and think we should also do it for the text track fetching, then please reopen this bug to have me do that.

If you disagree with the change done here, please reopen the bug so I can revert it and change it in a different way (what way?).

Checked in as WHATWG revision r8496.
Check-in comment: Clarify the order of events for media elements
http://html5.org/tools/web-apps-tracker?from=8495&to=8496

I agree with the change. I think that any tasks that are queued in HTMLMediaElement-related algorithms should use the media element event task source and that events fired at audio/video/source/track elements should only be fired from such tasks. Here are the cases I can see where that's currently not true:

In "Failed with attribute" and "Failed with elements" the DOM manipulation task source is used. That means the pending error event won't be canceled by load(), which is bad.

"Queue a task to fire a simple event named suspend at the element, using the DOM manipulation task source."

"When a media element's download has been suspended, the user agent must queue a task, using the DOM manipulation task source, to set the networkState to NETWORK_IDLE and fire a simple event named suspend at the element."

I'm actually not able to find the bits that need updating for track fetching, but I trust you know what you're talking about.

Finally, it's a bit odd to have "queue a task" in "For video elements, set the videoWidth and videoHeight attributes, and queue a task to fire a simple event named resize at the media element." now that those steps are already running in a task. Not sure how to fix that, maybe revert your recent change and queue a task for each step individually instead?

(In reply to Philip Jägenstedt from comment #6)
> In "Failed with attribute" and "Failed with elements" the DOM manipulation
> task source is used.

At least this one I think is to ensure that the error event on video/source is fired before window.onload. There are tests that assume that is the case.

> That means the pending error event won't be canceled by
> load(), which is bad.

I agree that is bad. Any ideas?

Oh wait, the load algorithm already says "If there are any tasks that were queued by the resource selection algorithm (including the algorithms that it itself invokes) for this same media element from the DOM manipulation task source in one of the task queues, then remove those tasks." so the problem doesn't exist.

However, it sure would be nicer to implement this if all tasks that can be cancelled are on the same task source...

As long as clearing the delaying-the-load-event is done in the same task as the error event is fired or in a later task on the same task source, there won't be any race condition. For both "Failed with attribute" and "Failed with elements", it seems to me that just using the media media task source and not clearing the flag until the error event has fired would be fine.

If I understand task sources correctly, there's currently actually no guarantee that loadstart will fire before error, since they're on different task sources? That seems very broken if true.

I should just do the same for the events that use the DOM manipulation task source as I did for the events that use the fetching task source, but in reverse: queue a task on the media element one, which just queues a task on the DOM one. (We already wait for the DOM one to be run before continuing, so the reverse problem doesn't exist.) Does that make sense?

Queueing and task to queue a task is rather strange, is that really necessary?

For "Failed with attribute" I don't understand why we need to involve the DOM manipulation task source at all. Why not just queue a task to do those substeps on the media element task source and then drop "queue a task" for clearing delaying-the-load-event flag?

For "Failed with elements" can we just "Queue a task to fire a simple event named error at the candidate element."? That task is guaranteed to be run before task that is queued after a stable state to clear the delaying-the-load-event flag, so I can't see a race condition.

Checked in as WHATWG revision r8514.
Check-in comment: Simplify task ordering in media elements to aid implementations.
http://html5.org/tools/web-apps-tracker?from=8513&to=8514

Ok in the "Failed with attribute" case queueing a task from the task was just an error I introduced in January (bug 24353). That's fixed. Using the DOM manipulation task source in theory was to avoid races between 'load' and 'error', but it turns out at some point I over-fixed this and the DOM manipulation task source is no longer needed as far as I can tell (bug 18570). Fixed that too.

In the "Failed with element" case, you're right that since we now stop delaying the load event using the media element task source, if we fire 'error' using the media element task source immediately before, the 'load' event can't fire before 'error'. So that doesn't need the DOM manipulation task source either now. Fixed.

There's a couple other events that used the DOM manipulation task source, 'suspend'. I've changed those too. It seems to me that other changes since have fixed the event ordering problems for those in a similar way.

MediaControllers and track lists still use the DOM source. Not sure what to do with those exactly. File a separate bug if you want that changed too (I didn't look into it very far).

I haven't yet done anything for these two comments, so leaving this open:

(In reply to Philip Jägenstedt from comment #6)
> 
> I'm actually not able to find the bits that need updating for track
> fetching, but I trust you know what you're talking about.
> 
> Finally, it's a bit odd to have "queue a task" in "For video elements, set
> the videoWidth and videoHeight attributes, and queue a task to fire a simple
> event named resize at the media element." now that those steps are already
> running in a task. Not sure how to fix that, maybe revert your recent change
> and queue a task for each step individually instead?

(In reply to Ian 'Hixie' Hickson from comment #13)
> Ok in the "Failed with attribute" case queueing a task from the task was
> just an error I introduced in January (bug 24353). That's fixed. Using the
> DOM manipulation task source in theory was to avoid races between 'load' and
> 'error', but it turns out at some point I over-fixed this and the DOM
> manipulation task source is no longer needed as far as I can tell (bug
> 18570). Fixed that too.
> 
> In the "Failed with element" case, you're right that since we now stop
> delaying the load event using the media element task source, if we fire
> 'error' using the media element task source immediately before, the 'load'
> event can't fire before 'error'. So that doesn't need the DOM manipulation
> task source either now. Fixed.
> 
> There's a couple other events that used the DOM manipulation task source,
> 'suspend'. I've changed those too. It seems to me that other changes since
> have fixed the event ordering problems for those in a similar way.

I've reviewed these changes and they look good.

> MediaControllers and track lists still use the DOM source. Not sure what to
> do with those exactly. File a separate bug if you want that changed too (I
> didn't look into it very far).

On second thought, maybe it isn't a problem. I'll file a bug if I find it to be problematic in test cases or implementation.

Going back to that flaky test case, it seems like the spec'd behavior for progress events still doesn't guarantee that networkState is NETWORK_LOADING during the progress event, because even though that's guaranteed for the last progress event fired at end of load, one queued to be fired while loading could still arrive after that point. Is that intentional?

Maybe "While the load is not suspended (see below), every 350ms (±200ms) or for every byte received, whichever is least frequent, queue a task to fire a simple event named progress at the element." could be rephrased as a step of "The networking task source tasks to process the data as it is being fetched must each immediately queue a task to run the first appropriate steps from the following list."?

(In reply to Philip Jägenstedt from comment #6)
> 
> Finally, it's a bit odd to have "queue a task" in "For video elements, set
> the videoWidth and videoHeight attributes, and queue a task to fire a simple
> event named resize at the media element." now that those steps are already
> running in a task. Not sure how to fix that, maybe revert your recent change
> and queue a task for each step individually instead?

I don't see why it's weird for 'resize', but not for 'durationchange' in the immediately previous step and 'loadedmetadata' in the immediately next step. If you want a change for this, please file a separate bug.

(In reply to Philip Jägenstedt from comment #15)
> Going back to that flaky test case, it seems like the spec'd behavior for
> progress events still doesn't guarantee that networkState is NETWORK_LOADING
> during the progress event, because even though that's guaranteed for the
> last progress event fired at end of load, one queued to be fired while
> loading could still arrive after that point. Is that intentional?
>
> Maybe "While the load is not suspended (see below), every 350ms (±200ms) or
> for every byte received, whichever is least frequent, queue a task to fire a
> simple event named progress at the element." could be rephrased as a step of
> "The networking task source tasks to process the data as it is being fetched
> must each immediately queue a task to run the first appropriate steps from
> the following list."?

How could a progress event fired before the last one get delivered after the last one? Aren't they in the same queue?

(In reply to Ian 'Hixie' Hickson from comment #16)
> (In reply to Philip Jägenstedt from comment #6)
> > 
> > Finally, it's a bit odd to have "queue a task" in "For video elements, set
> > the videoWidth and videoHeight attributes, and queue a task to fire a simple
> > event named resize at the media element." now that those steps are already
> > running in a task. Not sure how to fix that, maybe revert your recent change
> > and queue a task for each step individually instead?
> 
> I don't see why it's weird for 'resize', but not for 'durationchange' in the
> immediately previous step and 'loadedmetadata' in the immediately next step.
> If you want a change for this, please file a separate bug.

I thought it was odd to essentially "queue a task to queue a task", but in this case it seems simpler and I can't see an event ordering problem right now. Nothing to be done.

> (In reply to Philip Jägenstedt from comment #15)
> > Going back to that flaky test case, it seems like the spec'd behavior for
> > progress events still doesn't guarantee that networkState is NETWORK_LOADING
> > during the progress event, because even though that's guaranteed for the
> > last progress event fired at end of load, one queued to be fired while
> > loading could still arrive after that point. Is that intentional?
> >
> > Maybe "While the load is not suspended (see below), every 350ms (±200ms) or
> > for every byte received, whichever is least frequent, queue a task to fire a
> > simple event named progress at the element." could be rephrased as a step of
> > "The networking task source tasks to process the data as it is being fetched
> > must each immediately queue a task to run the first appropriate steps from
> > the following list."?
> 
> How could a progress event fired before the last one get delivered after the
> last one? Aren't they in the same queue?

I imagined a race between "for every byte received" and "Once the entire media resource has been fetched", but when spelled out that doesn't make sense. The problem has evaporated.

Thanks for the other fixes, this bug can be closed now as far as I am concerned.

Thanks for all the help getting this stuff better! It's non-trivial!