Bug 115354

Summary:

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

Product:

WebKit

Reporter:

Andre Moreira Magalhaes <andrunko>

Component:

WebCore Misc.

Assignee:

Gustavo Noronha (kov) <gustavo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, buildbot, cdumez, cgarcia, changseok, commit-queue, danw, darin, eflews.bot, eric.carlson, glenn, gtk-ews, gustavo, gyuyoung.kim, jer.noble, menard, mrobinson, pnormand, rakuco, rego+ews, rniwa, slomo, svillar, webkit-ews, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

115352, 115353, 120613

Bug Blocks:

116533, 116534

Attachments:

Description	Flags
Patch	eflews.bot: commit-queue-
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2	none
Do not check seek offset against internal size	none
Adjust internal size when receiving data if needed	none
Patch	none
Do not check seek offset against internal size	none
Adjust internal size when receiving data if needed	none
Patch for landing	none
Patch	none
Patch	none
Patch	none
Patch	buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion	none
Patch	none
Patch	pnormand: review+
Patch	none

Andre Moreira Magalhaes

Reported 2013-04-29 07:55:47 PDT

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.

Attachments
Patch (4.14 KB, patch) 2013-04-29 07:58 PDT, Andre Moreira Magalhaes	eflews.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (537.41 KB, application/zip) 2013-04-29 23:03 PDT, Build Bot	no flags	Details
Do not check seek offset against internal size (2.35 KB, patch) 2013-05-03 04:40 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Adjust internal size when receiving data if needed (2.61 KB, patch) 2013-05-03 04:41 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Patch (3.96 KB, patch) 2013-05-20 10:27 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Do not check seek offset against internal size (2.31 KB, patch) 2013-05-20 10:27 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Adjust internal size when receiving data if needed (2.58 KB, patch) 2013-05-20 10:28 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Patch for landing (2.70 KB, patch) 2013-08-07 07:20 PDT, Gustavo Noronha (kov)	no flags	Details Formatted Diff Diff
Patch (6.38 KB, patch) 2013-08-07 09:49 PDT, Gustavo Noronha (kov)	no flags	Details Formatted Diff Diff
Patch (6.40 KB, patch) 2013-08-07 10:33 PDT, Gustavo Noronha (kov)	no flags	Details Formatted Diff Diff
Patch (7.33 KB, patch) 2013-08-19 14:54 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Patch (11.90 KB, patch) 2013-08-22 12:46 PDT, Andre Moreira Magalhaes	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (753.55 KB, application/zip) 2013-08-22 14:59 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (487.14 KB, application/zip) 2013-08-22 22:35 PDT, Build Bot	no flags	Details
Patch (12.57 KB, patch) 2013-08-23 06:34 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Patch (12.61 KB, patch) 2013-08-23 07:25 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Patch (12.61 KB, patch) 2013-08-23 10:15 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Patch (12.58 KB, patch) 2013-08-23 10:17 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Patch (13.03 KB, patch) 2013-08-23 12:03 PDT, Andre Moreira Magalhaes	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (602.69 KB, application/zip) 2013-08-23 13:14 PDT, Build Bot	no flags	Details
Patch (12.41 KB, patch) 2013-08-23 14:41 PDT, Andre Moreira Magalhaes	no flags	Details Formatted Diff Diff
Patch (10.78 KB, patch) 2013-09-01 22:15 PDT, ChangSeok Oh	pnormand: review+	Details Formatted Diff Diff
Patch (7.27 KB, patch) 2013-09-02 22:55 PDT, ChangSeok Oh	no flags	Details Formatted Diff Diff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.

Andre Moreira Magalhaes

Comment 1 2013-04-29 07:58:45 PDT

Created attachment 200011 [details] Patch

EFL EWS Bot

Comment 2 2013-04-29 08:04:16 PDT

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 3 2013-04-29 08:04:17 PDT

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 4 2013-04-29 08:04:39 PDT

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 5 2013-04-29 08:04:46 PDT

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

Comment 6 2013-04-29 08:05:56 PDT

Could this be the cause of bug 90732 ?

Andre Moreira Magalhaes

Comment 7 2013-04-29 08:20:08 PDT

(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

Comment 8 2013-04-29 08:30:40 PDT

(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

Comment 9 2013-04-29 08:44:17 PDT

(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

Comment 10 2013-04-29 08:47:31 PDT

(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

Comment 11 2013-04-29 09:01:30 PDT

(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 12 2013-04-29 10:15:21 PDT

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)

Comment 13 2013-04-29 10:26:42 PDT

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 14 2013-04-29 11:31:16 PDT

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 15 2013-04-29 23:03:51 PDT

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

Comment 16 2013-04-29 23:03:55 PDT

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)

Comment 17 2013-05-03 02:44:06 PDT

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)

Comment 18 2013-05-03 02:54:40 PDT

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)

Comment 19 2013-05-03 03:41:22 PDT

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

Comment 20 2013-05-03 04:40:16 PDT

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

Comment 21 2013-05-03 04:41:50 PDT

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)

Comment 22 2013-05-03 04:45:27 PDT

Looks all good to me, thanks!

Andre Moreira Magalhaes

Comment 23 2013-05-20 10:27:12 PDT

Created attachment 202287 [details] Patch Updated Changelog to use proper format.

Andre Moreira Magalhaes

Comment 24 2013-05-20 10:27:51 PDT

Created attachment 202288 [details] Do not check seek offset against internal size Updated Changelog to use proper format.

Andre Moreira Magalhaes

Comment 25 2013-05-20 10:28:30 PDT

Created attachment 202290 [details] Adjust internal size when receiving data if needed Updated Changelog to use proper format.

Andre Moreira Magalhaes

Comment 26 2013-05-20 10:30:12 PDT

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

Early Warning System Bot

Comment 27 2013-05-20 10:31:45 PDT

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 28 2013-05-20 10:32:05 PDT

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

Comment 29 2013-05-20 10:32:17 PDT

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 30 2013-05-20 10:32:18 PDT

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 31 2013-05-20 10:33:03 PDT

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 32 2013-05-20 18:33:03 PDT

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

Comment 33 2013-05-21 02:20:59 PDT

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

Andre Moreira Magalhaes

Comment 34 2013-05-21 06:16:53 PDT

Comment on attachment 202288 [details] Do not check seek offset against internal size Added new bug #116533 for this patch

Andre Moreira Magalhaes

Comment 35 2013-05-21 06:17:10 PDT

Comment on attachment 202290 [details] Adjust internal size when receiving data if needed Added new bug #116534 for this patch

Andre Moreira Magalhaes

Comment 36 2013-05-21 06:20:13 PDT

(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 37 2013-05-27 03:47:19 PDT

Comment on attachment 202287 [details] Patch Ok, I suppose this patch builds correctly now?

Andre Moreira Magalhaes

Comment 38 2013-05-31 07:55:59 PDT

(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)

Comment 39 2013-07-10 00:53:21 PDT

Does it apply cleanly now?

Gustavo Noronha (kov)

Comment 40 2013-08-07 07:20:48 PDT

Created attachment 208265 [details] Patch for landing

kov's GTK+ EWS bot

Comment 41 2013-08-07 07:28:46 PDT

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 42 2013-08-07 07:32:17 PDT

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 43 2013-08-07 07:34:42 PDT

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 44 2013-08-07 07:39:05 PDT

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 45 2013-08-07 08:01:02 PDT

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)

Comment 46 2013-08-07 09:28:24 PDT

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

Gustavo Noronha (kov)

Comment 47 2013-08-07 09:49:57 PDT

Created attachment 208276 [details] Patch

Gustavo Noronha (kov)

Comment 48 2013-08-07 09:51:33 PDT

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 49 2013-08-07 09:55:28 PDT

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 50 2013-08-07 09:56:13 PDT

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 51 2013-08-07 10:07:00 PDT

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)

Comment 52 2013-08-07 10:33:41 PDT

Created attachment 208280 [details] Patch

Gustavo Noronha (kov)

Comment 53 2013-08-07 10:34:34 PDT

Comment on attachment 208280 [details] Patch Missing USE(SOUP) check, actually, this one should work!

Gustavo Noronha (kov)

Comment 54 2013-08-07 10:40:39 PDT

Comment on attachment 208280 [details] Patch Thanks phil!

WebKit Commit Bot

Comment 55 2013-08-07 11:39:31 PDT

Comment on attachment 208280 [details] Patch Clearing flags on attachment: 208280 Committed r153795: <http://trac.webkit.org/changeset/153795>

WebKit Commit Bot

Comment 56 2013-08-07 11:39:38 PDT

All reviewed patches have been landed. Closing bug.

Andre Moreira Magalhaes

Comment 57 2013-08-19 14:54:30 PDT

Created attachment 209126 [details] Patch

Andre Moreira Magalhaes

Comment 58 2013-08-19 14:56:08 PDT

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

Comment 59 2013-08-19 14:56:58 PDT

Arg, reopening again, see comment #58.

Gustavo Noronha (kov)

Comment 60 2013-08-19 15:29:28 PDT

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

Andre Moreira Magalhaes

Comment 61 2013-08-22 12:46:07 PDT

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 62 2013-08-22 14:59:29 PDT

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

Comment 63 2013-08-22 14:59:34 PDT

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 64 2013-08-22 22:35:51 PDT

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

Comment 65 2013-08-22 22:35:57 PDT

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

Comment 66 2013-08-23 06:34:58 PDT

Created attachment 209456 [details] Patch Mark test as failing on mac.

Andre Moreira Magalhaes

Comment 67 2013-08-23 07:25:52 PDT

Created attachment 209460 [details] Patch New attempt on top of today's master.

Eric Carlson

Comment 68 2013-08-23 09:42:49 PDT

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

Comment 69 2013-08-23 09:53:17 PDT

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

Comment 70 2013-08-23 10:09:31 PDT

(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

Comment 71 2013-08-23 10:15:06 PDT

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

Comment 72 2013-08-23 10:17:08 PDT

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

Comment 73 2013-08-23 10:32:32 PDT

(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

Comment 74 2013-08-23 11:21:55 PDT

> 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

Comment 75 2013-08-23 12:03:37 PDT

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

Comment 76 2013-08-23 12:17:28 PDT

(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

Comment 77 2013-08-23 12:21:27 PDT

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

Comment 78 2013-08-23 12:38:41 PDT

(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 79 2013-08-23 13:14:52 PDT

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

Comment 80 2013-08-23 13:14:59 PDT

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

Comment 81 2013-08-23 13:37:18 PDT

> 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

Comment 82 2013-08-23 13:48:42 PDT

(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

Comment 83 2013-08-23 14:02:50 PDT

(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

Comment 84 2013-08-23 14:41:43 PDT

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 85 2013-08-27 01:51:11 PDT

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)

Comment 86 2013-08-27 06:29:54 PDT

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

Comment 87 2013-08-29 13:32:36 PDT

(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

Comment 88 2013-08-29 22:40:05 PDT

(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

Comment 89 2013-09-01 22:15:10 PDT

Created attachment 210261 [details] Patch

ChangSeok Oh

Comment 90 2013-09-01 22:17:03 PDT

(In reply to comment #89) > Created an attachment (id=210261) [details] > Patch I just retriggered regression tests for andre's patch

Philippe Normand

Comment 91 2013-09-02 00:05:52 PDT

(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

Comment 92 2013-09-02 00:23:38 PDT

(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 93 2013-09-02 05:10:15 PDT

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

Comment 94 2013-09-02 07:27:27 PDT

(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 95 2013-09-02 08:14:21 PDT

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

Comment 96 2013-09-02 22:55:52 PDT

Created attachment 210324 [details] Patch

ChangSeok Oh

Comment 97 2013-09-03 05:53:02 PDT

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 98 2013-09-03 06:15:54 PDT

Comment on attachment 210324 [details] Patch Clearing flags on attachment: 210324 Committed r154977: <http://trac.webkit.org/changeset/154977>

Andre Moreira Magalhaes

Comment 99 2013-09-03 10:52:15 PDT

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)

Comment 100 2013-09-03 10:56:38 PDT

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

Comment 101 2013-09-03 20:34:27 PDT

(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.

Note You need to log in before you can comment on or make changes to this bug.