521178 - nsMediaCache::Update seek vs read optimization and enableReading interaction

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

nsMediaCache::Update uses desiredOffset to decide whether to enable reading on a HTTP channel.

An optimization to avoid creating a new HTTP channel for seeking when the distance between the current read position and the desired offset is smaller than a set threshold (SEEK_VS_READ_THRESHOLD) may cause desiredOffset to be modified.  This optimization is checked between the initial calculation of desiredOffset and the block of logic that decides whether to read.

roc pointed out that the optimization check and tweak of desiredOffset should happen after the begin-reading logic, so that the logic uses the true value of desiredOffset to set the stream reading state.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Trying to fix this, I encountered a subtle problem which I think could happen on trunk: playing a resource through to the end may not fire a 'load' event.

What happens is that initially we seek to just before the end of the an Ogg file to find its duration, let's call that position P. Those blocks land in the cache. Then we seek back to the beginning and read through the rest of the file. We reach P and then everything's cached, so we suspend the download. ResourceLoaded and 'load' never fire because we never actually read all the way to the end of the Necko stream.

Actually I wonder why our initial read of the last chunk of the file doesn't trigger ResourceLoaded then, way too early.

ResourceLoaded is intrinsically weird and bogus. There is no guarantee that you've actually got all the data, since there might be chunks early in the file that you seeked over, and you don't even necessarily need all the data (and "load" to fire) to play all the way through the resource.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In fact, the HTML5 spec specifically says that NETWORK_LOADED and the 'load' event should only fire when you've got all the data locally. So we need to rework ResourceLoaded so it only fires when all the data is cached. Although that might mean we never fire it because we don't need all the actual bytes in order to play through the media. I need to think about this.

http://www.whatwg.org/specs/web-apps/current-work/#loading-the-media-resource

The spec also has a problem that it doesn't envisage restarting the resource fetch algorithm after "load" has happened, which can happen if we discard blocks from the cache. I'll poke WHATWG about that.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed bug 521359 on the problems with "load". We should just remove it.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

For the reasons described above, this patch will cause test_progress1.html to fail intermittently (and maybe other tests) unless we land the test changes in bug 521359 first to stop the tests depending on 'load' firing.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/2d2a7f2f0a96