Bug 30435 - Update media layout tests to be codec aware and a V8 binding fix.
: Update media layout tests to be codec aware and a V8 binding fix.
Status: RESOLVED FIXED
: WebKit
Media Elements
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-10-16 02:05 PST by
Modified: 2010-04-01 20:12 PST (History)


Attachments
Round 1 (6.33 KB, patch)
2009-10-16 02:11 PST, Andrew Scherkus
mrowe: review-
Review Patch | Details | Formatted Diff | Diff
Round 2 (4.89 KB, patch)
2009-10-16 11:09 PST, Andrew Scherkus
abarth: review-
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Round 3 (5.53 KB, patch)
2009-10-20 18:32 PST, Andrew Scherkus
no flags Review Patch | Details | Formatted Diff | Diff
Round 3 updated (5.62 KB, patch)
2009-11-24 18:33 PST, Andrew Scherkus
no flags Review Patch | Details | Formatted Diff | Diff
Actual result (18.50 KB, image/png)
2009-11-24 18:39 PST, Andrew Scherkus
no flags Details
Expected result (18.40 KB, image/png)
2009-11-24 18:39 PST, Andrew Scherkus
no flags Details
Round 4 (8.83 KB, patch)
2009-12-23 13:26 PST, Andrew Scherkus
no flags Review Patch | Details | Formatted Diff | Diff
Round 4 image attachment (20.13 KB, image/png)
2009-12-23 13:28 PST, Andrew Scherkus
no flags Details
Round 5 (39.14 KB, patch)
2010-01-04 14:37 PST, Andrew Scherkus
no flags Review Patch | Details | Formatted Diff | Diff
Round 6 (36.35 KB, patch)
2010-01-04 14:38 PST, Andrew Scherkus
no flags Review Patch | Details | Formatted Diff | Diff
Round 6 + proper ChangeLog (36.35 KB, patch)
2010-01-04 14:41 PST, Andrew Scherkus
no flags Review Patch | Details | Formatted Diff | Diff
Round 7 (36.36 KB, patch)
2010-01-04 18:54 PST, Andrew Scherkus
no flags Review Patch | Details | Formatted Diff | Diff
Round 8 (83.58 KB, patch)
2010-02-25 16:31 PST, Andrew Scherkus
levin: review+
levin: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-16 02:05:36 PST
Updated the media layout tests, which unveild a bug in our V8 bindings.
------- Comment #1 From 2009-10-16 02:11:22 PST -------
Created an attachment (id=41278) [details]
Round 1

Not marking cq? as it contains a binary file.
------- Comment #2 From 2009-10-16 03:04:42 PST -------
(From update of attachment 41278 [details])
Please split these two unrelated changes out in to separate bugs + patches.
------- Comment #3 From 2009-10-16 11:09:15 PST -------
Created an attachment (id=41301) [details]
Round 2

V8 change split to https://bugs.webkit.org/show_bug.cgi?id=30448
------- Comment #4 From 2009-10-16 13:20:22 PST -------
(From update of attachment 41301 [details])
Why delay setting the src until the onload?  Could work just as well in a <script> tag after the <video> element.
------- Comment #5 From 2009-10-16 13:28:52 PST -------
> Why delay setting the src until the onload?  Could work just as well in a
> <script> tag after the <video> element.
> 
That would also work, would it make it a better test?
------- Comment #6 From 2009-10-16 13:30:39 PST -------
I remembered Eric Carlson suggested to use an onload handler instead of additional <script> blocks in https://bugs.webkit.org/show_bug.cgi?id=28327#c52

But if that was for a different reason than this, then I digress :)
------- Comment #7 From 2009-10-16 13:36:35 PST -------
(From update of attachment 41301 [details])
> diff --git a/LayoutTests/media/content/scaled-matrix.ogv b/LayoutTests/media/content/scaled-matrix.ogv
> new file mode 100644
> index 0000000..1684a7b
> Binary files /dev/null and b/LayoutTests/media/content/scaled-matrix.ogv differ

Converting this movie to ogg format doesn't sense unless ogg files can contain a scaling matrix. If they can't it would make more sense to change video-no-audio.html to use a different video-only movie.

r=me with this change if necessary
------- Comment #8 From 2009-10-19 08:58:23 PST -------
(From update of attachment 41301 [details])
Let commit bot land it
------- Comment #9 From 2009-10-19 09:09:13 PST -------
(From update of attachment 41301 [details])
Rejecting patch 41301 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=41301 from bug 30435 failed to download and apply.
------- Comment #10 From 2009-10-19 21:42:53 PST -------
(From update of attachment 41301 [details])
eric.carlson:    review+

However, we can't land this patch because

diff --git a/LayoutTests/media/content/scaled-matrix.ogv b/LayoutTests/media/content/scaled-matrix.ogv
new file mode 100644
index 0000000..1684a7b
Binary files /dev/null and b/LayoutTests/media/content/scaled-matrix.ogv differ

we don't have scaled-matrix.ogv!  Please either upload a diff with the binary content or attach the file separately to this bug.
------- Comment #11 From 2009-10-20 10:25:21 PST -------
Doh forgive my noobness -- I'll fix up the patch one more time and get someone who sits by me to land the patch.
------- Comment #12 From 2009-10-20 10:38:10 PST -------
> Converting this movie to ogg format doesn't sense unless ogg files can contain
> a scaling matrix. If they can't it would make more sense to change
> video-no-audio.html to use a different video-only movie.

Can you please at least comment on the issue I raised?
------- Comment #13 From 2009-10-20 18:32:17 PST -------
Created an attachment (id=41541) [details]
Round 3

I had serious issues rebaselining due to QuickTime version mismatch. You might have to tackle them Eric :(
------- Comment #14 From 2009-10-21 12:43:22 PST -------
(From update of attachment 41541 [details])
r=me
------- Comment #15 From 2009-11-12 18:51:29 PST -------
I believe committing this will cause Mac layout tests to fail... guess one way to find out :s
------- Comment #16 From 2009-11-12 19:06:47 PST -------
(From update of attachment 41541 [details])
Rejecting patch 41541 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Carlson', '--force']" exit_code: 1
Last 500 characters of output:
ith fuzz 3.
patching file LayoutTests/media/audio-constructor-src.html
Hunk #1 succeeded at 3 with fuzz 2.
patching file LayoutTests/media/audio-play-event-expected.txt
patching file LayoutTests/media/audio-play-event.html
patching file LayoutTests/media/video-no-audio.html
patching file LayoutTests/media/video-source-add-src.html
patching file LayoutTests/media/video-src-change.html
Hunk #1 FAILED at 7.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/media/video-src-change.html.rej
------- Comment #17 From 2009-11-18 15:10:13 PST -------
Ping?
------- Comment #18 From 2009-11-18 16:45:57 PST -------
I can upload a new version of the patch, but I can't create the baselines because I don't have Snow Leopard (QuickTime generates a completely different UI).
------- Comment #19 From 2009-11-24 07:49:35 PST -------
Oh.  OK.  The commit-queue is still running on Leopard, so only the leopard tests have to pass for it to actually commit.  But yes, you would need to look at the bots post-commit and post a patch with snow leopard results as well.  Sadly webkit doesn't have try-bots or a rebaselining tool to solve this for you yet.
------- Comment #20 From 2009-11-24 10:44:16 PST -------
Yikes.. ok I'll try and figure something out.

Thanks for the pointers!
------- Comment #21 From 2009-11-24 10:48:40 PST -------
I check in changes like this during a quiet time of the day and update the other port build results from the failed build-bot results.

It is a bit of a pain, but we don't have anything better just yet.
------- Comment #22 From 2009-11-24 10:51:17 PST -------
That might work.. but it's not a long-term solution.  I might be better off learning The Right Way to rebaseline media layout tests.

In the meantime I need to re-upload the patches.. they've gone a bit stale :)
------- Comment #23 From 2009-11-24 10:51:44 PST -------
There is plenty of historical precident for webkit committers using the build bots as try-bots, at least in terms of "check in and then get the new results off the bots" behavior.  Whether that is good or not is a topic for another discussion.  Regardless, existing committers do that all the time.  It's more difficult for you however, because if you manage to break a bot which the commit-queue blocks on:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/buildbot.py#L47
then it won't be able to commit any follow-up patches for you as it will be waiting for the bots to roll green.  The commit-queue is only useful for committing patches which are not expected to break any bots.  It's useless for committing tree fixes, or patches which are expected to break things. :(
------- Comment #24 From 2009-11-24 18:33:15 PST -------
Created an attachment (id=43822) [details]
Round 3 updated

Updated to work against tip of tree.
------- Comment #25 From 2009-11-24 18:39:00 PST -------
Created an attachment (id=43823) [details]
Actual result
------- Comment #26 From 2009-11-24 18:39:19 PST -------
Created an attachment (id=43824) [details]
Expected result
------- Comment #27 From 2009-11-24 18:42:22 PST -------
I've attached a sample expected/actual result... I have no idea why the buttons change position :(

I'm running the following:
Mac 10.5.8
QuickTime Player 7.6.4 (518.35)
QuickTime Version 7.6.4 (1327.73)

Hope that helps!
------- Comment #28 From 2009-11-30 12:40:06 PST -------
style-queue ran check-webkit-style on attachment 43822 [details] without any errors.
------- Comment #29 From 2009-12-09 14:02:40 PST -------
Ping?
------- Comment #30 From 2009-12-09 14:56:31 PST -------
(From update of attachment 43822 [details])
Oh, I didn't realize Andrew wasn't a committer (at least he's not in committers.py).  cq+ing
------- Comment #31 From 2009-12-09 15:14:46 PST -------
(From update of attachment 43822 [details])
Rejecting patch 43822 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11752 test cases.
media/video-no-audio.html -> failed

Exiting early after 1 failures. 9492 tests run.
278.20s total testing time

9491 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output
------- Comment #32 From 2009-12-20 19:07:32 PST -------
This patch will need update, it cannot be landed as is.
------- Comment #33 From 2009-12-21 13:20:01 PST -------
My mac isn't configured properly and I cannot generate proper rebaselines.

I'll resolve those issues and upload a new patch soonish.
------- Comment #34 From 2009-12-23 13:26:34 PST -------
Created an attachment (id=45448) [details]
Round 4

Has new image results, will have to attach and someone land manually.
------- Comment #35 From 2009-12-23 13:28:09 PST -------
Created an attachment (id=45449) [details]
Round 4 image attachment
------- Comment #36 From 2009-12-23 13:28:41 PST -------
Finally figured it out!!
------- Comment #37 From 2009-12-28 20:08:27 PST -------
(From update of attachment 45448 [details])
r=me
------- Comment #38 From 2009-12-28 22:20:32 PST -------
(From update of attachment 45448 [details])
Changing Maciej's r+ to an r-.  This bug is lacking the binary data for the changed png files, so the commit-queue cannot land it.  Andrew is not yet a committer (according to committers.py), so someone would have to land this for him.  Better would be for Andrew to attach a patch containing the png changes.  "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will handle git patches correctly.  Likewise one could just do a git diff --binary and attach that.
------- Comment #39 From 2010-01-04 14:37:35 PST -------
Created an attachment (id=45831) [details]
Round 5
------- Comment #40 From 2010-01-04 14:38:52 PST -------
Created an attachment (id=45832) [details]
Round 6
------- Comment #41 From 2010-01-04 14:41:00 PST -------
Created an attachment (id=45833) [details]
Round 6 + proper ChangeLog
------- Comment #42 From 2010-01-04 14:42:06 PST -------
wow sorry for the spam... had to figure out bugzilla-tool but wow is it ever nifty!!!

thanks for the tip!
------- Comment #43 From 2010-01-04 14:43:58 PST -------
style-queue ran check-webkit-style on attachment 45833 [details] without any errors.
------- Comment #44 From 2010-01-04 14:46:10 PST -------
(From update of attachment 45833 [details])
Is this result now platform specific?
RUN(mediaElement.src = 'content/silence.mpg')
 3 RUN(mediaElement.src = 'content/test.wav')
------- Comment #45 From 2010-01-04 14:54:38 PST -------
(From update of attachment 45833 [details])

> -        <video src="content/scaled-matrix.mov" controls onload="finish()"></video>
> +        <video controls onload="finish()"></video>
  This isn't new to your patch, but you should change this so it doesn't depend on a 'load' event being fired by the <audio> element.
------- Comment #46 From 2010-01-04 15:14:26 PST -------
(From update of attachment 45833 [details])

> +        * platform/mac-leopard/media/video-no-audio-expected.checksum: New results due to changing source video.
> +        * platform/mac-leopard/media/video-no-audio-expected.png: Ditto.
> +        * platform/mac-leopard/media/video-no-audio-expected.txt: Ditto.

You should check these in for SnowLeopard as well. 


> -    waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", "content/test.wav"); });
> +    waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", audioFile); });

Eric's comment is correct. While all ports support wav at the moment, the spec doesn't require it so you should change this to use findMediaFile(). 


> diff --git a/LayoutTests/platform/mac-leopard/media/video-no-audio-expected.png b/LayoutTests/platform/mac-leopard/media/video-no-audio-expected.png
> index 208d4788bdc6f6338561ea69aa9afd31e24b6202..e6b4ec557a2560b4e58d0afa28b3f6f470bd65b3 100644
> GIT binary patch
> literal 20611

> 
> literal 21975

Do you need to set the svn:mime-type on these files somehow?

r=me with these changes.
------- Comment #47 From 2010-01-04 18:31:16 PST -------
(In reply to comment #46)
> (From update of attachment 45833 [details] [details])
> 
> > +        * platform/mac-leopard/media/video-no-audio-expected.checksum: New results due to changing source video.
> > +        * platform/mac-leopard/media/video-no-audio-expected.png: Ditto.
> > +        * platform/mac-leopard/media/video-no-audio-expected.txt: Ditto.
> 
> You should check these in for SnowLeopard as well.

platform/mac-snowleopard/media doesn't exist... where you referring to platform/mac/media?

> > -    waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", "content/test.wav"); });
> > +    waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", audioFile); });
> 
> Eric's comment is correct. While all ports support wav at the moment, the spec
> doesn't require it so you should change this to use findMediaFile(). 

I'm confused... isn't that what my patch is doing in LayoutTests/media/audio-play-event.html ?

> > diff --git a/LayoutTests/platform/mac-leopard/media/video-no-audio-expected.png b/LayoutTests/platform/mac-leopard/media/video-no-audio-expected.png
> > index 208d4788bdc6f6338561ea69aa9afd31e24b6202..e6b4ec557a2560b4e58d0afa28b3f6f470bd65b3 100644
> > GIT binary patch
> > literal 20611
> 
> > 
> > literal 21975
> 
> Do you need to set the svn:mime-type on these files somehow?

Using git I can't set the mime-type.. would have to be a follow-up patch or the committer could set it.
------- Comment #48 From 2010-01-04 18:54:21 PST -------
Created an attachment (id=45857) [details]
Round 7
------- Comment #49 From 2010-01-04 18:55:16 PST -------
Round 7 replaces onload with oncanplaythrough (nice catch!)
------- Comment #50 From 2010-01-04 21:56:32 PST -------
style-queue ran check-webkit-style on attachment 45857 [details] without any errors.
------- Comment #51 From 2010-01-05 09:04:18 PST -------
(From update of attachment 45857 [details])
(In reply to comment #47)
> (In reply to comment #46)
> > (From update of attachment 45833 [details] [details] [details])
> > 
> > > +        * platform/mac-leopard/media/video-no-audio-expected.checksum: New results due to changing source video.
> > > +        * platform/mac-leopard/media/video-no-audio-expected.png: Ditto.
> > > +        * platform/mac-leopard/media/video-no-audio-expected.txt: Ditto.
> > 
> > You should check these in for SnowLeopard as well.
> 
> platform/mac-snowleopard/media doesn't exist... where you referring to
> platform/mac/media?
> 
  Yes, SnowLeopard is the current mac platform.

> > > -    waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", "content/test.wav"); });
> > > +    waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", audioFile); });
> > 
> > Eric's comment is correct. While all ports support wav at the moment, the spec
> > doesn't require it so you should change this to use findMediaFile(). 
> 
> I'm confused... isn't that what my patch is doing in
> LayoutTests/media/audio-play-event.html ?
>
  Yes, but I copied the wrong line from the diff. The problem is in audio-play-event.html where you are calling "run()" with the name of a platform specific media file. "run()" logs the name of the file passed to it so the test result has the platform specific name.

r=me with these changes
------- Comment #52 From 2010-02-02 14:01:09 PST -------
Checking on the status of this patch.  It's been approved for landing for almost a month.
------- Comment #53 From 2010-02-08 15:12:23 PST -------
(From update of attachment 41541 [details])
Cleared Eric Carlson's review+ from obsolete attachment 41541 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #54 From 2010-02-08 15:12:34 PST -------
(From update of attachment 43822 [details])
Cleared Darin Adler's review+ from obsolete attachment 43822 [details] so that this bug does not appear in http://webkit.org/pending-commit.
------- Comment #55 From 2010-02-08 15:47:25 PST -------
Now that I'm a committer and have some more spare time I'll tackle this and upload a new patch and attempt to land it by myself after the review.
------- Comment #56 From 2010-02-17 14:27:43 PST -------
Ping?
------- Comment #57 From 2010-02-25 16:31:37 PST -------
Created an attachment (id=49542) [details]
Round 8
------- Comment #58 From 2010-02-25 16:33:16 PST -------
OK so I fixed the RUN(...) stuff in audio-play-event.html so it's platform independent and copied* the image results from mac-leopard to mac

* I'm positive it won't pass on mac (i.e., Snow Leopard) because Snow Leopard displays a fullscreen button in the controls.. not sure what to do here other than land the patch and grab the correct results off a bot?

Guidance appreciated!
------- Comment #59 From 2010-03-11 15:05:03 PST -------
(From update of attachment 49542 [details])
r=me (and many others before me).

Please find a snow leopard machine and get some results for it to check in before you land it.
------- Comment #60 From 2010-03-25 11:30:45 PST -------
Committed as http://trac.webkit.org/changeset/56481
------- Comment #61 From 2010-03-31 15:57:04 PST -------
media/video-no-audio.html has failed on the Tiger bot ever since this commit.  Please fix. :)
------- Comment #62 From 2010-03-31 15:59:04 PST -------
What the!?  I thought I resolved that in https://trac.webkit.org/changeset/56807

Investigating...
------- Comment #63 From 2010-03-31 16:01:27 PST -------
I'm looking at http://build.webkit.org/builders/Tiger%20Intel%20Release and not seeing any media layout tests failing.

I'm still mastering the ropes here so by all means point out where/what I'm screwing up :)
------- Comment #64 From 2010-03-31 16:02:57 PST -------
My sincerest apologies.

I just finished writing a tool to help me look back in time to find failure like these.  Due to some problem builds on tiger, I started my crawl at r56619, which is in between when it broke and when you fixed it.  So my message to you was in error!

Again, sorry for the confusion.
------- Comment #65 From 2010-04-01 20:12:11 PST -------
No worries!