Bug 128049 – Subpixel rendering: Introduce device pixel snapping helper functions.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

ssia

[alan]

Created attachment 222889 [details]
Patch

[alan]

Created attachment 222901 [details]
Patch

[Simon Fraser (smfr)]

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.

[alan]

(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?

[Simon Fraser (smfr)]

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()?

[alan]

(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.

[alan]

> > 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.

[alan]

Created attachment 222911 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 222911 [details]
Patch

Clearing flags on attachment: 222911

Committed r163265: <http://trac.webkit.org/changeset/163265>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[alan]

(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.

[alan]

(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