[Webkit-unassigned] [Bug 125926] [GStreamer] WebKit gets stalled when trying to play a stream

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=125926


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #225346|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #15 from Alexey Proskuryakov <ap at webkit.org>  2014-02-27 11:10:34 PST ---
(From update of attachment 225346)
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"

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.