Bug 115353

Created attachment 200010 [details] Patch

(In reply to comment #0) > This is especially needed by the webkit gstreamer source element which needs to rely on the correct data size to download streams. That seems improbable... what do you do if there's no Content-Length header at all? I think the problem is just that the gstreamer source needs to know to not look at the declared Content-Length if the SOUP_MESSAGE_CONTENT_DECODED flag is set.

What happens here is that: 1 - If we don't set the size on gst appsrc (set to -1), it will break seek and consider the stream a live stream 2 - if we set the wrong size (in case the content-length has the compressed size opposed to the actual size), gst will consider EOF before the stream ends We could probably indeed set the size to -1 by checking this flag you mentioned, but seek would be broken for all servers returning compressed data.(In reply to comment #2) > (In reply to comment #0) > > This is especially needed by the webkit gstreamer source element which needs to rely on the correct data size to download streams. > > That seems improbable... what do you do if there's no Content-Length header at all? > > I think the problem is just that the gstreamer source needs to know to not look at the declared Content-Length if the SOUP_MESSAGE_CONTENT_DECODED flag is set. What happens here is that: 1 - If we don't set the size on gst appsrc (set to -1), it will break seek and consider the stream a live stream 2 - if we set the wrong size (in case the content-length has the compressed size opposed to the actual size), gst will consider EOF before the stream ends We could probably indeed set the size to -1 by checking this flag you mentioned, but seek would be "broken" (disabled) for all servers returning compressed data.

See also https://bugs.webkit.org/show_bug.cgi?id=115354#c13

(In reply to comment #3) > We could probably indeed set the size to -1 by checking this flag you mentioned, but seek would be broken for all servers returning compressed data. There's a bug open about adding an API to SoupMessage to extract the actual content length out of the gzip metadata when the stream is compressed. (https://bugzilla.gnome.org/show_bug.cgi?id=664620). So that would be another solution. But it's true, as Sebastian notes in the other bug, that multimedia streams ought to already contain compressed data and thus not be usefully compressible anyway...

Comment on attachment 200010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200010&action=review Overall I agree with Dan's comments, I think they're quite aligned with my concerns exposed in bug 115354. Likely we should use some libsoup API to provide such information but as we aren't bumping requirements soon I guess it's fine. Anyway, based on the fact that multimedia streams are most of them already compressed data, it shouldn't hurt not to use compression at HTTP level. > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:61 > + // Disable SoupContentDecoder as it adds a "Accept-Encoding:" field to the headers I don't think this comment is needed

Created attachment 202285 [details] Patch (In reply to comment #6) > > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:61 > > + // Disable SoupContentDecoder as it adds a "Accept-Encoding:" field to the headers > > I don't think this comment is needed Removed comment. Also updated Changelog to use the correct format.

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

Adding Gustavo and Martin in CC.

Comment on attachment 202285 [details] Patch It appears that nothing in this patch is actually calling setAcceptEncoding.

The patch in bug 115354 does :)

(In reply to comment #11) > The patch in bug 115354 does :) This should really be one change instead of pushing a patch that uses dead code. I'm also not sure about the name. Shouldn't it be something like setCompressionEnabled or similar? I'm also a bit wary of this because it's a new method on ResourceRequestBase, but only implemented for libsoup. Is this a problem for Qt?

(In reply to comment #12) > (In reply to comment #11) > > The patch in bug 115354 does :) > > This should really be one change instead of pushing a patch that uses dead code. I'm also not sure about the name. Shouldn't it be something like setCompressionEnabled or similar? I'm also a bit wary of this because it's a new method on ResourceRequestBase, but only implemented for libsoup. Is this a problem for Qt? As for the patch split, I am not used to the webkit commit scheme and I usually try to make patches per functionality. In this case one patch is to enable the support to disable AcceptEncoding field on requests (this bug) and the other is to actually disable it on the gstreamer player (bug #115354), hence the separate bugs. The name chosen was because the header is called Accept-Encoding, but I have no hard preference here. As for this being a problem with Qt, I don't think this is the case as the patch keeps the same default behaviour.

(In reply to comment #13) > As for this being a problem with Qt, I don't think this is the case as the patch keeps the same default behaviour. What I mean is that there is no implementation of this fix for the Qt networking layer, yet the Qt port uses libsoup. Does this problem affect Qt? If so we need to fix it as well or open a bug. If not, that may lead to the interesting question of "Why not?" and perhaps an alternate fix for this issue. Just some thoughts.

(In reply to comment #14) > (In reply to comment #13) > > > As for this being a problem with Qt, I don't think this is the case as the patch keeps the same default behaviour. > > What I mean is that there is no implementation of this fix for the Qt networking layer, yet the Qt port uses libsoup. Does this problem affect Qt? If so we need to fix it as well or open a bug. If not, that may lead to the interesting question of "Why not?" and perhaps an alternate fix for this issue. Just some thoughts. It affects any port, it's just not *that* bad (with Andre's other patches) and will not cause any actual problems... other than causing more load on the server to compress something that is already compressed.

In any case, we should not add a property to a base class that's only used by a subclass and only called in another patch, thus making it dead code until some future time. :)

(In reply to comment #14) > What I mean is that there is no implementation of this fix for the Qt networking layer, yet the Qt port uses libsoup. danw pointed out that "the Qt port uses libsoup" is wildly inaccurate. That's because I meant to write "the Qt port uses GStreamer." I'm just noting this for posterity. :)

(In reply to comment #16) > In any case, we should not add a property to a base class that's only used by a subclass and only called in another patch, thus making it dead code until some future time. :) Sounds fair enough :) Andre can you merge this patch and the one in bug 115354 ? If possible not on top of the one from bug 115352.

I'm working on this merge, I think it makes sense to handle it in the other bug, which talks about the actual change we want to make to behaviour, so closing this one as a dup. *** This bug has been marked as a duplicate of bug 115354 ***