Bug 93310 – [chromium] Rebaseline canvas-toDataURL-webp test expectations

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

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

Created attachment 193679 [details]
Patch

[Pascal Massimino]

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?

Created attachment 193689 [details]
Patch

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

[Pascal Massimino]

lgtm

[noel gordon]

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.

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

[noel gordon]

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

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

Created attachment 193746 [details]
Patch

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

Created attachment 193748 [details]
Patch

Created attachment 193749 [details]
Patch

[noel gordon]

Looks good James, thanks.  I'll modify the Changelog so any committer can cq+ this change.

[noel gordon]

Created attachment 193801 [details]
Patch for landing

[noel gordon]

James, let me know when you ready and I'll submit your patch.

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

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

[WebKit Review Bot]

Comment on attachment 193801 [details]
Patch for landing

Clearing flags on attachment: 193801

Committed r146288: <http://trac.webkit.org/changeset/146288>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Reopened to do the rebaseline once bug 112761 is complete.

Created attachment 194327 [details]
Patch

[noel gordon]

Looks fine to me.

[noel gordon]

Created attachment 194448 [details]
Patch

(In reply to comment #24)
> Created an attachment (id=194448) [details]
> Patch

Looks good, thanks. Can be submitted anytime.

[WebKit Review Bot]

Comment on attachment 194448 [details]
Patch

Clearing flags on attachment: 194448

Committed r146566: <http://trac.webkit.org/changeset/146566>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Reopened to do the rebaseline once bug 112761 is complete.

[noel gordon]

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.

[Peter Kasting]

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

[noel gordon]

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.

[noel gordon]

Created attachment 194680 [details]
Patch after r146639

(In reply to comment #32)
> Created an attachment (id=194680) [details]
> Patch after r146639

lgtm

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

[WebKit Review Bot]

Comment on attachment 194680 [details]
Patch after r146639

Clearing flags on attachment: 194680

Committed r146709: <http://trac.webkit.org/changeset/146709>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Reopened to do the rebaseline once bug 112761 is complete.

[noel gordon]

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.

[Urvang Joshi]

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

[noel gordon]

That's done by the webkit gardener.  http://www.chromium.org/developers/how-tos/webkit-gardening

[noel gordon]

Landed new expectations on http://trac.webkit.org/changeset/146848