Summary: | Subpixel rendering: Introduce device pixel snapping helper functions. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> | ||||||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, jonlee, simon.fraser | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 127524, 128009 | ||||||||||
Attachments: |
|
Description
zalan
2014-02-01 13:38:54 PST
Created attachment 222889 [details]
Patch
Created attachment 222901 [details]
Patch
Comment on attachment 222901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222901&action=review > Source/WebCore/platform/LayoutUnit.h:942 > +inline int snapSizeToPixel(LayoutUnit size, LayoutUnit location) I don't like "size" because we have IntSize/FloatSize with have widths and heights. > Source/WebCore/platform/LayoutUnit.h:948 > +inline float snapSizeToDevicePixel(LayoutUnit size, LayoutUnit location, float devicePixelRatio) Ditto. Do we ever use these on things that aren't rects? > Source/WebCore/platform/graphics/GraphicsContext.cpp:91 > + , m_paintPixelRatio(2) How can you assume 2 here? > Source/WebCore/platform/graphics/GraphicsContext.h:447 > + float paintPixelRatio() const { return m_paintPixelRatio; } We may have to rethink this name. Reading it in isolation here doesn't communicate what it's for. (In reply to comment #3) > (From update of attachment 222901 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222901&action=review > > > Source/WebCore/platform/LayoutUnit.h:942 > > +inline int snapSizeToPixel(LayoutUnit size, LayoutUnit location) > > I don't like "size" because we have IntSize/FloatSize with have widths and heights. > > > Source/WebCore/platform/LayoutUnit.h:948 > > +inline float snapSizeToDevicePixel(LayoutUnit size, LayoutUnit location, float devicePixelRatio) > > Ditto. ok. > Do we ever use these on things that aren't rects? No. They are part of the rect snapping logic, so it does not make much sense to use them standalone. > > > Source/WebCore/platform/graphics/GraphicsContext.cpp:91 > > + , m_paintPixelRatio(2) > > How can you assume 2 here? I assumed we support iOS retina only. I could be mistaken here. > > > Source/WebCore/platform/graphics/GraphicsContext.h:447 > > + float paintPixelRatio() const { return m_paintPixelRatio; } > > We may have to rethink this name. Reading it in isolation here doesn't communicate what it's for. I had devicePixelRatioForPaiting perviously. Does that sound better? Comment on attachment 222901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222901&action=review >>> Source/WebCore/platform/graphics/GraphicsContext.cpp:91 >>> + , m_paintPixelRatio(2) >> >> How can you assume 2 here? > > I assumed we support iOS retina only. I could be mistaken here. I don't think you should hardcode that assumption. >>> Source/WebCore/platform/graphics/GraphicsContext.h:447 >>> + float paintPixelRatio() const { return m_paintPixelRatio; } >> >> We may have to rethink this name. Reading it in isolation here doesn't communicate what it's for. > > I had devicePixelRatioForPaiting perviously. Does that sound better? pixelSnappingFactor()? (In reply to comment #5) > (From update of attachment 222901 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222901&action=review > > >>> Source/WebCore/platform/graphics/GraphicsContext.cpp:91 > >>> + , m_paintPixelRatio(2) > >> > >> How can you assume 2 here? > > > > I assumed we support iOS retina only. I could be mistaken here. > > I don't think you should hardcode that assumption. Ok, I'll change that. > > >>> Source/WebCore/platform/graphics/GraphicsContext.h:447 > >>> + float paintPixelRatio() const { return m_paintPixelRatio; } > >> > >> We may have to rethink this name. Reading it in isolation here doesn't communicate what it's for. > > > > I had devicePixelRatioForPaiting perviously. Does that sound better? > > pixelSnappingFactor()? Much better. Thanks. > > Do we ever use these on things that aren't rects?
> No. They are part of the rect snapping logic, so it does not make much sense to use them standalone.
hmm, actually we do use them in a few places outside of LayoutRect for example to snap offsetWidth/Height. This naming confused me too in the past, but I can't think of anything better now.
Created attachment 222911 [details]
Patch
Comment on attachment 222911 [details] Patch Clearing flags on attachment: 222911 Committed r163265: <http://trac.webkit.org/changeset/163265> All reviewed patches have been landed. Closing bug. Comment on attachment 222911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222911&action=review > Source/WebCore/platform/LayoutUnit.h:940 > + return round((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; Needs to be roundf to avoid converting to a double, rounding, and then converting back to a float. > Source/WebCore/platform/LayoutUnit.h:945 > + return floor((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; Needs to be floorf to avoid converting to a double, rounding, and then converting back to a float. > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:143 > + ASSERT(abs(baseDeviceMatrix.a) == abs(baseDeviceMatrix.d)); The function abs takes an int, and I assume that conversion to integer isn’t what we want here. I suggest using fabs, even though it will result in converting float to double on 32-bit platforms, because this is just an assertion. (In reply to comment #11) > (From update of attachment 222911 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222911&action=review > > > Source/WebCore/platform/LayoutUnit.h:940 > > + return round((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; > > Needs to be roundf to avoid converting to a double, rounding, and then converting back to a float. > > > Source/WebCore/platform/LayoutUnit.h:945 > > + return floor((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; > > Needs to be floorf to avoid converting to a double, rounding, and then converting back to a float. > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:143 > > + ASSERT(abs(baseDeviceMatrix.a) == abs(baseDeviceMatrix.d)); > > The function abs takes an int, and I assume that conversion to integer isn’t what we want here. I suggest using fabs, even though it will result in converting float to double on 32-bit platforms, because this is just an assertion. Thanks. I'll fix them right away. (In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 222911 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=222911&action=review > > > > > Source/WebCore/platform/LayoutUnit.h:940 > > > + return round((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; > > > > Needs to be roundf to avoid converting to a double, rounding, and then converting back to a float. > > > > > Source/WebCore/platform/LayoutUnit.h:945 > > > + return floor((value.rawValue() * pixelSnappingFactor) / kEffectiveFixedPointDenominator) / pixelSnappingFactor; > > > > Needs to be floorf to avoid converting to a double, rounding, and then converting back to a float. > > > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:143 > > > + ASSERT(abs(baseDeviceMatrix.a) == abs(baseDeviceMatrix.d)); > > > > The function abs takes an int, and I assume that conversion to integer isn’t what we want here. I suggest using fabs, even though it will result in converting float to double on 32-bit platforms, because this is just an assertion. > > Thanks. I'll fix them right away. bug 128075 |