Bug 85994 – [GStreamer] Seeking fails on media content provided by servers not supporting Range requests

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

Any .ogg resource from http://www.playmapscube.com is failing to download after retrieving 13 bytes from each of them (at least what is shown in the inspector).

According to philn-tp it's an issue in the source element.

PS: You do not need to load always that page for testing, it fails also on every single .ogg, for example http://www.playmapscube.com/proto/sounds/rolling-05.ogg

[Philippe Normand]

The relevant GST_DEBUG bit:

[msg 0xb1c19070] posting on bus, type error, GstMessageError, gerror=(GError)NULL, debug=(string)"../../Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp\(781\):\ didReceiveResponse\ \(\):\ /GstPlayBin2:play/GstURIDecodeBin:uridecodebin0/WebKitWebSrc:source"; from source <source>

[Simon Pena]

(In reply to comment #0)
> Any .ogg resource from http://www.playmapscube.com is failing to download after retrieving 13 bytes from each of them (at least what is shown in the inspector).
> 
> According to philn-tp it's an issue in the source element.
> 
> PS: You do not need to load always that page for testing, it fails also on every single .ogg, for example http://www.playmapscube.com/proto/sounds/rolling-05.ogg

When I started investigating this yesterday, I was unable to get the ogg playing, when I pointed GtkLauncher or MiniBrowser to the URL you give. This seems to be working fine now: if I directly open the ogg, it plays. 

However, if I open the main page (http://www.playmapscube.com), it gets stuck after a while (the load progress seems to be around the 75-80%). It doesn't continue from that point on.

Approximately at this point, I start getting GStreamer errors:

> Error: 9, Could not read from resource.
> Error: 1, GStreamer encountered a general core library error.
> Error: 9, Could not read from resource.
> Error: 9, Could not demultiplex stream

[Sergio Villar Senin]

(In reply to comment #2)
> (In reply to comment #0)
> > Any .ogg resource from http://www.playmapscube.com is failing to download after retrieving 13 bytes from each of them (at least what is shown in the inspector).
> > 
> > According to philn-tp it's an issue in the source element.
> > 
> > PS: You do not need to load always that page for testing, it fails also on every single .ogg, for example http://www.playmapscube.com/proto/sounds/rolling-05.ogg
> 
> When I started investigating this yesterday, I was unable to get the ogg playing, when I pointed GtkLauncher or MiniBrowser to the URL you give. This seems to be working fine now: if I directly open the ogg, it plays. 

You mean that after updating they're working fine now? I'll try

[Simon Pena]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > Any .ogg resource from http://www.playmapscube.com is failing to download after retrieving 13 bytes from each of them (at least what is shown in the inspector).
> > > 
> > > According to philn-tp it's an issue in the source element.
> > > 
> > > PS: You do not need to load always that page for testing, it fails also on every single .ogg, for example http://www.playmapscube.com/proto/sounds/rolling-05.ogg
> > 
> > When I started investigating this yesterday, I was unable to get the ogg playing, when I pointed GtkLauncher or MiniBrowser to the URL you give. This seems to be working fine now: if I directly open the ogg, it plays. 
> 
> You mean that after updating they're working fine now? I'll try

No, I didn't update the repository since yesterday. What wasn't working previously is working now, as if it was fixed in the server side, since I didn't change anything in WebKit.

However, I still get GStreamer errors when loading the page, so there's still something wrong going on.

[Philippe Normand]

It should be possible to write a layout test faking an http server not supporting Ranges and serving an ogg file. You can find examples in LayoutTests/http/tests/media

[Philippe Normand]

(In reply to comment #2)

> Approximately at this point, I start getting GStreamer errors:
> 
> > Error: 9, Could not read from resource.
> > Error: 1, GStreamer encountered a general core library error.
> > Error: 9, Could not read from resource.
> > Error: 9, Could not demultiplex stream

Usually I run the launcher with GST_DEBUG=4 GST_DEBUG_NO_COLOR=1 then have a look at the log with the gst-debug-viewer.

In this case however you can reduce the log with GST_DEBUG=webkitwebsrc:4 I think :)

[Simon Pena]

(In reply to comment #6)
> (In reply to comment #2)
> 
> > Approximately at this point, I start getting GStreamer errors:
> > 
> > > Error: 9, Could not read from resource.
> > > Error: 1, GStreamer encountered a general core library error.
> > > Error: 9, Could not read from resource.
> > > Error: 9, Could not demultiplex stream
> 
> Usually I run the launcher with GST_DEBUG=4 GST_DEBUG_NO_COLOR=1 then have a look at the log with the gst-debug-viewer.
> 
> In this case however you can reduce the log with GST_DEBUG=webkitwebsrc:4 I think :)

Thanks! I'll do that. I used GST_DEBUG=4, but didn't know about the other options :-)

[Simon Pena]

I'm not sure if that's the route cause of the issue, or just noise but,
as Phil mentions in comment #1, we get an error when we're expecting a
PARTIAL_CONTENT response and instead we get a 200 status.

I checked this a bit, and found that where Firefox gets the 206 status,
MiniBrowser doesn't seem to get them.

Also, using manually curl, with similar parameters as 

curl -v --header "transferMode.dlna:  Streaming" \
     --range 38050-
     --user-agent "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/536.10+ (KHTML, like Gecko) Chromium/17.0.963.56 Chrome/17.0.963.56 Safari/536.10+"\
     "http://www.playmapscube.com/proto/sounds/rolling-05.ogg" \
     --compressed 1>/dev/null 

> GET /proto/sounds/rolling-05.ogg HTTP/1.1
> Range: bytes=38050-
> User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/536.10+ (KHTML, like Gecko) Chromium/17.0.963.56 Chrome/17.0.963.56 Safari/536.10+
> Host: www.playmapscube.com
> Accept: */*
> Accept-Encoding: deflate, gzip
> transferMode.dlna:  Streaming
> 
< HTTP/1.1 206 Partial Content
< ETag: "zgk01w"
< Date: Mon, 14 May 2012 17:06:05 GMT
< Expires: Mon, 14 May 2012 17:16:05 GMT
< Cache-Control: public, max-age=600
< Content-Range: bytes 38050-111777/111778
< Content-Type: audio/ogg
< Server: Google Frontend
< Content-Length: 73728

You can see that the server replies wit the Partian Content status code.
However, this is what the logs show about MiniBrowser

> 0:00:29.926308602 12004      0x1460930 DEBUG           webkitwebsrc ../../Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:780:didReceiveResponse:<source> Received response: 200 from http://www.playmapscube.com/proto/sounds/rolling-05.ogg at 38050

[Sergio Villar Senin]

(In reply to comment #8)
> I'm not sure if that's the route cause of the issue, or just noise but,
> as Phil mentions in comment #1, we get an error when we're expecting a
> PARTIAL_CONTENT response and instead we get a 200 status.

User Agent issue maybe?

[Simon Pena]

(In reply to comment #9)
> (In reply to comment #8)
> > I'm not sure if that's the route cause of the issue, or just noise but,
> > as Phil mentions in comment #1, we get an error when we're expecting a
> > PARTIAL_CONTENT response and instead we get a 200 status.
> 
> User Agent issue maybe?

Nope, that's the same User Agent that GtkLauncher uses.

I've run GtkLauncher from behind a proxy, and all its ogg requests get a 200 response status. Some of them, when done with curl with exactly the same parameters, get the expected 206 status.

The thing is, this fails for resources that had already been requested previously. Basically:

> Resource A requested (no offset)
< Resource A retrieved (status 200)
> Resource A requested again (this time with an offset)
< Resource A retrieved (status 200, when 206 for partial content is expected)

[Simon Pena]

According to this:
http://www.greenbytes.de/tech/webdav/draft-ietf-httpbis-p5-range-latest.html

> If the server ignores a byte-range-spec (for example if it is
> syntactically invalid, or if it may be seen as a denial-of-service
> attack), the server SHOULD treat the request as if the invalid Range
> header field did not exist. (Normally, this means return a 200
> response containing the full representation).

Basically, we should be able to handle a 200 code even if we were
expecting the 206. I'm still unsure that this is the root cause of the
issue.

[Sergio Villar Senin]

Could it be related to this? https://bugzilla.gnome.org/show_bug.cgi?id=658309

I noticed that for example firefox sends the Range header in his first request (maybe because it infers we're requesting a potential "big" file from the url). WKGtk OTOH does only set the Range header once it receives a seek from GStreamer.

[Dan Winship]

(In reply to comment #10)
> The thing is, this fails for resources that had already been requested previously. Basically:
> 
> > Resource A requested (no offset)
> < Resource A retrieved (status 200)
> > Resource A requested again (this time with an offset)
> < Resource A retrieved (status 200, when 206 for partial content is expected)

It sounds like there is a caching reverse proxy in front of the server, that doesn't bother to implement ranged requests on cached resources. So if you make a non-ranged request the first time, then any further ranged requests after that (until the file expires from the cache) will return the whole resource.

(In reply to comment #11)
> Basically, we should be able to handle a 200 code even if we were
> expecting the 206.

Yes. Servers are not required to implement ranged requests, and clients are required to deal with getting back 200 instead of 206.

[Simon Pena]

I've currently stopped working on this bug. If someone wants to catch up from here, I'd recommend to check this:

* Ranged requests: we should support "200" status response besides "206", since it's not mandatory for servers to support it (see comment #11 and comment #13).

* Sergio, in comment #12, mentions that we're not doing a ranged request from the beginning, while Firefox does, so we might be doing an extra ranged request.

I imagine it should be possible to seek on the "cached" resource once it has been already downloaded, instead of expecting the server to provide it from the offset we want.

[Philippe Normand]

So it turns out that setting the stream-type of the appsrc element we use in our custom HTTP source to STREAMING (instead of SEEKABLE) works around the issue and the playmapscube.com website loads just fine...

This however disables seeking support in the source element. We should do this only if we're playing a live source, see also bug 90084. That playmapscube website doesn't contain live sources (to my knowledge). So...

I wonder what's going on exactly, around 20 media elements get created almost simultaneously and without the workaround some pipelines start spitting errors and block the whole loading progress bar of the website. This is with enabling support for 200 code along with code 206 as suggested in the comments.

[Philippe Normand]

(In reply to comment #14)
> I've currently stopped working on this bug. If someone wants to catch up from here, I'd recommend to check this:
> 
> * Ranged requests: we should support "200" status response besides "206", since it's not mandatory for servers to support it (see comment #11 and comment #13).
> 

Right.

> * Sergio, in comment #12, mentions that we're not doing a ranged request from the beginning, while Firefox does, so we might be doing an extra ranged request.
> 

Yeah, maybe we can improve the behavior of the element here.

> I imagine it should be possible to seek on the "cached" resource once it has been already downloaded, instead of expecting the server to provide it from the offset we want.

This is already the case. Seeking in an already buffered region of the media uses the locally downloaded media if available.

[Andres Gomez Garcia]

I'm getting a look to this now.

[Andres Gomez Garcia]

Created attachment 206063 [details]
Dummy html file with links to testing ogg files

I've downloaded some of the ogg files in http://www.playmapscube.com/ and uploaded to an own Apache server that allows MiniBrowser to play the files to check the differences in the HTTP communication.

This file is just a dummy html to make it easier the tests.

[Andres Gomez Garcia]

Created attachment 206064 [details]
Log from trying to play a file y playmapscube

[Andres Gomez Garcia]

Created attachment 206065 [details]
Log from trying to play a file from Igalia Apache server

[Sergio Villar Senin]

There are a couple of very interesting things in the log. First of all we get the whole contents of the ogg in the first request.

Network LOG: < Content-Length: 329308

As far as I remember our current media code handles it very bad, because in order to intercept the request it cancels the original request (so we waste a whole file download) and asks for the resource again.

Network LOG: > GET /agomez/files/music-loop-1.ogg HTTP/1.1
Network LOG: > Range: bytes=255580-

And it does that in a very weird way, I don't know why but it starts requesting from byte 255580 till the end and after that it requests again the whole file

Network LOG: > GET /agomez/files/music-loop-1.ogg HTTP/1.1
Network LOG: > Range: bytes=1-

Definitely there is a bug in the code that selects the ranges.

[Andres Gomez Garcia]

As it is explained, the difference seems to be that the Google Frontend server from playmapscube doesn't provide a "206 Partial Content" response to a "Range" request if it has already answered with a "200 OK" response to a previous request for the content.

Igalia's Apache server actually behaves like that and is what WebKitWebSrc expects.

Not that Google's server behavior is wrong. WebKitWebSrc should be able to handle that.

Other than that, WebKitGtk behavior doesn't seem really optimal.

From the log, we see that:
1. First, the internal ResourceHandle requests the resource, when it gets the headers and checks the one:
"Content-Type: audio/ogg"
Cancels the rest of the response message and passes the URL to the Mediaplayer, which uses the WebKitWebSrc to request the same resource.

This is, we re-start the communication, as if the previous would not have happened.

2. The WebKitWebSrc requests the resource. When it gets the "200 OK" response it starts buffering the received data but, at some point, it cancels the response message in order to request a "Range" as it seems it is a "seekable" resource.

3. The WebKitWebSrc requests the resource, but with a range from the byte in which it stopped to the end of the media.
Google frontend: answers with a "200 OK" and WebKitWebSrc doesn't know how to handle this.
Apache: answers with a "206 Partial Content" and sends the rest of the data.

4. For some reason, although receiving the rest of the content, the response message also gets cancelled.

5. The WebKitWebSrc requests the resource, but with a range from the first byte to the end of the media.

6. Now all the content is retrieved without interruption and the media starts playing.

---

It is clear that even in the working case the behavior is not really good.

[Andres Gomez Garcia]

(In reply to comment #21)
...
> Definitely there is a bug in the code that selects the ranges.

Yep, check comment #22.

[Andres Gomez Garcia]

I think I should also check the behavior with patch from bug 73743.

[Andres Gomez Garcia]

Created attachment 206066 [details]
DOT output of the GST pipeline from NULL to READY

[Andres Gomez Garcia]

Created attachment 206067 [details]
DOT output of the GST pipeline from PAUSED TO PLAYING

[Sergio Villar Senin]

(In reply to comment #22)
> As it is explained, the difference seems to be that the Google Frontend server from playmapscube doesn't provide a "206 Partial Content" response to a "Range" request if it has already answered with a "200 OK" response to a previous request for the content.
> 
> Igalia's Apache server actually behaves like that and is what WebKitWebSrc expects.
> 
> Not that Google's server behavior is wrong. WebKitWebSrc should be able to handle that.
> 
> Other than that, WebKitGtk behavior doesn't seem really optimal.

Yeah, so I the suboptimal behavior is already known but I guess we should focus on working around the issue with the google server if that behavior is shared by some other broken servers. If that's not the case I'd just forget about it as we should try to stick to standards as much as possible.

[Andres Gomez Garcia]

(In reply to comment #27)
> Yeah, so I the suboptimal behavior is already known but I guess we should focus on working around the issue with the google server if that behavior is shared by some other broken servers. If that's not the case I'd just forget about it as we should try to stick to standards as much as possible.

Sure!

Actually, Google's server behavior is not broken. It is a perfectly correct behavior and, most probably, the one any proxy server would implement.

[Sebastian Dröge (slomo)]

I think we currently distinguish between 200 and 206 in the source to check if our seeking has working or if the server just ignored the range request and gives us everything from the beginning again. I remember that this code was added in the GStreamer HTTP source element because some broken servers actually behave that way...

[Andre Moreira Magalhaes]

This looks very similar to the issue I had with bug 115354 which is now fixed. I tested http://www.playmapscube.com/proto/sounds/rolling-05.ogg with latest upstream/master and MiniBrowser and it works fine here (plays a 6 second audio as done by totem also).

I will build webkit with EGL support to test the full page at http://www.playmapscube.com/ but it looks like this issue is already solved :).

It would be good if someone else could confirm this with latest upstream/master.

[Andre Moreira Magalhaes]

(In reply to comment #30)
> This looks very similar to the issue I had with bug 115354 which is now fixed. I tested http://www.playmapscube.com/proto/sounds/rolling-05.ogg with latest upstream/master and MiniBrowser and it works fine here (plays a 6 second audio as done by totem also).
> 
> I will build webkit with EGL support to test the full page at http://www.playmapscube.com/ but it looks like this issue is already solved :).
> 
> It would be good if someone else could confirm this with latest upstream/master.

Ok, playing the ogg file directly seems to work but minibrowser will fail to load http://www.playmapscube.com/ with some gstreamer errors when downloading sounds, will investigate more.

[Andre Moreira Magalhaes]

Ok, I identified 2 issues so far:
- Fixed (pending review): Issue with compressed data when downloading stream. See last comment on bug #115354 and https://bugzilla.gnome.org/show_bug.cgi?id=706338
- Fixed locally: Issue with 200 returned on seek where only 206 is expected.

Even with those 2 issues above fixed the playmapscube page still won't load, I will continue the investigations here.

[Andre Moreira Magalhaes]

Created attachment 212174 [details]
Patch to handle 200 response on HTTP range requests

This patch should fix the issue with servers returning 200 on range requests (instead of 206).

But but, even with this patch the webpage http://playmapscube.com won't load. What happens is that this page tries to load 44 audio files, which creates 44 pipelines (and sinks) and the pipelines are put on PAUSED state (autoload). The problem is that pulsesink (used by autoaudiosink here) will create pulse streams on READY state and pulse has a limit on allowed streams (32 here on PA 3.0) and pulsesink will fail with "not-negotiated" when hitting this limit, thus failing the pipeline and preventing the page from loading.

I investigated the issue but there doesn't seem to be an easy solution here. To fix this we have the following options:
1 - Change pulsesink to only create pulse streams on PLAYING (and possible release them on EOS) - requires upstream blessing, I asked upstream what they think (awaiting response) but I am not that confident we can make this change
2 - Change pulse to allow setting this limit on runtime and add a property on pulsesink to set this property. We could then update this property on the webkit player if we ever hit the limit - not ideal also, requires changing on pulse/gst/webkit
3 - Increase the limit on pulse (already planned by upstream for PA 5.0 according to Ford_Perfect). This could solve the issue with this specific page but the general issue would still be there and we could still hit it even with a higher limit.
4 - Use a custom audiosink that would mix all audios into one output and only have one connection to pulse (or any other sink). This would be ideal and would fix the issue once and for all but it could bring its own issues and it would require a good amount of non-trivial work. We would have to deal with the pipelines separately (e.g. pause) even though they would share the same sink, we would also have to make sure videos are in sync with audios accordingly, etc. So I wouldn't go this route unless we can't find other option.
5 - Change webkit to use fakesink for all audio sinks until we actually start the playback, where we would update the pipeline sink to use autoaudiosink. This also has its own set of issues. When the pipeline is on PAUSED the sink already contains the first buffer and changing sinks would at least loose this first buffer. Also changing sinks requires the pipeline to be on NULL state (which resets the pipeline and all cached data) or we could use padblocks but this is also not 100% reliable.
6 - *HACK* - set a limit of streams on webkit and use fakesink for all new audio sinks when hitting this limit. No further comments on this "solution" :P

Any other idea?

To test that the attached patch works just update MediaPlayerPrivateGStreamer.cpp to use fakesink instead of autoaudiosink on createAudioSink() and try loading playmapscube (it should work).

I am probably not working on this issue anymore (apart from review requests on this patch), but would gladly discuss the possible solutions.

[Philippe Normand]

Comment on attachment 212174 [details]
Patch to handle 200 response on HTTP range requests

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-924
> -        locker.unlock();

Why remove this?

[Andre Moreira Magalhaes]

Created attachment 212178 [details]
Patch to handle 200 response on HTTP range requests

(In reply to comment #34)
> (From update of attachment 212174 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212174&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-924
> > -        locker.unlock();
> 
> Why remove this?

Oops, missed this one, fixed.

Btw, all the seeks that happen when loading this page are requested by oggdemux when trying to determine the stream duration. oggdemux will try to seek near the end of the file and then seek back to pos=1 (skip header) to determine the duration. Maybe we could find a way to cache the already downloaded resources and reuse them instead of re-requesting the data to the server (and getting the whole data again) on every seek.

[Philippe Normand]

(In reply to comment #35)
> Created an attachment (id=212178) [details]
> Patch to handle 200 response on HTTP range requests
> 
> (In reply to comment #34)
> > (From update of attachment 212174 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=212174&action=review
> > 
> > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-924
> > > -        locker.unlock();
> > 
> > Why remove this?
> 
> Oops, missed this one, fixed.
> 
> Btw, all the seeks that happen when loading this page are requested by oggdemux when trying to determine the stream duration. oggdemux will try to seek near the end of the file and then seek back to pos=1 (skip header) to determine the duration. Maybe we could find a way to cache the already downloaded resources and reuse them instead of re-requesting the data to the server (and getting the whole data again) on every seek.

In some cases fixing bug 119477 could help improve this yes.

[Andre Moreira Magalhaes]

(In reply to comment #33)
> 3 - Increase the limit on pulse (already planned by upstream for PA 5.0 according to Ford_Perfect). This could solve the issue with this specific page but the general issue would still be there and we could still hit it even with a higher limit.

Arun (Ford_Perfect) said he increased the limit to 256 (from 32) on pulse now, which should be quite reasonable here. There is also a possibility of removing this limit altogether which would be even better.

The only problem is that we'd have to "depend" on a new pulseaudio release and wait for distros to catch up to fix this, but it looks like this is the best option we have here.

[Sergio Villar Senin]

Comment on attachment 212178 [details]
Patch to handle 200 response on HTTP range requests

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

Looks like the patch is a step ahead but I miss some testing, we do not want to regress it in the future.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:942
> +    if (length > 0 && priv->requestedOffset && response.httpStatusCode() != 200)

I guess we should use == 206 here instead...

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1053
> +        }

Not sure this is correct. AFAIK the server might return a valid range in the request even if it's smaller than the one we requested.

[Andres Gomez Garcia]

Created attachment 216935 [details]
Patch

[Andres Gomez Garcia]

(In reply to comment #38)
> (From update of attachment 212178 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212178&action=review
> 
> Looks like the patch is a step ahead but I miss some testing, we do not want to regress it in the future.
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:942
> > +    if (length > 0 && priv->requestedOffset && response.httpStatusCode() != 200)
> 
> I guess we should use == 206 here instead...
...

Done.

... 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1053
> > +        }
> 
> Not sure this is correct. AFAIK the server might return a valid range in the request even if it's smaller than the one we requested.
...

In a seek, requesting a Range, if we received a 200 response, this discards the data buffers from the beginning of the media file until the requested seek position. This code doesn't run on 206 PARTIAL_CONTENT responses.

[Philippe Normand]

Comment on attachment 216935 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:171
> +void resizeGstBuffer(GstBuffer* buffer, int offset, int size)

the offset should be guint64 no?

[Build Bot]

Comment on attachment 216935 [details]
Patch

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

New failing tests:
http/tests/media/media-seeking-no-ranges-server.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 216935 [details]
Patch

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

New failing tests:
http/tests/media/media-seeking-no-ranges-server.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 216935 [details]
Patch

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

New failing tests:
http/tests/media/media-seeking-no-ranges-server.html

[Build Bot]

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

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

[Sergio Villar Senin]

Changed title as requested by Andres Gomez.

[Andres Gomez Garcia]

Created attachment 217683 [details]
Patch

[Build Bot]

Comment on attachment 217683 [details]
Patch

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

New failing tests:
http/tests/media/media-seeking-no-ranges-server.html

[Build Bot]

Created attachment 217688 [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.5

[Andres Gomez Garcia]

(In reply to comment #51)
> Created an attachment (id=217688) [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.5
...

The fail happening in the Mac Port is due to the fact that the playback download control when not supporting "Range" requests is passed to the low level framework used by AVFoundation that identifies itself as "AppleCoreMedia".

When this happens with a "normal" URL like <host>/media/resources/silence.wav , the playback is successful and the HTTP network traffic is as follows:


GET /media/media-seeking-no-ranges-server.html HTTP/1.1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/538.7+ (KHTML, like 

HTTP/1.1 200 OK

GET /media-resources/video-test.js HTTP/1.1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/538.7+ (KHTML, like 

HTTP/1.1 200 OK

GET /media-resources/media-file.js HTTP/1.1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/538.7+ (KHTML, like 

HTTP/1.1 200 OK

GET /media-resources/content/silence.wav HTTP/1.1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/538.7+ (KHTML, like 

HTTP/1.1 200 OK

GET /media-resources/content/silence.wav HTTP/1.1
User-Agent: AppleCoreMedia/1.0.0.12F45 (Macintosh; U; Intel Mac OS X 10_8_5; en_us)

HTTP/1.1 200 OK

GET /media-resources/content/silence.wav HTTP/1.1
User-Agent: QuickTime/7.7.1 (qtver=7.7.1;cpu=IA32;os=Mac 10.8.5)

HTTP/1.1 200 OK


However, when the requested URL is our PHP URL, as in <host>/media/resources/load-video.php?name=silence.wav&type=audio/wav&ranges=no, the communications stops just after the first request to it, with an error in our audio element. This is as follows:


GET /media/media-seeking-no-ranges-server.html HTTP/1.1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/538.7+ (KHTML, like 

HTTP/1.1 200 OK

GET /media-resources/video-test.js HTTP/1.1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/538.7+ (KHTML, like 

HTTP/1.1 200 OK

GET /media-resources/media-file.js HTTP/1.1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/538.7+ (KHTML, like 

HTTP/1.1 200 OK

GET /media/resources/load-video.php?name=silence.wav&type=audio/wav&ranges=no HTTP/1.1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/538.7+ (KHTML, like 

HTTP/1.1 200 OK


After doing quite some investigation, the conclusion is that the only difference in the traffic is the actual URL.


After talking with Eric Carlson the conclusion is that the team in charge of the CoreMedia low level framework used by AVFoundation doesn't actually want to support servers not supporting "Range" requests so it is OK to just skip this test in the Mac and Apple-Win ports.

As the Apple-Win port is actually passing the test successfully, I will only add the exception in the TestExpections for the Mac port.

[Andres Gomez Garcia]

Created attachment 217737 [details]
Patch

[Philippe Normand]

Comment on attachment 217737 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:926
> +        GST_DEBUG_OBJECT(src, "Seek in progress, ignoring response");

I think the locker should be unlocked before returning, no?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:932
> +        if (200 == response.httpStatusCode()) {

I'm not sure this order of values is used elsewhere in WebKit. Usually we do something() == someConstant and not the opposite.

[Andres Gomez Garcia]

Created attachment 217783 [details]
Patch

[Andres Gomez Garcia]

Created attachment 217784 [details]
Patch

[Andres Gomez Garcia]

(In reply to comment #54)
> (From update of attachment 217737 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217737&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:926
> > +        GST_DEBUG_OBJECT(src, "Seek in progress, ignoring response");
> 
> I think the locker should be unlocked before returning, no?

Added, and corrected a couple more.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:932
> > +        if (200 == response.httpStatusCode()) {
> 
> I'm not sure this order of values is used elsewhere in WebKit. Usually we do something() == someConstant and not the opposite.

I discussed this with Martin and Carlos some time ago. There is no formal style rule about this but it seems that the most used is the one you said.

Changed for coherence, then ...

[Sergio Villar Senin]

Comment on attachment 217783 [details]
Patch

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

Awesome work! I just have some minor comments that could be fixed at land time and do not require an extra review process.

> Source/WebCore/ChangeLog:10
> +        Properly handle 200 response when doing HTTP range requests.

Perhaps a bit more detailed explanation needed (what we were doing wrong and how is fixed)

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:863
> +    if (length > 0 && priv->requestedOffset && 206 == response.httpStatusCode())

response.httpStatusCode() == 206

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:963
> +            // If exists, just reset the buffer's offset. Its size will be updated below.

I'd remove this comment. The code is self-explanatory IMO.

> LayoutTests/http/tests/media/media-seeking-no-ranges-server.html:22
> +            function canplay()

canPlay()

> LayoutTests/http/tests/media/resources/serve-video.php:32
> +        header("Content-Range: bytes " . $start . "-" . $end . "/" . $fileSize);

some tab instead of spaces maybe?

[Philippe Normand]

Comment on attachment 217783 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:844
>          return;

locker.unlock() before return?

[Andres Gomez Garcia]

Created attachment 217785 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 217783 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:844
>>          return;
> 
> locker.unlock() before return?

No, you don't need to explicitly call unlock in this case, because it will be called by the GMutexLocker destructor. It was called before because webKitWebSrcStop takes the lock too.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:945
> +        locker.unlock();

Si, this not needed either.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:955
> +            locker.unlock();

Ditto.

[Andres Gomez Garcia]

Thanks a lot for your review!

(In reply to comment #58)
> (From update of attachment 217783 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217783&action=review
> 
> Awesome work! I just have some minor comments that could be fixed at land time and do not require an extra review process.
> 
> > Source/WebCore/ChangeLog:10
> > +        Properly handle 200 response when doing HTTP range requests.
> 
> Perhaps a bit more detailed explanation needed (what we were doing wrong and how is fixed)

Done.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:863
> > +    if (length > 0 && priv->requestedOffset && 206 == response.httpStatusCode())
> 
> response.httpStatusCode() == 206

Done.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:963
> > +            // If exists, just reset the buffer's offset. Its size will be updated below.
> 
> I'd remove this comment. The code is self-explanatory IMO.

Done.

> > LayoutTests/http/tests/media/media-seeking-no-ranges-server.html:22
> > +            function canplay()
> 
> canPlay()

Done.

> > LayoutTests/http/tests/media/resources/serve-video.php:32
> > +        header("Content-Range: bytes " . $start . "-" . $end . "/" . $fileSize);
> 
> some tab instead of spaces maybe?

No, it was just a correction in the indentation to set it correctly, as in the rest of the file.

[Philippe Normand]

Comment on attachment 217785 [details]
Patch

Thanks! I'll cq+ after EWS passed.

[Andres Gomez Garcia]

Created attachment 217788 [details]
Patch

[Andres Gomez Garcia]

Thanks a lot for the review!

(In reply to comment #61)
> (From update of attachment 217783 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217783&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:844
> >>          return;
> > 
> > locker.unlock() before return?
> 
> No, you don't need to explicitly call unlock in this case, because it will be called by the GMutexLocker destructor. It was called before because webKitWebSrcStop takes the lock too.
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:945
> > +        locker.unlock();
> 
> Si, this not needed either.
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:955
> > +            locker.unlock();
> 
> Ditto.

Done.

[Andres Gomez Garcia]

(In reply to comment #63)
> (From update of attachment 217785 [details])
> Thanks! I'll cq+ after EWS passed.

Be sure is the last one; attachment 217788 [details] :)

Thanks a lot for your help!

[WebKit Commit Bot]

Comment on attachment 217788 [details]
Patch

Clearing flags on attachment: 217788

Committed r159746: <http://trac.webkit.org/changeset/159746>

[Carlos Garcia Campos]

Comment on attachment 212178 [details]
Patch to handle 200 response on HTTP range requests

Clearing flags