Bug 128049 - Subpixel rendering: Introduce device pixel snapping helper functions.
Summary: Subpixel rendering: Introduce device pixel snapping helper functions.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks: 127524 128009
  Show dependency treegraph
 
Reported: 2014-02-01 13:38 PST by zalan
Modified: 2014-02-02 13:10 PST (History)
3 users (show)

See Also:


Attachments
Patch (6.91 KB, patch)
2014-02-01 13:58 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (7.08 KB, patch)
2014-02-01 19:49 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (6.56 KB, patch)
2014-02-01 23:11 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2014-02-01 13:38:54 PST
ssia
Comment 1 zalan 2014-02-01 13:58:27 PST
Created attachment 222889 [details]
Patch
Comment 2 zalan 2014-02-01 19:49:22 PST
Created attachment 222901 [details]
Patch
Comment 3 Simon Fraser (smfr) 2014-02-01 21:25:06 PST
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.
Comment 4 zalan 2014-02-01 21:30:42 PST
(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 5 Simon Fraser (smfr) 2014-02-01 22:22:50 PST
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()?
Comment 6 zalan 2014-02-01 22:26:07 PST
(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.
Comment 7 zalan 2014-02-01 22:36:03 PST
> > 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.
Comment 8 zalan 2014-02-01 23:11:22 PST
Created attachment 222911 [details]
Patch
Comment 9 WebKit Commit Bot 2014-02-02 10:34:00 PST
Comment on attachment 222911 [details]
Patch

Clearing flags on attachment: 222911

Committed r163265: <http://trac.webkit.org/changeset/163265>
Comment 10 WebKit Commit Bot 2014-02-02 10:34:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Darin Adler 2014-02-02 11:01:01 PST
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.
Comment 12 zalan 2014-02-02 11:02:35 PST
(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.
Comment 13 zalan 2014-02-02 13:10:11 PST
(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