Summary: | Media layout tests should have a way to provide test files in different formats | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hin-Chung Lam <hclam> | ||||||||||||||||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, eric, levin, scherkus | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Hin-Chung Lam
2009-08-14 15:50:12 PDT
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. (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. Created attachment 38997 [details]
First round of changes - 10 tests
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). (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). Created attachment 39232 [details]
Patch to be submitted
Comment on attachment 39232 [details]
Patch to be submitted
oops.. uploaded a different patch..
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.
Created attachment 39239 [details]
Fixes for 10 tests
The new patch is good for another look. Comment on attachment 39239 [details]
Fixes for 10 tests
media-file.js and video-paint-test.js are both missing from the patch.
Created attachment 39249 [details]
10 layout tests change
Added back media-file.js, video-paint-test.js is not modified any more. Please take another look. 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 Thanks for the review. I'll address them before I commit. Created attachment 39282 [details]
Updated patch for 10 layout tests
Uploaded a new patch that addresses Eric's comments. 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! 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..
Comment on attachment 39284 [details] Updated patch for layout tests (again) Rejecting patch 39284 from commit-queue. hclam@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py. 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
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. 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. 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. 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
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.
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.
I removed the <script></script> block and moved the script to onload of body. So this won't change the full text dump. Thanks for the review! :) Comment on attachment 39304 [details] Updated patch for 10 layout tests Clearing flags on attachment: 39304 Committed r48237: <http://trac.webkit.org/changeset/48237> All reviewed patches have been landed. Closing bug. Created attachment 39403 [details]
Second round of changes - 20 tests
Second round changes to the layout tests. Changing 20 tests at a time.
Created attachment 39405 [details]
Second round of changes - 20 tests
Added ChangeLog
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 I want to keep this bug open until all layout tests are changed. (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. > 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.
(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. Created attachment 39461 [details]
Second round of changes - 20 tests
Added back the comments and corrected my previous change.
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.
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. 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 <source> 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. Created attachment 39715 [details]
Second round of changes - 19 tests
Fixing my second round of changes.
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 Created attachment 39721 [details]
Second round of changes - 19 tests
Fixed only the ChangeLog by explaining why .wav is preferred over other formats.
Comment on attachment 39721 [details]
Second round of changes - 19 tests
r=me
Comment on attachment 39721 [details] Second round of changes - 19 tests Clearing flags on attachment: 39721 Committed r48494: <http://trac.webkit.org/changeset/48494> All reviewed patches have been landed. Closing bug. > > --- 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. Created attachment 39732 [details]
First round of changes - 8 patches
> 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!
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. Sounds good -- I'll keep those in mind for the next round. 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.
Comment on attachment 39779 [details]
Third round of changes by alpha - 25 tests
r=me
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. 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> All reviewed patches have been landed. Closing bug. 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.
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. |