Closed Bug 614760 Opened 9 years ago Closed 9 years ago

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

Categories

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

Other
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: BenB, Assigned: BenB)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

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.
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)
> 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
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.
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.
Assignee: nobody → ben.bucksch
Attachment #493213 - Flags: feedback?(chris.double)
Attached file Testcase 1
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.
Blocks: 614775
I verified that my patch removes Accept-Encoding from the request.

However, Gecko still doesn't estimate the video length. I filed bug 614775.
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.
(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.
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.
Attachment #493217 - Flags: review?(chris.double)
Attachment #493213 - Attachment description: Possible fix, v1 → Possible fix, v1 - in nsMediaStream
Attachment #493213 - Flags: feedback?(chris.double) → review?(chris.double)
> 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.
Sorry, the last comment should have been in bug 614775.
same thing, indention and comment nits fixed
Attachment #493217 - Attachment is obsolete: true
Attachment #493219 - Flags: review?(chris.double)
Attachment #493217 - Flags: review?(chris.double)
I'll reply to comment 12 in bug 614775.
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 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.
Attachment #493213 - Attachment is obsolete: true
Attachment #493213 - Flags: review?(chris.double)
> With the patch applied does the video show a duration?

With patch 2b, it works. Thanks for the feedback. Can you review?
Comment on attachment 493219 [details] [diff] [review]
Possible fix, v2b - in nsHTMLMediaElement

Thanks for working on this!
Attachment #493219 - Flags: review?(chris.double) → review+
Attachment #493219 - Flags: approval2.0?
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.
http://hg.mozilla.org/mozilla-central/rev/c194928bb4a9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.