Bug 125926 – [GStreamer] WebKit gets stalled when trying to play a stream

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Andres Gomez Garcia]

Make MiniBrowser try to play, for example:
 * http://playerservices.streamtheworld.com/api/livestream-redirect/SER_ASO_LEON.mp3
 * http://orelha.radiolivre.org:8000/muda.ogg

Playback doesn't start and WebKit gets stalled.

[Andres Gomez Garcia]

I'm working on this.

[Andres Gomez Garcia]

This seems to be a regression introduced by https://trac.webkit.org/changeset/154683

[Andres Gomez Garcia]

I'm also creating a LayoutTest for the streaming cases so we can realize when this is broken once again.

[Andres Gomez Garcia]

Bug 127452 is a DUPLICATED of this one which already corrected the problem but didn't provide the proper tests.

I will keep working on providing some tests so we can detect any regression earlier.

[Andres Gomez Garcia]

Created attachment 225065 [details]
Patch

[Philippe Normand]

Looks good but a ChangeLog is missing

[Gustavo Noronha (kov)]

Comment on attachment 225065 [details]
Patch

LGTM as well, but like Phil says it lacks a changelog entry, please use Tools/Scripts/prepare-ChangeLog to generate one

[Andres Gomez Garcia]

(In reply to comment #6)
> Looks good but a ChangeLog is missing

mmm ... somehow, I've messed up with wekit-patch ... let's upload again ...

[Eric Carlson]

Comment on attachment 225065 [details]
Patch

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

> LayoutTests/http/tests/media/resources/stream/media.db:1
> +a:4:{i:0;a:11:{s:8:"fileName";s:11:"silence.m4a";s:8:"fileSize";i:20447;s:8:"playTime";d:1.3699773242630386;s:10:"audioStart";i:4096;s:8:"audioEnd";i:20447;s:11:"audioLength";i:16351;s:6:"artist";s:7:"silence";s:5:"title";N;s:7:"bitRate";d:95481.87235169491;s:8:"mimeType";s:9:"audio/aac";s:10:"fileFormat";s:3:"mp4";}i:1;a:11:{s:8:"fileName";s:11:"silence.mp3";s:8:"fileSize";i:17961;s:8:"playTime";d:1.4105833333333333;s:10:"audioStart";i:1034;s:8:"audioEnd";i:17961;s:11:"audioLength";i:16927;s:6:"artist";s:7:"silence";s:5:"title";N;s:7:"bitRate";i:96000;s:8:"mimeType";s:10:"audio/mpeg";s:10:"fileFormat";s:3:"mp3";}i:2;a:11:{s:8:"fileName";s:11:"silence.oga";s:8:"fileSize";i:12983;s:8:"playTime";d:1.3844897959183673;s:10:"audioStart";i:172;s:8:"audioEnd";i:10460;s:11:"audioLength";i:10288;s:6:"artist";s:7:"silence";s:5:"title";N;s:7:"bitRate";d:59447.16981132076;s:8:"mimeType";s:9:"audio/ogg";s:10:"fileFormat";s:3:"ogg";}i:3;a:11:{s:8:"fileName";s:11:"silence.wav";s:8:"fileSize";i:120876;s:8:"playTime";d:1.3699773242630386;s:10:"audioStart";i:44;s:8:"audioEnd";i:120876;s:11:"audioLength";i:120832;s:6:"artist";s:7:"silence";s:5:"title";N;s:7:"bitRate";i:705600;s:8:"mimeType";s:9:"audio/wav";s:10:"fileFormat";s:4:"riff";}}

How was this database generated, with getid3? Please a script to your patch so someone later can regenerate it if necessary.

> LayoutTests/http/tests/media/resources/stream/stream-media.php:13
> +    /* $fileName = basename($filePath); */

Nit: if this was left here for a purpose, please say so in a comment. if not, please remove.

> LayoutTests/http/tests/media/resources/stream/stream-media.php:21
> +    $ranges = array_key_exists("ranges", $_GET) ? $_GET["ranges"] : "yes";

Is there any reason to not use byte ranges?

> LayoutTests/http/tests/media/resources/stream/stream-media.php:22
> +    $setMetadata = array_key_exists("send-metaint", $_GET) ? $_GET["send-metaint"] : "no";

Is there any reason to not send metadata?

> LayoutTests/http/tests/media/resources/stream/stream-media.php:106
> +    if ($httpStatus == "404 Not Fond")
> +        exit;

It would be good to fail with something more informative than a 404. Is it possible to log something to stderr so it shows up in the test results?

[Andres Gomez Garcia]

Created attachment 225151 [details]
Patch

[Eric Carlson]

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

Did you miss my review comments?

[Andres Gomez Garcia]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=225151) [details] [details]
> > Patch
> 
> Did you miss my review comments?

No, sorry, as I said above, I messed with webkit-patch and was just uploading the real patch while you were adding your review.

I'm now going over your comments. Most of them still apply.

Thanks a lot for taking your time, I will upload a new version of the patch soon :)

[Andres Gomez Garcia]

Created attachment 225346 [details]
Patch

[Andres Gomez Garcia]

First, sorry for the refactoring but I thought that it actually made more sense to add this functionality to the existent "serve-video.php" file.

(In reply to comment #9)
> (From update of attachment 225065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225065&action=review
> 
> > LayoutTests/http/tests/media/resources/stream/media.db:1
...
> How was this database generated, with getid3? Please a script to your patch so someone later can regenerate it if necessary.

I've renamed the db to metadata.db and moved to the directory scanned at each moment, as it makes more sense.

Now, the db is regenerated by the own code of the PHP script by downloading getid3. Its regeneration can also be forced if passed the "create-database=yes" in the GET method.

> > LayoutTests/http/tests/media/resources/stream/stream-media.php:13
> > +    /* $fileName = basename($filePath); */
> 
> Nit: if this was left here for a purpose, please say so in a comment. if not, please remove.

This disappeared already in the first correcting patch.

> > LayoutTests/http/tests/media/resources/stream/stream-media.php:21
> > +    $ranges = array_key_exists("ranges", $_GET) ? $_GET["ranges"] : "yes";
> 
> Is there any reason to not use byte ranges?

Now this is actually part of the existent previous funcionality to be able to keep testing servers not supporting range requests.

> > LayoutTests/http/tests/media/resources/stream/stream-media.php:22
> > +    $setMetadata = array_key_exists("send-metaint", $_GET) ? $_GET["send-metaint"] : "no";
> 
> Is there any reason to not send metadata?

This is actually a flag to send a specific Icecast/Shoutcast header informing at which frequency it will be sent specific metadata of the file being played currently.

This is only supported when streaming MP3 files.

Maybe creating a specific test case for this would make sense but I'm unsure right now of whether this is something we want and also how to proceed with a test when MP3 files support is probably not guarantee on every port.

I've removed this by now, as it is not needed for this specific test.

> > LayoutTests/http/tests/media/resources/stream/stream-media.php:106
> > +    if ($httpStatus == "404 Not Fond")
> > +        exit;
> 
> It would be good to fail with something more informative than a 404. Is it possible to log something to stderr so it shows up in the test results?

Now we have logging in Apache log and we return a more verbose HTML.

[Alexey Proskuryakov]

Comment on attachment 225346 [details]
Patch

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

> LayoutTests/http/tests/media/media-play-stream-chunked-icy.html:5
> +        <script src=../../media-resources/video-test.js></script>
> +        <script src=../../media-resources/media-file.js></script>

As this is an http test, please just use absolute paths to resources:

<script src=/media-resources/video-test.js></script>

> LayoutTests/http/tests/media/media-play-stream-chunked-icy.html:8
> +            var media = findMediaFile('audio', '../../../../media/content/silence');

Do we still need a relative path here?

> LayoutTests/http/tests/media/resources/media-stream-chunked-icy.php:10
> +    $fileName = $_GET["name"];
> +    $type = $_GET["type"];
> +
> +    $_GET = array();
> +    $_GET["name"] = $fileName;
> +    $_GET["type"] = $type;
> +    $_GET["content-length"] = "no";
> +    $_GET["icy-data"] = "yes";

I don't understand why this script is needed. All it does is add two _GET arguments, so why don't we just pass all the arguments in media-play-stream-chunked-icy.html?

> LayoutTests/http/tests/media/resources/serve-video.php:11
> +        $tmpFile = tempnam($dir, $prefix);
> +        if (!file_exists($tmpFile))

Please see how other tests store temporary files. We use portabilityLayer.php for cross-platform compatibility, and make it clear from file names which specific test the temporary file belongs to. E.g. http/tests/appcache/resources/counter.php. This uses sys_get_temp_dir() to get temporary directory.

The last thing we'd want is to have temporary files shared across tests, they must be all unique.

> LayoutTests/http/tests/media/resources/serve-video.php:35
> +        "httpStatus" => "404 Not Fond",

Typo: "404 Not Fond".

> LayoutTests/http/tests/media/resources/serve-video.php:46
> +    // 404 on errors

Why not a 5xx code?

> LayoutTests/http/tests/media/resources/serve-video.php:79
> +                $id3Url = "http://sourceforge.net/projects/getid3/files/latest/download?source=files";

The script to update the database should not be the same script that's run as part of tests, it's just too confusing. I can think if a few options, but Eric is the better one to advise on the course of action:

- Check in the whole getid3 (assuming its license is compatible), and have a script to update the database, which one will need to run when changing the assets.
- Have a separate script for this purpose, that will download getid3 like here.
- Check in the whole getid3 (assuming its license is compatible), and just parse the files every time, not checking in the database.

> LayoutTests/http/tests/media/resources/serve-video.php:184
> +    header("Status: " . $settings["httpStatus"]);
> +    header("HTTP/1.1 " . $settings["httpStatus"]);

This looks suspicious. Is sending the HTTP/1.1 line something one actually needs to do?

> LayoutTests/http/tests/media/resources/serve-video.php:187
> +    if ($settings["httpStatus"] == "404 Not Fond") {

Typo: "404 Not Fond"

[Eric Carlson]

Comment on attachment 225346 [details]
Patch

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

> LayoutTests/http/tests/media/media-play-stream-chunked-icy.html:10
> +            var type = mimeTypeForExtension(media.split('.').pop());
> +            var audio;

Nit: these variables are only used inside of start() so they can be declared there.

>> LayoutTests/http/tests/media/resources/serve-video.php:79
>> +                $id3Url = "http://sourceforge.net/projects/getid3/files/latest/download?source=files";
> 
> The script to update the database should not be the same script that's run as part of tests, it's just too confusing. I can think if a few options, but Eric is the better one to advise on the course of action:
> 
> - Check in the whole getid3 (assuming its license is compatible), and have a script to update the database, which one will need to run when changing the assets.
> - Have a separate script for this purpose, that will download getid3 like here.
> - Check in the whole getid3 (assuming its license is compatible), and just parse the files every time, not checking in the database.

I agree with Alexey, we will not need to recreate the database very often so it doesn't make sense to have the database creation code inside of the test. I think a separate script that downloads getid3 as necessary is the way to go.

[Andres Gomez Garcia]

Comment on attachment 225346 [details]
Patch

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

>> LayoutTests/http/tests/media/resources/media-stream-chunked-icy.php:10
>> +    $_GET["icy-data"] = "yes";
> 
> I don't understand why this script is needed. All it does is add two _GET arguments, so why don't we just pass all the arguments in media-play-stream-chunked-icy.html?

The rest of the existing tests using the previously existing "serve-video.php" are using this kind of proxy php scripts.

I can just pass the arguments and call "serve-video.php" directly but I was following the examples. What do you think?

>> LayoutTests/http/tests/media/resources/serve-video.php:184
>> +    header("HTTP/1.1 " . $settings["httpStatus"]);
> 
> This looks suspicious. Is sending the HTTP/1.1 line something one actually needs to do?

This line is part of the existing script. My update was just the variable used. I didn't want to change the functionality that is already working for the existing tests.

[Eric Carlson]

(In reply to comment #17)
> (From update of attachment 225346 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225346&action=review
> 
> >> LayoutTests/http/tests/media/resources/media-stream-chunked-icy.php:10
> >> +    $_GET["icy-data"] = "yes";
> > 
> > I don't understand why this script is needed. All it does is add two _GET arguments, so why don't we just pass all the arguments in media-play-stream-chunked-icy.html?
> 
> The rest of the existing tests using the previously existing "serve-video.php" are using this kind of proxy php scripts.
> 
> I can just pass the arguments and call "serve-video.php" directly but I was following the examples. What do you think?
> 
I think calling serve-video.php directly is a good idea since you have folded the icy server functionality into it.

> >> LayoutTests/http/tests/media/resources/serve-video.php:184
> >> +    header("HTTP/1.1 " . $settings["httpStatus"]);
> > 
> > This looks suspicious. Is sending the HTTP/1.1 line something one actually needs to do?
> 
> This line is part of the existing script. My update was just the variable used. I didn't want to change the functionality that is already working for the existing tests.

I believe it is necessary because the script generates the entire header.

[Andres Gomez Garcia]

Comment on attachment 225346 [details]
Patch

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

>>> LayoutTests/http/tests/media/resources/serve-video.php:79
>>> +                $id3Url = "http://sourceforge.net/projects/getid3/files/latest/download?source=files";
>> 
>> The script to update the database should not be the same script that's run as part of tests, it's just too confusing. I can think if a few options, but Eric is the better one to advise on the course of action:
>> 
>> - Check in the whole getid3 (assuming its license is compatible), and have a script to update the database, which one will need to run when changing the assets.
>> - Have a separate script for this purpose, that will download getid3 like here.
>> - Check in the whole getid3 (assuming its license is compatible), and just parse the files every time, not checking in the database.
> 
> I agree with Alexey, we will not need to recreate the database very often so it doesn't make sense to have the database creation code inside of the test. I think a separate script that downloads getid3 as necessary is the way to go.

I agree it would be better to move to a different script.

Due to licensing, best is to download under demand and recreate the db then.

Still, I'm unsure how to provide this script. Do you think it is OK just by providing a single PHP file with comments for calling ./Tools/Scripts/run-webkit-httpd and the URL to call for regenerating the db or are you thinking in something more complex like another python script in ./Tools/Scripts that would be doing also this?

[Eric Carlson]

(In reply to comment #19)
> 
> I agree it would be better to move to a different script.
> 
> Due to licensing, best is to download under demand and recreate the db then.
> 
> Still, I'm unsure how to provide this script. Do you think it is OK just by providing a single PHP file with comments for calling ./Tools/Scripts/run-webkit-httpd and the URL to call for regenerating the db or are you thinking in something more complex like another python script in ./Tools/Scripts that would be doing also this?

It definitely doesn't have to be complex! I think a single PHP file in LayoutTests/http/tests/media/resources with a comment block documenting of how to run it, plus a comment in serve-video.php that points to that script should be fine.

[Andres Gomez Garcia]

Created attachment 226111 [details]
Patch

[Andres Gomez Garcia]

Comment on attachment 225346 [details]
Patch

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

>> LayoutTests/http/tests/media/media-play-stream-chunked-icy.html:5
>> +        <script src=../../media-resources/media-file.js></script>
> 
> As this is an http test, please just use absolute paths to resources:
> 
> <script src=/media-resources/video-test.js></script>

Done.

>> LayoutTests/http/tests/media/media-play-stream-chunked-icy.html:8
>> +            var media = findMediaFile('audio', '../../../../media/content/silence');
> 
> Do we still need a relative path here?

I do think so. I've not figured out another way of doing it.

>> LayoutTests/http/tests/media/media-play-stream-chunked-icy.html:10
>> +            var audio;
> 
> Nit: these variables are only used inside of start() so they can be declared there.

Done.

>>>> LayoutTests/http/tests/media/resources/media-stream-chunked-icy.php:10
>>>> +    $_GET["icy-data"] = "yes";
>>> 
>>> I don't understand why this script is needed. All it does is add two _GET arguments, so why don't we just pass all the arguments in media-play-stream-chunked-icy.html?
>> 
>> The rest of the existing tests using the previously existing "serve-video.php" are using this kind of proxy php scripts.
>> 
>> I can just pass the arguments and call "serve-video.php" directly but I was following the examples. What do you think?
> 
> I think calling serve-video.php directly is a good idea since you have folded the icy server functionality into it.

Done.

>> LayoutTests/http/tests/media/resources/serve-video.php:11
>> +        if (!file_exists($tmpFile))
> 
> Please see how other tests store temporary files. We use portabilityLayer.php for cross-platform compatibility, and make it clear from file names which specific test the temporary file belongs to. E.g. http/tests/appcache/resources/counter.php. This uses sys_get_temp_dir() to get temporary directory.
> 
> The last thing we'd want is to have temporary files shared across tests, they must be all unique.

I've checked this and the rest of possible version conflictive functions. I've added a couple more to the portabilityLayer.php

I've also moved the helper functions to its own PHP script.

>> LayoutTests/http/tests/media/resources/serve-video.php:35
>> +        "httpStatus" => "404 Not Fond",
> 
> Typo: "404 Not Fond".

Fixed.

>> LayoutTests/http/tests/media/resources/serve-video.php:46
>> +    // 404 on errors
> 
> Why not a 5xx code?

That's right. Now, I'm using always "500".

>>>> LayoutTests/http/tests/media/resources/serve-video.php:79
>>>> +                $id3Url = "http://sourceforge.net/projects/getid3/files/latest/download?source=files";
>>> 
>>> The script to update the database should not be the same script that's run as part of tests, it's just too confusing. I can think if a few options, but Eric is the better one to advise on the course of action:
>>> 
>>> - Check in the whole getid3 (assuming its license is compatible), and have a script to update the database, which one will need to run when changing the assets.
>>> - Have a separate script for this purpose, that will download getid3 like here.
>>> - Check in the whole getid3 (assuming its license is compatible), and just parse the files every time, not checking in the database.
>> 
>> I agree with Alexey, we will not need to recreate the database very often so it doesn't make sense to have the database creation code inside of the test. I think a separate script that downloads getid3 as necessary is the way to go.
> 
> I agree it would be better to move to a different script.
> 
> Due to licensing, best is to download under demand and recreate the db then.
> 
> Still, I'm unsure how to provide this script. Do you think it is OK just by providing a single PHP file with comments for calling ./Tools/Scripts/run-webkit-httpd and the URL to call for regenerating the db or are you thinking in something more complex like another python script in ./Tools/Scripts that would be doing also this?

I've moved the script to an independent one called create-id3-db.php

>> LayoutTests/http/tests/media/resources/serve-video.php:187
>> +    if ($settings["httpStatus"] == "404 Not Fond") {
> 
> Typo: "404 Not Fond"

Fixed.

[Eric Carlson]

Comment on attachment 226111 [details]
Patch

This is great! 

Thanks for taking the time to respond to the previous review feedback.

[Andres Gomez Garcia]

Awesome!

Thanks, Alexey and Eric, and also Gustavo and Philippe for devoting your time to review this :)

[Csaba Osztrogonác]

Comment on attachment 226111 [details]
Patch

Clearing flags on attachment: 226111

Committed r165536: <http://trac.webkit.org/changeset/165536>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

http/tests/media/media-play-stream-chunked-icy.html starts failing after this.
Please have a look or revert.

[Andres Gomez Garcia]

Reverted r165536 for reason:

It breaks http/tests/media/media-play-stream-chunked-icy.html

Committed r165569: <http://trac.webkit.org/changeset/165569>

[Andres Gomez Garcia]

mmm, it seems to work in the Mountain Lion port:
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r165537%20(18182)/results.html
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r165537%20(16762)/results.html
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/13188
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK2%20%28Tests%29/builds/16570

But not in the Maverick:
http://build.webkit.org/builders/Apple%20Mavericks%20Release%20WK1%20%28Tests%29/builds/4214
http://build.webkit.org/builders/Apple%20Mavericks%20Release%20WK2%20%28Tests%29/builds/3973
http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/3751/
http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/3299

[Philippe Normand]

rs=me to reland the patch with the test skipped on Mavericks (and a bug filled :))

[Andres Gomez Garcia]

Reported bug 130234.

[Andres Gomez Garcia]

Created attachment 226706 [details]
Patch

[Philippe Normand]

Comment on attachment 226706 [details]
Patch

LGTM

[Andres Gomez Garcia]

Comment on attachment 226706 [details]
Patch

Clearing flags on attachment: 226706

Committed r165616: <http://trac.webkit.org/changeset/165616>

[Andres Gomez Garcia]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

(In reply to comment #34)
> (From update of attachment 226706 [details])
> Clearing flags on attachment: 226706
> 
> Committed r165616: <http://trac.webkit.org/changeset/165616>

Thanks for following up on this Andres!

[Alexey Proskuryakov]

Did it have to be a [ Skip ] entry? Seems like a [ Fail ] is more appropriate, as long as the test is not taking too long to fail, and doesn't crash every time.

[Andres Gomez Garcia]

(In reply to comment #37)
> Did it have to be a [ Skip ] entry? Seems like a [ Fail ] is more appropriate, as long as the test is not taking too long to fail, and doesn't crash every time.

Thanks for the tip, Alexey, I will have this into account in further patches.

Should I file a new bug and patch, or can we go with [ Skip ] for this one?

[Andres Gomez Garcia]

(In reply to comment #36)
> (In reply to comment #34)
> > (From update of attachment 226706 [details] [details])
> > Clearing flags on attachment: 226706
> > 
> > Committed r165616: <http://trac.webkit.org/changeset/165616>
> 
> Thanks for following up on this Andres!

My pleasure! :)