517818 - Seeking to the end of the stream causes problems

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

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

1) Sometimes we do an HTTP seek to the end of the content. This is unnecessary, we know there is no content to read there.

2) This can trigger an HTTP response 416 "Requested Range Not Satisfiable", which we treat as data and put into the media cache at the end of the stream.

3) That can confuse nsWaveDecoder, making it think there's data to play after the current mPlaybackPosition, which causes test failures because we might be in the ended state but readyState is HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA.

Comment 1

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

Attachment: Part 1: Fix Resume to not seek to EOF

If someone calls Resume and we know the current offset is at the end of the stream, don't recreate the channel and seek to the offset. It's a waste of an HTTP transaction because unless the server lied to us, there's no data to read there. We will also probably get a 416 response which will trigger some other problems.

Comment 2

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

Attachment: Part 2: don't seek to end of stream in nsMediaCache::Update

Another case where we might seek to EOF is in nsMediaCache::Update. We'd rather suspend than carry out an actual seek operation to the end. But if we're already reading from the end, we may as well carry on without suspending.

Comment 3

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

Attachment: Part 3: handle HTTP errors

We should never allow an error page to be treated as media data. Non-416 errors should be treated as network errors. 416s can still happen if the server supports seeking, doesn't send us a Content-Type, and we get really unlucky trying to seek to the end of the data we've got and finding that we had just reached the end of the available data.

Comment 4

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

Attachment: Part 4: Fix Wave decoder

The Wave decoder should not say we have more data to play if there is data in the file beyond the current position but the current position is at the end of the sample data.

Comment 5

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

Part 1 checked in to fix test issues: http://hg.mozilla.org/mozilla-central/rev/f54f960bfb83

Comment 6

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

These issues are pretty bad since they cause test failures and lead to us placing completely bogus data in the media cache.

Comment 7

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

Part 1 checked in on 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5be1b212e8d8

Comment 8

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

Part 3 caused some errors in tests on the try-servers. The problem turns out to be when a request gets canceled before we reach OnStartRequest. Then in OnStartRequest nsIHttpChannel::GetRequestSucceeded returns false which triggers a call to NetworkError. So I adjusted part 3 to ignore cases where we have a lower-level error reported in GetStatus. Those errors are handled in OnStopRequest, which calls NotifyDownloadEnded, which doesn't call NetworkError for canceled requests.

Comment 9

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

Checked in parts 2, 3 and 4:
http://hg.mozilla.org/mozilla-central/rev/6e4ad2b20482
http://hg.mozilla.org/mozilla-central/rev/a1e033c280f0
http://hg.mozilla.org/mozilla-central/rev/7ac2d53330db

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Checked in parts 2, 3 and 4 to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2c5444a4d3f7
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a5e97e4f5228
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d86425d3f6a8