Keyboard Shortcuts

File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(282)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 7135060: This patch fixes the existing blend modes.

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 12 months ago by RikC
Modified:
12 years, 11 months ago
Reviewers:
caryclark1;reed, reed1
CC:
skia-review_googlegroups.com
Base URL:
http://skia.googlecode.com/svn/trunk/
Visibility:
Public.
This patch fixes the existing blend modes. It also introduces a new 'multiply' blend mode. This mode is different from the existing one that just multiplies all components (instead of doing actual blending). Hard-light and overlay have some rounding issues. This will be addressed later. This patch also does not add the non-separable blend modes. See review for 7103061. BUG=1037

Patch Set 1 #

Created: 12 years, 12 months ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+182 lines, -147 lines) Patch
M include/core/SkXfermode.h View 1 chunk +2 lines, -1 line 0 comments Download
M src/core/SkXfermode.cpp View 3 chunks +180 lines, -146 lines 0 comments Download
Total messages: 7
|
reed1
12 years, 11 months ago (2013年01月22日 19:01:02 UTC) #1
Sign in to reply to this message.
reed1
Good catch on MULTIPLY. The old code was written to support modulating between two shaders ...
12 years, 11 months ago (2013年01月22日 19:31:04 UTC) #2
Good catch on MULTIPLY. The old code was written to support modulating between
two shaders (for drawVertices). It was unfortunate that I chose that name.
However, scanning known google callers, I think we can do the following:
1. rename kMultiply_Mode to kModulate_Mode
1.1 add comment // multiplies corresponding components
2. add kMultiply_Mode to implement the CSS/SVG behavor
2.1 look at the impl in SkBlendImageFilter.cpp, and see how that compares to
yours (speed and numerical correctness).
Sign in to reply to this message.
reed1
For sanity, perhaps we should just do #1 first (the rename), so we're sure we ...
12 years, 11 months ago (2013年01月22日 19:33:11 UTC) #3
For sanity, perhaps we should just do #1 first (the rename), so we're sure we
catch all of the places in skia. Then in a follow-up CL we can do a clean ADD of
a new mode.
Sign in to reply to this message.
RikC
On 2013年01月22日 19:33:11, reed1 wrote: > For sanity, perhaps we should just do #1 first ...
12 years, 11 months ago (2013年01月22日 23:48:41 UTC) #4
On 2013年01月22日 19:33:11, reed1 wrote:
> For sanity, perhaps we should just do #1 first (the rename), so we're sure we
> catch all of the places in skia. Then in a follow-up CL we can do a clean ADD
of
> a new mode.
should that be done in a new review?
Sign in to reply to this message.
reed1
Yes, I suggest two sequential CLs. 1. rename the existing Multiply to Modulate (and fix ...
12 years, 11 months ago (2013年01月23日 14:07:43 UTC) #5
Yes, I suggest two sequential CLs.
1. rename the existing Multiply to Modulate (and fix up existing skia callers)
2. add Multiply (implemented to match CSS spec).
Sign in to reply to this message.
RikC
On 2013年01月23日 14:07:43, reed1 wrote: > Yes, I suggest two sequential CLs. > > 1. ...
12 years, 11 months ago (2013年01月23日 18:47:11 UTC) #6
On 2013年01月23日 14:07:43, reed1 wrote:
> Yes, I suggest two sequential CLs.
> 
> 1. rename the existing Multiply to Modulate (and fix up existing skia callers)
Who are the existing callers? Is it anything that uses skia (such as chrome?) or
just the test files?
> 2. add Multiply (implemented to match CSS spec).
Will do.
Sign in to reply to this message.
RikC
On 2013年01月23日 18:47:11, RikC wrote: > On 2013年01月23日 14:07:43, reed1 wrote: > > Yes, I ...
12 years, 11 months ago (2013年01月23日 19:01:01 UTC) #7
On 2013年01月23日 18:47:11, RikC wrote:
> On 2013年01月23日 14:07:43, reed1 wrote:
> > Yes, I suggest two sequential CLs.
> > 
> > 1. rename the existing Multiply to Modulate (and fix up existing skia
callers)
> Who are the existing callers? Is it anything that uses skia (such as chrome?)
or
> just the test files?
> 
> > 2. add Multiply (implemented to match CSS spec).
> Will do.
Added multiply to modulate patch with https://codereview.appspot.com/7205045/ 
Sign in to reply to this message.
|
Powered by Google App Engine
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /