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
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
lgtm
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.
(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
Looks good James, thanks. I'll modify the Changelog so any committer can cq+ this change.
Created attachment 193801 [details] Patch for landing
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.
Comment on attachment 193801 [details] Patch for landing Clearing flags on attachment: 193801 Committed r146288: <http://trac.webkit.org/changeset/146288>
All reviewed patches have been landed. Closing bug.
Reopened to do the rebaseline once bug 112761 is complete.
Created attachment 194327 [details] Patch
Looks fine to me.
Created attachment 194448 [details] Patch
(In reply to comment #24) > Created an attachment (id=194448) [details] > Patch Looks good, thanks. Can be submitted anytime.
Comment on attachment 194448 [details] Patch Clearing flags on attachment: 194448 Committed r146566: <http://trac.webkit.org/changeset/146566>
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.
(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.
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.
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.
Comment on attachment 194680 [details] Patch after r146639 Clearing flags on attachment: 194680 Committed r146709: <http://trac.webkit.org/changeset/146709>
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.
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)?
That's done by the webkit gardener. http://www.chromium.org/developers/how-tos/webkit-gardening
Landed new expectations on http://trac.webkit.org/changeset/146848