Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
776673002:
Collapse consecutive SkTableColorFilters (Closed)
DescriptionCollapse consecutive SkTableColorFilters
BUG=skia:1366
For the added bench, the collapsing makes the bench take:
- 70% of the time for CPU rendering of 3 consecutive matrix filters
- almost no change in the GPU rendering of the matrix filters
- 50% of the time for CPU and GPU rendering of 3 consecutive table filters
Committed: https://skia.googlesource.com/skia/+/c12b74dc413ef024b13e0ed478491c4b1bafe6b1
Patch Set 1 #
Total comments: 10
Patch Set 2 : Take into account senorblanco@'s comments #
Total comments: 1
Patch Set 3 : Implement the benchmark. #
Total comments: 11
Patch Set 4 : Take into account most of junov@'s comments #Patch Set 5 : Add a comment to explain what the gm does #
Total comments: 3
Patch Set 6 : Take into account Reed's comments #Patch Set 7 : Add an ignore for the changed GM #
Total comments: 7
Patch Set 8 : Add comments for the positioning logic #Patch Set 9 : Steven's comments, -ignored-tests.txt #
Total comments: 1
Patch Set 10 : Fix MSVC warning #Patch Set 11 : Rebased #Patch Set 12 : Fix another msvc warning #
Messages
Total messages: 53 (16 generated)
cwallez@google.com changed reviewers: + junov@chromium.org, senorblanco@google.com
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
Almost there: just some nits and cleanup, and also needs a bench to show the perf improvement. https://codereview.chromium.org/776673002/diff/1/gm/tablecolorfilter.cpp File gm/tablecolorfilter.cpp (right): https://codereview.chromium.org/776673002/diff/1/gm/tablecolorfilter.cpp#newc... gm/tablecolorfilter.cpp:127: y += bm.height() * 9 / 8; Nit: put this computation into a constant (yOffset? ySpacing?) to avoid repeating it below. https://codereview.chromium.org/776673002/diff/1/gm/tablecolorfilter.cpp#newc... gm/tablecolorfilter.cpp:130: for (int i = -1; i < (int)SK_ARRAY_COUNT(cfMakers); ++i) { Hmm, that's slightly ugly. Maybe make cfMakers[0] a function that returns NULL, then make int i -> size_t to avoid the cast here (and the -1 check and the reset() below). Would that work? https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... src/effects/SkColorFilterImageFilter.cpp:77: if (input && input->asColorFilter(&inputColorFilter)) { I think we still need the check for inputColorFilter being non-NULL, since the CreateProc (via SkReadBuffer::readFlattenable) might return NULL. https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... src/effects/SkColorFilterImageFilter.cpp:94: colorTable.lockPixels(); Nit: maybe use an SkAutoLockPixels guard here instead? https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... src/effects/SkColorFilterImageFilter.cpp:96: combine_color_tables(colorTable.getAddr8(0, 0), inputTable.getAddr8(0, 0), combinedTable); Nit: Skia coding style has a 100-col limit.
cwallez@google.com changed reviewers: - junov@chromium.org, senorblanco@chromium.org
Still need to make a bench https://codereview.chromium.org/776673002/diff/1/gm/tablecolorfilter.cpp File gm/tablecolorfilter.cpp (right): https://codereview.chromium.org/776673002/diff/1/gm/tablecolorfilter.cpp#newc... gm/tablecolorfilter.cpp:127: y += bm.height() * 9 / 8; On 2014/12/04 15:36:24, Stephen White wrote: > Nit: put this computation into a constant (yOffset? ySpacing?) to avoid > repeating it below. Done. https://codereview.chromium.org/776673002/diff/1/gm/tablecolorfilter.cpp#newc... gm/tablecolorfilter.cpp:130: for (int i = -1; i < (int)SK_ARRAY_COUNT(cfMakers); ++i) { On 2014/12/04 15:36:24, Stephen White wrote: > Hmm, that's slightly ugly. Maybe make cfMakers[0] a function that returns NULL, > then make int i -> size_t to avoid the cast here (and the -1 check and the > reset() below). Would that work? Done. https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... src/effects/SkColorFilterImageFilter.cpp:77: if (input && input->asColorFilter(&inputColorFilter)) { On 2014/12/04 15:36:24, Stephen White wrote: > I think we still need the check for inputColorFilter being non-NULL, since the > CreateProc (via SkReadBuffer::readFlattenable) might return NULL. Done. https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... src/effects/SkColorFilterImageFilter.cpp:94: colorTable.lockPixels(); On 2014/12/04 15:36:24, Stephen White wrote: > Nit: maybe use an SkAutoLockPixels guard here instead? Done. https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... src/effects/SkColorFilterImageFilter.cpp:96: combine_color_tables(colorTable.getAddr8(0, 0), inputTable.getAddr8(0, 0), combinedTable); On 2014/12/04 15:36:24, Stephen White wrote: > Nit: Skia coding style has a 100-col limit. Done.
On 2014/12/15 19:19:52, cwallez wrote: > Still need to make a bench > > https://codereview.chromium.org/776673002/diff/1/gm/tablecolorfilter.cpp > File gm/tablecolorfilter.cpp (right): > > https://codereview.chromium.org/776673002/diff/1/gm/tablecolorfilter.cpp#newc... > gm/tablecolorfilter.cpp:127: y += bm.height() * 9 / 8; > On 2014/12/04 15:36:24, Stephen White wrote: > > Nit: put this computation into a constant (yOffset? ySpacing?) to avoid > > repeating it below. > > Done. > > https://codereview.chromium.org/776673002/diff/1/gm/tablecolorfilter.cpp#newc... > gm/tablecolorfilter.cpp:130: for (int i = -1; i < (int)SK_ARRAY_COUNT(cfMakers); > ++i) { > On 2014/12/04 15:36:24, Stephen White wrote: > > Hmm, that's slightly ugly. Maybe make cfMakers[0] a function that returns > NULL, > > then make int i -> size_t to avoid the cast here (and the -1 check and the > > reset() below). Would that work? > > Done. > > https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... > File src/effects/SkColorFilterImageFilter.cpp (right): > > https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... > src/effects/SkColorFilterImageFilter.cpp:77: if (input && > input->asColorFilter(&inputColorFilter)) { > On 2014/12/04 15:36:24, Stephen White wrote: > > I think we still need the check for inputColorFilter being non-NULL, since the > > CreateProc (via SkReadBuffer::readFlattenable) might return NULL. > > Done. > > https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... > src/effects/SkColorFilterImageFilter.cpp:94: colorTable.lockPixels(); > On 2014/12/04 15:36:24, Stephen White wrote: > > Nit: maybe use an SkAutoLockPixels guard here instead? > > Done. > > https://codereview.chromium.org/776673002/diff/1/src/effects/SkColorFilterIma... > src/effects/SkColorFilterImageFilter.cpp:96: > combine_color_tables(colorTable.getAddr8(0, 0), inputTable.getAddr8(0, 0), > combinedTable); > On 2014/12/04 15:36:24, Stephen White wrote: > > Nit: Skia coding style has a 100-col limit. > > Done. Looks good, once the bench is done.
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/776673002/diff/20001/gm/tablecolorfilter.cpp File gm/tablecolorfilter.cpp (right): https://codereview.chromium.org/776673002/diff/20001/gm/tablecolorfilter.cpp#... gm/tablecolorfilter.cpp:114: static SkColorFilter* (*cfMakers[])() = {make_null_cf, make_cf0, make_cf1, make_cf2, make_cf3}; Nit: 100 cols.
cwallez@google.com changed reviewers: + junov@google.com - senorblanco@chromium.org, senorblanco@google.com
+junov
junov@chromium.org changed reviewers: + junov@chromium.org
https://codereview.chromium.org/776673002/diff/40001/bench/ImageFilterCollaps... File bench/ImageFilterCollapse.cpp (right): https://codereview.chromium.org/776673002/diff/40001/bench/ImageFilterCollaps... bench/ImageFilterCollapse.cpp:32: for(int i = nFilters; i --> 0;) { Add comment to explain you are creating a chain of color filters https://codereview.chromium.org/776673002/diff/40001/bench/ImageFilterCollaps... bench/ImageFilterCollapse.cpp:84: uint8_t table1[256], table2[256], table3[256]; Only the things you really want to measure should go in onDraw. The the code that fills-in the color tables should be in here. You could do that work at construction time. Same goes for the set-up parts of BaseImageFilterCollapseBench::doDraw https://codereview.chromium.org/776673002/diff/40001/bench/ImageFilterCollaps... bench/ImageFilterCollapse.cpp:139: SkColorFilter* colorFilters[] = { Same thing here: this initialization should not be in onDraw. https://codereview.chromium.org/776673002/diff/40001/gm/tablecolorfilter.cpp File gm/tablecolorfilter.cpp (right): https://codereview.chromium.org/776673002/diff/40001/gm/tablecolorfilter.cpp#... gm/tablecolorfilter.cpp:114: static SkColorFilter* (*cfMakers[])() = { make_null_cf, make_cf0, make_cf1, Rename: gColorFilterMakers, and below gMakers should be disambiguated: gBitmapMakers FYI 'g' means global (because the variable is static) https://codereview.chromium.org/776673002/diff/40001/gm/tablecolorfilter.cpp#... gm/tablecolorfilter.cpp:150: x += xOffset; This is very confusing, the x and y being offsetted in multiple places with th three nested loops. I think you should consider breaking this into more than one test
cwallez@google.com changed reviewers: + senorblanco@google.com
https://codereview.chromium.org/776673002/diff/40001/bench/ImageFilterCollaps... File bench/ImageFilterCollapse.cpp (right): https://codereview.chromium.org/776673002/diff/40001/bench/ImageFilterCollaps... bench/ImageFilterCollapse.cpp:32: for(int i = nFilters; i --> 0;) { On 2014/12/19 23:54:37, junov wrote: > Add comment to explain you are creating a chain of color filters Done. https://codereview.chromium.org/776673002/diff/40001/bench/ImageFilterCollaps... bench/ImageFilterCollapse.cpp:84: uint8_t table1[256], table2[256], table3[256]; On 2014/12/19 23:54:37, junov wrote: > Only the things you really want to measure should go in onDraw. The the code > that fills-in the color tables should be in here. You could do that work at > construction time. Same goes for the set-up parts of > BaseImageFilterCollapseBench::doDraw Done. https://codereview.chromium.org/776673002/diff/40001/bench/ImageFilterCollaps... bench/ImageFilterCollapse.cpp:139: SkColorFilter* colorFilters[] = { On 2014/12/19 23:54:37, junov wrote: > Same thing here: this initialization should not be in onDraw. Done. https://codereview.chromium.org/776673002/diff/40001/gm/tablecolorfilter.cpp File gm/tablecolorfilter.cpp (right): https://codereview.chromium.org/776673002/diff/40001/gm/tablecolorfilter.cpp#... gm/tablecolorfilter.cpp:114: static SkColorFilter* (*cfMakers[])() = { make_null_cf, make_cf0, make_cf1, On 2014/12/19 23:54:37, junov wrote: > Rename: gColorFilterMakers, and below gMakers should be disambiguated: > gBitmapMakers > FYI 'g' means global (because the variable is static) Done. https://codereview.chromium.org/776673002/diff/40001/gm/tablecolorfilter.cpp#... gm/tablecolorfilter.cpp:150: x += xOffset; On 2014/12/19 23:54:37, junov wrote: > This is very confusing, the x and y being offsetted in multiple places with th > three nested loops. I think you should consider breaking this into more than one > test Yes, it is somewhat confusing, the two solutions I see to this are 1) add a comment describing what the test result looks like (and what goes where) 2) split the tests by bitmap 1) is fine, if not really a fix but 2) is kind of an arbitrary split, which I don't like 100%. What do you think is best?
https://codereview.chromium.org/776673002/diff/40001/gm/tablecolorfilter.cpp File gm/tablecolorfilter.cpp (right): https://codereview.chromium.org/776673002/diff/40001/gm/tablecolorfilter.cpp#... gm/tablecolorfilter.cpp:150: x += xOffset; On 2015/01/19 19:56:13, cwallez wrote: > On 2014/12/19 23:54:37, junov wrote: > > This is very confusing, the x and y being offsetted in multiple places with th > > three nested loops. I think you should consider breaking this into more than > one > > test > Yes, it is somewhat confusing, the two solutions I see to this are > 1) add a comment describing what the test result looks like (and what goes > where) > 2) split the tests by bitmap > > 1) is fine, if not really a fix but 2) is kind of an arbitrary split, which I > don't like 100%. What do you think is best? Yeah, I suppose a good comment would be fine
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/776673002/diff/80001/src/effects/SkColorFilte... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.chromium.org/776673002/diff/80001/src/effects/SkColorFilte... src/effects/SkColorFilterImageFilter.cpp:25: out[i+j*5] += SkScalarMul(a[k+j*5], b[i+k*5]); unrelated nit: SkScalarMul no long needed, * is just fine. https://codereview.chromium.org/776673002/diff/80001/src/effects/SkColorFilte... src/effects/SkColorFilterImageFilter.cpp:30: void combine_color_tables(const uint8_t a[4 * 256], const uint8_t b[4 * 256], uint8_t out[4 * 256]) { nit: 100col max https://codereview.chromium.org/776673002/diff/80001/src/effects/SkColorFilte... src/effects/SkColorFilterImageFilter.cpp:30: void combine_color_tables(const uint8_t a[4 * 256], const uint8_t b[4 * 256], uint8_t out[4 * 256]) { // combine means ...
I added the changed gm to ignored-tests.txt so unless you have other comments, everything is ready.
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
https://codereview.chromium.org/776673002/diff/120001/bench/ImageFilterCollap... File bench/ImageFilterCollapse.cpp (right): https://codereview.chromium.org/776673002/diff/120001/bench/ImageFilterCollap... bench/ImageFilterCollapse.cpp:17: // Chains several matrix color filter imager filter or several Nit: imager filter -> image filters https://codereview.chromium.org/776673002/diff/120001/bench/ImageFilterCollap... bench/ImageFilterCollapse.cpp:28: imageFilter->unref(); Nit: could also use SkSafeUnref(imageFilter). https://codereview.chromium.org/776673002/diff/120001/bench/ImageFilterCollap... bench/ImageFilterCollapse.cpp:43: imageFilter = filter; Could use SkRefCnt_SafeAssign. This will eliminate the need for the unref() above, but it will ref the filter (it won't eat the one created by SkColorFilterImageFilter::create(). So the loop contents could be: SkAutoTUnref<SkImageFilter> filter(SkColorFilterImageFilter::Create(colorFilters[i], imageFilter, NULL, 0)); SkRefCnt_SafeAssign(imageFilter, filter.get()); https://codereview.chromium.org/776673002/diff/120001/bench/ImageFilterCollap... bench/ImageFilterCollapse.cpp:58: SkImageFilter* imageFilter; Fields in Skia should be prefixed with 'f', so this should be fImageFilter. https://codereview.chromium.org/776673002/diff/120001/bench/ImageFilterCollap... bench/ImageFilterCollapse.cpp:59: SkBitmap bm; Same here. fBM, or fbm, or (my preference) fBitmap. https://codereview.chromium.org/776673002/diff/120001/bench/ImageFilterCollap... bench/ImageFilterCollapse.cpp:76: paint.setShader(s)->unref(); Could use SkAutoTUnref here to avoid the need for explicit unref (your call).
https://codereview.chromium.org/776673002/diff/120001/gm/tablecolorfilter.cpp File gm/tablecolorfilter.cpp (right): https://codereview.chromium.org/776673002/diff/120001/gm/tablecolorfilter.cpp... gm/tablecolorfilter.cpp:150: x += xOffset; The complex x/y positioning logic still needs to be explained in comments.
On 2015/01/21 16:50:32, junov wrote: > https://codereview.chromium.org/776673002/diff/120001/gm/tablecolorfilter.cpp > File gm/tablecolorfilter.cpp (right): > > https://codereview.chromium.org/776673002/diff/120001/gm/tablecolorfilter.cpp... > gm/tablecolorfilter.cpp:150: x += xOffset; > The complex x/y positioning logic still needs to be explained in comments. Is the latest patch better?
fine by me. Will let senorblanco give final approval since he knows this area of the code better.
LGTM https://codereview.chromium.org/776673002/diff/160001/bench/ImageFilterCollap... File bench/ImageFilterCollapse.cpp (right): https://codereview.chromium.org/776673002/diff/160001/bench/ImageFilterCollap... bench/ImageFilterCollapse.cpp:36: SkColorFilterImageFilter::Create(colorFilters[i], fImageFilter, NULL, 0) Nit: weird spacing here.
The CQ bit was checked by cwallez@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/776673002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile (http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)
The MSVC warning should be fixed now.
The CQ bit was checked by cwallez@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/776673002/180001
The CQ bit was checked by cwallez@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/776673002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile (http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)
The CQ bit was unchecked by cwallez@google.com
The CQ bit was checked by cwallez@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/776673002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://skia.googlesource.com/skia/+/c12b74dc413ef024b13e0ed478491c4b1bafe6b1
Message was sent while issue was closed.
Did this CL change results in layouttests? webkit_tests webkit_tests failures: css3/filters/effect-combined.html svg/W3C-SVG-1.1/filters-comptran-01-b.svg svg/custom/feComponentTransfer-Discrete.svg svg/custom/feComponentTransfer-Gamma.svg svg/custom/feComponentTransfer-Linear.svg svg/custom/feComponentTransfer-Table.svg svg/dynamic-updates/SVGFEComponentTransferElement-svgdom-tableValues-prop.html svg/dynamic-updates/SVGFEComponentTransferElement-dom-tableValues-attr.html Here is the failed DEPS roll: http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...
Message was sent while issue was closed.
Message was sent while issue was closed.
The edge-only diffs are somewhat odd - are these safe to rebaseline? https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
Message was sent while issue was closed.
On 2015/01/26 21:28:20, f(malita) wrote: > The edge-only diffs are somewhat odd - are these safe to rebaseline? > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... I'm ok with rebaselining these. The visual differences seem imperceptible to me.
Message was sent while issue was closed.
On 2015/01/26 21:35:41, Stephen White wrote: > On 2015/01/26 21:28:20, f(malita) wrote: > > The edge-only diffs are somewhat odd - are these safe to rebaseline? > > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > I'm ok with rebaselining these. The visual differences seem imperceptible to me. Did we expect *any* changes? Did the CL cause any changes in our GMs?
Message was sent while issue was closed.
Stephen/Corentin: if it's not clear whether these diffs are expected, shall I revert so you can investigate/re-land with suppressions? (If they are expected please add suppressions to unblock the roll)
Message was sent while issue was closed.
On 2015/01/26 21:40:10, reed1 wrote: > On 2015/01/26 21:35:41, Stephen White wrote: > > On 2015/01/26 21:28:20, f(malita) wrote: > > > The edge-only diffs are somewhat odd - are these safe to rebaseline? > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > > > I'm ok with rebaselining these. The visual differences seem imperceptible to > me. > > Did we expect *any* changes? Did the CL cause any changes in our GMs? Yes, some of the GMs have changed (see the list of ignored-tests.txt uprev; removed due to the switch to skiagold). IIRC, it's due to the premult/unpremult steps: when non-chained, each filter does unpremult/LUT/premult. With chaining, we get unpremult/LUT1/LUT2/premult. So the result can be slightly different.
Message was sent while issue was closed.
On 2015/01/26 21:47:20, f(malita) wrote: > Stephen/Corentin: if it's not clear whether these diffs are expected, shall I > revert so you can investigate/re-land with suppressions? > > (If they are expected please add suppressions to unblock the roll) Sorry for the mess; I should've had Corentin run the layout tests. Florin will add the suppressions to skia/skia_test_expectations.txt, and I'll rebaseline in Blink once it's rolled in.
Message was sent while issue was closed.
On 2015/01/26 21:56:01, Stephen White wrote: > On 2015/01/26 21:47:20, f(malita) wrote: > > Stephen/Corentin: if it's not clear whether these diffs are expected, shall I > > revert so you can investigate/re-land with suppressions? > > > > (If they are expected please add suppressions to unblock the roll) > > Sorry for the mess; I should've had Corentin run the layout tests. > > Florin will add the suppressions to skia/skia_test_expectations.txt, and I'll > rebaseline in Blink once it's rolled in. For the future, lets following the following work-flow: If you CL blocks the DEPS roll due to layouttest changes, either: 1. revert and add suppressions to blink (and then wait for blink to roll) 2. amend the DEPS roll (manually) with suppression in chrome file (and then move them into blink) Either of these should be done by the author, as the sheriff's responsibility is only to identify the breakage.
Message was sent while issue was closed.
On 2015/01/26 21:47:49, Stephen White wrote: > On 2015/01/26 21:40:10, reed1 wrote: > > On 2015/01/26 21:35:41, Stephen White wrote: > > > On 2015/01/26 21:28:20, f(malita) wrote: > > > > The edge-only diffs are somewhat odd - are these safe to rebaseline? > > > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > > > > > I'm ok with rebaselining these. The visual differences seem imperceptible to > > me. > > > > Did we expect *any* changes? Did the CL cause any changes in our GMs? > > Yes, some of the GMs have changed (see the list of ignored-tests.txt uprev; > removed due to the switch to skiagold). > > IIRC, it's due to the premult/unpremult steps: when non-chained, each filter > does unpremult/LUT/premult. With chaining, > we get unpremult/LUT1/LUT2/premult. So the result can be slightly different. Ah, good explanation, thanks.
Message was sent while issue was closed.
On 2015/01/26 22:14:21, reed1 wrote: > On 2015/01/26 21:56:01, Stephen White wrote: > > On 2015/01/26 21:47:20, f(malita) wrote: > > > Stephen/Corentin: if it's not clear whether these diffs are expected, shall > I > > > revert so you can investigate/re-land with suppressions? > > > > > > (If they are expected please add suppressions to unblock the roll) > > > > Sorry for the mess; I should've had Corentin run the layout tests. > > > > Florin will add the suppressions to skia/skia_test_expectations.txt, and I'll > > rebaseline in Blink once it's rolled in. > > For the future, lets following the following work-flow: > > If you CL blocks the DEPS roll due to layouttest changes, either: > 1. revert and add suppressions to blink (and then wait for blink to roll) > 2. amend the DEPS roll (manually) with suppression in chrome file (and then move > them into blink) > > Either of these should be done by the author, as the sheriff's responsibility is > only to identify the breakage. It would be great to have the Skia Chrome canaries run layout tests too. That should catch the case where the submitter didn't know it would break them.
Message was sent while issue was closed.
On 2015/01/26 22:23:32, Stephen White wrote: > On 2015/01/26 22:14:21, reed1 wrote: > > On 2015/01/26 21:56:01, Stephen White wrote: > > > On 2015/01/26 21:47:20, f(malita) wrote: > > > > Stephen/Corentin: if it's not clear whether these diffs are expected, > shall > > I > > > > revert so you can investigate/re-land with suppressions? > > > > > > > > (If they are expected please add suppressions to unblock the roll) > > > > > > Sorry for the mess; I should've had Corentin run the layout tests. > > > > > > Florin will add the suppressions to skia/skia_test_expectations.txt, and > I'll > > > rebaseline in Blink once it's rolled in. > > > > For the future, lets following the following work-flow: > > > > If you CL blocks the DEPS roll due to layouttest changes, either: > > 1. revert and add suppressions to blink (and then wait for blink to roll) > > 2. amend the DEPS roll (manually) with suppression in chrome file (and then > move > > them into blink) > > > > Either of these should be done by the author, as the sheriff's responsibility > is > > only to identify the breakage. > > It would be great to have the Skia Chrome canaries run layout tests too. That > should > catch the case where the submitter didn't know it would break them. Yes, we have been trying to add that, but it has proved difficult. Eric has more details. I have no problem that we didn't notice the break until the DEPS roller actually tried. That is ok. *After* we discover this, it is the responsibility of the author to handle the suppressions/reverts/etc.
Message was sent while issue was closed.
On 2015/01/26 22:25:30, reed1 wrote: > On 2015/01/26 22:23:32, Stephen White wrote: > > On 2015/01/26 22:14:21, reed1 wrote: > > > On 2015/01/26 21:56:01, Stephen White wrote: > > > > On 2015/01/26 21:47:20, f(malita) wrote: > > > > > Stephen/Corentin: if it's not clear whether these diffs are expected, > > shall > > > I > > > > > revert so you can investigate/re-land with suppressions? > > > > > > > > > > (If they are expected please add suppressions to unblock the roll) > > > > > > > > Sorry for the mess; I should've had Corentin run the layout tests. > > > > > > > > Florin will add the suppressions to skia/skia_test_expectations.txt, and > > I'll > > > > rebaseline in Blink once it's rolled in. > > > > > > For the future, lets following the following work-flow: > > > > > > If you CL blocks the DEPS roll due to layouttest changes, either: > > > 1. revert and add suppressions to blink (and then wait for blink to roll) > > > 2. amend the DEPS roll (manually) with suppression in chrome file (and then > > move > > > them into blink) > > > > > > Either of these should be done by the author, as the sheriff's > responsibility > > is > > > only to identify the breakage. > > > > It would be great to have the Skia Chrome canaries run layout tests too. That > > should > > catch the case where the submitter didn't know it would break them. > > Yes, we have been trying to add that, but it has proved difficult. Eric has more > details. > > I have no problem that we didn't notice the break until the DEPS roller actually > tried. That is ok. *After* we discover this, it is the responsibility of the > author to handle the suppressions/reverts/etc. SGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
