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.
I'm working on this.
This seems to be a regression introduced by https://trac.webkit.org/changeset/154683
I'm also creating a LayoutTest for the streaming cases so we can realize when this is broken once again.
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.
Created attachment 225065 [details] Patch
Looks good but a ChangeLog is missing
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
(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 ...
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?
Created attachment 225151 [details] Patch
(In reply to comment #10) > Created an attachment (id=225151) [details] > Patch Did you miss my review comments?
(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 :)
Created attachment 225346 [details] Patch
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.
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"
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.
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.
(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.
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?
(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.
Created attachment 226111 [details] Patch
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.
Comment on attachment 226111 [details] Patch This is great! Thanks for taking the time to respond to the previous review feedback.
Awesome! Thanks, Alexey and Eric, and also Gustavo and Philippe for devoting your time to review this :)
Comment on attachment 226111 [details] Patch Clearing flags on attachment: 226111 Committed r165536: <http://trac.webkit.org/changeset/165536>
All reviewed patches have been landed. Closing bug.
http/tests/media/media-play-stream-chunked-icy.html starts failing after this. Please have a look or revert.
Reverted r165536 for reason: It breaks http/tests/media/media-play-stream-chunked-icy.html Committed r165569: <http://trac.webkit.org/changeset/165569>
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
rs=me to reland the patch with the test skipped on Mavericks (and a bug filled :))
Reported bug 130234.
Created attachment 226706 [details] Patch
Comment on attachment 226706 [details] Patch LGTM
Comment on attachment 226706 [details] Patch Clearing flags on attachment: 226706 Committed r165616: <http://trac.webkit.org/changeset/165616>
(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!
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.
(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?
(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! :)