Bug 93310 - [chromium] Rebaseline canvas-toDataURL-webp test expectations
Summary: [chromium] Rebaseline canvas-toDataURL-webp test expectations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: noel gordon
URL:
Keywords:
Depends on: 112761
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-06 17:11 PDT by jzern
Modified: 2013-03-25 21:07 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.43 KB, patch)
2013-03-18 16:09 PDT, jzern
no flags Details | Formatted Diff | Diff
Patch (4.42 KB, patch)
2013-03-18 16:47 PDT, jzern
no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2013-03-19 00:00 PDT, jzern
no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2013-03-19 00:07 PDT, jzern
no flags Details | Formatted Diff | Diff
Patch (2.15 KB, patch)
2013-03-19 00:10 PDT, jzern
no flags Details | Formatted Diff | Diff
Patch for landing (2.16 KB, patch)
2013-03-19 05:28 PDT, noel gordon
no flags Details | Formatted Diff | Diff
Patch (2.26 KB, patch)
2013-03-21 13:51 PDT, jzern
no flags Details | Formatted Diff | Diff
Patch (2.18 KB, patch)
2013-03-21 23:10 PDT, noel gordon
no flags Details | Formatted Diff | Diff
Patch after r146639 (1.52 KB, patch)
2013-03-22 19:25 PDT, noel gordon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jzern 2012-08-06 17:11:57 PDT
Encoder changes from 0.1.3 to 0.2.0 will cause the test to fail due to excessive image differences.

0.2.0 will be posted to:
https://code.google.com/p/webp/downloads/list
Comment 1 jzern 2013-03-18 16:09:37 PDT
Created attachment 193679 [details]
Patch
Comment 2 Pascal Massimino 2013-03-18 16:30:22 PDT
Comment on attachment 193679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193679&action=review

> LayoutTests/fast/canvas/canvas-toDataURL-webp.html:14
> +        document.getElementById('log').textContent += "FAIL: picture sizes differ";

sizes -> dimensions ?

(to avoid the confusion with filesize)

> LayoutTests/fast/canvas/canvas-toDataURL-webp.html:19
> +        line_offset = y * pic1.width * 4;

'var line_offset' ?

> LayoutTests/fast/canvas/canvas-toDataURL-webp.html:21
> +            // skip alpha

comment could go at line 22

> LayoutTests/fast/canvas/canvas-toDataURL-webp.html:22
> +            if ((x + 1) % 4 == 0) continue;

i'd write it as: "if (x % 4 == 3)" rather.

Btw, is the output format RGBA or ARGB ? (or else?) always on all platforms?
Comment 3 jzern 2013-03-18 16:47:07 PDT
Created attachment 193689 [details]
Patch
Comment 4 jzern 2013-03-18 16:48:18 PDT
Comment on attachment 193679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193679&action=review

>> LayoutTests/fast/canvas/canvas-toDataURL-webp.html:14
>> +        document.getElementById('log').textContent += "FAIL: picture sizes differ";
> 
> sizes -> dimensions ?
> 
> (to avoid the confusion with filesize)

done.

>> LayoutTests/fast/canvas/canvas-toDataURL-webp.html:19
>> +        line_offset = y * pic1.width * 4;
> 
> 'var line_offset' ?

missed that one or left it in another tree, thanks!

>> LayoutTests/fast/canvas/canvas-toDataURL-webp.html:21
>> +            // skip alpha
> 
> comment could go at line 22

done

>> LayoutTests/fast/canvas/canvas-toDataURL-webp.html:22
>> +            if ((x + 1) % 4 == 0) continue;
> 
> i'd write it as: "if (x % 4 == 3)" rather.
> 
> Btw, is the output format RGBA or ARGB ? (or else?) always on all platforms?

Seems so:

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-imagedata-data
Comment 5 Pascal Massimino 2013-03-18 16:58:31 PDT
lgtm
Comment 6 noel gordon 2013-03-18 21:20:13 PDT
We are not the only port using webp.  I would disable the test for chromium by adding the following line to LayoutTests/platform/chromium/TestExpectations

  webkit.org/b/93310 fast/canvas/canvas-toDataURL-webp.html [ ImageOnlyFailure ]

and land that change.  Then land libwebp 0.2.0 in chromium.

Once the chromium change has rolled into webkit, and the webkit bots have cycled, rebaseline the test images and re-enable the test in chromium TestExpectations.
Comment 7 jzern 2013-03-18 22:31:35 PDT
(In reply to comment #6)
> We are not the only port using webp.  I would disable the test for chromium by adding the following line to LayoutTests/platform/chromium/TestExpectations
> 
>   webkit.org/b/93310 fast/canvas/canvas-toDataURL-webp.html [ ImageOnlyFailure ]
> 
> and land that change.  Then land libwebp 0.2.0 in chromium.
> 
> Once the chromium change has rolled into webkit, and the webkit bots have cycled, rebaseline the test images and re-enable the test in chromium TestExpectations.

The point of this change is to avoid that cycle. This works with current chromium (0.2.1 -some encoder changes, so roughly =0.1.3) and the current 0.3.0 branch.
Comment 8 noel gordon 2013-03-18 22:48:18 PDT
(In reply to comment #7)

> The point of this change is to avoid that cycle. This works with current chromium (0.2.1 -some encoder changes, so roughly =0.1.3) and the current 0.3.0 branch.

Does it work on GTK and QT?
Comment 9 jzern 2013-03-18 22:56:25 PDT
(In reply to comment #8)
> (In reply to comment #7)
> 
> > The point of this change is to avoid that cycle. This works with current chromium (0.2.1 -some encoder changes, so roughly =0.1.3) and the current 0.3.0 branch.
> 
> Does it work on GTK and QT?

The bots seem down with it.
Comment 10 jzern 2013-03-19 00:00:06 PDT
Created attachment 193746 [details]
Patch
Comment 11 jzern 2013-03-19 00:02:18 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > 
> > > The point of this change is to avoid that cycle. This works with current chromium (0.2.1 -some encoder changes, so roughly =0.1.3) and the current 0.3.0 branch.
> > 
> > Does it work on GTK and QT?
> 
> The bots seem down with it.

OK Noel set me straight with regard to the process in these things...
Comment 12 jzern 2013-03-19 00:07:58 PDT
Created attachment 193748 [details]
Patch
Comment 13 jzern 2013-03-19 00:10:43 PDT
Created attachment 193749 [details]
Patch
Comment 14 noel gordon 2013-03-19 05:25:39 PDT
Looks good James, thanks.  I'll modify the Changelog so any committer can cq+ this change.
Comment 15 noel gordon 2013-03-19 05:28:54 PDT
Created attachment 193801 [details]
Patch for landing
Comment 16 noel gordon 2013-03-19 18:14:00 PDT
James, let me know when you ready and I'll submit your patch.
Comment 17 jzern 2013-03-19 18:15:09 PDT
(In reply to comment #16)
> James, let me know when you ready and I'll submit your patch.

Looks good. Thanks again for helping with the process.
Comment 18 jzern 2013-03-19 18:18:01 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > James, let me know when you ready and I'll submit your patch.
> 
> Looks good. Thanks again for helping with the process.

Which is to say, yes I'm ready to submit.
Comment 19 WebKit Review Bot 2013-03-19 18:50:19 PDT
Comment on attachment 193801 [details]
Patch for landing

Clearing flags on attachment: 193801

Committed r146288: <http://trac.webkit.org/changeset/146288>
Comment 20 WebKit Review Bot 2013-03-19 18:50:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 noel gordon 2013-03-19 19:01:20 PDT
Reopened to do the rebaseline once bug 112761 is complete.
Comment 22 jzern 2013-03-21 13:51:35 PDT
Created attachment 194327 [details]
Patch
Comment 23 noel gordon 2013-03-21 22:52:31 PDT
Looks fine to me.
Comment 24 noel gordon 2013-03-21 23:10:23 PDT
Created attachment 194448 [details]
Patch
Comment 25 jzern 2013-03-21 23:22:32 PDT
(In reply to comment #24)
> Created an attachment (id=194448) [details]
> Patch

Looks good, thanks. Can be submitted anytime.
Comment 26 WebKit Review Bot 2013-03-22 00:12:22 PDT
Comment on attachment 194448 [details]
Patch

Clearing flags on attachment: 194448

Committed r146566: <http://trac.webkit.org/changeset/146566>
Comment 27 WebKit Review Bot 2013-03-22 00:12:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 noel gordon 2013-03-22 00:19:55 PDT
Reopened to do the rebaseline once bug 112761 is complete.
Comment 29 noel gordon 2013-03-22 17:59:21 PDT
Android expectations were removed in http://trac.webkit.org/changeset/146639. There is no explanation in the ChangeLog as to why.

The r146639 change undid the work on this bug, so we'll need to bring it back.  Let me prepare a patch.
Comment 30 Peter Kasting 2013-03-22 18:36:24 PDT
(In reply to comment #29)
> Android expectations were removed in http://trac.webkit.org/changeset/146639. There is no explanation in the ChangeLog as to why.

Android doesn't want its test expectations in the Chromium files at all for now, so I removed them on their behalf.  I sent an email to the Chromium WebKit gardening list about this and Peter Beverloo sent one to the Android developers.  There wasn't anything really to say in the ChangeLog other than "Remove these for now", especially when I didn't have the longer context Peter did.

> The r146639 change undid the work on this bug, so we'll need to bring it back.  Let me prepare a patch.

Sorry!  I missed the two lines here.  (In fairness, having them in the midst of a huge block of [ Android ] lines, in a section that said "ANDROID PORT TESTS", was perhaps asking for trouble...)  Please feel free to re-add them to TestExpectations, or let me know you'd like me to do it and I'll take care of it once I'm back at my main workstation later tonight.
Comment 31 noel gordon 2013-03-22 18:53:07 PDT
Appreciate the (longer) explanation.  We didn't expect to be in the Android section very long (a day or so), and we certainly didn't anticipate the total removal of Android expectations.  No mind, I'm preparing a patch.
Comment 32 noel gordon 2013-03-22 19:25:22 PDT
Created attachment 194680 [details]
Patch after r146639
Comment 33 jzern 2013-03-22 19:28:39 PDT
(In reply to comment #32)
> Created an attachment (id=194680) [details]
> Patch after r146639

lgtm
Comment 34 jzern 2013-03-22 19:31:50 PDT
(In reply to comment #31)
> Appreciate the (longer) explanation.  We didn't expect to be in the Android section very long (a day or so), and we certainly didn't anticipate the total removal of Android expectations.  No mind, I'm preparing a patch.

Nice catch by the way. The chromium change is in the commit-queue with local expectations set, but a later WebKit roll might knock those out without this change now -- assuming it lands.
Comment 35 WebKit Review Bot 2013-03-22 20:36:41 PDT
Comment on attachment 194680 [details]
Patch after r146639

Clearing flags on attachment: 194680

Committed r146709: <http://trac.webkit.org/changeset/146709>
Comment 36 WebKit Review Bot 2013-03-22 20:36:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 noel gordon 2013-03-22 20:54:33 PDT
Reopened to do the rebaseline once bug 112761 is complete.
Comment 38 noel gordon 2013-03-24 21:31:36 PDT
Landed http://trac.webkit.org/changeset/146743

Mark canvas-toDataURL-webp.html imageOnlyFailure to start generating new test images on the bots now that libwebp 0.3.0 has landed in chromium webkit.
Comment 39 Urvang Joshi 2013-03-25 00:19:59 PDT
I believe the webkit_revision in DEPS file (http://src.chromium.org/viewvc/chrome/trunk/src/DEPS?view=markup) needs an update too (so that new TestExpectations file from Webkit gets pulled)?
Comment 40 noel gordon 2013-03-25 00:48:35 PDT
That's done by the webkit gardener.  http://www.chromium.org/developers/how-tos/webkit-gardening
Comment 41 noel gordon 2013-03-25 21:07:10 PDT
Landed new expectations on http://trac.webkit.org/changeset/146848