[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





--- Comment #22 from Andres Gomez Garcia <agomez at igalia.com>  2014-03-07 04:01:27 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/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.

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