Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1529833003:
Repeating SkBitmapProcState rounding bias (Closed)
DescriptionRepeating SkBitmapProcState rounding bias
Observe the bias in repeat matrix procs also.
Introduce a utility class to handle device space -> bitmap space
mapping.
BUG=skia:4680, skia:4649
R=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1529833003
Committed: https://skia.googlesource.com/skia/+/5ae7fdcc3d7712da3193c39a751e88b092aa82db
Patch Set 1 #Patch Set 2 : bump SkTBlitterAllocator storage #Patch Set 3 : we only need the fractional bits for bias #Patch Set 4 : comments #
Total comments: 4
Patch Set 5 : used int8_t for bias storage #Patch Set 6 : v2 (SkBitmapProcStateAutoMapper) #
Messages
Total messages: 36 (15 generated)
Description was changed from ========== Repeating SkBitmapProcState rounding bias 1) the bias is constant for a given state/inv matrix, so we can store it (as SkFixed) in the state itself. 2) also observe the bias in the repeat matrix procs BUG=skia:4680,skia:4649 R=reed@google.com ========== to ========== Repeating SkBitmapProcState rounding bias 1) the bias is constant for a given state/inv matrix, so we can store it (as SkFixed) in the state itself. 2) also observe the bias in the repeat matrix procs BUG=skia:4680,skia:4649 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
~38 diffs locally, most of which are a rehash of the prev change due to the SkFixed -> SkFractionalInt bias conversion.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1529833003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1529833003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1529833003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1529833003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/12/15 22:45:32, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, > http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) I had to bump the SkTBlitterAllocator size to accommodate the extra fields. Not sure whether this is something we're trying to keep small.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1529833003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1529833003/40001
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1529833003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1529833003/60001
https://codereview.chromium.org/1529833003/diff/60001/src/core/SkBitmapProcSt... File src/core/SkBitmapProcState.h (right): https://codereview.chromium.org/1529833003/diff/60001/src/core/SkBitmapProcSt... src/core/SkBitmapProcState.h:81: // geometry. These are treated as SkFixed, but we only store the fractional part. We just need 0 or 1 for each, right? why using int16_t? https://codereview.chromium.org/1529833003/diff/60001/src/core/SkBitmapProcSt... File src/core/SkBitmapProcState_matrix.h (right): https://codereview.chromium.org/1529833003/diff/60001/src/core/SkBitmapProcSt... src/core/SkBitmapProcState_matrix.h:105: SkFixed fx = SkScalarToFixed(srcPt.fX) - (oneX >> 1) + s.fInvBiasX; Is this needed for filtered versions?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1529833003/diff/60001/src/core/SkBitmapProcSt... File src/core/SkBitmapProcState.h (right): https://codereview.chromium.org/1529833003/diff/60001/src/core/SkBitmapProcSt... src/core/SkBitmapProcState.h:81: // geometry. These are treated as SkFixed, but we only store the fractional part. On 2015/12/16 16:25:03, reed1 wrote: > We just need 0 or 1 for each, right? why using int16_t? The current impl also expects an explicit sign. We could a) make the sign implicit and squeeze these down to 1 bit each b) squeeze them down to int8_ts - I think these should fit in the struct padding if placed at the end Do you think b) will work? https://codereview.chromium.org/1529833003/diff/60001/src/core/SkBitmapProcSt... File src/core/SkBitmapProcState_matrix.h (right): https://codereview.chromium.org/1529833003/diff/60001/src/core/SkBitmapProcSt... src/core/SkBitmapProcState_matrix.h:105: SkFixed fx = SkScalarToFixed(srcPt.fX) - (oneX >> 1) + s.fInvBiasX; On 2015/12/16 16:25:03, reed1 wrote: > Is this needed for filtered versions? Not sure. The current/main issue with filtered nine-patches is that we're drawing the corners in sprite mode/unfiltered => significant discrepancies at corner seams. Once we get that out of the way, we can revisit whether we need to bias filtered samplers also.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1529833003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1529833003/80001
On 2015/12/16 16:45:55, f(malita) wrote: > b) squeeze them down to int8_ts - I think these should fit in the struct padding > if placed at the end > > Do you think b) will work? (done in PS#5)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/12/16 16:51:50, f(malita) wrote: > On 2015/12/16 16:45:55, f(malita) wrote: > > b) squeeze them down to int8_ts - I think these should fit in the struct > padding > > if placed at the end > > > > Do you think b) will work? > > (done in PS#5) Or how about something like PS#6, which uses a mapper utility class to encapsulate the bias logic (instead of exposing it on SkBitmapProcState)?
I love it -- we should have done this even w/o the new tricky bias. lgtm
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1529833003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1529833003/100001
The CQ bit was unchecked by fmalita@chromium.org
On 2015/12/18 14:37:54, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1529833003/100001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1529833003/100001 On second thought, I'll hold this off just a bit longer. Don't want to have to triage it out of the current 1200 gold diffs :)
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1529833003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1529833003/100001
Description was changed from ========== Repeating SkBitmapProcState rounding bias 1) the bias is constant for a given state/inv matrix, so we can store it (as SkFixed) in the state itself. 2) also observe the bias in the repeat matrix procs BUG=skia:4680,skia:4649 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Repeating SkBitmapProcState rounding bias Observe the bias in repeat matrix procs also. Introduce a utility class to handle device space -> bitmap space mapping. BUG=skia:4680,skia:4649 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Message was sent while issue was closed.
Description was changed from ========== Repeating SkBitmapProcState rounding bias Observe the bias in repeat matrix procs also. Introduce a utility class to handle device space -> bitmap space mapping. BUG=skia:4680,skia:4649 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Repeating SkBitmapProcState rounding bias Observe the bias in repeat matrix procs also. Introduce a utility class to handle device space -> bitmap space mapping. BUG=skia:4680,skia:4649 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/5ae7fdcc3d7712da3193c39a751e88b092aa82db ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/5ae7fdcc3d7712da3193c39a751e88b092aa82db
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1531423003/ by fmalita@chromium.org. The reason for reverting is: We need a SkFractionalInt auto mapper flavor, otherwise we're losing precision for some procs => seaming artifacs (https://gold.skia.org/diff?test=giantbitmap_mirror_point_rotate&left=0dd7a412...). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
