Bug 30435 – Update media layout tests to be codec aware and a V8 binding fix.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Andrew Scherkus]

Updated the media layout tests, which unveild a bug in our V8 bindings.

[Andrew Scherkus]

Created attachment 41278 [details]
Round 1

Not marking cq? as it contains a binary file.

[Mark Rowe (bdash)]

Comment on attachment 41278 [details]
Round 1

Please split these two unrelated changes out in to separate bugs + patches.

[Andrew Scherkus]

Created attachment 41301 [details]
Round 2

V8 change split to https://bugs.webkit.org/show_bug.cgi?id=30448

[Eric Seidel (no email)]

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.

[Eric Carlson]

> 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?

[Andrew Scherkus]

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 :)

[Eric Carlson]

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

[Yong Li]

Comment on attachment 41301 [details]
Round 2

Let commit bot land it

[WebKit Commit Bot]

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.

[Adam Barth]

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.

[Andrew Scherkus]

Doh forgive my noobness -- I'll fix up the patch one more time and get someone who sits by me to land the patch.

[Eric Carlson]

> 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?

[Andrew Scherkus]

Created attachment 41541 [details]
Round 3

I had serious issues rebaselining due to QuickTime version mismatch. You might have to tackle them Eric :(

[Eric Carlson]

Comment on attachment 41541 [details]
Round 3

r=me

[Andrew Scherkus]

I believe committing this will cause Mac layout tests to fail... guess one way to find out :s

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Ping?

[Andrew Scherkus]

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).

[Eric Seidel (no email)]

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.

[Andrew Scherkus]

Yikes.. ok I'll try and figure something out.

Thanks for the pointers!

[Eric Carlson]

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.

[Andrew Scherkus]

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 :)

[Eric Seidel (no email)]

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. :(

[Andrew Scherkus]

Created attachment 43822 [details]
Round 3 updated

Updated to work against tip of tree.

[Andrew Scherkus]

Created attachment 43823 [details]
Actual result

[Andrew Scherkus]

Created attachment 43824 [details]
Expected result

[Andrew Scherkus]

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!

[Adam Barth]

style-queue ran check-webkit-style on attachment 43822 [details] without any errors.

[Eric Seidel (no email)]

Ping?

[Eric Seidel (no email)]

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

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

This patch will need update, it cannot be landed as is.

[Andrew Scherkus]

My mac isn't configured properly and I cannot generate proper rebaselines.

I'll resolve those issues and upload a new patch soonish.

[Andrew Scherkus]

Created attachment 45448 [details]
Round 4

Has new image results, will have to attach and someone land manually.

[Andrew Scherkus]

Created attachment 45449 [details]
Round 4 image attachment

[Andrew Scherkus]

Finally figured it out!!

[Maciej Stachowiak]

Comment on attachment 45448 [details]
Round 4

r=me

[Eric Seidel (no email)]

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.

[Andrew Scherkus]

Created attachment 45831 [details]
Round 5

[Andrew Scherkus]

Created attachment 45832 [details]
Round 6

[Andrew Scherkus]

Created attachment 45833 [details]
Round 6 + proper ChangeLog

[Andrew Scherkus]

wow sorry for the spam... had to figure out bugzilla-tool but wow is it ever nifty!!!

thanks for the tip!

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45833 [details] without any errors.

[Eric Seidel (no email)]

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')

[Eric Carlson]

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.

[Eric Carlson]

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.

[Andrew Scherkus]

(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.

[Andrew Scherkus]

Created attachment 45857 [details]
Round 7

[Andrew Scherkus]

Round 7 replaces onload with oncanplaythrough (nice catch!)

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45857 [details] without any errors.

[Eric Carlson]

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

[Eric Seidel (no email)]

Checking on the status of this patch.  It's been approved for landing for almost a month.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Andrew Scherkus]

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.

[Eric Seidel (no email)]

Ping?

[Andrew Scherkus]

Created attachment 49542 [details]
Round 8

[Andrew Scherkus]

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!

[David Levin]

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.

[Andrew Scherkus]

Committed as http://trac.webkit.org/changeset/56481

[Eric Seidel (no email)]

media/video-no-audio.html has failed on the Tiger bot ever since this commit.  Please fix. :)

[Andrew Scherkus]

What the!?  I thought I resolved that in https://trac.webkit.org/changeset/56807

Investigating...

[Andrew Scherkus]

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 :)

[Eric Seidel (no email)]

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.

[Andrew Scherkus]

No worries!