Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1350)

Issue 6244068: Extract mask gamma; share more scaler contexts. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 6 months ago by bungeman
Modified:
12 years, 3 months ago
Reviewers:
thakis, DerekS, reed1
CC:
skia-review_googlegroups.com
Base URL:
http://skia.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Extract mask gamma; share more scaler contexts.

Patch Set 1 #

Total comments: 4

Patch Set 2 : Assume SK_USE_COLOR_LUMINANCE. #

Total comments: 2

Patch Set 3 : Checkpoint #

Total comments: 2

Patch Set 4 : Checkpoint: More realistic changes for Mac. #

Patch Set 5 : Checkpoint: Mac gray from smoothed lcd. #

Patch Set 6 : Checkpoint: Mac gray only and other fixes. #

Patch Set 7 : Checkpoint: Works in Chrome with minimal rebaselines. #

Total comments: 41

Patch Set 8 : Address comments and clean up. #

Patch Set 9 : Ready for review. #

Total comments: 11

Patch Set 10 : Address comments. #

Total comments: 20

Patch Set 11 : Some incremental improvements. #

Patch Set 12 : Move gamma mask lut code. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+982 lines, -1017 lines) Patch
M gyp/common.gypi View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download
M gyp/common_conditions.gypi View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -3 lines 0 comments Download
M gyp/core.gyp View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M gyp/ports.gyp View 1 2 3 4 5 6 7 8 9 10 3 chunks +0 lines, -4 lines 0 comments Download
M include/core/SkColorPriv.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +14 lines, -0 lines 0 comments Download
M include/core/SkFontHost.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -9 lines 0 comments Download
M include/core/SkScalerContext.h View 1 2 3 4 5 6 7 8 9 10 8 chunks +58 lines, -41 lines 0 comments Download
A src/core/SkMaskGamma.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +264 lines, -0 lines 0 comments Download
A src/core/SkMaskGamma.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +46 lines, -0 lines 0 comments Download
M src/core/SkPaint.cpp View 1 2 3 4 5 6 7 8 9 10 9 chunks +121 lines, -76 lines 1 comment Download
M src/core/SkScalerContext.cpp View 1 2 3 4 5 6 7 8 9 10 11 chunks +82 lines, -84 lines 0 comments Download
M src/ports/SkFontHost_FreeType.cpp View 1 2 3 4 5 6 7 8 9 10 14 chunks +41 lines, -165 lines 0 comments Download
D src/ports/SkFontHost_gamma.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -107 lines 0 comments Download
D src/ports/SkFontHost_gamma_none.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -23 lines 0 comments Download
M src/ports/SkFontHost_mac_coretext.cpp View 1 2 3 4 5 6 7 8 9 10 13 chunks +215 lines, -332 lines 0 comments Download
M src/ports/SkFontHost_win.cpp View 1 2 3 4 5 6 7 8 9 10 17 chunks +134 lines, -122 lines 0 comments Download
D src/ports/sk_predefined_gamma.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -51 lines 0 comments Download

Messages

Total messages: 24
bungeman
12 years, 6 months ago (2012-05-30 22:48:19 UTC) #1
bungeman
Assume SK_USE_COLOR_LUMINANCE.
12 years, 6 months ago (2012-06-01 21:50:08 UTC) #2
reed1
http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h File src/utils/SkMaskGamma.h (right): http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h#newcode14 src/utils/SkMaskGamma.h:14: */ static function? http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h#newcode20 src/utils/SkMaskGamma.h:20: */ Where is the ...
12 years, 6 months ago (2012-06-04 12:30:19 UTC) #3
reed1
It *seems* that fonthosts shouldn't have to know about indices -vs- colors. Can't we just ...
12 years, 6 months ago (2012-06-04 12:35:20 UTC) #4
bungeman
http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h File src/utils/SkMaskGamma.h (right): http://codereview.appspot.com/6244068/diff/1/src/utils/SkMaskGamma.h#newcode14 src/utils/SkMaskGamma.h:14: */ On 2012/06/04 12:30:19, reed1 wrote: > static function? ...
12 years, 6 months ago (2012-06-04 13:41:13 UTC) #5
bungeman
On 2012/06/04 12:35:20, reed1 wrote: > It *seems* that fonthosts shouldn't have to know about ...
12 years, 6 months ago (2012-06-04 13:42:47 UTC) #6
bungeman
Initial ideas on piping the information down, both FreeType and GDI back-ends updated. CoreGraphics back-end ...
12 years, 5 months ago (2012-06-27 17:55:24 UTC) #7
reed1
http://codereview.appspot.com/6244068/diff/13001/src/core/SkMaskGamma.h File src/core/SkMaskGamma.h (right): http://codereview.appspot.com/6244068/diff/13001/src/core/SkMaskGamma.h#newcode21 src/core/SkMaskGamma.h:21: should this be a Static method? http://codereview.appspot.com/6244068/diff/13001/src/core/SkMaskGamma.h#newcode22 src/core/SkMaskGamma.h:22: U8CPU ...
12 years, 5 months ago (2012-06-27 18:22:31 UTC) #8
bungeman
Take a look, still rough around the edges, but may do what it is we ...
12 years, 5 months ago (2012-07-18 16:21:45 UTC) #9
bungeman
By keeping the changes to gray on mac and linux behind the SK_GAMMA_APPLY_TO_A8, only minimal ...
12 years, 4 months ago (2012-07-19 21:02:41 UTC) #10
reed1
In general, this is a big (important) change, so more clarity/dox/obviousness would help, esp. when ...
12 years, 4 months ago (2012-07-20 17:24:14 UTC) #11
bungeman
Still quite rough, but uploading something to look at. https://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h File include/core/SkFontHost.h (left): https://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h#oldcode234 include/core/SkFontHost.h:234: ...
12 years, 4 months ago (2012-07-20 22:50:25 UTC) #12
DerekS
http://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h File include/core/SkFontHost.h (left): http://codereview.appspot.com/6244068/diff/32001/include/core/SkFontHost.h#oldcode234 include/core/SkFontHost.h:234: */ There are no callers of this function in ...
12 years, 4 months ago (2012-07-23 12:36:18 UTC) #13
bungeman
Now seems to do what we want it to do, with minimal rebaselines required.
12 years, 4 months ago (2012-07-25 17:18:51 UTC) #14
reed1
https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContext.h File include/core/SkScalerContext.h (left): https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContext.h#oldcode80 include/core/SkScalerContext.h:80: #endif Is there a perf win to not make ...
12 years, 4 months ago (2012-07-25 19:14:02 UTC) #15
bungeman
https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContext.h File include/core/SkScalerContext.h (right): https://codereview.appspot.com/6244068/diff/43001/include/core/SkScalerContext.h#newcode71 include/core/SkScalerContext.h:71: On 2012/07/25 19:14:02, reed1 wrote: > Seems like the ...
12 years, 4 months ago (2012-07-26 20:51:05 UTC) #16
reed1
mostly requests for more clarity, some requests for code-sharing https://codereview.appspot.com/6244068/diff/44018/include/core/SkScalerContext.h File include/core/SkScalerContext.h (right): https://codereview.appspot.com/6244068/diff/44018/include/core/SkScalerContext.h#newcode26 include/core/SkScalerContext.h:26: ...
12 years, 4 months ago (2012-07-27 14:29:07 UTC) #17
bungeman
I think I've gotten to all of the existing comments now. https://codereview.appspot.com/6244068/diff/44018/include/core/SkScalerContext.h File include/core/SkScalerContext.h (right): ...
12 years, 4 months ago (2012-07-27 19:30:52 UTC) #18
reed1
lets move that big guy into a .cpp, but after that, lgtm
12 years, 4 months ago (2012-07-27 20:32:41 UTC) #19
bungeman
Committed revision r4841. Rebaselined r4843. Fixed r4889. Chromium roll http://codereview.chromium.org/10825089/ .
12 years, 4 months ago (2012-08-01 19:16:35 UTC) #20
thakis
https://codereview.appspot.com/6244068/diff/52002/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.appspot.com/6244068/diff/52002/src/core/SkPaint.cpp#newcode1628 src/core/SkPaint.cpp:1628: gDeviceLuminance = SkNEW_ARGS(SkGammaLuminance, (gammaExponent)); This leaks if called repeatedly ...
12 years, 3 months ago (2012-08-27 05:11:10 UTC) #21
bungeman
> This leaks if called repeatedly with different gamma exponents. True, but this cannot currently ...
12 years, 3 months ago (2012-08-31 21:17:58 UTC) #22
thakis
On Fri, Aug 31, 2012 at 2:17 PM, <bungeman@google.com> wrote: > This leaks if called ...
12 years, 3 months ago (2012-08-31 21:19:56 UTC) #23
bungeman
12 years, 3 months ago (2012-08-31 21:56:28 UTC) #24
> so I guess it does happen somehow.

I now understand why I haven't seen this before. It doesn't happen in Skia, but
it does happen in Chrome. This is because Chrome does not define
SK_GAMMA_APPLY_TO_A8. When this is not defined, on Linux the regular
anti-aliased text is not gamma corrected, mostly because there is currently no
good means of re-base-lining the layout tests. I will need to re-think how
ignorePreBlend works or increase the cache size to 2.

I have opened http://code.google.com/p/skia/issues/detail?id=825 to track this.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b