Bug 115354 – [gstreamer] Disable HTTP request "Accept-Encoding:" header field on gstreamer source element to avoid receiving the wrong size when retrieving data

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andre Moreira Magalhaes]

The default value for the HTTP "Accept-Encoding:" header field in the request allows compressed data to be received on requests.
While this works fine for most cases, it can break the webkit source that needs to rely on the the proper data size when receiving the response (even though the received data is already uncompressed the size reported in ResourceResponse::expectedContentLength() is the same represented by the HTTP header field "Content-Length" which is the size of the compressed data).
    
We should disable the HTTP "Accept-Encoding:" header field when using the request as we don't want the received response to be encoded in any way, as we need to rely on the proper size of the returned data on ResourceHandle::didReceiveResponse().

Patch to follow.

[Andre Moreira Magalhaes]

Created attachment 200011 [details]
Patch

[EFL EWS Bot]

Comment on attachment 200011 [details]
Patch

Attachment 200011 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/195698

[EFL EWS Bot]

Comment on attachment 200011 [details]
Patch

Attachment 200011 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/137878

[Early Warning System Bot]

Comment on attachment 200011 [details]
Patch

Attachment 200011 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/229513

[kov's GTK+ EWS bot]

Comment on attachment 200011 [details]
Patch

Attachment 200011 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.appspot.com/results/243140

[Philippe Normand]

Could this be the cause of bug 90732 ?

[Andre Moreira Magalhaes]

(In reply to comment #6)
> Could this be the cause of bug 90732 ?

hmm, could be, but most likely you need the patch from bug 115353. What happens here is that libsoup downloads the compressed data and extracts it using SoupContentDecoder, but returns the size of the specified Content-Length instead, thus the size of the uncompressed data differs from the actual reported size.

[Sergio Villar Senin]

(In reply to comment #7)
> (In reply to comment #6)
> > Could this be the cause of bug 90732 ?
> 
> hmm, could be, but most likely you need the patch from bug 115353. What happens here is that libsoup downloads the compressed data and extracts it using SoupContentDecoder, but returns the size of the specified Content-Length instead, thus the size of the uncompressed data differs from the actual reported size.

I don't think this is the right fix. By doing that you're increasing the http transfers potentially by a lot since the server won't compress data. The problem is that you cannot assume that you could know the actual size of the data after receiving the http headers. It is not true for compressed data as you said but it isn't also true for "Transfer-Encoding: chunked" or for responses without encoding (ended when the connection closes).

So my advice would be to try to reorganize the gstreamer code in order not to depend on the size of the data, not sure if that's possible tough.

[Andre Moreira Magalhaes]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Could this be the cause of bug 90732 ?
> > 
> > hmm, could be, but most likely you need the patch from bug 115353. What happens here is that libsoup downloads the compressed data and extracts it using SoupContentDecoder, but returns the size of the specified Content-Length instead, thus the size of the uncompressed data differs from the actual reported size.
> 
> I don't think this is the right fix. By doing that you're increasing the http transfers potentially by a lot since the server won't compress data. The problem is that you cannot assume that you could know the actual size of the data after receiving the http headers. It is not true for compressed data as you said but it isn't also true for "Transfer-Encoding: chunked" or for responses without encoding (ended when the connection closes).
> 
> So my advice would be to try to reorganize the gstreamer code in order not to depend on the size of the data, not sure if that's possible tough.

hmm, the webkit source element uses the content length to compute the duration of the stream. If the size is reported as 0, the stream will be considered a live stream, failing to seek and other small "issues".

I am not sure what would happen if using "Transfer-Encoding: chunked" (not even sure if that is set by libsoup when requesting). Right now IIRC ResourceHandleClient::didReceiveResponse() is called only once and we get the size from response.expectedContentLength() and set the duration based on it. ResourceHandleClient::didReceiveData() is called on every data chunk downloaded though, but we don't use the size reported there as we want to know the duration in advance, not during playback (when the data is actually downloaded).

In other words, it could be that "Transfer-Encoding: chunked" would lead to some issue, but I am not sure and we tested these patches in a series of scenarios using DASH/MSS/HLS adaptive and other non-adaptive streams and didn't find any issue.

[Andre Moreira Magalhaes]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > Could this be the cause of bug 90732 ?
> > > 
> > > hmm, could be, but most likely you need the patch from bug 115353. What happens here is that libsoup downloads the compressed data and extracts it using SoupContentDecoder, but returns the size of the specified Content-Length instead, thus the size of the uncompressed data differs from the actual reported size.
> > 
> > I don't think this is the right fix. By doing that you're increasing the http transfers potentially by a lot since the server won't compress data. The problem is that you cannot assume that you could know the actual size of the data after receiving the http headers. It is not true for compressed data as you said but it isn't also true for "Transfer-Encoding: chunked" or for responses without encoding (ended when the connection closes).
> > 
> > So my advice would be to try to reorganize the gstreamer code in order not to depend on the size of the data, not sure if that's possible tough.
> 
> hmm, the webkit source element uses the content length to compute the duration of the stream. If the size is reported as 0, the stream will be considered a live stream, failing to seek and other small "issues".
> 
> I am not sure what would happen if using "Transfer-Encoding: chunked" (not even sure if that is set by libsoup when requesting). Right now IIRC ResourceHandleClient::didReceiveResponse() is called only once and we get the size from response.expectedContentLength() and set the duration based on it. ResourceHandleClient::didReceiveData() is called on every data chunk downloaded though, but we don't use the size reported there as we want to know the duration in advance, not during playback (when the data is actually downloaded).
> 
> In other words, it could be that "Transfer-Encoding: chunked" would lead to some issue, but I am not sure and we tested these patches in a series of scenarios using DASH/MSS/HLS adaptive and other non-adaptive streams and didn't find any issue.

Additionally, I agree that the best approach would be to get the proper size but still use compressed data to avoid increasing network traffic. That would probably require some patch on libsoup itself, but at least the patch above only disables compression for playback, which is currently "broken" (wrong duration) when the server returns compressed data.

[Sergio Villar Senin]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > Could this be the cause of bug 90732 ?
> > > 
> > > hmm, could be, but most likely you need the patch from bug 115353. What happens here is that libsoup downloads the compressed data and extracts it using SoupContentDecoder, but returns the size of the specified Content-Length instead, thus the size of the uncompressed data differs from the actual reported size.
> > 
> > I don't think this is the right fix. By doing that you're increasing the http transfers potentially by a lot since the server won't compress data. The problem is that you cannot assume that you could know the actual size of the data after receiving the http headers. It is not true for compressed data as you said but it isn't also true for "Transfer-Encoding: chunked" or for responses without encoding (ended when the connection closes).
> > 
> > So my advice would be to try to reorganize the gstreamer code in order not to depend on the size of the data, not sure if that's possible tough.
> 
> hmm, the webkit source element uses the content length to compute the duration of the stream. If the size is reported as 0, the stream will be considered a live stream, failing to seek and other small "issues".

What I mean is, for non live streams, you cannot know for sure the size of the stream until all the data is received except in very specific cases (non compressed data with Content-Length header).

> I am not sure what would happen if using "Transfer-Encoding: chunked" (not even sure if that is set by libsoup when requesting). Right now IIRC ResourceHandleClient::didReceiveResponse() is called only once and we get the size from response.expectedContentLength() and set the duration based on it. ResourceHandleClient::didReceiveData() is called on every data chunk downloaded though, but we don't use the size reported there as we want to know the duration in advance, not during playback (when the data is actually downloaded).
> 
> In other words, it could be that "Transfer-Encoding: chunked" would lead to some issue, but I am not sure and we tested these patches in a series of scenarios using DASH/MSS/HLS adaptive and other non-adaptive streams and didn't find any issue.

Maybe because media is normally streamed using content-length encodings (no idea here, maybe philn knows better). My point is, disabling compression is a bad idea IMO not only because of the increase in the amount of data but also because the problem is still there for chunked or EOF transfer-encodings. Again, maybe multimedia streams are not generally transferred that way and compression does not significantly increase the size of the data (since multimedia data is normally already compressed), I honestly don't know, I just give my opinion from the network stack POV.

I'm pretty sure all the multimedia libraries have to deal with these issues, how do they normally handle this case?

[kov's GTK+ EWS bot]

Comment on attachment 200011 [details]
Patch

Attachment 200011 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/121826

[Sebastian Dröge (slomo)]

The source should already handle wrongly returned values for the Content-Length field, the only problem then is that this will break seeking in quite some cases. What could be improved here is handling of compressed streams by actively interpolating the actual size from the so-far detected compression ratio. It wouldn't be accurate and can still cause the beforementioned seeking problems (if e.g. something is using the size in bytes for a duration calculation, or seeking to the end of the stream is done for whatever reason, like getting a footer there, or ....)


Turning off compression for multimedia streams should be no loss, if a generic compression scheme can compress a multimedia stream by a significant amount something is really wrong elsewhere.

[Early Warning System Bot]

Comment on attachment 200011 [details]
Patch

Attachment 200011 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/220920

[Build Bot]

Comment on attachment 200011 [details]
Patch

Attachment 200011 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/238736

New failing tests:
editing/style/5065910.html

[Build Bot]

Created attachment 200082 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Sebastian Dröge (slomo)]

As discussed on IRC the situation is as follows:
a) This patch is fine, it doesn't make sense to compress media streams. It only causes additional load on the server. And due to the size in bytes being not the uncompressed size in bytes, there can be problems with seeking.
b) More important there seems to be a bug somewhere between webkitwebsrc, appsrc, dashdemux/hlsdemux that causes dashdemux/hlsdemux to receive EOS too early when the size in bytes is not accurate, especially it seems to receive EOS before it receives the last missing buffer. This must be fixed in any case, wherever this bug is.

Andre is continuing to debug b), a) should nonetheless be considered too here.

[Sebastian Dröge (slomo)]

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp#L755

This could be a problem. Also, how are range requests handled for compressed HTTP streams? Is the byte range given there for the compressed version or for the underlying uncompressed version of the stream? Is the length of the underlying uncompressed version of the stream sent somewhere in the HTTP headers?


The normal EOS event of the source is sent here:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp#L945

If this is called before the last call to the following line, things will definitely go wrong. Can that happen?
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp#L886

[Sebastian Dröge (slomo)]

Ok to summarize here:
a) This patch is fine, it doesn't make sense to compress media streams. It only causes additional load on the server. And due to the size in bytes being not the uncompressed size in bytes, there can be problems with seeking.

b) http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp#L755 is wrong, it should just try to seek and will fail later already if necessary

c) webkitwebsource should tell appsrc and basesrc about the newly observed size whenever it receives a buffer after the reported size and before pushing it to appsrc. Otherwise basesrc will just assume EOS because it's already after the size.

[Andre Moreira Magalhaes]

Created attachment 200408 [details]
Do not check seek offset against internal size

(In reply to comment #19)
> b) http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp#L755 is wrong, it should just try to seek and will fail later already if necessary
> 
Attached patch should fix this.

[Andre Moreira Magalhaes]

Created attachment 200409 [details]
Adjust internal size when receiving data if needed 

(In reply to comment #19)
> c) webkitwebsource should tell appsrc and basesrc about the newly observed size whenever it receives a buffer after the reported size and before pushing it to appsrc. Otherwise basesrc will just assume EOS because it's already after the size.
> 
Attached patch should fix this.

[Sebastian Dröge (slomo)]

Looks all good to me, thanks!

[Andre Moreira Magalhaes]

Created attachment 202287 [details]
Patch

Updated Changelog to use proper format.

[Andre Moreira Magalhaes]

Created attachment 202288 [details]
Do not check seek offset against internal size

Updated Changelog to use proper format.

[Andre Moreira Magalhaes]

Created attachment 202290 [details]
Adjust internal size when receiving data if needed 

Updated Changelog to use proper format.

[Andre Moreira Magalhaes]

Thanks all for the review, all patches are updated now with comments applied.

[Early Warning System Bot]

Comment on attachment 202287 [details]
Patch

Attachment 202287 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/370390

[EFL EWS Bot]

Comment on attachment 202287 [details]
Patch

Attachment 202287 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/415525

[WebKit Commit Bot]

Attachment 202290 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:917:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[kov's GTK+ EWS bot]

Comment on attachment 202287 [details]
Patch

Attachment 202287 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/502013

[Early Warning System Bot]

Comment on attachment 202287 [details]
Patch

Attachment 202287 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/524004

[EFL EWS Bot]

Comment on attachment 202287 [details]
Patch

Attachment 202287 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/527019

[Philippe Normand]

We deal with one patch per bug usually, can you please merge those 3 patches in one? Thanks!

[Andre Moreira Magalhaes]

Comment on attachment 202288 [details]
Do not check seek offset against internal size

Added new bug #116533 for this patch

[Andre Moreira Magalhaes]

Comment on attachment 202290 [details]
Adjust internal size when receiving data if needed 

Added new bug #116534 for this patch

[Andre Moreira Magalhaes]

(In reply to comment #33)
> We deal with one patch per bug usually, can you please merge those 3 patches in one? Thanks!

Added separate bugs for each patch

[Philippe Normand]

Comment on attachment 202287 [details]
Patch

Ok, I suppose this patch builds correctly now?

[Andre Moreira Magalhaes]

(In reply to comment #37)
> (From update of attachment 202287 [details])
> Ok, I suppose this patch builds correctly now?

I am not sure what is the issue with this patch, maybe because it depends on #115352 and #115353 being applied?

[Sebastian Dröge (slomo)]

Does it apply cleanly now?

[Gustavo Noronha (kov)]

Created attachment 208265 [details]
Patch for landing

[kov's GTK+ EWS bot]

Comment on attachment 208265 [details]
Patch for landing

Attachment 208265 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1379257

[Early Warning System Bot]

Comment on attachment 208265 [details]
Patch for landing

Attachment 208265 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1370792

[Early Warning System Bot]

Comment on attachment 208265 [details]
Patch for landing

Attachment 208265 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1370793

[EFL EWS Bot]

Comment on attachment 208265 [details]
Patch for landing

Attachment 208265 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1386204

[EFL EWS Bot]

Comment on attachment 208265 [details]
Patch for landing

Attachment 208265 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1384268

[Gustavo Noronha (kov)]

*** Bug 115353 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Created attachment 208276 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 208276 [details]
Patch

All feedback has been included, hopefully. Uploading it here so Philippe can give it a final once-over =)

[Early Warning System Bot]

Comment on attachment 208276 [details]
Patch

Attachment 208276 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1370837

[Early Warning System Bot]

Comment on attachment 208276 [details]
Patch

Attachment 208276 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1380291

[Philippe Normand]

Comment on attachment 208276 [details]
Patch

Ok! I guess the qt EWS is broken for some other reason? I haven't checked but gtk-wk2 is green

[Gustavo Noronha (kov)]

Created attachment 208280 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 208280 [details]
Patch

Missing USE(SOUP) check, actually, this one should work!

[Gustavo Noronha (kov)]

Comment on attachment 208280 [details]
Patch

Thanks phil!

[WebKit Commit Bot]

Comment on attachment 208280 [details]
Patch

Clearing flags on attachment: 208280

Committed r153795: <http://trac.webkit.org/changeset/153795>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andre Moreira Magalhaes]

Created attachment 209126 [details]
Patch

[Andre Moreira Magalhaes]

The merged patch was missing 2 things:
- Respect ResourceRequest::acceptEncoding() on ResourceRequest::toSoupMessage().
- Set Accept-Encoding header to identity (http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html) when ResourceRequest::acceptEncoding() is false.

Apart from that libsoup also had an issue (https://bugzilla.gnome.org/show_bug.cgi?id=706338) when copying messages where the disabled features were not respected.

The patch from comment #57 should fix all issues above and it also includes the libsoup patch for jhbuild.

[Andre Moreira Magalhaes]

Arg, reopening again, see comment #58.

[Gustavo Noronha (kov)]

I think we should have a layout test for this, to ensure it's fixed this time (oops) and not regress.

[Andre Moreira Magalhaes]

Created attachment 209384 [details]
Patch

(In reply to comment #60)
> I think we should have a layout test for this, to ensure it's fixed this time (oops) and not regress.

Attached patch attempts to add the test. The patch to disable encoding requests on video is specific to libsoup/gstreamer but I made the test generic after discussing it with Gustavo. We can update the test expectations for specific platforms if EWS complains.

[Build Bot]

Comment on attachment 209384 [details]
Patch

Attachment 209384 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1523708

New failing tests:
http/tests/media/video-accept-encoding.html

[Build Bot]

Created attachment 209396 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209384 [details]
Patch

Attachment 209384 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1543193

New failing tests:
http/tests/media/video-accept-encoding.html

[Build Bot]

Created attachment 209430 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Andre Moreira Magalhaes]

Created attachment 209456 [details]
Patch

Mark test as failing on mac.

[Andre Moreira Magalhaes]

Created attachment 209460 [details]
Patch

New attempt on top of today's master.

[Eric Carlson]

Comment on attachment 209460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209460&action=review

> LayoutTests/http/tests/media/video-accept-encoding.cgi:14
> +# Get MIME type if provided.  Default to video/mp4.
> +my $type = $query->param('type') || "video/mp4";

This is only used by one test, which always passes type, so why not just die if the parameter is missing?

[Alexey Proskuryakov]

Do any other media engines (such as QuickTime) send "Accept-Encoding: identity"? My understanding is that this is a fairly well established technology, so we should not invent ad hoc solutions unless absolutely necessary.

[Andre Moreira Magalhaes]

(In reply to comment #69)
> Do any other media engines (such as QuickTime) send "Accept-Encoding: identity"? My understanding is that this is a fairly well established technology, so we should not invent ad hoc solutions unless absolutely necessary.

I don't know what other media engines send, but from the test result on the mac-ews we can see that the mac build is not sending /any/ Accept-Encoding header, but according to http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html if the header is not sent the server may assume that the client accepts any encoding, which is what we are trying to solve here. Quoting from rfc:
  "If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding. In this case, if "identity" is one of the available content-codings, then the server SHOULD use the "identity" content-coding, unless it has additional information that a different content-coding is meaningful to the client. "

[Andre Moreira Magalhaes]

Created attachment 209469 [details]
Patch

(In reply to comment #68)
> (From update of attachment 209460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209460&action=review
> 
> > LayoutTests/http/tests/media/video-accept-encoding.cgi:14
> > +# Get MIME type if provided.  Default to video/mp4.
> > +my $type = $query->param('type') || "video/mp4";
> 
> This is only used by one test, which always passes type, so why not just die if the parameter is missing?

Fixed.

[Andre Moreira Magalhaes]

Created attachment 209470 [details]
Patch

(In reply to comment #71)
> Created an attachment (id=209469) [details]
> Patch
> 
> (In reply to comment #68)
> > (From update of attachment 209460 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=209460&action=review
> > 
> > > LayoutTests/http/tests/media/video-accept-encoding.cgi:14
> > > +# Get MIME type if provided.  Default to video/mp4.
> > > +my $type = $query->param('type') || "video/mp4";
> > 
> > This is only used by one test, which always passes type, so why not just die if the parameter is missing?
> 
> Fixed.

Now for real :P

[Eric Carlson]

(In reply to comment #70)
> (In reply to comment #69)
> > Do any other media engines (such as QuickTime) send "Accept-Encoding: identity"? My understanding is that this is a fairly well established technology, so we should not invent ad hoc solutions unless absolutely necessary.
> 
> I don't know what other media engines send, but from the test result on the mac-ews we can see that the mac build is not sending /any/ Accept-Encoding header, 

We have two media engines on OSX, AVFoundation and QTKit. We always try to open media files with AVFoundation first because it is generally more capable (and modern), and only try to QTKit if AVFoundation fails. This test will always fall back to QTKit because the cgi does not support HTTP_RANGE, which AVFoundation requires (we include LayoutTests/http/tests/media/resources/serve-video.php for the php based cgis that work with AVFoundation).

I don't know if adding range support will make this test work in AVFoundation, but it would be nice to try because it should be easy enough to add.

[Alexey Proskuryakov]

> if the header is not sent the server may assume that the client accepts any encoding

Of course.

What I'm saying is that we shouldn't be trying to solve this in a way that's different from how everyone else solves it. You need to find out whether other media engines send "Accept-Encoding: identity". If they don't send it, then why don't they have the same problem?

[Andre Moreira Magalhaes]

Created attachment 209485 [details]
Patch

(In reply to comment #73)
> (In reply to comment #70)
> > I don't know what other media engines send, but from the test result on the mac-ews we can see that the mac build is not sending /any/ Accept-Encoding header, 
> 
> We have two media engines on OSX, AVFoundation and QTKit. We always try to open media files with AVFoundation first because it is generally more capable (and modern), and only try to QTKit if AVFoundation fails. This test will always fall back to QTKit because the cgi does not support HTTP_RANGE, which AVFoundation requires (we include LayoutTests/http/tests/media/resources/serve-video.php for the php based cgis that work with AVFoundation).
> 
> I don't know if adding range support will make this test work in AVFoundation, but it would be nice to try because it should be easy enough to add.

New patch contains cgi with range support (code based on video-throttled-load.cgi - see also bug #120219). I left the test enabled on mac and will update the patch again if it fails there.

[Andre Moreira Magalhaes]

(In reply to comment #74)
> > if the header is not sent the server may assume that the client accepts any encoding
> 
> Of course.
> 
> What I'm saying is that we shouldn't be trying to solve this in a way that's different from how everyone else solves it. You need to find out whether other media engines send "Accept-Encoding: identity". If they don't send it, then why don't they have the same problem?

The point here is that we already committed the patch to disable Accept-Encoding support for the gstreamer source (see previous comments for reasoning, especially comments #13 and #17 from Sebastian), this new patch is just to add some bits that were missing from the previous patch that was already applied (unless we want to revert the patch from master we should include this complete fix).

[Alexey Proskuryakov]

That sounds like some kind of temporary hack in the lack of a proper solution. I don't want to have a cross-platform test documenting that expected WebKit behavior is to send identity, unless this is actually our position.

[Andre Moreira Magalhaes]

(In reply to comment #77)
> That sounds like some kind of temporary hack in the lack of a proper solution. I don't want to have a cross-platform test documenting that expected WebKit behavior is to send identity, unless this is actually our position.

I don't see it as a hack at least for the gst case, but maybe this should not be considered cross-platform. As stated before compressing media streams would have no real gain as media formats are already compressed and would only cause extra load on the server and clients - see comment #13 for more details).

What do you suggest in this case? The options I see here are:
1 - Push this fix as is (unless it fails on mac and then I will update the patch to add the exception)
2 - Move the test to be specific to some port (gtk?)
3 - Ignore this fix
4 - Ignore this fix and revert patch already applied

Please let me know what you think. I am leaning towards 1 or 2.

[Build Bot]

Comment on attachment 209485 [details]
Patch

Attachment 209485 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1558079

New failing tests:
http/tests/media/video-accept-encoding.html

[Build Bot]

Created attachment 209500 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Alexey Proskuryakov]

> As stated before compressing media streams would have no real gain as media formats are already compressed

This is not a decision for WebKit, server operators decide what's best for their content, and they shouldn't have to deal with peculiarities of media engines, especially when it's a single port of WebKit that stands out.

I don't see it anywhere in this bug explaining what the user impact is. Are there any live servers that serve media using a non-identity encoding?

If we really think that we should force identity encoding on all WebKit ports, that needs a stronger rationale in my opinion. One example of such could be a popular server that works worse when using some non-gstreamer port (e.g. more CPU time is used than would be used with identity encoding, or playback is lower framerate).

> 1 - Push this fix as is (unless it fails on mac and then I will update the patch to add the exception)
> 2 - Move the test to be specific to some port (gtk?)
> 3 - Ignore this fix
> 4 - Ignore this fix and revert patch already applied

I would be OK with (2) or (4). (2) leaves a certain impression of incompleteness though.

[Andre Moreira Magalhaes]

(In reply to comment #80)
> Created an attachment (id=209500) [details]
> Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
> 
> The attached test failures were seen while running run-webkit-tests on the mac-ews.
> Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

So, the patch failed on mac-ews even with the cgi range support, so expanding on the options we have:

1 - Push this fix but add an exception for mac-ews on platform/mac/TestExpectations
2 - Push this fix but without sending "Accept-Encoding: identity" and update the test accordingly. This would probably make the test pass on mac also but still could bring "issues" on the gst source impl when the server replies with compressed data when no Accept-Encoding header is received as described in the rfc2616.
3 - Push this fix but move test to some specific port (gtk?)
4 - Ignore this fix
5 - Ignore this fix and revert patch already applied

I checked souphttpsrc (another gst element) and it doesn't send any Accept-Encoding header. Tested with "gst-launch playbin uri=http://localhost:8000/multimedia/sample-simple-playback.mp4"

HTTP Traffic:
 GET /multimedia/sample-simple-playback.mp4 HTTP/1.1
 Host: localhost:8000
 Connection: close
 icy-metadata: 1
 transferMode.dlna.org: Streaming
 User-Agent: GStreamer souphttpsrc libsoup/2.40.3

[Andre Moreira Magalhaes]

(In reply to comment #82)
> (In reply to comment #80)
> > Created an attachment (id=209500) [details] [details]
> > Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
> > 
> > The attached test failures were seen while running run-webkit-tests on the mac-ews.
> > Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
> 
> So, the patch failed on mac-ews even with the cgi range support, so expanding on the options we have:
> 
> 1 - Push this fix but add an exception for mac-ews on platform/mac/TestExpectations
> 2 - Push this fix but without sending "Accept-Encoding: identity" and update the test accordingly. This would probably make the test pass on mac also but still could bring "issues" on the gst source impl when the server replies with compressed data when no Accept-Encoding header is received as described in the rfc2616.
> 3 - Push this fix but move test to some specific port (gtk?)
> 4 - Ignore this fix
> 5 - Ignore this fix and revert patch already applied
> 
> I checked souphttpsrc (another gst element) and it doesn't send any Accept-Encoding header. Tested with "gst-launch playbin uri=http://localhost:8000/multimedia/sample-simple-playback.mp4"
> 
> HTTP Traffic:
>  GET /multimedia/sample-simple-playback.mp4 HTTP/1.1
>  Host: localhost:8000
>  Connection: close
>  icy-metadata: 1
>  transferMode.dlna.org: Streaming
>  User-Agent: GStreamer souphttpsrc libsoup/2.40.3

Ok, I checked here and with the patches for bug #116533 and bug #116534 already applied this patch should not be required anymore, so I guess the best to do here is option 5. Either that or we pick option 2 and do not send any Accept-Encoding header for media streams.

[Andre Moreira Magalhaes]

Created attachment 209514 [details]
Patch

(In reply to comment #83)
> (In reply to comment #82)
> > So, the patch failed on mac-ews even with the cgi range support, so expanding on the options we have:
> > 
> > 1 - Push this fix but add an exception for mac-ews on platform/mac/TestExpectations
> > 2 - Push this fix but without sending "Accept-Encoding: identity" and update the test accordingly. This would probably make the test pass on mac also but still could bring "issues" on the gst source impl when the server replies with compressed data when no Accept-Encoding header is received as described in the rfc2616.
> > 3 - Push this fix but move test to some specific port (gtk?)
> > 4 - Ignore this fix
> > 5 - Ignore this fix and revert patch already applied
> 
> Ok, I checked here and with the patches for bug #116533 and bug #116534 already applied this patch should not be required anymore, so I guess the best to do here is option 5. Either that or we pick option 2 and do not send any Accept-Encoding header for media streams.

Ok, new patch implementing option 2 (not sending any accept-encoding header) just in case. This should pass on mac also, lets see. Not sure we want to include the test though, maybe this should be considered implementation specific.

Let me know what you think.

[Build Bot]

Comment on attachment 209514 [details]
Patch

Attachment 209514 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1579211

[Gustavo Noronha (kov)]

Can't find the error on the windows build log, but it's most likely unrelated to this patch, the EWS seems to be broken (by the looks of http://webkit-queues.appspot.com/queue-status/win-ews)

[Andre Moreira Magalhaes]

(In reply to comment #86)
> Can't find the error on the windows build log, but it's most likely unrelated to this patch, the EWS seems to be broken (by the looks of http://webkit-queues.appspot.com/queue-status/win-ews)

So, any decision here on whether to apply the new patch or revert the old one? I would vote for applying the new patch as it makes the webkit source implementation more similar to what is done on mac (sending no Accept-Encoding header at all on media requests).

[ChangSeok Oh]

(In reply to comment #87)
> (In reply to comment #86)
> > Can't find the error on the windows build log, but it's most likely unrelated to this patch, the EWS seems to be broken (by the looks of http://webkit-queues.appspot.com/queue-status/win-ews)
> 
> So, any decision here on whether to apply the new patch or revert the old one? I would vote for applying the new patch as it makes the webkit source implementation more similar to what is done on mac (sending no Accept-Encoding header at all on media requests).

The red for win ews might be a false alarm. how about retrying?

[ChangSeok Oh]

Created attachment 210261 [details]
Patch

[ChangSeok Oh]

(In reply to comment #89)
> Created an attachment (id=210261) [details]
> Patch

I just retriggered regression tests for andre's patch

[Philippe Normand]

(In reply to comment #88)
> (In reply to comment #87)
> > (In reply to comment #86)
> > > Can't find the error on the windows build log, but it's most likely unrelated to this patch, the EWS seems to be broken (by the looks of http://webkit-queues.appspot.com/queue-status/win-ews)
> > 
> > So, any decision here on whether to apply the new patch or revert the old one? I would vote for applying the new patch as it makes the webkit source implementation more similar to what is done on mac (sending no Accept-Encoding header at all on media requests).
> 
> The red for win ews might be a false alarm. how about retrying?

The win EWS is not known for being very stable :)

[ChangSeok Oh]

(In reply to comment #91)
> (In reply to comment #88)
> > (In reply to comment #87)
> > > (In reply to comment #86)
> > > > Can't find the error on the windows build log, but it's most likely unrelated to this patch, the EWS seems to be broken (by the looks of http://webkit-queues.appspot.com/queue-status/win-ews)
> > > 
> > > So, any decision here on whether to apply the new patch or revert the old one? I would vote for applying the new patch as it makes the webkit source implementation more similar to what is done on mac (sending no Accept-Encoding header at all on media requests).
> > 
> > The red for win ews might be a false alarm. how about retrying?
> 
> The win EWS is not known for being very stable :)

Philippe, do you have any concern to review this?

[Gustavo Noronha (kov)]

Comment on attachment 210261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210261&action=review

> Tools/gtk/jhbuild.modules:187
> +            md5sum="a632a38d2983c2a88679672d00940128">
> +      <patch file="libsoup-2.42.0-also-copy-disabled-features-when-copying-messages.patch" strip="1"/>

AFAIK this patch has been accepted in libsoup, perhaps we can bump the version here instead of adding the patch?

[ChangSeok Oh]

(In reply to comment #93)
> (From update of attachment 210261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210261&action=review
> 
> > Tools/gtk/jhbuild.modules:187
> > +            md5sum="a632a38d2983c2a88679672d00940128">
> > +      <patch file="libsoup-2.42.0-also-copy-disabled-features-when-copying-messages.patch" strip="1"/>
> 
> AFAIK this patch has been accepted in libsoup, perhaps we can bump the version here instead of adding the patch?

O.K Let me open a new bug for it, if necessary.

[ChangSeok Oh]

Comment on attachment 210261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210261&action=review

>>> Tools/gtk/jhbuild.modules:187
>>> +      <patch file="libsoup-2.42.0-also-copy-disabled-features-when-copying-messages.patch" strip="1"/>
>> 
>> AFAIK this patch has been accepted in libsoup, perhaps we can bump the version here instead of adding the patch?
> 
> O.K Let me open a new bug for it, if necessary.

The latest release of libsoup 2.43.90 doesn't include the Andre's patch. So currently we should use its git repo directly. Any idea?

[ChangSeok Oh]

Created attachment 210324 [details]
Patch

[ChangSeok Oh]

Comment on attachment 210324 [details]
Patch

This patch got a review from Philippe and I just removed libsoup patch part from the previous patch.

[WebKit Commit Bot]

Comment on attachment 210324 [details]
Patch

Clearing flags on attachment: 210324

Committed r154977: <http://trac.webkit.org/changeset/154977>

[Andre Moreira Magalhaes]

What happened to the libsoup patch here? Did we update jhbuild to use libsoup from git? The libsoup patch is required to make this change work.

[Gustavo Noronha (kov)]

I was also curious, it turns out the bump in jhbuild got landed separately: http://trac.webkit.org/changeset/154972

Marking this bug resolved/fixed.

[ChangSeok Oh]

(In reply to comment #100)
> I was also curious, it turns out the bump in jhbuild got landed separately: http://trac.webkit.org/changeset/154972
> 
> Marking this bug resolved/fixed.

Opps. I forgot to leave the address for libsoup upversion. Sorry.