Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
977623002:
Glyph positions maintain 32 bit integer part. (Closed)
|
Created:
5 years, 9 months ago by bungeman-skia Modified:
5 years, 9 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionGlyph positions maintain 32 bit integer part.
A glyph position when mapped from canvas space to device space may land
outside the bounds of the current 16 bit integer part of device space.
Device space is already limited to 32 bits for the integer part,
but for a short space in drawText and drawPosText it is currently
limited to 16 bits (SkFixed). Raise this limit by moving to 48.16.
This matches the current similar fix for measureText.
BUG=chromium:375322
Committed: https://skia.googlesource.com/skia/+/79738cc7bf12d212bef4ff80591d1bf6f383663d
Patch Set 1 #Patch Set 2 : A different approach. Clamp to device space. #Patch Set 3 : SkScalarPin already exists. #Patch Set 4 : Branchless (on Clang). #
Total comments: 6
Patch Set 5 : Use Sk48Dot6, do no fixed math. #
Total comments: 7
Patch Set 6 : Update GPU side as well. #Patch Set 7 : Address comments. #Patch Set 8 : Add explicit SkIntToScalar for documentation and uniformity. #
Total comments: 5
Patch Set 9 : Warnings as errors on Windows. #
Messages
Total messages: 35 (5 generated)
bungeman@google.com changed reviewers: + jvanverth@google.com, mtklein@google.com, reed@google.com
Essentially we need to make sure coordinates in canvas space are SkScalars, and only convert to SkFixed when actually in device space. I'll need to run this by the Blink layout test bots to see what the fallout might be, but considering that as far as I know they don't actually use this feature, it shouldn't make a difference there. I had started working on this for the attached bug some time ago, but became quickly overwhelmed by the size change needed. As a result, I'm going to try fixing this a little at a time, starting from the top.
bungeman@google.com changed reviewers: - jvanverth@google.com
This change (at PS2) solves the issue seen on Android in the linked bug. This is really a first pass 'this works'. I'm not yet sure what's the fastest way to do this, and it seems like it should at least not be slow. Let me know if this seems like a better approach.
This change should now be just a few added instructions. https://codereview.chromium.org/977623002/diff/60001/include/core/SkScalar.h File include/core/SkScalar.h (right): https://codereview.chromium.org/977623002/diff/60001/include/core/SkScalar.h#... include/core/SkScalar.h:177: static inline SkScalar SkScalarPin(SkScalar x, SkScalar min, SkScalar max) { With clang 3.5 this code turns into branch-less maxss and minss, and the existing code turns into compare and branch. I couldn't get gcc 4.8.2 to emit the branch-less version, but this version turns into the same code the old code did there. Hopefully newer gcc will also optimize this.
https://codereview.chromium.org/977623002/diff/60001/include/core/SkScalar.h File include/core/SkScalar.h (right): https://codereview.chromium.org/977623002/diff/60001/include/core/SkScalar.h#... include/core/SkScalar.h:177: static inline SkScalar SkScalarPin(SkScalar x, SkScalar min, SkScalar max) { On 2015/03/04 21:38:05, bungeman1 wrote: > With clang 3.5 this code turns into branch-less maxss and minss, and the > existing code turns into compare and branch. I couldn't get gcc 4.8.2 to emit > the branch-less version, but this version turns into the same code the old code > did there. Hopefully newer gcc will also optimize this. Ah, that explains it. Thanks! This bit LGTM. https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... File src/core/SkTextMapStateProc.h (right): https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, SkIntToScalar(-16384), SkIntToScalar(16384)); What fixed format are we clamping to? Signed 1.14? 2.14? Not SkFixed, right? Doesn't the max need to be 16383 if the min is -16384?
https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... File src/core/SkTextMapStateProc.h (right): https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, SkIntToScalar(-16384), SkIntToScalar(16384)); On 2015/03/04 21:45:30, mtklein wrote: > What fixed format are we clamping to? Signed 1.14? 2.14? Not SkFixed, right? > > Doesn't the max need to be 16383 if the min is -16384? This is SkFixed we're clamping to. SkFixed is symmetrical with the 'most negative' value being the NaN. Anyhow, there's a sign bit and then 15 bits for the whole part and then 16 for the fractional part. So the range of SkFixed is actually +-2^15, but this clamps to +-2^14. Te reason for the slop is just to avoid glyphs being nudged over by subpixel and the like, as well as to ensure the entire bounds of the glyph stays inside device space (but outside any view-able space).
On 2015/03/04 21:57:36, bungeman1 wrote: > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > File src/core/SkTextMapStateProc.h (right): > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, SkIntToScalar(-16384), > SkIntToScalar(16384)); > On 2015/03/04 21:45:30, mtklein wrote: > > What fixed format are we clamping to? Signed 1.14? 2.14? Not SkFixed, > right? > > > > Doesn't the max need to be 16383 if the min is -16384? > > This is SkFixed we're clamping to. SkFixed is symmetrical with the 'most > negative' value being the NaN. Anyhow, there's a sign bit and then 15 bits for > the whole part and then 16 for the fractional part. So the range of SkFixed is > actually +-2^15, but this clamps to +-2^14. Te reason for the slop is just to > avoid glyphs being nudged over by subpixel and the like, as well as to ensure > the entire bounds of the glyph stays inside device space (but outside any > view-able space). So if SkFixed is signed 15.16, clamping to [-1<<14, +1<<14] is clamping to the logical range [-1/4, +1/4]. That's what we need here? Am I confused? Don't you want to scoot all this over to the left, or just use the constants SK_FixedMin and SK_FixedMax?
On 2015/03/04 22:03:47, mtklein wrote: > On 2015/03/04 21:57:36, bungeman1 wrote: > > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > > File src/core/SkTextMapStateProc.h (right): > > > > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > > src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, SkIntToScalar(-16384), > > SkIntToScalar(16384)); > > On 2015/03/04 21:45:30, mtklein wrote: > > > What fixed format are we clamping to? Signed 1.14? 2.14? Not SkFixed, > > right? > > > > > > Doesn't the max need to be 16383 if the min is -16384? > > > > This is SkFixed we're clamping to. SkFixed is symmetrical with the 'most > > negative' value being the NaN. Anyhow, there's a sign bit and then 15 bits for > > the whole part and then 16 for the fractional part. So the range of SkFixed is > > actually +-2^15, but this clamps to +-2^14. Te reason for the slop is just to > > avoid glyphs being nudged over by subpixel and the like, as well as to ensure > > the entire bounds of the glyph stays inside device space (but outside any > > view-able space). > > So if SkFixed is signed 15.16, clamping to [-1<<14, +1<<14] is clamping to the > logical range [-1/4, +1/4]. That's what we need here? Am I confused? Don't > you want to scoot all this over to the left, or just use the constants > SK_FixedMin and SK_FixedMax? Ah, but I'm not clamping to 1<<14 (as a bit pattern), I'm clamping to 2^14 (as a number). I suppose this could be written return SkScalarPin(a, SkFixedToScalar(SK_FixedMin/2), SkFixedToScaler(SK_FixedMax/2)); and I think at one point I had it written this way. I was trying to simplify to get the branch-less version. I can re-write it like that and see if I still get the branchless version though.
https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... File src/core/SkTextMapStateProc.h (right): https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, SkIntToScalar(-16384), SkIntToScalar(16384)); On 2015/03/04 21:57:35, bungeman1 wrote: > On 2015/03/04 21:45:30, mtklein wrote: > > What fixed format are we clamping to? Signed 1.14? 2.14? Not SkFixed, > right? > > > > Doesn't the max need to be 16383 if the min is -16384? > > This is SkFixed we're clamping to. SkFixed is symmetrical with the 'most > negative' value being the NaN. Anyhow, there's a sign bit and then 15 bits for > the whole part and then 16 for the fractional part. So the range of SkFixed is > actually +-2^15, but this clamps to +-2^14. Te reason for the slop is just to > avoid glyphs being nudged over by subpixel and the like, as well as to ensure > the entire bounds of the glyph stays inside device space (but outside any > view-able space). Oh, duh, we're clamping SkScalars, not SkFixed. OK, unconfused now I think. But why do we want to clamp to [min/2, max/2] instead of [min,max] ?
On 2015/03/04 22:11:01, mtklein wrote: > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > File src/core/SkTextMapStateProc.h (right): > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, SkIntToScalar(-16384), > SkIntToScalar(16384)); > On 2015/03/04 21:57:35, bungeman1 wrote: > > On 2015/03/04 21:45:30, mtklein wrote: > > > What fixed format are we clamping to? Signed 1.14? 2.14? Not SkFixed, > > right? > > > > > > Doesn't the max need to be 16383 if the min is -16384? > > > > This is SkFixed we're clamping to. SkFixed is symmetrical with the 'most > > negative' value being the NaN. Anyhow, there's a sign bit and then 15 bits for > > the whole part and then 16 for the fractional part. So the range of SkFixed is > > actually +-2^15, but this clamps to +-2^14. Te reason for the slop is just to > > avoid glyphs being nudged over by subpixel and the like, as well as to ensure > > the entire bounds of the glyph stays inside device space (but outside any > > view-able space). > > Oh, duh, we're clamping SkScalars, not SkFixed. OK, unconfused now I think. > > But why do we want to clamp to [min/2, max/2] instead of [min,max] ? We need some wiggle room, though we probably don't need this much in practice. This value will be turned into a SkFixed and then * possibly have a subpixel offset added. * a glyph bounds will be created at this point and clipped against Now that I think about this, the second one may not be relevant, since the bounds checks are done with signed integers (non-fixed). So maybe in practice we only need a wiggle room of 1? In any event, having lots of wiggle room seems nice, and at the moment I think we can get away with it. The one place where this could go wrong is with printing. At least XPS currently tries to place things (and not duplicate this sort of code) by calling back through here with it's own draw1glyph override. So in theory, someone could see the effect of this if they created a really big page for the xps backend. Of course, they'd probably run into some other issue first (and I think these would still get clipped out anyway).
On 2015/03/04 22:23:28, bungeman1 wrote: > On 2015/03/04 22:11:01, mtklein wrote: > > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > > File src/core/SkTextMapStateProc.h (right): > > > > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > > src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, SkIntToScalar(-16384), > > SkIntToScalar(16384)); > > On 2015/03/04 21:57:35, bungeman1 wrote: > > > On 2015/03/04 21:45:30, mtklein wrote: > > > > What fixed format are we clamping to? Signed 1.14? 2.14? Not SkFixed, > > > right? > > > > > > > > Doesn't the max need to be 16383 if the min is -16384? > > > > > > This is SkFixed we're clamping to. SkFixed is symmetrical with the 'most > > > negative' value being the NaN. Anyhow, there's a sign bit and then 15 bits > for > > > the whole part and then 16 for the fractional part. So the range of SkFixed > is > > > actually +-2^15, but this clamps to +-2^14. Te reason for the slop is just > to > > > avoid glyphs being nudged over by subpixel and the like, as well as to > ensure > > > the entire bounds of the glyph stays inside device space (but outside any > > > view-able space). > > > > Oh, duh, we're clamping SkScalars, not SkFixed. OK, unconfused now I think. > > > > But why do we want to clamp to [min/2, max/2] instead of [min,max] ? > > We need some wiggle room, though we probably don't need this much in practice. > This value will be turned into a SkFixed and then > > * possibly have a subpixel offset added. > * a glyph bounds will be created at this point and clipped against > > Now that I think about this, the second one may not be relevant, since the > bounds checks are done with signed integers (non-fixed). So maybe in practice we > only need a wiggle room of 1? In any event, having lots of wiggle room seems > nice, and at the moment I think we can get away with it. > > The one place where this could go wrong is with printing. At least XPS currently > tries to place things (and not duplicate this sort of code) by calling back > through here with it's own draw1glyph override. So in theory, someone could see > the effect of this if they created a really big page for the xps backend. Of > course, they'd probably run into some other issue first (and I think these would > still get clipped out anyway). That sounds reasonable. Let's get some of this explanation into the code. Maybe, rename to clamp_scalar_to_half_fixed(), and then comment that we're clamping to [SkFixed_Min/2, SkFixedMax/2] to give us wiggle-room later for at least subpixel offsets. It does seem like a shame to cut off half our space. Seems just as easy to use [SK_FixedMin+SK_Fixed1, SK_FixedMax-SK_Fixed1] ?
On 2015/03/04 22:30:15, mtklein wrote: > On 2015/03/04 22:23:28, bungeman1 wrote: > > On 2015/03/04 22:11:01, mtklein wrote: > > > > > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > > > File src/core/SkTextMapStateProc.h (right): > > > > > > > > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > > > src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, > SkIntToScalar(-16384), > > > SkIntToScalar(16384)); > > > On 2015/03/04 21:57:35, bungeman1 wrote: > > > > On 2015/03/04 21:45:30, mtklein wrote: > > > > > What fixed format are we clamping to? Signed 1.14? 2.14? Not SkFixed, > > > > right? > > > > > > > > > > Doesn't the max need to be 16383 if the min is -16384? > > > > > > > > This is SkFixed we're clamping to. SkFixed is symmetrical with the 'most > > > > negative' value being the NaN. Anyhow, there's a sign bit and then 15 bits > > for > > > > the whole part and then 16 for the fractional part. So the range of > SkFixed > > is > > > > actually +-2^15, but this clamps to +-2^14. Te reason for the slop is just > > to > > > > avoid glyphs being nudged over by subpixel and the like, as well as to > > ensure > > > > the entire bounds of the glyph stays inside device space (but outside any > > > > view-able space). > > > > > > Oh, duh, we're clamping SkScalars, not SkFixed. OK, unconfused now I think. > > > > > > But why do we want to clamp to [min/2, max/2] instead of [min,max] ? > > > > We need some wiggle room, though we probably don't need this much in practice. > > This value will be turned into a SkFixed and then > > > > * possibly have a subpixel offset added. > > * a glyph bounds will be created at this point and clipped against > > > > Now that I think about this, the second one may not be relevant, since the > > bounds checks are done with signed integers (non-fixed). So maybe in practice > we > > only need a wiggle room of 1? In any event, having lots of wiggle room seems > > nice, and at the moment I think we can get away with it. > > > > The one place where this could go wrong is with printing. At least XPS > currently > > tries to place things (and not duplicate this sort of code) by calling back > > through here with it's own draw1glyph override. So in theory, someone could > see > > the effect of this if they created a really big page for the xps backend. Of > > course, they'd probably run into some other issue first (and I think these > would > > still get clipped out anyway). > > That sounds reasonable. Let's get some of this explanation into the code. > Maybe, rename to clamp_scalar_to_half_fixed(), and then comment that we're > clamping to [SkFixed_Min/2, SkFixedMax/2] to give us wiggle-room later for at > least subpixel offsets. > > It does seem like a shame to cut off half our space. Seems just as easy to use > [SK_FixedMin+SK_Fixed1, SK_FixedMax-SK_Fixed1] ? Arg, why do you have to make me think about this more. I don't want this clamping in SkTextMapStateProc.h at all. drawPosText_asPaths needs it not to. I actually just want to do this clamping at the current point where it's being done, which is either in drawPosText or in the align proc. At this point maybe the best thing to do is combine this change (PS4) with the align proc change in PS1. Then the clamping and conversion to fixed can happen in one place. It seems much easier to do this conversion last, just after the 'last step' of adding half the sampling frequency.
On 2015/03/04 23:11:59, bungeman1 wrote: > On 2015/03/04 22:30:15, mtklein wrote: > > On 2015/03/04 22:23:28, bungeman1 wrote: > > > On 2015/03/04 22:11:01, mtklein wrote: > > > > > > > > > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > > > > File src/core/SkTextMapStateProc.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... > > > > src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, > > SkIntToScalar(-16384), > > > > SkIntToScalar(16384)); > > > > On 2015/03/04 21:57:35, bungeman1 wrote: > > > > > On 2015/03/04 21:45:30, mtklein wrote: > > > > > > What fixed format are we clamping to? Signed 1.14? 2.14? Not > SkFixed, > > > > > right? > > > > > > > > > > > > Doesn't the max need to be 16383 if the min is -16384? > > > > > > > > > > This is SkFixed we're clamping to. SkFixed is symmetrical with the 'most > > > > > negative' value being the NaN. Anyhow, there's a sign bit and then 15 > bits > > > for > > > > > the whole part and then 16 for the fractional part. So the range of > > SkFixed > > > is > > > > > actually +-2^15, but this clamps to +-2^14. Te reason for the slop is > just > > > to > > > > > avoid glyphs being nudged over by subpixel and the like, as well as to > > > ensure > > > > > the entire bounds of the glyph stays inside device space (but outside > any > > > > > view-able space). > > > > > > > > Oh, duh, we're clamping SkScalars, not SkFixed. OK, unconfused now I > think. > > > > > > > > But why do we want to clamp to [min/2, max/2] instead of [min,max] ? > > > > > > We need some wiggle room, though we probably don't need this much in > practice. > > > This value will be turned into a SkFixed and then > > > > > > * possibly have a subpixel offset added. > > > * a glyph bounds will be created at this point and clipped against > > > > > > Now that I think about this, the second one may not be relevant, since the > > > bounds checks are done with signed integers (non-fixed). So maybe in > practice > > we > > > only need a wiggle room of 1? In any event, having lots of wiggle room seems > > > nice, and at the moment I think we can get away with it. > > > > > > The one place where this could go wrong is with printing. At least XPS > > currently > > > tries to place things (and not duplicate this sort of code) by calling back > > > through here with it's own draw1glyph override. So in theory, someone could > > see > > > the effect of this if they created a really big page for the xps backend. Of > > > course, they'd probably run into some other issue first (and I think these > > would > > > still get clipped out anyway). > > > > That sounds reasonable. Let's get some of this explanation into the code. > > Maybe, rename to clamp_scalar_to_half_fixed(), and then comment that we're > > clamping to [SkFixed_Min/2, SkFixedMax/2] to give us wiggle-room later for at > > least subpixel offsets. > > > > It does seem like a shame to cut off half our space. Seems just as easy to > use > > [SK_FixedMin+SK_Fixed1, SK_FixedMax-SK_Fixed1] ? > > Arg, why do you have to make me think about this more. I don't want this > clamping in SkTextMapStateProc.h at all. drawPosText_asPaths needs it not to. I > actually just want to do this clamping at the current point where it's being > done, which is either in drawPosText or in the align proc. At this point maybe > the best thing to do is combine this change (PS4) with the align proc change in > PS1. Then the clamping and conversion to fixed can happen in one place. It seems > much easier to do this conversion last, just after the 'last step' of adding > half the sampling frequency. I understood some of those words. But seriously, that actually does sound best to clamp right before going to SkFixed.
https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... File src/core/SkTextMapStateProc.h (right): https://codereview.chromium.org/977623002/diff/60001/src/core/SkTextMapStateP... src/core/SkTextMapStateProc.h:59: return SkScalarPin(a, SkIntToScalar(-16384), SkIntToScalar(16384)); On 2015/03/04 22:11:00, mtklein wrote: > On 2015/03/04 21:57:35, bungeman1 wrote: > > On 2015/03/04 21:45:30, mtklein wrote: > > > What fixed format are we clamping to? Signed 1.14? 2.14? Not SkFixed, > > right? > > > > > > Doesn't the max need to be 16383 if the min is -16384? > > > > This is SkFixed we're clamping to. SkFixed is symmetrical with the 'most > > negative' value being the NaN. Anyhow, there's a sign bit and then 15 bits for > > the whole part and then 16 for the fractional part. So the range of SkFixed is > > actually +-2^15, but this clamps to +-2^14. Te reason for the slop is just to > > avoid glyphs being nudged over by subpixel and the like, as well as to ensure > > the entire bounds of the glyph stays inside device space (but outside any > > view-able space). > > Oh, duh, we're clamping SkScalars, not SkFixed. OK, unconfused now I think. > > But why do we want to clamp to [min/2, max/2] instead of [min,max] ? Lets modify its name to make it more obvious that its not clamping to fixed's range. clamp-to-half-fixed-range?
bungeman@google.com changed reviewers: + jvanverth@google.com
At PS5 I've gone back to what was in PS1 and completed that idea. The idea here is to stay in SkScalar for all calculation then convert once to Sk48dot16 to extract the integer and fractional parts. This is sufficient, since the procs are limited to 32 bits for the integer part in any event. PS5 has been verified to fix the issue seen in the attached bug report. The issue with clamping is getting the right range and where to do it. It cannot actually be done in the tms proc because the fact that it stays in full SkScalars is depended on elsewhere. It seems easier to just expand the number of bits for the integer part to the point clamping is no longer needed. At PS5 this does not yet update the copy of the code on the GPU side. This code has the same issue, where most of the calculation has at least 32 bits for the integer except for the small amount working in SkFixed. I could update this as well, if desired. https://codereview.chromium.org/977623002/diff/80001/src/core/SkDrawProcs.h File src/core/SkDrawProcs.h (left): https://codereview.chromium.org/977623002/diff/80001/src/core/SkDrawProcs.h#o... src/core/SkDrawProcs.h:96: // Returns the position of the glyph in fixed point, which may be rounded or not Removing all of this does mean there is one extra SkFixedToScalar in drawPosText when aligning other than left, but it seems cleaner to have less code since we're not sure there are any users of drawPosText with alignment other than left.
https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h#n... include/core/SkFixed.h:157: typedef int64_t Sk48Dot16; Let's harmonize the names of SkFixed3232 and Sk48Dot16. Either way seems fine. Can we not just use SkFixed3232? There are ops up there to convert to and from SkFixed.
https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h#n... include/core/SkFixed.h:157: typedef int64_t Sk48Dot16; On 2015/03/05 21:00:03, mtklein wrote: > Let's harmonize the names of SkFixed3232 and Sk48Dot16. Either way seems fine. > > Can we not just use SkFixed3232? There are ops up there to convert to and from > SkFixed. As seen here, I pulled Sk48Dot16 out of SkPaint because measureText was using it for exactly the reasons I want to use it here. The really nice thing about using 48.16 is that you don't have to keep shifting back a forth to work with SkFixed (with SkFixed3232 there are a bunch of extra shifts, and we don't even want the extra bits on the low end). Note that we currently have SkFixed, SkFixed3232, Sk48Dot16, FDot8, and SkFDot6. Making any two of these consistent seems inconsistent.
On 2015/03/05 21:17:56, bungeman1 wrote: > https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h > File include/core/SkFixed.h (right): > > https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h#n... > include/core/SkFixed.h:157: typedef int64_t Sk48Dot16; > On 2015/03/05 21:00:03, mtklein wrote: > > Let's harmonize the names of SkFixed3232 and Sk48Dot16. Either way seems > fine. > > > > Can we not just use SkFixed3232? There are ops up there to convert to and > from > > SkFixed. > > As seen here, I pulled Sk48Dot16 out of SkPaint because measureText was using it > for exactly the reasons I want to use it here. The really nice thing about using > 48.16 is that you don't have to keep shifting back a forth to work with SkFixed > (with SkFixed3232 there are a bunch of extra shifts, and we don't even want the > extra bits on the low end). > > Note that we currently have SkFixed, SkFixed3232, Sk48Dot16, FDot8, and SkFDot6. > Making any two of these consistent seems inconsistent. Sorry about that, left off Dot14
BitmapTextContext bit looks okay, though I'm not thrilled that we're adding two
additional SkScalarToFixed calls per glyph. But as you say, it's only in the
non-left-aligned cases, which should be rare.
Any reason you stuck with SK_FixedHalf for fHalfSample{XY} in
GrBitmapTextContext, but used SK_ScalarHalf in SkDraw?
I wonder if we want to decompose this into 1. fix the overflow in text 2. restructure all of the fixed types in skia bikeshed: I think I like SkFixedN(some separator)M e.g. SkFixed32Dot32 SkFixed48_16 SkFixed26$6
On 2015/03/05 22:00:40, Jvsquare wrote:
> BitmapTextContext bit looks okay, though I'm not thrilled that we're adding
two
> additional SkScalarToFixed calls per glyph. But as you say, it's only in the
> non-left-aligned cases, which should be rare.
>
> Any reason you stuck with SK_FixedHalf for fHalfSample{XY} in
> GrBitmapTextContext, but used SK_ScalarHalf in SkDraw?
Well, there should only be one extra in any event, and it's rare. I haven't
fixed the issue in GrBitmapTextContext yet, so I haven't updated it except to
keep it working with the other changes here.
On 2015/03/05 22:02:37, reed1 wrote: > I wonder if we want to decompose this into > > 1. fix the overflow in text > 2. restructure all of the fixed types in skia > > bikeshed: I think I like SkFixedN(some separator)M > > e.g. > SkFixed32Dot32 > SkFixed48_16 > SkFixed26$6 I started taking a look into this, and I would like to land the 48Dot16 stuff like this for now, and then create a SkTFixed class to unify all of these. Unifying these is bit too much for this already touchy CL. Considering we already have 6 fixed point types, and this CL is just moving one of them around a bit, it seems like a bit of a bunny trail (for this particular issue).
https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h#n... include/core/SkFixed.h:162: #define SkFloatTo48Dot16(x) ((Sk48Dot16)((x) * SK_Fixed1)) I know the value is correct, but it might read clearer if this was 65536, to match the comment above in the toscalar version. https://codereview.chromium.org/977623002/diff/80001/include/core/SkScalar.h File include/core/SkScalar.h (right): https://codereview.chromium.org/977623002/diff/80001/include/core/SkScalar.h#... include/core/SkScalar.h:173: x = SkTMax(x, SkIntToScalar(0)); I actually think SkIntToScalar(0) is harder to read. I prefer just 0.
https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/977623002/diff/80001/include/core/SkFixed.h#n... include/core/SkFixed.h:162: #define SkFloatTo48Dot16(x) ((Sk48Dot16)((x) * SK_Fixed1)) On 2015/03/09 16:33:56, reed1 wrote: > I know the value is correct, but it might read clearer if this was 65536, to > match the comment above in the toscalar version. Made things line up in a more sane way. https://codereview.chromium.org/977623002/diff/80001/include/core/SkScalar.h File include/core/SkScalar.h (right): https://codereview.chromium.org/977623002/diff/80001/include/core/SkScalar.h#... include/core/SkScalar.h:173: x = SkTMax(x, SkIntToScalar(0)); On 2015/03/09 16:33:56, reed1 wrote: > I actually think SkIntToScalar(0) is harder to read. I prefer just 0. I went and pulled this change out into it's own review, since this CL doesn't need it and it's an independent change. The issue here is that SkTMin and SkTMax take (T, T) so just '0' (as an int) doesn't compile.
api lgtm
A couple of comments: https://codereview.chromium.org/977623002/diff/140001/src/gpu/GrBitmapTextCon... File src/gpu/GrBitmapTextContext.cpp (right): https://codereview.chromium.org/977623002/diff/140001/src/gpu/GrBitmapTextCon... src/gpu/GrBitmapTextContext.cpp:156: Sk48Dot16 fx = SkScalarTo48Dot16(x + halfSampleX); It's not clear to me why we need 48Dot16 in this case -- we use fx, fy with the masks to get the fractional part, then we take the floor of fx, fy and pass the integer part to appendGlyph. Seems like we could pass in SkScalars into appendGlyph (which are floorf(x + halfSampleX), etc.), keep fx, fy as SkFixed and only use their fractional parts, and do the float-to-int conversion in appendGlyph() as part of the clip check. https://codereview.chromium.org/977623002/diff/140001/src/gpu/GrBitmapTextCon... src/gpu/GrBitmapTextContext.cpp:451: translate.setTranslate(SkIntToScalar(vx), SkIntToScalar(vy)); Not sure this is right -- we need to subtract out the upper left of the bounds rect when drawing a path.
https://codereview.chromium.org/977623002/diff/140001/src/gpu/GrBitmapTextCon... File src/gpu/GrBitmapTextContext.cpp (right): https://codereview.chromium.org/977623002/diff/140001/src/gpu/GrBitmapTextCon... src/gpu/GrBitmapTextContext.cpp:156: Sk48Dot16 fx = SkScalarTo48Dot16(x + halfSampleX); On 2015/03/10 18:44:26, jvanverth1 wrote: > It's not clear to me why we need 48Dot16 in this case -- we use fx, fy with the > masks to get the fractional part, then we take the floor of fx, fy and pass the > integer part to appendGlyph. Seems like we could pass in SkScalars into > appendGlyph (which are floorf(x + halfSampleX), etc.), keep fx, fy as SkFixed > and only use their fractional parts, and do the float-to-int conversion in > appendGlyph() as part of the clip check. If we have 'fx' and 'fy' as SkFixed and wished to pass SkScalars into appendGlyph, we'd also have to maintain 'sx' and 'sy'. Maintaining 'sx' and 'sy' here means three SkFixed to SkScalar conversions (and I'm not sure how well the 'fx' and 'sx' would stay in sync anyway). This change is mostly mechanical in the sense that I just want to maintain at least 32 bits for the integer part. appendGlyph would take Sk48Dot16 (like the draw_1_glyph_procs do) but it turns out there is no need. appendGlyph used to do some unneeded work trying to keep things in fixed point when it provably had only integers. So I just made that explicit so that the one shift happens here instead of several times in appendGlyph (and we can pass only the 32 bits it wants). If appendGlyph takes only one form of these points as parameters, it's still going to need to do at least one set of conversions, since it needs the numbers as ints and SkScalars. https://codereview.chromium.org/977623002/diff/140001/src/gpu/GrBitmapTextCon... src/gpu/GrBitmapTextContext.cpp:451: translate.setTranslate(SkIntToScalar(vx), SkIntToScalar(vy)); On 2015/03/10 18:44:26, jvanverth1 wrote: > Not sure this is right -- we need to subtract out the upper left of the bounds > rect when drawing a path. The upper left bounds are no longer being added to these. Before it was being added and then subtracted. This simply keeps the unmodified values and just uses them directly. 'x' and 'y' now have the upper left bounds added, and 'vx' and 'vy' are left untouched.
https://codereview.chromium.org/977623002/diff/140001/src/gpu/GrBitmapTextCon... File src/gpu/GrBitmapTextContext.cpp (right): https://codereview.chromium.org/977623002/diff/140001/src/gpu/GrBitmapTextCon... src/gpu/GrBitmapTextContext.cpp:242: Sk48Dot16 fx = SkScalarTo48Dot16(tmsLoc.fX + halfSampleX); Now, down here the above argument might have a point. The fixed to scalar cost has been paid elsewhere, and here are some scalars. Here we could potentially have one conversion to SkFixed (though potentially an undefined one) to call glyphCacheProc but pass the SkScalar into appendGlyph. However, append would still need to do a scalar to int conversion (instead of an int to scalar conversion) and a floor. In other words, this code has the same number of conversions and trades a floorf for a 64 bit shift.
GPU text lgtm
The CQ bit was checked by bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, reed@google.com, jvanverth@google.com Link to the patchset: https://codereview.chromium.org/977623002/#ps160001 (title: "Warnings as errors on Windows.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/977623002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/79738cc7bf12d212bef4ff80591d1bf6f383663d |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
