469446 - Audio/video resources are never cached

Status: RESOLVED FIXED

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

Description

[Matthew Gregan [:kinetik]]

We tried to enable caching in bug 462378, but it's no longer working.  It turns out that this is because we always open HTTP requests using a 0-Inf. byte range (so that we can determine if the server supports range requests for seeking), and Necko does not seem to be caching these requests.

Hacking the code to stop doing range requests allows caching to work as expected, but the Wave playback has a little glitch of noise at the start, indicating some other problem that needs investigating.

I also noticed that nsHTMLMediaElement.cpp includes nsNetUtils.h twice.

Comment 1

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

So, my plan is:

* When seeking to time=0, have nsHTTPStreamStrategy use normal requests. This will be cached.
* When channel is first opened, check if the server sends "Accept-Ranges: bytes" header, and if so we know byte-range seeking will work. If it returns "Accept-Ranges: none" we know it can't. If it doesn't have an Accept-Ranges we should assume it "may".
* When we seek if we know the server accepts byte-ranges, and it's not 0- range, do a seek as per normal. This won't be cached. Oh well.
* If we're not sure if a server supports byte-range requests, try, and if the response returns code "206 Partial Content", we can safely assume that it does, proceed as normal, else we know it can't support them.
* If the server doesn't support byte range requests:
  - If were seeking to a position after the current offset, just consume the stream until we get there.
  - else if we're seeking to a position before the current offset, reset to 0- and read until we get there.

See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html for details of Accept-Ranges header, and see also "14.35.1 Byte Ranges".

We should probably to watch out for being redirected to another server with different byte range support.

Comment 2

[cajbir (:cajbir)]

I don't think we should try to fake seeks if the server doesn't support byte range requests. Seeks can't be cancelled and seeking two hours into a video could get a bit annoying if you have to sit and wait for two hours.

Comment 3

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

Attachment: testcase - video without byte range support

Test case - video of seek.ogg hosted in bugzilla attachment - which means it doesn't have byte-range request support, because attachment.cgi doesn't support it.

Comment 4

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

I don't think that we should take the approach of making the initial request not a byte-range request. We should change Necko so that it can cache byte ranges, or just buffer ourselves in nsHttpStreamStrategy. Here's my reasoning:

* Most servers out there support byte-range requests.
* An appreciable number of the servers that support byte-range requests don't report that they do via Accept-Ranges:bytes (e.g. tinyvid, YouTube, wikimedia).
* The only reliable way to determine if a server supports byte-range requests is to try one and see if you get a 206 response.
* If the user tries to seek on servers that don't support byte-ranges, the stream just resets to 0, and liboggplay seems to handle this ok, but if the seek is inside oggplay_get_duration(), liboggplay gets confused, and bad things happen.
* nsOggDecoder::mIsSeekable is true by default, and is set to false in nsChannelToPipeListener::OnStartRequest() if the response code is not 206 for the initial 0- byte-range request.
* If !nsOggDecoder::mIsSeekable, unless the user seeks, liboggplay won't otherwise do any seeks. So provided we don't call oggplay_get_duration() or seek, we can safely play video on a non-byte-range enabled server.
* In nsOggStateMachine::LoadOggHeaders(), if nsOggDecoder::mIsSeekable, we call oggplay_get_duration() on the stream. This does seeks. This is a good time to know the duration.
* If the server honestly can't support byte-ranges, we must know by the time we we decide whether nor not to call oggplay_get_duration(), else bad things will happen.
* At the time LoadOggHeaders() runs, we will have received the nsChannelToPipeListener::OnStartRequest(). OnStartRequest() is basically our only vector to accurately determine if we can seek, before we start wanting to know the duration if possible, and thus want to seek.

So because we can't reliably know in advance if a byte-range request will work without trying one, and we should try one from 0- on the initial stream open, because if it doesn't work, nothing bad happens, except we don't get cached. If we try a byte-range request at a later time, liboggplay may get confused, especially in oggplay_get_duration().

Alternativley, we could try sending a HEAD request first to determine whether the channel is seekable, and not kick off the load until we know that. I suspect making those changes are more hassle than they're worth though.

So I think the best approach is probably to just fix byte-range caching in Necko.

Comment 5

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

Doing general byte-range caching in Necko sounds hard. But making it work for the special case where the range starts at 0 sounds easy --- *if* servers cooperate to support caching of byte-range requests. It's quite possible that they just don't support byte-range request caching. We should test this before we implement anything on our side.

Comment 6

[cajbir (:cajbir)]

I've found at least one server that supports byte ranges but returns a 200 status code when we request a '0-' range. That's thttpd. It does return an Accept-Ranges: bytes header however. This means duration requests don't work on thttpd since we think it can't handle byte ranges.

My testing does show that Wikimedia sends Accept-Ranges. Unfortunately Youtube doesn't but does support byte range requests (for mp4 downloads anyway). So we could change back to an initial non-range request and looking for Accept-Ranges: bytes as an indicator of support. That will allow us to detect that thttpd supports seeking but not for sites that don't send Accept-Ranges. Are there any others apart from YouTube?

Another approach might be to do the initial 0- send, and detect a return of 216 or a return of (200 and accept-ranges) as indicator of seeking support and do the fix for caching as per comment 5.

I tested if the servers are disabling byte range caching and they aren't.

Comment 7

[Biju]

If we do caching for video/audio whether it will have separate quota or it will use the same one for html and images?

Even if we have separate quota for video/audio do we need to cache more than initial 20sec(or user set) of data?

Comment 8

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

(In reply to comment #0)
> Hacking the code to stop doing range requests allows caching to work as
> expected, but the Wave playback has a little glitch of noise at the start,
> indicating some other problem that needs investigating.

Note that when seeking the nsWaveStateMachine always adds mWavePCMOffset to the seek position, so even when you seek to time 0, the cache still won't be used, as the underlying nsMediaStream isn't being seeked to 0. So the patch for this bug won't improve WAV playback at all, only video.

Comment 9

[Matthew Gregan [:kinetik]]

That's easy to change, we can seek to the beginning of the stream and discard up to mWavePCMOffset bytes before considering the seek complete.

We could use some threshold value to decide between discarding data and actually seeking to mWavePCMOffset (since it could start quite far into the stream), but the overwhelming common case is that mWavePCMOffset is 44 bytes, so it's probably not worth the complexity.

Comment 10

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

Attachment: Patch v1

Patch v1, simple fix to make netwerk cache 0- byte range requests. Also amends nsWaveDecoder so that it seeks to 0 before seeking to position+mPCMWaveOffset when position is 0.

All video mochitests still pass on Vista and Linux.

Comment 11

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

Comment on attachment 355520 [details] [diff] [review]
Patch v1

         // we don't support caching for byte range requests initiated
-        // by our clients or via nsIResumableChannel.
-        // XXX perhaps we could munge their byte range into the cache
-        // key to make caching sort'a work.
-        return NS_OK;
+        // via nsIResumableChannel.

You mean "we don't support caching for requests initiated via nsIResumableChannel"?

+    if (mRequestHead.PeekHeader(nsHttp::Range)) {
+        nsCAutoString byteRange;
+        mRequestHead.GetHeader(nsHttp::Range, byteRange);
+        if (!byteRange.EqualsLiteral("bytes=0-")) {

Could this chunk of code be shared in a small helper function?

We'll need a Necko peer review here, bz if he can do it, or maybe dcamp, or in a pinch biesi.

The rest looks good.

Comment 12

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

Attachment: Patch v1.1

As patch v1, with roc's nits picked. r+doublec sr+roc.

Comment 13

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

Comment on attachment 355897 [details] [diff] [review]
Patch v1.1

bz, do you have time to take a quick look at the small netwerk cache changes in this patch?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 355897 [details] [diff] [review]
Patch v1.1

Makes sense.

Comment 15

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

Pushed http://hg.mozilla.org/mozilla-central/rev/fba21452603e

Comment 16

[Daniel Holbert [:dholbert]]

Pushed to 1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/68e40f23b239