614760 - Should not send Accept-Encoding: gzip,deflate when requesting audio/video

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ben Bucksch (:BenB)]

https://developer.mozilla.org/en/Configuring_servers_for_Ogg_media says that servers should disable Accept-Encoding: gzip when serving ogg video files, because it provides no benefit and uses server CPU resources. Worse, it also prevents Apache from sending the Content-Length in the response, which prevents Gecko from being able to estimate the video length (in seconds) and from skipping. Both of these are very noticeable and a severe limitation for users.

Instead of requiring all servers to change their config, we should just avoid sending "Accept-Encoding: gzip,deflate" in our HTTP request header when requesting video/audio streams.

Comment 1

[Ben Bucksch (:BenB)]

Testcase:
1) Case 5 on <http://smormedia.gavagai.nl/geckovideo/>
2) <http://smormedia.gavagai.nl/2010/10/TiranaTraffic.ogv>
   (same as Case 5 above, but loaded as top-level browser document)

Comment 2

[Ben Bucksch (:BenB)]

> 2) <http://smormedia.gavagai.nl/2010/10/TiranaTraffic.ogv>
>    ... loaded as top-level browser document

In this case, it's not possible to change the request, because we don't know yet that it's going to be a video file. However, we can know it
- once we make another request, to seek - that request is initiated by our
  video controls.
- if we abort the first HTTP request and make a new one without gzip (harder)
- if the <video> tag is used, i.e. 1) above

Comment 3

[Ben Bucksch (:BenB)]

http://mxr.mozilla.org/comm-central/source/mozilla/content/media/nsMediaStream.cpp#172

Comment 4

[cajbir (:cajbir)]

The easiest way to change this for the <video> and <audio> case would be to modify the Accept-Encoding header during nsHTMLVideoElement::SetAcceptHeader and nsHTMLAudioElement::SetAcceptHeader.

Comment 5

[Ben Bucksch (:BenB)]

Attachment: Possible fix, v1 - in nsMediaStream

Something like this...
(Yes, we could do it in nsHTMLVideo|AudioElement.cpp, too, but then we'd have the same code twice.)

I didn't verify whether that actually fixes it, though.

Comment 6

[Ben Bucksch (:BenB)]

Attachment: Testcase 1

Comment 7

[cajbir (:cajbir)]

You should probably do it in nsHTMLMediaElement::SetRequestHeaders. This is called by the method you've changed in your patch as well as in the other place where channels are made (nsHTMLMediaElement::LoadResource). Apart from that, looks fine.

Comment 8

[Ben Bucksch (:BenB)]

I verified that my patch removes Accept-Encoding from the request.

However, Gecko still doesn't estimate the video length. I filed bug 614775.

Comment 9

[Ben Bucksch (:BenB)]

Chris D, sure, I can put it in nsHTMLMediaElement, but what would be the advantage? It seems to me that nsMediaStream.cpp is both logically more correct (it's a stream thing, not element thing) and might cover more cases.

Comment 10

[cajbir (:cajbir)]

(In reply to comment #9)
> Chris D, sure, I can put it in nsHTMLMediaElement, but what would be the
> advantage? It seems to me that nsMediaStream.cpp is both logically more correct
> (it's a stream thing, not element thing) and might cover more cases.

It doesn't cover more cases, it covers less. It misses the case where the request is made in the media element's LoadResource call.

> However, Gecko still doesn't estimate the video length.

Does your server support byte ranges? Is it sending the 'Accept-Ranges' header? We need to seek to get the duration.

Comment 11

[Ben Bucksch (:BenB)]

Attachment: Possible fix, v1b - in nsHTMLMediaElement

OK, based on request by Chris Double, here's the same thing, but in nsHTMLMediaElement. For the reason stated above, I personally think nsMediaStream is more appropriate, though.

Comment 12

[Ben Bucksch (:BenB)]

> Does your server support byte ranges? Is it sending the 'Accept-Ranges' header?

It's not my server, I just use the testcase from bug 614695:<http://smormedia.gavagai.nl/2010/10/TiranaTraffic.ogv>.
wget says "Accept-Ranges: bytes" is in the server response, yes.

Comment 13

[Ben Bucksch (:BenB)]

Sorry, the last comment should have been in bug 614775.

Comment 14

[Ben Bucksch (:BenB)]

Attachment: Possible fix, v2b - in nsHTMLMediaElement

same thing, indention and comment nits fixed

Comment 15

[cajbir (:cajbir)]

I'll reply to comment 12 in bug 614775.

Comment 16

[cajbir (:cajbir)]

With the patch applied does the video show a duration? If I set network.http.accept-encoding to an empty string then it shows a duration and seeks work. If it's not working with your patch you should confirm that the Header is not being sent using the web console or similar.

Comment 17

[Ben Bucksch (:BenB)]

Comment on attachment 493213 [details] [diff] [review]
Possible fix, v1 - in nsMediaStream

You're right: The change in nsMediaStream doesn't fix the actual bug, see bug 614775. Obsoleting this patch.

Comment 18

[Ben Bucksch (:BenB)]

> With the patch applied does the video show a duration?

With patch 2b, it works. Thanks for the feedback. Can you review?

Comment 19

[cajbir (:cajbir)]

Comment on attachment 493219 [details] [diff] [review]
Possible fix, v2b - in nsHTMLMediaElement

Thanks for working on this!

Comment 20

[Wicher Minnaard]

Just to avoid confusion: I now serve http://smormedia.gavagai.nl/2010/10/TiranaTraffic.ogv *with gzip disabled serverside*, as that seemed to solve most of my issues of #614695.

Comment 21

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

http://hg.mozilla.org/mozilla-central/rev/c194928bb4a9

Comment 22

[Eric Shepherd [:sheppy]]

Updated docs:

https://developer.mozilla.org/en/Configuring_servers_for_Ogg_media#Don%27t_use_gzip.2fdeflate_compression