| 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: | P2 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 115352, 115353, 120613 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 116533, 116534 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Created attachment 200011 [details]
Patch
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 Comment on attachment 200011 [details] Patch Attachment 200011 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/137878 Comment on attachment 200011 [details] Patch Attachment 200011 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/229513 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 (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. (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. (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. (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. (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? Comment on attachment 200011 [details] Patch Attachment 200011 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/121826 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. 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 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 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
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. 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 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. 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. 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. Looks all good to me, thanks! Created attachment 202287 [details]
Patch
Updated Changelog to use proper format.
Created attachment 202288 [details]
Do not check seek offset against internal size
Updated Changelog to use proper format.
Created attachment 202290 [details]
Adjust internal size when receiving data if needed
Updated Changelog to use proper format.
Thanks all for the review, all patches are updated now with comments applied. Comment on attachment 202287 [details] Patch Attachment 202287 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/370390 Comment on attachment 202287 [details] Patch Attachment 202287 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/415525 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.
Comment on attachment 202287 [details] Patch Attachment 202287 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/502013 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 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 We deal with one patch per bug usually, can you please merge those 3 patches in one? Thanks! Comment on attachment 202288 [details] Do not check seek offset against internal size Added new bug #116533 for this patch Comment on attachment 202290 [details] Adjust internal size when receiving data if needed Added new bug #116534 for this patch (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 Comment on attachment 202287 [details]
Patch
Ok, I suppose this patch builds correctly now?
(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? Does it apply cleanly now? Created attachment 208265 [details]
Patch for landing
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 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 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 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 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 *** Bug 115353 has been marked as a duplicate of this bug. *** Created attachment 208276 [details]
Patch
Comment on attachment 208276 [details]
Patch
All feedback has been included, hopefully. Uploading it here so Philippe can give it a final once-over =)
Comment on attachment 208276 [details] Patch Attachment 208276 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1370837 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 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
Created attachment 208280 [details]
Patch
Comment on attachment 208280 [details]
Patch
Missing USE(SOUP) check, actually, this one should work!
Comment on attachment 208280 [details]
Patch
Thanks phil!
Comment on attachment 208280 [details] Patch Clearing flags on attachment: 208280 Committed r153795: <http://trac.webkit.org/changeset/153795> All reviewed patches have been landed. Closing bug. Created attachment 209126 [details]
Patch
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. Arg, reopening again, see comment #58. I think we should have a layout test for this, to ensure it's fixed this time (oops) and not regress. 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. 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 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
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 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
Created attachment 209456 [details]
Patch
Mark test as failing on mac.
Created attachment 209460 [details]
Patch
New attempt on top of today's master.
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? 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. (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. " 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. 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 (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. > 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?
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. (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). 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. (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. 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 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
> 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. (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 (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. 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. Comment on attachment 209514 [details] Patch Attachment 209514 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1579211 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) (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). (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? Created attachment 210261 [details]
Patch
(In reply to comment #89) > Created an attachment (id=210261) [details] > Patch I just retriggered regression tests for andre's patch (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 :) (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? 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? (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. 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? Created attachment 210324 [details]
Patch
Comment on attachment 210324 [details]
Patch
This patch got a review from Philippe and I just removed libsoup patch part from the previous patch.
Comment on attachment 210324 [details] Patch Clearing flags on attachment: 210324 Committed r154977: <http://trac.webkit.org/changeset/154977> 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. 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. (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. |
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.