Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
931483002:
Suggested version with 'undo'. (Closed)
DescriptionSuggested version with 'undo'.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/d603b22903bf7c023226bf52bd7c1f49a9bee1bf
Patch Set 1 #Patch Set 2 : other #Patch Set 3 : more #Patch Set 4 : both #Patch Set 5 : commna #Patch Set 6 : relock #Patch Set 7 : simpler, fix #Patch Set 8 : Upright #Patch Set 9 : cast to scalar #
Messages
Total messages: 29 (7 generated)
mtklein@chromium.org changed reviewers: + reed@google.com
Just want to make sure you see this. It's easy to miss links to other CLs in code reviews.
reed@chromium.org changed reviewers: + reed@chromium.org
lgtm Will the matrix parameters (values) appear in the "info" field in Gold? If not, we may want to find some mechanism to distinguish different "matrix" runs (e.g. flips and rotates).
On 2015/02/15 17:27:02, reed2 wrote:
> lgtm
>
> Will the matrix parameters (values) appear in the "info" field in Gold? If
not,
> we may want to find some mechanism to distinguish different "matrix" runs
(e.g.
> flips and rotates).
Not as written. If you want that, I'd name the matrices and add new VIA lines
for each to create_via() in DM.cpp:
SkMatrix rot90;
rot90.setRotate(90);
VIA("rot90", ViaMatrix, rot90, wrapped);
VIA("unrot90", ViaUndo, rot90, wrapped);
SkMatrix flipX;
flipX.setScale(-1, 1);
VIA("flipx", ViaMatrix, flipX, wrapped);
VIA("unflipx", ViaUndo, flipX, wrapped);
etc.
Hmmm, lets land this for now, so we can play with it, and then figure out how we want to automate things for the bots and gold.
#7 isn't correct for undo-ing. The undo step needs to be 1-to-1, and only "undo" flips and rotates, not scaling. Example out/Debug/dm --src gm --config 8888 matrix-8888 matrix-undo-8888 --matrix -0.25 0 0 0.25 -w ../tmp --match arithmode The undo image should still be 0.25 scaled, just not flipped, but instead we get a very fuzzy full-size image.
On 2015/02/16 02:43:58, reed2 wrote: > #7 isn't correct for undo-ing. The undo step needs to be 1-to-1, and only "undo" > flips and rotates, not scaling. > > Example > > out/Debug/dm --src gm --config 8888 matrix-8888 matrix-undo-8888 --matrix -0.25 > 0 0 0.25 -w ../tmp --match arithmode > > The undo image should still be 0.25 scaled, just not flipped, but instead we get > a very fuzzy full-size image. I'm not sure I get why it's important to not undo scaling. Can't we just... not use that matrix?
On 2015/02/16 04:10:46, mtklein wrote: > On 2015/02/16 02:43:58, reed2 wrote: > > #7 isn't correct for undo-ing. The undo step needs to be 1-to-1, and only > "undo" > > flips and rotates, not scaling. > > > > Example > > > > out/Debug/dm --src gm --config 8888 matrix-8888 matrix-undo-8888 --matrix > -0.25 > > 0 0 0.25 -w ../tmp --match arithmode > > > > The undo image should still be 0.25 scaled, just not flipped, but instead we > get > > a very fuzzy full-size image. > > I'm not sure I get why it's important to not undo scaling. Can't we just... not > use that matrix? Still want to land and iterate?
On 2015/02/16 04:10:46, mtklein wrote: > On 2015/02/16 02:43:58, reed2 wrote: > > #7 isn't correct for undo-ing. The undo step needs to be 1-to-1, and only > "undo" > > flips and rotates, not scaling. > > > > Example > > > > out/Debug/dm --src gm --config 8888 matrix-8888 matrix-undo-8888 --matrix > -0.25 > > 0 0 0.25 -w ../tmp --match arithmode > > > > The undo image should still be 0.25 scaled, just not flipped, but instead we > get > > a very fuzzy full-size image. > > I'm not sure I get why it's important to not undo scaling. Can't we just... not > use that matrix? My intent with an undo step was to not change any pixel values at all, but to rearrange them to help with comparisons. Perhaps another word could be "upright" instead of "undo". As written, we are inverting any matrix, which can make us throw away some of the pixel values (or if we filtered, we'd be creating new values). This might also be interesting, but introduces another source of error/noise when comparing.
On 2015/02/17 14:29:42, reed2 wrote: > On 2015/02/16 04:10:46, mtklein wrote: > > On 2015/02/16 02:43:58, reed2 wrote: > > > #7 isn't correct for undo-ing. The undo step needs to be 1-to-1, and only > > "undo" > > > flips and rotates, not scaling. > > > > > > Example > > > > > > out/Debug/dm --src gm --config 8888 matrix-8888 matrix-undo-8888 --matrix > > -0.25 > > > 0 0 0.25 -w ../tmp --match arithmode > > > > > > The undo image should still be 0.25 scaled, just not flipped, but instead we > > get > > > a very fuzzy full-size image. > > > > I'm not sure I get why it's important to not undo scaling. Can't we just... > not > > use that matrix? > > My intent with an undo step was to not change any pixel values at all, but to > rearrange them to help with comparisons. Perhaps another word could be "upright" > instead of "undo". > > As written, we are inverting any matrix, which can make us throw away some of > the pixel values (or if we filtered, we'd be creating new values). This might > also be interesting, but introduces another source of error/noise when > comparing. Did you make your change (#7) because you wanted to invert the matrix completely, or because you were looking to simplify the src-code?
> Did you make your change (#7) because you wanted to invert the matrix > completely, or because you were looking to simplify the src-code? Without that change, we were over-allocating the bitmap for a matrix like --matrix 0 2 2 0. Only its corner was drawn, and the rest was leftover uninitialized junk. Isn't the undo step always just an invert, and only certain matrices are invertible in a ways that will preserve pixels in a 1-to-1 way? Isn't your use case a subset of this undo?
looking again, I see that your version also simplified the logic in unxform, and you invert matrixes that are not just recttorects. I think that is part of the same difference between my intent and this code. If you think it is valuable to perform arbitrary inverts, perhaps that is a separate flag from the flag that just performs "uprightness" (my feature request)... I think our two CLs have drifted in features, perhaps deliberately, perhaps accidentally.
On 2015/02/17 14:37:27, reed2 wrote: > looking again, I see that your version also simplified the logic in unxform, and > you invert matrixes that are not just recttorects. I think that is part of the > same difference between my intent and this code. > > If you think it is valuable to perform arbitrary inverts, perhaps that is a > separate flag from the flag that just performs "uprightness" (my feature > request)... > > I think our two CLs have drifted in features, perhaps deliberately, perhaps > accidentally. Can you give me an example of a matrix where uprighting it isn't also done by inverting the matrix? Why not just support the general case?
On 2015/02/17 14:40:45, mtklein wrote: > On 2015/02/17 14:37:27, reed2 wrote: > > looking again, I see that your version also simplified the logic in unxform, > and > > you invert matrixes that are not just recttorects. I think that is part of the > > same difference between my intent and this code. > > > > If you think it is valuable to perform arbitrary inverts, perhaps that is a > > separate flag from the flag that just performs "uprightness" (my feature > > request)... > > > > I think our two CLs have drifted in features, perhaps deliberately, perhaps > > accidentally. > > Can you give me an example of a matrix where uprighting it isn't also done by > inverting the matrix? Why not just support the general case? --matrix -0.25 0 0 1 To upright this is to just flip in X. To invert it is to scale by -4 in X.
> --matrix -0.25 0 0 1 > > To upright this is to just flip in X. To invert it is to scale by -4 in X. I think I understand now. Working on a new patch now. What I'm still not clear on is why you'd want to use a matrix like this. Uprighting it doesn't have any particular value unless we can compare against the 8888 baseline, right? Won't all the matrices we want to upright be at 1x scale, so we can compare them?
New patchsets have been uploaded after l-g-t-m from reed@chromium.org
On 2015/02/17 15:02:22, mtklein wrote: > > --matrix -0.25 0 0 1 > > > > To upright this is to just flip in X. To invert it is to scale by -4 in X. > > I think I understand now. Working on a new patch now. What I'm still not clear > on is why you'd want to use a matrix like this. Uprighting it doesn't have any > particular value unless we can compare against the 8888 baseline, right? Won't > all the matrices we want to upright be at 1x scale, so we can compare them? Is 8 what you'd expect? The output images for --matrix -0.25 0 0 1 are just flipped back across x.
On 2015/02/17 15:09:19, mtklein wrote: > On 2015/02/17 15:02:22, mtklein wrote: > > > --matrix -0.25 0 0 1 > > > > > > To upright this is to just flip in X. To invert it is to scale by -4 in X. > > > > I think I understand now. Working on a new patch now. What I'm still not > clear > > on is why you'd want to use a matrix like this. Uprighting it doesn't have > any > > particular value unless we can compare against the 8888 baseline, right? > Won't > > all the matrices we want to upright be at 1x scale, so we can compare them? > > Is 8 what you'd expect? The output images for --matrix -0.25 0 0 1 are just > flipped back across x. I think so (given the SkScalarSignAsScalar) but I'll check now. Do you think "undo" is the clearest name, given that we may later want a complete "invert"? Is "upright" any better? (just bikeshedding while my test runs).
On 2015/02/17 18:41:19, reed2 wrote: > On 2015/02/17 15:09:19, mtklein wrote: > > On 2015/02/17 15:02:22, mtklein wrote: > > > > --matrix -0.25 0 0 1 > > > > > > > > To upright this is to just flip in X. To invert it is to scale by -4 in X. > > > > > > I think I understand now. Working on a new patch now. What I'm still not > > clear > > > on is why you'd want to use a matrix like this. Uprighting it doesn't have > > any > > > particular value unless we can compare against the 8888 baseline, right? > > Won't > > > all the matrices we want to upright be at 1x scale, so we can compare them? > > > > Is 8 what you'd expect? The output images for --matrix -0.25 0 0 1 are just > > flipped back across x. > > I think so (given the SkScalarSignAsScalar) but I'll check now. Do you think > "undo" is the clearest name, given that we may later want a complete "invert"? > Is "upright" any better? (just bikeshedding while my test runs). Duh -- I didn't look at DM.cpp and the name change. My test ran perfect. lgtm
> I think so (given the SkScalarSignAsScalar) but I'll check now. Do you think > "undo" is the clearest name, given that we may later want a complete "invert"? > Is "upright" any better? (just bikeshedding while my test runs). I like "upright" for this operation for its complete lack of cognitive baggage. It'd certainly be nutso to call this "invert", and "undo" sounds to me like it'd also do a full inverse. If we end up with two options here, they'd better be "upright" and "invert". "undo" doesn't contrast clearly with "invert" or "upright".
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/931483002/130001
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 (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)
New patchsets have been uploaded after l-g-t-m from reed@chromium.org
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/931483002/150001
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as https://skia.googlesource.com/skia/+/d603b22903bf7c023226bf52bd7c1f49a9bee1bf |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
