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
Product: WebKit
Classification: Unclassified
Component: Media Elements
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Andrew Scherkus
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-16 02:05 PDT by Andrew Scherkus
Modified: 2010-04-01 20:12 PDT (History)
5 users (show)

See Also:


Attachments
Round 1 (6.33 KB, patch)
2009-10-16 02:11 PDT, Andrew Scherkus
mrowe: review-
Details | Formatted Diff | Diff
Round 2 (4.89 KB, patch)
2009-10-16 11:09 PDT, Andrew Scherkus
abarth: review-
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
Round 3 (5.53 KB, patch)
2009-10-20 18:32 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Round 3 updated (5.62 KB, patch)
2009-11-24 18:33 PST, Andrew Scherkus
no flags 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 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 Details | Formatted Diff | Diff
Round 6 (36.35 KB, patch)
2010-01-04 14:38 PST, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Round 6 + proper ChangeLog (36.35 KB, patch)
2010-01-04 14:41 PST, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Round 7 (36.36 KB, patch)
2010-01-04 18:54 PST, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Round 8 (83.58 KB, patch)
2010-02-25 16:31 PST, Andrew Scherkus
levin: review+
levin: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Scherkus 2009-10-16 02:05:36 PDT
Updated the media layout tests, which unveild a bug in our V8 bindings.
Comment 1 Andrew Scherkus 2009-10-16 02:11:22 PDT
Created attachment 41278 [details]
Round 1

Not marking cq? as it contains a binary file.
Comment 2 Mark Rowe (bdash) 2009-10-16 03:04:42 PDT
Comment on attachment 41278 [details]
Round 1

Please split these two unrelated changes out in to separate bugs + patches.
Comment 3 Andrew Scherkus 2009-10-16 11:09:15 PDT
Created attachment 41301 [details]
Round 2

V8 change split to https://bugs.webkit.org/show_bug.cgi?id=30448
Comment 4 Eric Seidel 2009-10-16 13:20:22 PDT
Comment on attachment 41301 [details]
Round 2

Why delay setting the src until the onload?  Could work just as well in a <script> tag after the <video> element.
Comment 5 Eric Carlson 2009-10-16 13:28:52 PDT
> 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 Andrew Scherkus 2009-10-16 13:30:39 PDT
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 Eric Carlson 2009-10-16 13:36:35 PDT
Comment on attachment 41301 [details]
Round 2

> 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 Yong Li 2009-10-19 08:58:23 PDT
Comment on attachment 41301 [details]
Round 2

Let commit bot land it
Comment 9 WebKit Commit Bot 2009-10-19 09:09:13 PDT
Comment on attachment 41301 [details]
Round 2

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 Adam Barth 2009-10-19 21:42:53 PDT
Comment on attachment 41301 [details]
Round 2

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 Andrew Scherkus 2009-10-20 10:25:21 PDT
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 Eric Carlson 2009-10-20 10:38:10 PDT
> 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 Andrew Scherkus 2009-10-20 18:32:17 PDT
Created attachment 41541 [details]
Round 3

I had serious issues rebaselining due to QuickTime version mismatch. You might have to tackle them Eric :(
Comment 14 Eric Carlson 2009-10-21 12:43:22 PDT
Comment on attachment 41541 [details]
Round 3

r=me
Comment 15 Andrew Scherkus 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 WebKit Commit Bot 2009-11-12 19:06:47 PST
Comment on attachment 41541 [details]
Round 3

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 Eric Seidel 2009-11-18 15:10:13 PST
Ping?
Comment 18 Andrew Scherkus 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 Eric Seidel 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 Andrew Scherkus 2009-11-24 10:44:16 PST
Yikes.. ok I'll try and figure something out.

Thanks for the pointers!
Comment 21 Eric Carlson 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 Andrew Scherkus 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 Eric Seidel 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 Andrew Scherkus 2009-11-24 18:33:15 PST
Created attachment 43822 [details]
Round 3 updated

Updated to work against tip of tree.
Comment 25 Andrew Scherkus 2009-11-24 18:39:00 PST
Created attachment 43823 [details]
Actual result
Comment 26 Andrew Scherkus 2009-11-24 18:39:19 PST
Created attachment 43824 [details]
Expected result
Comment 27 Andrew Scherkus 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 Adam Barth 2009-11-30 12:40:06 PST
style-queue ran check-webkit-style on attachment 43822 [details] without any errors.
Comment 29 Eric Seidel 2009-12-09 14:02:40 PST
Ping?
Comment 30 Eric Seidel 2009-12-09 14:56:31 PST
Comment on attachment 43822 [details]
Round 3 updated

Oh, I didn't realize Andrew wasn't a committer (at least he's not in committers.py).  cq+ing
Comment 31 WebKit Commit Bot 2009-12-09 15:14:46 PST
Comment on attachment 43822 [details]
Round 3 updated

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 Eric Seidel 2009-12-20 19:07:32 PST
This patch will need update, it cannot be landed as is.
Comment 33 Andrew Scherkus 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 Andrew Scherkus 2009-12-23 13:26:34 PST
Created attachment 45448 [details]
Round 4

Has new image results, will have to attach and someone land manually.
Comment 35 Andrew Scherkus 2009-12-23 13:28:09 PST
Created attachment 45449 [details]
Round 4 image attachment
Comment 36 Andrew Scherkus 2009-12-23 13:28:41 PST
Finally figured it out!!
Comment 37 Maciej Stachowiak 2009-12-28 20:08:27 PST
Comment on attachment 45448 [details]
Round 4

r=me
Comment 38 Eric Seidel 2009-12-28 22:20:32 PST
Comment on attachment 45448 [details]
Round 4

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 Andrew Scherkus 2010-01-04 14:37:35 PST
Created attachment 45831 [details]
Round 5
Comment 40 Andrew Scherkus 2010-01-04 14:38:52 PST
Created attachment 45832 [details]
Round 6
Comment 41 Andrew Scherkus 2010-01-04 14:41:00 PST
Created attachment 45833 [details]
Round 6 + proper ChangeLog
Comment 42 Andrew Scherkus 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 WebKit Review Bot 2010-01-04 14:43:58 PST
style-queue ran check-webkit-style on attachment 45833 [details] without any errors.
Comment 44 Eric Seidel 2010-01-04 14:46:10 PST
Comment on attachment 45833 [details]
Round 6 + proper ChangeLog

Is this result now platform specific?
RUN(mediaElement.src = 'content/silence.mpg')
 3 RUN(mediaElement.src = 'content/test.wav')
Comment 45 Eric Carlson 2010-01-04 14:54:38 PST
Comment on attachment 45833 [details]
Round 6 + proper ChangeLog


> -        <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 Eric Carlson 2010-01-04 15:14:26 PST
Comment on attachment 45833 [details]
Round 6 + proper ChangeLog


> +        * 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 Andrew Scherkus 2010-01-04 18:31:16 PST
(In reply to comment #46)
> (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.

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 Andrew Scherkus 2010-01-04 18:54:21 PST
Created attachment 45857 [details]
Round 7
Comment 49 Andrew Scherkus 2010-01-04 18:55:16 PST
Round 7 replaces onload with oncanplaythrough (nice catch!)
Comment 50 WebKit Review Bot 2010-01-04 21:56:32 PST
style-queue ran check-webkit-style on attachment 45857 [details] without any errors.
Comment 51 Eric Carlson 2010-01-05 09:04:18 PST
Comment on attachment 45857 [details]
Round 7

(In reply to comment #47)
> (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?
> 
  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 Eric Seidel 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 Eric Seidel 2010-02-08 15:12:23 PST
Comment on attachment 41541 [details]
Round 3

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 Eric Seidel 2010-02-08 15:12:34 PST
Comment on attachment 43822 [details]
Round 3 updated

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 Andrew Scherkus 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 Eric Seidel 2010-02-17 14:27:43 PST
Ping?
Comment 57 Andrew Scherkus 2010-02-25 16:31:37 PST
Created attachment 49542 [details]
Round 8
Comment 58 Andrew Scherkus 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 David Levin 2010-03-11 15:05:03 PST
Comment on attachment 49542 [details]
Round 8

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 Andrew Scherkus 2010-03-25 11:30:45 PDT
Committed as http://trac.webkit.org/changeset/56481
Comment 61 Eric Seidel 2010-03-31 15:57:04 PDT
media/video-no-audio.html has failed on the Tiger bot ever since this commit.  Please fix. :)
Comment 62 Andrew Scherkus 2010-03-31 15:59:04 PDT
What the!?  I thought I resolved that in https://trac.webkit.org/changeset/56807

Investigating...
Comment 63 Andrew Scherkus 2010-03-31 16:01:27 PDT
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 Eric Seidel 2010-03-31 16:02:57 PDT
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 Andrew Scherkus 2010-04-01 20:12:11 PDT
No worries!