Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1124373002:
Implement Porter Duff XP with a blend table (Closed)
|
Created:
5 years, 7 months ago by Chris Dalton Modified:
5 years, 7 months ago CC:
reviews_skia.org, Kimmo Kinnunen, vbuzinov, Mark Kilgard Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionImplement Porter Duff XP with a blend table
Removes the runtime logic used by PorterDuffXferProcessor to decide
blend coeffs and shader outputs, and instead uses a compile-time
constant table of pre-selected blend formulas. Separates out the dst
read fallback into its own XP.
Introduces a new blend strategy for srcCoeff=0 that can apply coverage
with a reverse subtract blend equation instead of dual source
blending.
Adds new macros in GrBlend.h to analyze blend formulas both runtime.
Removes kSetCoverageDrawing_OptFlag and GrSimplifyBlend as they are no
longer used.
Adds a GM that verifies all xfermodes, including arithmetic, with the
color/coverage invariants used by Porter Duff.
Adds a unit test that verifies each Porter Duff formula with every
color/coverage invariant.
Major changes:
* Uses a reverse subtract blend equation for coverage when srcCoeff=0
(clear, dst-out [Sa=1], dst-in, modulate). Platforms that don't
support dual source blending no longer require a dst copy for
dst-in and modulate.
* Sets BlendInfo::fWriteColor to false when the blend does not modify
the dst. GrGLGpu will now use glColorMask instead of blending for
these modes (dst, dst-in [Sa=1], modulate ignored for [Sc=1]).
* Converts all SA blend coeffs to One for opaque inputs, and ISA to
Zero if there is also no coverage. (We keep ISA around when there
is coverage because we use it to tweak alpha for coverage.)
* Abandons solid white optimizations for the sake of simplicity
(screen was the only mode that previous had solid white opts).
Minor differences:
* Inconsequential differences in opt flags (e.g. we now return
kCanTweakAlphaForCoverage_OptFlag even when there is no coverage).
* Src coeffs when the shader outputs 0.
* IS2C vs IS2A when the secondary output is scalar.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/9a70920db22b6309c671f8e5d519bb95570e4414
Committed: https://skia.googlesource.com/skia/+/6fd158ea47472c4d038e48980a95e36623f840c9
Patch Set 1 #Patch Set 2 : comment #
Total comments: 1
Patch Set 3 : rebase #
Total comments: 1
Patch Set 4 : redo #Patch Set 5 : var name #
Total comments: 3
Patch Set 6 : GrBlend.h bugs #
Total comments: 25
Patch Set 7 : Add unit test, touchups #Patch Set 8 : var names #
Total comments: 15
Patch Set 9 : add coverage at runtime #
Total comments: 6
Patch Set 10 : Back to table #Patch Set 11 : Fix issues with clang and msvc #
Total comments: 2
Patch Set 12 : comment #Patch Set 13 : bugs #Patch Set 14 : separate XP for dst read fallback #
Total comments: 11
Patch Set 15 : rebase, feedback #Patch Set 16 : Finish test for no dual source blending #
Total comments: 3
Patch Set 17 : Use resource provider instead of gpu for tests #
Messages
Total messages: 50 (7 generated)
cdalton@nvidia.com changed reviewers: + bsalomon@google.com, egdaniel@google.com
This lets these modes work without a dst copy on unextended OpenGL, and also makes them compatible with mixed samples. https://codereview.chromium.org/1124373002/diff/20001/src/gpu/effects/GrPorte... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (left): https://codereview.chromium.org/1124373002/diff/20001/src/gpu/effects/GrPorte... src/gpu/effects/GrPorterDuffXferProcessor.cpp:366: fDstBlend = kIS2C_GrBlendCoeff; This branch is no longer used. Modulate was the only mode that used SC for the dst coefficient, and it is now handled by reverse subtract.
Nice optimization! I leave the details to Greg.
Definitely liking this change. Just the one comment about merging functions. https://codereview.chromium.org/1124373002/diff/40001/src/gpu/effects/GrPorte... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/40001/src/gpu/effects/GrPorte... src/gpu/effects/GrPorterDuffXferProcessor.cpp:331: if (kZero_GrBlendCoeff == fSrcBlend) { I wonder if it is time to merge getInternalOptimizations and calcOutputTypes into a single getOptimizations again. I think originally I had pulled them apart because LCD support did different things, but LCD is now out of the normal porter duff. It just seems like we do a lot of similar checks in both places and outside of actually setting the primary and secondary outputs there really isn't anything functionally different. I think it would also allow us to get rid of the "weird" kSetCoverageDraw_OptFlag (which is a very confusing flag) since I believe it is only used in passing info from porterduff internalOpt to porterduff calcOutputTypes.
This one merges getInternalOptimizations and calcOutputTypes, and removes kSetCoverageDrawing_OptFlag. It also documents the different blend strategies and puts them all together in one place, and centralizes the logic for onGetOptimizations/willReadDstColor/getInvariantOutput. I updated the commit message and tried to put a good overview of the patch in there, so hopefully it's helpful. https://codereview.chromium.org/1124373002/diff/80001/src/gpu/effects/GrPorte... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/80001/src/gpu/effects/GrPorte... src/gpu/effects/GrPorterDuffXferProcessor.cpp:117: * the primary output type to none. For clear, do we want to keep making the shader output zero like before? Or do we want to just output the standard kModulate? It seems like outputting zero may be more GPU-efficient in some scenarios, but there is also an advantage to not compiling a special shader just to output zero when we can set the blend coeffs to (Zero, Zero) instead.
Haven't made it through the GrPorterDuffXferProcessor.cpp yet, but noticed one thing in GrBlend.h https://codereview.chromium.org/1124373002/diff/80001/src/gpu/GrBlend.h File src/gpu/GrBlend.h (right): https://codereview.chromium.org/1124373002/diff/80001/src/gpu/GrBlend.h#newco... src/gpu/GrBlend.h:73: struct GrTBlendCoeffRefsBlendColor : SkTBool<kDC_GrBlendCoeff == Coeff || This doesn't look correct, looks like a copy of GrTBlendCoeffRefsDst
https://codereview.chromium.org/1124373002/diff/80001/src/gpu/GrBlend.h File src/gpu/GrBlend.h (right): https://codereview.chromium.org/1124373002/diff/80001/src/gpu/GrBlend.h#newco... src/gpu/GrBlend.h:73: struct GrTBlendCoeffRefsBlendColor : SkTBool<kDC_GrBlendCoeff == Coeff || On 2015/05/18 17:26:39, bsalomon wrote: > This doesn't look correct, looks like a copy of GrTBlendCoeffRefsDst Oops you're right. Fixed. This currently isn't being used anywhere, I just threw it in last minute for the sake of completeness.. I also fixed a stupid mistake in can_tweak below that wasn't currently affecting anything either.
Added a unit test that verifies every xfermode with every color/coverage POI. https://codereview.chromium.org/1124373002/diff/140001/include/gpu/effects/Gr... File include/gpu/effects/GrPorterDuffXferProcessor.h (right): https://codereview.chromium.org/1124373002/diff/140001/include/gpu/effects/Gr... include/gpu/effects/GrPorterDuffXferProcessor.h:51: static int TestGetXPOutputSecondary(const GrXferProcessor*); Not sure if there is a "more preferred" way to get specifics to the unit tests... https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:133: * the primary output type to none. <copying this comment from before so it doesn't get lost> For clear, do we want to keep making the shader output zero like before? Or do we want to just output the standard kModulate? It seems like outputting zero may be more GPU-efficient in some scenarios, but there is also an advantage to not compiling a special shader just to output zero when we can set the blend coeffs to (Zero, Zero) instead.
https://codereview.chromium.org/1124373002/diff/140001/tests/GrPorterDuffTest... File tests/GrPorterDuffTest.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/tests/GrPorterDuffTest... tests/GrPorterDuffTest.cpp:126: TEST_ASSERT(xpi.fInvariantOutput.fWillBlendWithDst); This one is confusing. Does "will blend with dst" mean the final color output by the blend equation is dependent on the dst? (Meaning this value should be true for dst mode?) Or should "will blend with dst" be false for this xfer mode since it doesn't change anything? (The previous functionality was to set fWillBlendWithDst=true)
I just noticed the COVERAGE_* formulas are all compatible with RGB coverage. This means that after this change, we can make another one to always support RGB coverage on Porter Duff when we have dual source blending. It also means we can support RGB coverage even without dual source blending for the reverse subtract modes (clear, dst-in, modulate), and for the modes where the normal coeffs "just work" (dst, dst-over, plus, screen).
... And we can also support RGB coverage without dual source blend for dst-atop when the color is opaque, and for every mode when the color is solid white.
I tried going over the table of blendFormula's and I think they are all correct in terms of optimizations, but I won't guarantee I didn't miss one :) Haven't looked at the gm yet, but will do soon. Just before i start looking at it, what makes it different from all our other xfermode gm's? What is our gap in coverage in all the old ones? https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:42: kUsesGrPaintColor_Property = 1 << 2, It doesn't necessarily use GrPaint's color, it just uses the color passed to it which could come from some color processor. Does kUsesInputColor make sense? https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:86: OutputType fPrimaryOutputType : 3; should we have some static assert on the sizes here to make sure we fit. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:195: static const BlendFormula gBlendTable[3][2][SkXfermode::kLastCoeffMode + 1] = { I don't this we are gaining much at all by also branch on solid white color. It seems like only 3 things change: modulate & screen when no coverage modulate when coverage Screen and modulate are just not common enough to really justify the extra size on complexity added by solidWhite. Also the philosophical question of why is solidWhite optimized and not transparent black or other colors. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:197: /*>> No coverage, input color unknown: <<*/ {{ Would this look any clearer if we reordered the table so that individual modes were grouped together? So for example you can quickly see how we change src-over depending on coverage/color. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:310: SkASSERT(!colorPOI.isSolidWhite() || colorPOI.isOpaque()); What is this assert trying to check? This will trivial always be true since if !solidWhite() -> true solidWhite() -> isOpaque() -> true https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:400: output, xp.readsCoverage() ? inCoverage : "vec4(1.0)"); can we even get into this state? how can we be setting that the xp will not read coverage, but at the same time setting a coverage equation? https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:492: key |= blendFormula->fPrimaryOutputType << 5; add some assert on max value of Output type?
Also my latest comments are from the first version of the blend table, haven't looked at any new changes yet
On 2015/05/20 14:15:11, egdaniel wrote: > what makes it different from all our other xfermode gm's? So aaxfermodes draws all combinations of: (every coeff mode, advanced blend mode, and arithmetic mode) x (with/without coverage) x (transparent, opaque, solid white) I had two intended purposes for it: 1. To exercise every entry in the Porter Duff blend table 2. To validate mixed samples once we add support for it in the XP's I'm only moderately familiar with the existing GM's for xfermodes.. Do you know if (pixel) coverage already has (test) coverage for all combinations of xfermode and colorPOI/coveragePOI? > Also my latest comments are from the first version of the blend table, haven't > looked at any new changes yet The blend table didn't change, I just reordered it so "coverage" is at index 0 and "no coverage" is at index 1. In fact, the reason I added the unit test with that change was to guarantee the table didn't change. so if you're happy with the blend table in a previous patch we should be good to go.
Thanks for the review! Your comments got me thinking and I realized we can exclude coverage from the table and apply it instead at runtime. Then the table would only be two dimensional: [mode] x [colorPOI]. We can add entries for unknown, opaque, solid white, and trans black. (I can't think of any other color traits that would reduce the blend formula.) I'm booked for today but I'll try and get the new patch in by the end of the day tomorrow. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:42: kUsesGrPaintColor_Property = 1 << 2, On 2015/05/20 14:13:55, egdaniel wrote: > It doesn't necessarily use GrPaint's color, it just uses the color passed to it > which could come from some color processor. Does kUsesInputColor make sense? Yeah, I like that name better. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:86: OutputType fPrimaryOutputType : 3; On 2015/05/20 14:13:55, egdaniel wrote: > should we have some static assert on the sizes here to make sure we fit. My reasoning for not including static asserts was that gcc will already issue a warning if you try to stuff an enum into a bitfield that's too small for it, which in skia turns into an error. I can't speak for other compilers or the future of Werror though. Do you think this is sufficient or would you rather have the static asserts? (I'm kind of on the fence myself..) https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:195: static const BlendFormula gBlendTable[3][2][SkXfermode::kLastCoeffMode + 1] = { On 2015/05/20 14:13:55, egdaniel wrote: > I don't this we are gaining much at all by also branch on solid white color. It > seems like only 3 things change: > modulate & screen when no coverage > modulate when coverage > > Screen and modulate are just not common enough to really justify the extra size > on complexity added by solidWhite. Also the philosophical question of why is > solidWhite optimized and not transparent black or other colors. Right I hear what you're saying. I actually did try having just coverage in the table, and then adding a runtime optimizer to figure in the colorPOI. It ended up being more confusing in my mind than just laying it out in a table, not to mention more lines of code and probably a larger compiled size... I just had a thought though, we can do the opposite and only have a table for (xfermode x colorPOI), and then apply coveragePOI at runtime. This will actually work out really nicely because the coverage formulas are pretty canned, and the one you use depends only on the coefficients. And since the coeffs will be layed out in the table ahead of time, we can decide at compile-time which coverage formula to use. This will also make it easier to add support for RGB coverage. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:197: /*>> No coverage, input color unknown: <<*/ {{ On 2015/05/20 14:13:55, egdaniel wrote: > Would this look any clearer if we reordered the table so that individual modes > were grouped together? So for example you can quickly see how we change src-over > depending on coverage/color. This is another one of those things I was on the fence about. I can reorder it. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:310: SkASSERT(!colorPOI.isSolidWhite() || colorPOI.isOpaque()); On 2015/05/20 14:13:55, egdaniel wrote: > What is this assert trying to check? This will trivial always be true since if > !solidWhite() -> true > solidWhite() -> isOpaque() -> true Right, this is verifying that we can do "colorPOI.isOpaque() + colorPOI.isSolidWhite()" to get an index of 2 for the solid white case. Maybe it's OCD, since the only reason it would fail is if there is a bug elsewhere? https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:400: output, xp.readsCoverage() ? inCoverage : "vec4(1.0)"); On 2015/05/20 14:13:55, egdaniel wrote: > can we even get into this state? how can we be setting that the xp will not read > coverage, but at the same time setting a coverage equation? Mixed samples :) (My motivation for this patch was actually to get Porter Duff ready for mixed samples) Shader outputs 1.0, which then gets modulated by HW-computed coverage in the ROP before it reaches the blend hardware. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:492: key |= blendFormula->fPrimaryOutputType << 5; On 2015/05/20 14:13:55, egdaniel wrote: > add some assert on max value of Output type? I think this already has what you are asking for? (line 489) GR_STATIC_ASSERT(BlendFormula::kLast_OutputType < 8); I'll move it to this location in the code, I think that makes more sense.
https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:86: OutputType fPrimaryOutputType : 3; On 2015/05/20 16:11:22, Chris Dalton wrote: > On 2015/05/20 14:13:55, egdaniel wrote: > > should we have some static assert on the sizes here to make sure we fit. > > My reasoning for not including static asserts was that gcc will already issue a > warning if you try to stuff an enum into a bitfield that's too small for it, > which in skia turns into an error. I can't speak for other compilers or the > future of Werror though. Do you think this is sufficient or would you rather > have the static asserts? (I'm kind of on the fence myself..) I feel uneasy relying on an arbitrary compiler warning to pick this up. So I'm leaning towards an Assert but I'm not against being convinced otherwise. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:195: static const BlendFormula gBlendTable[3][2][SkXfermode::kLastCoeffMode + 1] = { On 2015/05/20 16:11:22, Chris Dalton wrote: > On 2015/05/20 14:13:55, egdaniel wrote: > > I don't this we are gaining much at all by also branch on solid white color. > It > > seems like only 3 things change: > > modulate & screen when no coverage > > modulate when coverage > > > > Screen and modulate are just not common enough to really justify the extra > size > > on complexity added by solidWhite. Also the philosophical question of why is > > solidWhite optimized and not transparent black or other colors. > > Right I hear what you're saying. I actually did try having just coverage in the > table, and then adding a runtime optimizer to figure in the colorPOI. It ended > up being more confusing in my mind than just laying it out in a table, not to > mention more lines of code and probably a larger compiled size... > > I just had a thought though, we can do the opposite and only have a table for > (xfermode x colorPOI), and then apply coveragePOI at runtime. This will actually > work out really nicely because the coverage formulas are pretty canned, and the > one you use depends only on the coefficients. And since the coeffs will be layed > out in the table ahead of time, we can decide at compile-time which coverage > formula to use. This will also make it easier to add support for RGB coverage. So in terms of dealing with coverage here I'm fine with either table or runtime approach. If you feel like you've got a good way to do it a runtime (that you like better than table) then go for it. For color however, I really don't see much benefit in having anything besides opaque and not opaque. My mentioning earlier of trans black wasn't about we should really support it (it is so rare out in the wild). Solid white is basically identical to opaque except for screen and modulate. There are probably so little use cases of solid white in screen/modulate blend for us to actually support that optimization. As for the RGB coverage you mentioned, I would prefer that we just handle all porter duffer RGB coverage in the PDLCDXferProcessor (maybe should be PDRGBXferProcessor). The logic needed to handle that in this XP just added extra complexity to all our normal PD blending. Now if you think that there is a very clean way of doing all PD blending in on e XP and deleting PDLCD I'm fine with that as well. But anyways I think any RGB changes should be done in a different CL :) https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:197: /*>> No coverage, input color unknown: <<*/ {{ On 2015/05/20 16:11:22, Chris Dalton wrote: > On 2015/05/20 14:13:55, egdaniel wrote: > > Would this look any clearer if we reordered the table so that individual modes > > were grouped together? So for example you can quickly see how we change > src-over > > depending on coverage/color. > > This is another one of those things I was on the fence about. I can reorder it. So IMO it comes down to how many optimization groupings we have (cov, color). If it ends up being only 2 I say keep as is and group by optimizations. If it >=4 I think it is clearer to group by xfer mode. If it is 3...well by mode? maybe? https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:310: SkASSERT(!colorPOI.isSolidWhite() || colorPOI.isOpaque()); On 2015/05/20 16:11:22, Chris Dalton wrote: > On 2015/05/20 14:13:55, egdaniel wrote: > > What is this assert trying to check? This will trivial always be true since if > > !solidWhite() -> true > > solidWhite() -> isOpaque() -> true > > Right, this is verifying that we can do "colorPOI.isOpaque() + > colorPOI.isSolidWhite()" to get an index of 2 for the solid white case. Maybe > it's OCD, since the only reason it would fail is if there is a bug elsewhere? Ahhh okay I get it. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:400: output, xp.readsCoverage() ? inCoverage : "vec4(1.0)"); On 2015/05/20 16:11:22, Chris Dalton wrote: > On 2015/05/20 14:13:55, egdaniel wrote: > > can we even get into this state? how can we be setting that the xp will not > read > > coverage, but at the same time setting a coverage equation? > > Mixed samples :) > > (My motivation for this patch was actually to get Porter Duff ready for mixed > samples) > > Shader outputs 1.0, which then gets modulated by HW-computed coverage in the ROP > before it reaches the blend hardware. interesting https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:492: key |= blendFormula->fPrimaryOutputType << 5; On 2015/05/20 16:11:22, Chris Dalton wrote: > On 2015/05/20 14:13:55, egdaniel wrote: > > add some assert on max value of Output type? > > I think this already has what you are asking for? (line 489) > > GR_STATIC_ASSERT(BlendFormula::kLast_OutputType < 8); > > I'll move it to this location in the code, I think that makes more sense. oh yeah, woops https://codereview.chromium.org/1124373002/diff/140001/gm/aaxfermodes.cpp File gm/aaxfermodes.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/gm/aaxfermodes.cpp#new... gm/aaxfermodes.cpp:122: SkPackARGB32(0xff, 0xc0, 0xc0, 0xc0), we have a helper function in sk_tool_util that will make/draw a checkerboard for you. You can look gm/xfermodeimagefilter.cpp as an example of using it. https://codereview.chromium.org/1124373002/diff/140001/include/gpu/effects/Gr... File include/gpu/effects/GrPorterDuffXferProcessor.h (right): https://codereview.chromium.org/1124373002/diff/140001/include/gpu/effects/Gr... include/gpu/effects/GrPorterDuffXferProcessor.h:51: static int TestGetXPOutputSecondary(const GrXferProcessor*); On 2015/05/19 21:47:13, Chris Dalton wrote: > Not sure if there is a "more preferred" way to get specifics to the unit > tests... I'm not really a fan of making these public getters. Our general approach is to friend something in the unit test thus these can be made private https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:564: , fBlendFormula(get_unoptimized_blend_formula(xfermode)) { so what did we gain with the get_unoptimized? Seems like we will just be doing 2 copies/assignments to fBlendForumula now instead of 1. https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:847: int GrPorterDuffXPFactory::TestGetXPOutputPrimay(const GrXferProcessor* xp) { missing an r in Primay https://codereview.chromium.org/1124373002/diff/140001/tests/GrPorterDuffTest... File tests/GrPorterDuffTest.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/tests/GrPorterDuffTest... tests/GrPorterDuffTest.cpp:126: TEST_ASSERT(xpi.fInvariantOutput.fWillBlendWithDst); On 2015/05/19 22:40:34, Chris Dalton wrote: > This one is confusing. Does "will blend with dst" mean the final color output by > the blend equation is dependent on the dst? (Meaning this value should be true > for dst mode?) Or should "will blend with dst" be false for this xfer mode since > it doesn't change anything? (The previous functionality was to set > fWillBlendWithDst=true) Yeah the intent is that Dst has an effect on the final color that is produced.
https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:564: , fBlendFormula(get_unoptimized_blend_formula(xfermode)) { On 2015/05/20 18:59:42, egdaniel wrote: > so what did we gain with the get_unoptimized? Seems like we will just be doing 2 > copies/assignments to fBlendForumula now instead of 1. That's right.. BlendFormula is 32 bits, so it probably doesn't make much difference either way. Is it required to call getOptimizations though? If it's required we can leave it uninitialized until getOptimizations, but otherwise we might want to initialize it to something.
This should address all the comments except the checkerboard in the new gm. I'll address that tomorrow (or maybe today, your time :) I think I like this approach better of applying coverage at runtime. It keeps the table less complex, the coverage formulas are very clear, and it leaves a route open to do RGB coverage very cleanly (left for a different CL, of course :) I would plan to keep the PDLCDXP for platforms that don't have dual source blending though. I'll talk more about it later.) https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:42: kUsesGrPaintColor_Property = 1 << 2, On 2015/05/20 14:13:55, egdaniel wrote: > It doesn't necessarily use GrPaint's color, it just uses the color passed to it > which could come from some color processor. Does kUsesInputColor make sense? Done. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:86: OutputType fPrimaryOutputType : 3; On 2015/05/20 18:59:42, egdaniel wrote: > On 2015/05/20 16:11:22, Chris Dalton wrote: > > On 2015/05/20 14:13:55, egdaniel wrote: > > > should we have some static assert on the sizes here to make sure we fit. > > > > My reasoning for not including static asserts was that gcc will already issue > a > > warning if you try to stuff an enum into a bitfield that's too small for it, > > which in skia turns into an error. I can't speak for other compilers or the > > future of Werror though. Do you think this is sufficient or would you rather > > have the static asserts? (I'm kind of on the fence myself..) > > I feel uneasy relying on an arbitrary compiler warning to pick this up. So I'm > leaning towards an Assert but I'm not against being convinced otherwise. Done. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:195: static const BlendFormula gBlendTable[3][2][SkXfermode::kLastCoeffMode + 1] = { On 2015/05/20 18:59:42, egdaniel wrote: > On 2015/05/20 16:11:22, Chris Dalton wrote: > > On 2015/05/20 14:13:55, egdaniel wrote: > > > I don't this we are gaining much at all by also branch on solid white color. > > It > > > seems like only 3 things change: > > > modulate & screen when no coverage > > > modulate when coverage > > > > > > Screen and modulate are just not common enough to really justify the extra > > size > > > on complexity added by solidWhite. Also the philosophical question of why is > > > solidWhite optimized and not transparent black or other colors. > > > > Right I hear what you're saying. I actually did try having just coverage in > the > > table, and then adding a runtime optimizer to figure in the colorPOI. It ended > > up being more confusing in my mind than just laying it out in a table, not to > > mention more lines of code and probably a larger compiled size... > > > > I just had a thought though, we can do the opposite and only have a table for > > (xfermode x colorPOI), and then apply coveragePOI at runtime. This will > actually > > work out really nicely because the coverage formulas are pretty canned, and > the > > one you use depends only on the coefficients. And since the coeffs will be > layed > > out in the table ahead of time, we can decide at compile-time which coverage > > formula to use. This will also make it easier to add support for RGB coverage. > > So in terms of dealing with coverage here I'm fine with either table or runtime > approach. If you feel like you've got a good way to do it a runtime (that you > like better than table) then go for it. > > For color however, I really don't see much benefit in having anything besides > opaque and not opaque. My mentioning earlier of trans black wasn't about we > should really support it (it is so rare out in the wild). Solid white is > basically identical to opaque except for screen and modulate. There are probably > so little use cases of solid white in screen/modulate blend for us to actually > support that optimization. > > As for the RGB coverage you mentioned, I would prefer that we just handle all > porter duffer RGB coverage in the PDLCDXferProcessor (maybe should be > PDRGBXferProcessor). The logic needed to handle that in this XP just added extra > complexity to all our normal PD blending. Now if you think that there is a very > clean way of doing all PD blending in on e XP and deleting PDLCD I'm fine with > that as well. But anyways I think any RGB changes should be done in a different > CL :) Done. I reduced it to just opaque and not. I was also uneasy about including new tables just to optimize modulate and screen in the solid white case. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:197: /*>> No coverage, input color unknown: <<*/ {{ On 2015/05/20 18:59:41, egdaniel wrote: > On 2015/05/20 16:11:22, Chris Dalton wrote: > > On 2015/05/20 14:13:55, egdaniel wrote: > > > Would this look any clearer if we reordered the table so that individual > modes > > > were grouped together? So for example you can quickly see how we change > > src-over > > > depending on coverage/color. > > > > This is another one of those things I was on the fence about. I can reorder > it. > > So IMO it comes down to how many optimization groupings we have (cov, color). If > it ends up being only 2 I say keep as is and group by optimizations. If it >=4 I > think it is clearer to group by xfer mode. If it is 3...well by mode? maybe? Done. https://codereview.chromium.org/1124373002/diff/100001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:492: key |= blendFormula->fPrimaryOutputType << 5; On 2015/05/20 18:59:42, egdaniel wrote: > On 2015/05/20 16:11:22, Chris Dalton wrote: > > On 2015/05/20 14:13:55, egdaniel wrote: > > > add some assert on max value of Output type? > > > > I think this already has what you are asking for? (line 489) > > > > GR_STATIC_ASSERT(BlendFormula::kLast_OutputType < 8); > > > > I'll move it to this location in the code, I think that makes more sense. > > oh yeah, woops Done. https://codereview.chromium.org/1124373002/diff/140001/include/gpu/effects/Gr... File include/gpu/effects/GrPorterDuffXferProcessor.h (right): https://codereview.chromium.org/1124373002/diff/140001/include/gpu/effects/Gr... include/gpu/effects/GrPorterDuffXferProcessor.h:51: static int TestGetXPOutputSecondary(const GrXferProcessor*); On 2015/05/20 18:59:42, egdaniel wrote: > On 2015/05/19 21:47:13, Chris Dalton wrote: > > Not sure if there is a "more preferred" way to get specifics to the unit > > tests... > > I'm not really a fan of making these public getters. Our general approach is to > friend something in the unit test thus these can be made private Done. https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:847: int GrPorterDuffXPFactory::TestGetXPOutputPrimay(const GrXferProcessor* xp) { On 2015/05/20 18:59:42, egdaniel wrote: > missing an r in Primay Done. https://codereview.chromium.org/1124373002/diff/140001/tests/GrPorterDuffTest... File tests/GrPorterDuffTest.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/tests/GrPorterDuffTest... tests/GrPorterDuffTest.cpp:126: TEST_ASSERT(xpi.fInvariantOutput.fWillBlendWithDst); On 2015/05/20 18:59:42, egdaniel wrote: > On 2015/05/19 22:40:34, Chris Dalton wrote: > > This one is confusing. Does "will blend with dst" mean the final color output > by > > the blend equation is dependent on the dst? (Meaning this value should be true > > for dst mode?) Or should "will blend with dst" be false for this xfer mode > since > > it doesn't change anything? (The previous functionality was to set > > fWillBlendWithDst=true) > > Yeah the intent is that Dst has an effect on the final color that is produced. Ok that's kinda what I figured. Thanks for clarifying.
So part of me kind of liked the version where coverage was in the table as well. The reasoning being that, one of the issues in the current world is we have these optimizations separated across a few functions so it wasn't really easy to tell where a given mode would end up (in terms of coeffs, equations, etc.) which just stepping through and following all the calls. Everything in a table really cleans this up and it is quick and simple to see what is happening. With coverage pulled out and implemented in a different way (coverage strat comes from table but it isn't necessarily used, and then we actually apply coverage through a different function call) it once again becomes a lot harder to follow what is happening and when we apply various things. This is also my worry as what will happen when adding RGB into the normal PD. Just my thoughts. https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:564: , fBlendFormula(get_unoptimized_blend_formula(xfermode)) { On 2015/05/21 04:00:36, Chris Dalton wrote: > On 2015/05/20 18:59:42, egdaniel wrote: > > so what did we gain with the get_unoptimized? Seems like we will just be doing > 2 > > copies/assignments to fBlendForumula now instead of 1. > > That's right.. BlendFormula is 32 bits, so it probably doesn't make much > difference either way. > > Is it required to call getOptimizations though? If it's required we can leave it > uninitialized until getOptimizations, but otherwise we might want to initialize > it to something. Okay yeah I think you were right before. Our current contract with getOptimizations is that a client doesn't have to call it, but if they do, they must honor the flags we return. So we will need to initialize the value of fBlendFormula with some reasonable value. https://codereview.chromium.org/1124373002/diff/160001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/160001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:39: enum CoverageStrategy { comments on what these strategies mean? https://codereview.chromium.org/1124373002/diff/160001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:158: #define DST_CLEAR_FORMULA COEFF_FORMULA(kZero_GrBlendCoeff, kZero_GrBlendCoeff) I don't really care one way or the other, but do you think these two helper macros are adding clarity at this point or just there
On 2015/05/21 18:36:45, egdaniel wrote: > So part of me kind of liked the version where coverage was in the table as well. > The reasoning being that, one of the issues in the current world is we have > these optimizations separated across a few functions so it wasn't really easy to > tell where a given mode would end up (in terms of coeffs, equations, etc.) which > just stepping through and following all the calls. Everything in a table really > cleans this up and it is quick and simple to see what is happening. > > With coverage pulled out and implemented in a different way (coverage strat > comes from table but it isn't necessarily used, and then we actually apply > coverage through a different function call) it once again becomes a lot harder > to follow what is happening and when we apply various things. This is also my > worry as what will happen when adding RGB into the normal PD. Just my thoughts. Yeah I agree with you. I liked it better in the table too. I'll put together one more where the table has 4 different sets of formulas (with/without coverage) x (opaque/unknown color). Do you still have a preference on how to order the table? I'd probably still do it with the xfermode on the inner-most node, but it's not a strong preference and I'm open to grouping the formulas for the POI's side-by-side for a single xfermode.
On 2015/05/21 19:03:12, Chris Dalton wrote: > > Yeah I agree with you. I liked it better in the table too. I'll put together one > more where the table has 4 different sets of formulas (with/without coverage) x > (opaque/unknown color). Do you still have a preference on how to order the > table? I'd probably still do it with the xfermode on the inner-most node, but > it's not a strong preference and I'm open to grouping the formulas for the POI's > side-by-side for a single xfermode. I really don't have a strong opinion on the order. I know while reviewing I found myself going mode by mode and checking all POI's per mode, and thus it would have been nice having them grouped. But I don't think that one way or the other is obviously superior.
https://codereview.chromium.org/1124373002/diff/160001/src/gpu/GrBlend.h File src/gpu/GrBlend.h (right): https://codereview.chromium.org/1124373002/diff/160001/src/gpu/GrBlend.h#newc... src/gpu/GrBlend.h:93: General cleanup thought...do we need both the inline functions and the macros? Can we get rid of one? Also I know you added all the combinations of these calls that might be useful, but for now if something is not being used we might as well delete it. It will only take a second to copy/paste a new one if we find use later on.
https://codereview.chromium.org/1124373002/diff/140001/gm/aaxfermodes.cpp File gm/aaxfermodes.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/gm/aaxfermodes.cpp#new... gm/aaxfermodes.cpp:122: SkPackARGB32(0xff, 0xc0, 0xc0, 0xc0), On 2015/05/20 18:59:42, egdaniel wrote: > we have a helper function in sk_tool_util that will make/draw a checkerboard for > you. You can look gm/xfermodeimagefilter.cpp as an example of using it. Done. https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/140001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:564: , fBlendFormula(get_unoptimized_blend_formula(xfermode)) { On 2015/05/21 18:36:45, egdaniel wrote: > On 2015/05/21 04:00:36, Chris Dalton wrote: > > On 2015/05/20 18:59:42, egdaniel wrote: > > > so what did we gain with the get_unoptimized? Seems like we will just be > doing > > 2 > > > copies/assignments to fBlendForumula now instead of 1. > > > > That's right.. BlendFormula is 32 bits, so it probably doesn't make much > > difference either way. > > > > Is it required to call getOptimizations though? If it's required we can leave > it > > uninitialized until getOptimizations, but otherwise we might want to > initialize > > it to something. > > Okay yeah I think you were right before. Our current contract with > getOptimizations is that a client doesn't have to call it, but if they do, they > must honor the flags we return. So we will need to initialize the value of > fBlendFormula with some reasonable value. Done. Went with the coverage by default. It's slower but always correct. If the caller wants a faster formula they have to call getOptimizattions. https://codereview.chromium.org/1124373002/diff/160001/src/gpu/GrBlend.h File src/gpu/GrBlend.h (right): https://codereview.chromium.org/1124373002/diff/160001/src/gpu/GrBlend.h#newc... src/gpu/GrBlend.h:93: On 2015/05/21 19:35:39, egdaniel wrote: > General cleanup thought...do we need both the inline functions and the macros? > Can we get rid of one? Also I know you added all the combinations of these calls > that might be useful, but for now if something is not being used we might as > well delete it. It will only take a second to copy/paste a new one if we find > use later on. Done. https://codereview.chromium.org/1124373002/diff/160001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/160001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:39: enum CoverageStrategy { On 2015/05/21 18:36:45, egdaniel wrote: > comments on what these strategies mean? Done. (Code removed) https://codereview.chromium.org/1124373002/diff/160001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:158: #define DST_CLEAR_FORMULA COEFF_FORMULA(kZero_GrBlendCoeff, kZero_GrBlendCoeff) On 2015/05/21 18:36:45, egdaniel wrote: > I don't really care one way or the other, but do you think these two helper > macros are adding clarity at this point or just there Done. (They have a purpose again now that we've gone back to the table)
LGTM. Just need Brian's as well.
lgtm, really like this patch.
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124373002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
clang just had a trivial warning, but msvc had a pretty serious semantic issue... https://codereview.chromium.org/1124373002/diff/200001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/200001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:100: // where it will sign-extend them if the last bit is set. This appears in violation of the C++03 9.6/4 standard: If the value of an enu- merator is stored into a bit-field of the same enumeration type and the number of bits in the bit-field is large enough to hold all the values of that enumeration type, the original enumerator value and the value of the bit-field shall compare equal. (http://stackoverflow.com/questions/11983231/is-it-safe-to-use-an-enum-in-a-bi...) https://codereview.chromium.org/1124373002/diff/200001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:114: GR_STATIC_ASSERT(kLast_Property < (1 << 6)); Oops, I meant to add these before but they got lost somewhere along the way.
The CQ bit was checked by cdalton@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1124373002/#ps220001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124373002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://skia.googlesource.com/skia/+/9a70920db22b6309c671f8e5d519bb95570e4414
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1153993002/ by bungeman@google.com. The reason for reverting is: Blocking DEPS roll into Chromium. Crashing virtual/gpu/fast/canvas/canvas-composite-*.html tests with the assert ../../third_party/skia/src/gpu/gl/builders/GrGLFragmentShaderBuilder.cpp:281: failed assertion "k110_GrGLSLGeneration != gpu->glslGeneration() || fOutputs.empty()" .
Ugh, that last one had a couple simple bugs. Changes since last time: * Take the LCD XP into account in the factory functions (getInvariantOutput, willReadDstColor) * Always return false from hasSecondaryOutput if we are doing an in-shader blend with a dst read (fixes the virtual/gpu/fast/canvas layout test crashes) * Initialize the XP to the non-coverage formula before getOptimizations (otherwise we might assign a dual source blending formula on a platform that doesn't have it. This was the original behavior, but I believe it's incorrect if the client doesn't call getOptimizations and then issues a draw that has coverage. I have a couple upcoming mixed samples patches that I think help clean it up) * Reorder the blend table so it starts with the non-coverage formulas * Add tests for LCD coverage and non-dual-source-blending * Draw quadratic paths instead of ellipses for aaxfermodes (more likely to use nvpr)
The patch keeps getting bigger, but I feel like this is a better way of handling the dst read fallbacks with blend formulas.
On 2015/05/26 10:10:55, Chris Dalton wrote: > The patch keeps getting bigger, but I feel like this is a better way of handling > the dst read fallbacks with blend formulas. (By "better way of handling the dst read fallbacks", I meant better way of fixing the bug this patch previously introduced with the layout tests.)
https://codereview.chromium.org/1124373002/diff/260001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/260001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:455: class PDFallbackXferProcessor : public GrXferProcessor { Can we use a term that's a little more specific than Fallback? ShaderPDXferProcessor? https://codereview.chromium.org/1124373002/diff/260001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:494: bool append_porterduff_term(GrGLXPFragmentBuilder* fsBuilder, SkXfermode::Coeff coeff, static? https://codereview.chromium.org/1124373002/diff/260001/tests/GrPorterDuffTest... File tests/GrPorterDuffTest.cpp (right): https://codereview.chromium.org/1124373002/diff/260001/tests/GrPorterDuffTest... tests/GrPorterDuffTest.cpp:1294: const GrCaps& nilCaps = GrGLCaps(); Is your tree up to date? I refactored caps creation recently. I don't think this will compile.
https://codereview.chromium.org/1124373002/diff/260001/gm/aaxfermodes.cpp File gm/aaxfermodes.cpp (right): https://codereview.chromium.org/1124373002/diff/260001/gm/aaxfermodes.cpp#new... gm/aaxfermodes.cpp:46: class AAXfermodesGM : public GM { So for this GM I'm think we can take off the solid white group since that is no longer a special case we have. And with the extra room I would suggest adding a column of mode labels going down the left side of the gm. https://codereview.chromium.org/1124373002/diff/260001/gm/aaxfermodes.cpp#new... gm/aaxfermodes.cpp:146: // Check for overflow and dim the src and dst colors if we need to, otherwise we might get 100 chars
https://codereview.chromium.org/1124373002/diff/260001/gm/aaxfermodes.cpp File gm/aaxfermodes.cpp (right): https://codereview.chromium.org/1124373002/diff/260001/gm/aaxfermodes.cpp#new... gm/aaxfermodes.cpp:46: class AAXfermodesGM : public GM { On 2015/05/26 14:15:22, egdaniel wrote: > So for this GM I'm think we can take off the solid white group since that is no > longer a special case we have. And with the extra room I would suggest adding a > column of mode labels going down the left side of the gm. Done. https://codereview.chromium.org/1124373002/diff/260001/gm/aaxfermodes.cpp#new... gm/aaxfermodes.cpp:146: // Check for overflow and dim the src and dst colors if we need to, otherwise we might get On 2015/05/26 14:15:22, egdaniel wrote: > 100 chars Done. https://codereview.chromium.org/1124373002/diff/260001/src/gpu/effects/GrPort... File src/gpu/effects/GrPorterDuffXferProcessor.cpp (right): https://codereview.chromium.org/1124373002/diff/260001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:455: class PDFallbackXferProcessor : public GrXferProcessor { On 2015/05/26 13:19:51, bsalomon wrote: > Can we use a term that's a little more specific than Fallback? > ShaderPDXferProcessor? Done. https://codereview.chromium.org/1124373002/diff/260001/src/gpu/effects/GrPort... src/gpu/effects/GrPorterDuffXferProcessor.cpp:494: bool append_porterduff_term(GrGLXPFragmentBuilder* fsBuilder, SkXfermode::Coeff coeff, On 2015/05/26 13:19:51, bsalomon wrote: > static? Done. Good catch, also made append_color_output static. (BTW, this change isn't as complex as it might look like, I didn't write this function, I just relocated it.) https://codereview.chromium.org/1124373002/diff/260001/tests/GrPorterDuffTest... File tests/GrPorterDuffTest.cpp (right): https://codereview.chromium.org/1124373002/diff/260001/tests/GrPorterDuffTest... tests/GrPorterDuffTest.cpp:1294: const GrCaps& nilCaps = GrGLCaps(); On 2015/05/26 13:19:51, bsalomon wrote: > Is your tree up to date? I refactored caps creation recently. I don't think this > will compile. Waiting for the upcoming change (tomorrow, hopefully?) that will make it possible to create a neutered GrCaps.
This is ready for final review https://codereview.chromium.org/1124373002/diff/260001/tests/GrPorterDuffTest... File tests/GrPorterDuffTest.cpp (right): https://codereview.chromium.org/1124373002/diff/260001/tests/GrPorterDuffTest... tests/GrPorterDuffTest.cpp:1294: const GrCaps& nilCaps = GrGLCaps(); On 2015/05/26 22:10:28, Chris Dalton wrote: > On 2015/05/26 13:19:51, bsalomon wrote: > > Is your tree up to date? I refactored caps creation recently. I don't think > this > > will compile. > > Waiting for the upcoming change (tomorrow, hopefully?) that will make it > possible to create a neutered GrCaps. Done.
lgtm https://codereview.chromium.org/1124373002/diff/300001/tests/GrPorterDuffTest... File tests/GrPorterDuffTest.cpp (right): https://codereview.chromium.org/1124373002/diff/300001/tests/GrPorterDuffTest... tests/GrPorterDuffTest.cpp:951: GrGpu* gpu = ctx->getGpu(); I'm trying to remove getGpu() from GrContext and am slowly overtaking the addition of more callers. Can we just kill this SkFail()?
https://codereview.chromium.org/1124373002/diff/300001/tests/GrPorterDuffTest... File tests/GrPorterDuffTest.cpp (right): https://codereview.chromium.org/1124373002/diff/300001/tests/GrPorterDuffTest... tests/GrPorterDuffTest.cpp:951: GrGpu* gpu = ctx->getGpu(); On 2015/05/27 20:46:20, bsalomon wrote: > I'm trying to remove getGpu() from GrContext and am slowly overtaking the > addition of more callers. Can we just kill this SkFail()? The only place it calls getGpu() now is to access the caps. So once we have a getter for the GrCaps on GrContext we can completely remove getGpu() from the test.
https://codereview.chromium.org/1124373002/diff/300001/tests/GrPorterDuffTest... File tests/GrPorterDuffTest.cpp (right): https://codereview.chromium.org/1124373002/diff/300001/tests/GrPorterDuffTest... tests/GrPorterDuffTest.cpp:951: GrGpu* gpu = ctx->getGpu(); On 2015/05/27 21:09:41, Chris Dalton wrote: > On 2015/05/27 20:46:20, bsalomon wrote: > > I'm trying to remove getGpu() from GrContext and am slowly overtaking the > > addition of more callers. Can we just kill this SkFail()? > > The only place it calls getGpu() now is to access the caps. So once we have a > getter for the GrCaps on GrContext we can completely remove getGpu() from the > test. ok I'll updcate it at that time. thanks.
The CQ bit was checked by cdalton@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1124373002/#ps320001 (title: "Use resource provider instead of gpu for tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124373002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://skia.googlesource.com/skia/+/6fd158ea47472c4d038e48980a95e36623f840c9 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
