Media layout tests should have a way to provide test files in different formats

[Hin-Chung Lam]

All media/ layout tests are now using hard-coded file names, and the test files are mostly H264/AAC, if a port doesn't support such formats, the tests will all fail. We should provide a method to swap in test files with different formats easily.

[Eric Carlson]

How about a function in the shared js file that takes a string and returns a file name appropriate for the current platform? For example "test" would return "test.mp4" for platforms that support MPEG-4.   Many of the older tests would have to be restructured, but we can switch them gradually.

[Hin-Chung Lam]

(In reply to comment #1)
That sounds like a good idea, we could set up a platform specific list of test files and a lookup method. For example:

var safariTests=new Array("test.mp4","sound.m4a");
var chromiumTests=new Array("test.ogv","sound.ogg");

function find(name) {
  if (platform == "safari") { /* find in safariTests */}
  if (platform == "chromium") { /* find in chromiumTests */}
}

The question is how can we determine which platform we are in?

In terms of modifying the tests, I can help with the effort.

[Hin-Chung Lam]

Created attachment 38997 [details]
First round of changes - 10 tests

[Eric Carlson]

Comment on attachment 38997 [details]
First round of changes - 10 tests

> +++ b/LayoutTests/media/controls-strict.html
> +++ b/LayoutTests/media/controls-styling.html
> +++ b/LayoutTests/media/video-aspect-ratio.html
> +++ b/LayoutTests/media/video-controls-rendering.html
> +++ b/LayoutTests/media/video-layer-crash.html
> +++ b/LayoutTests/media/video-transformed.html
> +++ b/LayoutTests/media/video-zoom-controls.html

  Passing the media file path to init() will only work as long as every pixel tests use a single media file. I think it would make more sense to add a function to each test file to set the src attribute for each video element before calling the existing init().

> +++ b/LayoutTests/media/media-file.js

> +function findMediaFile(type, name) {
> +    var codecs;
> +    if (type == "audio")
> +        codecs = audioCodecs;
> +    else
> +        codecs = videoCodecs;
> +
> +    var element = document.createElement(type);
> +    for (var i = 0; i < codecs.length; ++i) {
> +        if (element.canPlayType(codecs[i][0]))
> +            return name + "." + codecs[i][1];
> +    }
> +
> +    return "";
> +}

  
  It should be more efficient to call canPlayType() on an existing element if there is one. I would check to see if one exists before creating a new one.


> --- a/LayoutTests/media/video-append-source.html

>  <script>
>      testExpected("video.currentSrc", "");
>      var source = document.createElement("source");
> -    source.setAttribute("src", "content/test.mp4");
> +    source.setAttribute("src", findMediaFile("video", "content/test"));
>      video.appendChild(source);
>  
>      testExpected("video.currentSrc", "");
>  
>      waitForEvent("load", function () {
> -        testExpected("relativeURL(video.currentSrc) ", "content/test.mp4");
> +        testExpected("relativeURL(video.currentSrc) ", findMediaFile("video", "content/test"));
>          endTest();
>      });

  'findMediaFile("video", "content/test")' will always return the same result, you should only call it once.

+++ b/LayoutTests/media/video-paint-test.js
@@ -1,6 +1,11 @@
-function init()
+function init(videoFile)
 {
-    var totalCount = document.getElementsByTagName('video').length;
+    var videos = document.getElementsByTagName('video');
+    var totalCount = videos.length;
+    for (var i = 0; i < totalCount; ++i) {
+        videos[i].src = videoFile;
+    }

  These changes aren't necessary with the first suggestion above.

r+ if you make these changes (or convince me they aren't necessary).

[Hin-Chung Lam]

(In reply to comment #4)
> (From update of attachment 38997 [details])
> > +++ b/LayoutTests/media/controls-strict.html
> > +++ b/LayoutTests/media/controls-styling.html
> > +++ b/LayoutTests/media/video-aspect-ratio.html
> > +++ b/LayoutTests/media/video-controls-rendering.html
> > +++ b/LayoutTests/media/video-layer-crash.html
> > +++ b/LayoutTests/media/video-transformed.html
> > +++ b/LayoutTests/media/video-zoom-controls.html
> 
>   Passing the media file path to init() will only work as long as every pixel
> tests use a single media file. I think it would make more sense to add a
> function to each test file to set the src attribute for each video element
> before calling the existing init().

As far as I can tell, all tests that use init() are using the same test file: test.mp4. If it makes more sense to you I can add two helper methods to media-file.js:

// Sets src for all tags of name "tagName"
function setSrcByTagName(tagName, src)

// Sets src of tag of id "id"
function setSrcById(id, src)

> 
> > +++ b/LayoutTests/media/media-file.js
> 
> > +function findMediaFile(type, name) {
> > +    var codecs;
> > +    if (type == "audio")
> > +        codecs = audioCodecs;
> > +    else
> > +        codecs = videoCodecs;
> > +
> > +    var element = document.createElement(type);
> > +    for (var i = 0; i < codecs.length; ++i) {
> > +        if (element.canPlayType(codecs[i][0]))
> > +            return name + "." + codecs[i][1];
> > +    }
> > +
> > +    return "";
> > +}
> 
> 
>   It should be more efficient to call canPlayType() on an existing element if
> there is one. I would check to see if one exists before creating a new one.
> 
> 
> > --- a/LayoutTests/media/video-append-source.html
> 
> >  <script>
> >      testExpected("video.currentSrc", "");
> >      var source = document.createElement("source");
> > -    source.setAttribute("src", "content/test.mp4");
> > +    source.setAttribute("src", findMediaFile("video", "content/test"));
> >      video.appendChild(source);
> >  
> >      testExpected("video.currentSrc", "");
> >  
> >      waitForEvent("load", function () {
> > -        testExpected("relativeURL(video.currentSrc) ", "content/test.mp4");
> > +        testExpected("relativeURL(video.currentSrc) ", findMediaFile("video", "content/test"));
> >          endTest();
> >      });
> 
>   'findMediaFile("video", "content/test")' will always return the same result,
> you should only call it once.
> 
> +++ b/LayoutTests/media/video-paint-test.js
> @@ -1,6 +1,11 @@
> -function init()
> +function init(videoFile)
>  {
> -    var totalCount = document.getElementsByTagName('video').length;
> +    var videos = document.getElementsByTagName('video');
> +    var totalCount = videos.length;
> +    for (var i = 0; i < totalCount; ++i) {
> +        videos[i].src = videoFile;
> +    }
> 
>   These changes aren't necessary with the first suggestion above.
> 
> r+ if you make these changes (or convince me they aren't necessary).

[Hin-Chung Lam]

Created attachment 39232 [details]
Patch to be submitted

[Hin-Chung Lam]

Comment on attachment 39232 [details]
Patch to be submitted

oops.. uploaded a different patch..

[Hin-Chung Lam]

Created attachment 39238 [details]
Fixes for 10 tests

Thanks for the review!

I have updated the tests to not use init(file), but instead use a helper method setSrcByTagName to set src for all video tags. They all use a single test file for testing, i.e. test.mp4. So using a helper method to set them altogether is safe. I added another helper method, setSrcById() to set the src for individual media tags in case it is needed.

[Hin-Chung Lam]

Created attachment 39239 [details]
Fixes for 10 tests

[Hin-Chung Lam]

The new patch is good for another look.

[Eric Carlson]

Comment on attachment 39239 [details]
Fixes for 10 tests

media-file.js and video-paint-test.js are both missing from the patch.

[Hin-Chung Lam]

Created attachment 39249 [details]
10 layout tests change

[Hin-Chung Lam]

Added back media-file.js, video-paint-test.js is not modified any more. Please take another look.

[Eric Carlson]

Comment on attachment 39249 [details]
10 layout tests change


> +function findMediaFile(type, name) {
> +function setSrcByTagName(tagName, src) {

  It would be clearer to use the same parameter names for the element type for both functions, I think "tagName" is clearer than "type".
  

> +function setSrcById(id, src) {
> +    var elements = document.getElementById(id);
> +    if (elements)
> +        id.src = src;

  getElementById can only return a single element, so "elements" doesn't need to be plural.

r=me

[Hin-Chung Lam]

Thanks for the review. I'll address them before I commit.

[Hin-Chung Lam]

Created attachment 39282 [details]
Updated patch for 10 layout tests

[Hin-Chung Lam]

Uploaded a new patch that addresses Eric's comments.

[Eric Carlson]

Comment on attachment 39282 [details]
Updated patch for 10 layout tests

> +        * media/video-paint-test.js: Changed init() to take a file name and sets src of all video elements.
> +        (init):

  This comment in the change log is no longer true and can be removed. Sorry I missed this in the previous review!

[Hin-Chung Lam]

Created attachment 39284 [details]
Updated patch for layout tests (again)

doh! The previous one I manually removed those two lines from chagelog, forgot to do it this time..

[WebKit Commit Bot]

Comment on attachment 39284 [details]
Updated patch for layout tests (again)

Rejecting patch 39284 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[WebKit Commit Bot]

Comment on attachment 39284 [details]
Updated patch for layout tests (again)

Rejecting patch 39284 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 39284 [details]
Updated patch for layout tests (again)

I suspect that you have been bitten by the infamous bug 28845.
media/controls-strict.html -> failed

But maybe this is a real failure from this patch, since it touches media stuff.

[Hin-Chung Lam]

I just ran it locally, the failure I think is due to that I added a <script></script> that inserts an additional RenderText block? If this is the case the new result should correct.

[Hin-Chung Lam]

Yup, I just confirmed, there's three failures of layout tests:
media/controls-strict.html
media/controls-styling.html
media/video-aspect-ratio.html

They failed because this change is to modify the tests, and that additional "<script></script>" inserts a RenderText block. The results is actually correct. Sorry this change slipped in through my updated patch, I'll produce another one that doesn't have such problems.

[WebKit Commit Bot]

Comment on attachment 39284 [details]
Updated patch for layout tests (again)

Rejecting patch 39284 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 39284 [details]
Updated patch for layout tests (again)

Looks like there is a real failure for this patch:
media/controls-strict.html -> failed
when running on a Mac in release mode.

[Hin-Chung Lam]

Created attachment 39304 [details]
Updated patch for 10 layout tests

Updated this patch to fix the failures. The failures slipped in my previous updated patch when I added a <script></script> to the test. This inserts an additional RenderText block which makes the render tree different.

[Hin-Chung Lam]

I removed the <script></script> block and moved the script to onload of body. So this won't change the full text dump.

[Hin-Chung Lam]

Thanks for the review! :)

[WebKit Commit Bot]

Comment on attachment 39304 [details]
Updated patch for 10 layout tests

Clearing flags on attachment: 39304

Committed r48237: <http://trac.webkit.org/changeset/48237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

Created attachment 39403 [details]
Second round of changes - 20 tests

Second round changes to the layout tests. Changing 20 tests at a time.

[Hin-Chung Lam]

Created attachment 39405 [details]
Second round of changes - 20 tests

Added ChangeLog

[Eric Carlson]

Comment on attachment 39405 [details]
Second round of changes - 20 tests

> -                // controller should still be visible after a second
> -                setTimeout(function() { layoutTestController.notifyDone(); } , 1000);
> -            }
> +                setTimeout(function() {
> +                    if (window.layoutTestController)
> +                        layoutTestController.notifyDone();
> +                } , 1000);

Is there any reason to remove the comment about what the 1 second timeout is for?

r=me

[Hin-Chung Lam]

I want to keep this bug open until all layout tests are changed.

[Hin-Chung Lam]

(In reply to comment #34)
> (From update of attachment 39405 [details])
> > -                // controller should still be visible after a second
> > -                setTimeout(function() { layoutTestController.notifyDone(); } , 1000);
> > -            }
> > +                setTimeout(function() {
> > +                    if (window.layoutTestController)
> > +                        layoutTestController.notifyDone();
> > +                } , 1000);
> 
> Is there any reason to remove the comment about what the 1 second timeout is
> for?
> 
> r=me

This comment is not relavent any more since it's refering to that using layoutTestController is safe given that it is within an if statement that checks layoutTestController. In the new code I check for layoutTestController explicitly within the timeout function so the statement is not accurate.

[Eric Carlson]

> This comment is not relavent any more since it's refering to that using
> layoutTestController is safe given that it is within an if statement that
> checks layoutTestController. In the new code I check for layoutTestController
> explicitly within the timeout function so the statement is not accurate.

Not true! The controller for a <video> element normally hides when a movie is playing, but this movie has only audio media so we want it to always remain visible. To test this we start the movie playing, move the mouse out of the <video> element, and set a timer to end the test in one second. The controller visibility is reflected in the test results because we have it dump as text, so we know if the behavior is correct or not.

[Hin-Chung Lam]

(In reply to comment #37)
> > This comment is not relavent any more since it's refering to that using
> > layoutTestController is safe given that it is within an if statement that
> > checks layoutTestController. In the new code I check for layoutTestController
> > explicitly within the timeout function so the statement is not accurate.
> 
> Not true! The controller for a <video> element normally hides when a movie is
> playing, but this movie has only audio media so we want it to always remain
> visible. To test this we start the movie playing, move the mouse out of the
> <video> element, and set a timer to end the test in one second. The controller
> visibility is reflected in the test results because we have it dump as text, so
> we know if the behavior is correct or not.

Aha! I thought the comment is referring to "layoutTestController", I'll add the comment back and submit another patch.

[Hin-Chung Lam]

Created attachment 39461 [details]
Second round of changes - 20 tests

Added back the comments and corrected my previous change.

[Hin-Chung Lam]

Created attachment 39674 [details]
Third round of changes - 25 tests

This is the third round of changes to 25 tests. This depends on the previous change.

[Eric Carlson]

Comment on attachment 39461 [details]
Second round of changes - 20 tests

> --- a/LayoutTests/media/video-controls-visible-audio-only.html

> -        <video id="no-video-media" controls loop src="content/test.wav" onload="start()"></video>
> +        <video id="no-video-media" controls loop></video>

> +        <script>
> +            setSrcById("fr", findMediaFile("audio", "content/test"));
> +            start();
> +        </script>

Calling start() immediately after setting the src attribute instead of triggering it from the <video> element's 'load' event makes it extremely timing dependent as the test assumes the movie has been loaded and parsed. You should be able to leave the onload attribute on the <video> element even though you set the src attribute later.



> --- a/LayoutTests/media/video-display-toggle.html

> -    <body onload="test()">
> +    <body onload="setSrcById('vid', findMediaFile('video', 'content/test')); test()">

It would be cleaner to call setSrcById(...) from inside of the test function, since it is already triggered on load.


> --- a/LayoutTests/media/video-document-types-expected.txt

>  
> -This tests that a standalone MPEG-4 file with 'sdsm' and 'odsm' tracks is opened in a MediaDocument.
> +This tests that a standalone video file is opened in a MediaDocument.

This test is specific to MPEG-4 files with 'sdsm' and 'odsm' tracks. I think it makes more sense to skip it on platforms that don't support MPEG-4 instead of running it with another file type.

[Eric Carlson]

Comment on attachment 39674 [details]
Third round of changes - 25 tests


> --- a/LayoutTests/ChangeLog

> +
> +        Updating 20 media layout tests to use media files based on supported codecs.
> +
> +        * media/media-file.js: Swap the order of audio file extension preference.

Please expand the comment to say why the preferred order was changed.


> +++ b/LayoutTests/media/video-played-reset.html

>  
> -                run("video.src = \"content/test.mp4\"");
> +                run("video.src = \"" + findMediaFile("video", "content/test") + "\"");

I think the pattern you used in the previous patch, where the media file path is put into a local variable in a case like this:

                var mediaFile = findMediaFile("video", "content/test");
                run("video.src = \"" + mediaFile + "\"");

would make it easier to read this test.


> --- a/LayoutTests/media/video-seek-past-end-paused.html

> -    run("video.src = 'content/test.mp4'");
> +    run("video.src = '" + findMediaFile("video", "content/test") + "'");

Ditto.

> --- a/LayoutTests/media/video-seek-past-end-playing.html

> -    run("video.src = 'content/test.mp4'");
> +    run("video.src = '" + findMediaFile("video", "content/test") + "'");

Ditto.


> --- a/LayoutTests/media/video-source.html
> +++ b/LayoutTests/media/video-source.html

I really hate tests like this that use a document fragment and put script in the body for no obvious reason, so I try to clean them up when I have to change them anyway. It obviously isn't necessary for what you are fixing with this patch, but taking a few extra minutes to tidy up ugly tests like this would be good for everyone. Something like the following is what I would prefer (warning, I haven't actually tested this):

<html>
    <head>
        <script src=media-file.js></script>
        <script src=video-test.js></script>
        <script>
            function loadStart()
            {
                testExpected("relativeURL(video.currentSrc) ", "content/test.mp4");
                endTest();
            }

            function setup()
            {
                video = mediaElement = document.getElementsByTagName('video')[0];

                var source = document.createElement('source'); 
                source.src = findMediaFile("video", "content/test");
                video.appendChild(source);
            }
        </script>
    </head>
    <body onload="setup()">
        <video controls onloadstart="loadStart()"> </video>
    </body>
</html>


> +++ a/LayoutTests/media/video-src-invalid-remove.html

Looking at this test I see that it is already quite timing dependent, so it would be nice to restructure it up as long as you are changing it. I suggest you include an empty <video> element, and set the 'src' and add the <source> element in a body onload handler so the listeners are setup before the file can load. Maybe something like this (again, not tested):

<html>
    <head>
        <script src=media-file.js></script>
        <script src=video-test.js></script>
        <script>
            var loadCount = 0;
            var mediaFile = findMediaFile("video", "content/test");;

            function loadStart() { [code goes here] }
            function loadedmetadata() { [code goes here] }
            function errorEvent() { [code goes here] }

            function setup()
            {
                video = mediaElement = document.getElementsByTagName('video')[0];

                consoleWrite("");
                waitForEvent('loadedmetadata', loadedmetadata);
                waitForEvent('loadstart', loadStart );
                waitForEvent('error', errorEvent);

                video.src = "bogus.mov"

                var source = document.createElement('source'); 
                source.src = mediaFile;
                video.appendChild(source);
            }
        </script>
    </head>
    <body onload="setup()">
        <video controls > </video>
        <p>Test that removing invalid 'src' attribute triggers load of &lt;source&gt; elements</p>
    </body>
</html>


> --- a/LayoutTests/media/video-src-remove.html

> -    <video src=content/silence.mpg controls onloadedmetadata="loadedmetadata()" >
> -        <source src=content/test.mp4>
> -    </video>
> +    <div id=panel></div>
> +    <script>
> +        var panel = document.getElementById("panel");
> +        var mediaFile = findMediaFile("video", "content/test");
> +        panel.innerHTML = "<video src=" + mediaFile + " controls onloadedmetadata='loadedmetadata()'><source src=content/counting.mp4></video>";
> +    </script>

I would prefer to see the video element set up with DOM calls instead innerHTML.


> --- a/LayoutTests/media/video-src-source.html

> -<video src=content/test.mp4 controls>
> -    <source src=content/error.mpeg>
> -</video>
> +<div id=panel></div>
> +<script>
> +    var panel = document.getElementById("panel");
> +    var mediaFile = findMediaFile("video", "content/test");
> +    panel.innerHTML = "<video src=" + mediaFile + " controls><source src=content/error.mpeg></video>";
> +</script>

Same here.


> --- a/LayoutTests/media/video-zoom.html

> -<body onload="init()">
> +<body onload="setSrcByTagName('video', findMediaFile('video', 'content/test')); init();">

It would be cleaner to call setSrcByTagName() in the init() function.

[Hin-Chung Lam]

Created attachment 39715 [details]
Second round of changes - 19 tests

Fixing my second round of changes.

[Eric Carlson]

Comment on attachment 39715 [details]
Second round of changes - 19 tests

> +        * media/media-file.js: Swap the order of audio formats to prefer .wav.

Looks like this change snuck its way in from the https://bugs.webkit.org/attachment.cgi?id=39674, so like I noted in that review it would be helpful to expand the comment to say why the preferred order was changed.

r=me

[Hin-Chung Lam]

Created attachment 39721 [details]
Second round of changes - 19 tests

Fixed only the ChangeLog by explaining why .wav is preferred over other formats.

[Eric Carlson]

Comment on attachment 39721 [details]
Second round of changes - 19 tests

r=me

[WebKit Commit Bot]

Comment on attachment 39721 [details]
Second round of changes - 19 tests

Clearing flags on attachment: 39721

Committed r48494: <http://trac.webkit.org/changeset/48494>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

> > --- a/LayoutTests/media/video-source.html
> > +++ b/LayoutTests/media/video-source.html
> 
> I really hate tests like this that use a document fragment and put script in
> the body for no obvious reason, so I try to clean them up when I have to change
> them anyway. It obviously isn't necessary for what you are fixing with this
> patch, but taking a few extra minutes to tidy up ugly tests like this would be
> good for everyone. Something like the following is what I would prefer
> (warning, I haven't actually tested this):

> > +++ a/LayoutTests/media/video-src-invalid-remove.html
> > --- a/LayoutTests/media/video-src-remove.html
> > --- a/LayoutTests/media/video-src-source.html

The reason why I'm coding the tests to use innerHTML instead of setting .src or
appendChild() is that these are supposed to be testing the attributes /
<source> from their text / tag form.

You'll find tests for changing .src, appendChild() already in:
video-src.html
video-src-set.html
video-append-source.html

I think if we are not testing src and <source> in their "src=" / <source> form
we'll be missing some test cases.

[Andrew Scherkus]

Created attachment 39732 [details]
First round of changes - 8 patches

[Eric Carlson]

> The reason why I'm coding the tests to use innerHTML instead of setting .src or
> appendChild() is that these are supposed to be testing the attributes /
> <source> from their text / tag form.

Fair enough!

[Eric Carlson]

Comment on attachment 39732 [details]
First round of changes - 8 patches

> +++ b/LayoutTests/ChangeLog

> +        * media/progress-event-total-expected.txt: Ditto

The only change to the expected results is to include the new event.total value.

> +        * media/progress-event-total.html: Ditto

Accomodates new file duration as well as switching to findMediaFile().


> +++ b/LayoutTests/media/event-attributes.html

>  
>      <body onload="start()">
>  
> -        <video controls src="content/test.mp4" 
> +        <video controls
>              onabort="eventHandler()"
>              oncanplay="eventHandler()"
>              oncanplaythrough="eventHandler()"
> @@ -97,5 +99,9 @@
>              >
>          </video>
>  
> +        <script>
> +            setSrcByTagName("video", findMediaFile("video", "content/test"));
> +        </script>

Again, I would really prefer to have this done in the (existing) body load event handler
instead of adding another script element.

r=me with these minor fixes.

[Andrew Scherkus]

Sounds good -- I'll keep those in mind for the next round.

[Hin-Chung Lam]

Created attachment 39779 [details]
Third round of changes by alpha - 25 tests

Fixed the style nits, but I leave 4 tests using innerHTML since they are used to test the tag attributes in html form.

[Eric Carlson]

Comment on attachment 39779 [details]
Third round of changes by alpha - 25 tests

r=me

[Eric Seidel (no email)]

You should feel free to set both r=? and cq=? at the same time if you are going to want auto commit.  Then the reviewer can set both when reviewing.

[WebKit Commit Bot]

Comment on attachment 39779 [details]
Third round of changes by alpha - 25 tests

Clearing flags on attachment: 39779

Committed r48539: <http://trac.webkit.org/changeset/48539>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

Created attachment 39877 [details]
Test files for ogg and theora, transcoded from originals

As the layout tests conversion are close to finish, I'm uploading the test files for ogg and theora.

[Eric Seidel (no email)]

Comment on attachment 39877 [details]
Test files for ogg and theora, transcoded from originals

This bug is closed, so we shouldn't be adding more patches to it.  Also, I don't think the commit-queue can handle these patches, because svn-apply (which is what it uses) can't handle git binary patches.  Maybe it can handle git binary adds, I can't remember.  see bug 26830 and bug 28456.