Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
712223002:
Combine similar DrawPaths calls in GrInOrderDrawBuffer (Closed)
|
Created:
6 years, 1 month ago by Chris Dalton Modified:
6 years, 1 month ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionCombine similar DrawPaths calls in GrInOrderDrawBuffer
Combines adjacent DrawPaths commands into one single call when a
conservative set of conditions are met. The end result is that whole
paragraphs can be drawn with a single call to
gliStencilThenCoverFillPathInstancedNV(), rather than one call for
each line.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/3fc6a2fdb260aa85ad724d575afede2c00db111b
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 7
Patch Set 4 : SkTDArray, willEffectReadDstColor() #
Total comments: 1
Patch Set 5 : Use willBlendWithDst #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 20 (3 generated)
cdalton@nvidia.com changed reviewers: + bsalomon@google.com, kkinnunen@nvidia.com, markkilgard@gmail.com
https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrDrawState.h File src/gpu/GrDrawState.h (right): https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrDrawState.h#ne... src/gpu/GrDrawState.h:148: bool srcAlphaWillBeOne() const; Move here to make public
bsalomon@google.com changed reviewers: + egdaniel@google.com
https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:375: draw_state_is_opaque(this->getDrawState())) { Could/should you check that the b.boxes don't overlap || the draw is opaque? https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.h (right): https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:318: template<typename T> class DataBuffer : SkNoncopyable { Could you use SkTDArray here instead of a new type?
https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:375: draw_state_is_opaque(this->getDrawState())) { On 2014/11/11 14:41:25, bsalomon wrote: > Could/should you check that the b.boxes don't overlap || the draw is opaque? In theory, yes. But right now there isn't a way of knowing this, because GrPathRange doesn't provide or even know the bboxes for individual paths. I think it's a good optimization though. Is it something we can punt on for now but keep in mind for the future? https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.h (right): https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:318: template<typename T> class DataBuffer : SkNoncopyable { On 2014/11/11 14:41:25, bsalomon wrote: > Could you use SkTDArray here instead of a new type? I did look into this. * SkTDArray doesn't grow exponentially, and only barely grows enough to squeeze in the new data when its out of room for an append. So unless it has enough memory preallocated ahead of time, it has to do a realloc on every onDrawPaths call. * SkTArray does grow exponentially, but can't reset the count to 0 without deleting its memory, so we can't hold on to the already-allocated memory for future draws. * Neither SkTDArray nor SkTArray (if it had the ability to not delete memory after reset) have built-in logic for reducing the amount of memory reserved once we start using less. SkTDArray does give enough control that we could run the logic ourselves within GrInOrderDrawBuffer::reset(), but that takes about the same amount of extra code as this class. * Would it be possible to expose a bit of new functionality on SkTArray? I think we could use it if it let us change fReserveCount on the fly. * Otherwise, this class is pretty minimal. It's mostly just a wrapper for SkAutoTMalloc plus logic to try and adapt the amount of reserved memory to the amount being required during each draw.
> * SkTDArray doesn't grow exponentially, and only barely grows enough to squeeze > in the new data when its out of room for an append. So unless it has enough > memory preallocated ahead of time, it has to do a realloc on every onDrawPaths > call. SkTDArray does grow exponentially, by approximately 1.25x. That's these two lines: fReserve = count + 4; fReserve += fReserve / 4; > * Neither SkTDArray nor SkTArray (if it had the ability to not delete memory > after reset) have built-in logic for reducing the amount of memory reserved once > we start using less. SkTDArray does give enough control that we could run the > logic ourselves within GrInOrderDrawBuffer::reset(), but that takes about the > same amount of extra code as this class. Neither shrinks on its own, correct, but SkTDArray has shrinkToFit().
https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:119: static bool draw_state_is_opaque(const GrDrawState& drawState) { I don't believe this function fully captures what your trying to do. For example there is no check to make sure there is no coverage, or if we are reading the Dst and blending in fragment code. We will add a call on GrDrawState, isUnblended(), that should capture the functionality that you are looking for here.
On 2014/11/11 17:49:07, Chris Dalton wrote: > https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... > File src/gpu/GrInOrderDrawBuffer.cpp (right): > > https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... > src/gpu/GrInOrderDrawBuffer.cpp:375: draw_state_is_opaque(this->getDrawState())) > { > On 2014/11/11 14:41:25, bsalomon wrote: > > Could/should you check that the b.boxes don't overlap || the draw is opaque? > > In theory, yes. But right now there isn't a way of knowing this, because > GrPathRange doesn't provide or even know the bboxes for individual paths. I > think it's a good optimization though. Is it something we can punt on for now > but keep in mind for the future? > > https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... > File src/gpu/GrInOrderDrawBuffer.h (right): > > https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... > src/gpu/GrInOrderDrawBuffer.h:318: template<typename T> class DataBuffer : > SkNoncopyable { > On 2014/11/11 14:41:25, bsalomon wrote: > > Could you use SkTDArray here instead of a new type? > > > * Would it be possible to expose a bit of new functionality on SkTArray? I think > we could use it if it let us change fReserveCount on the fly. That seems ok with me, but if you go that route be aware that in the SkSTArray case there is the fPreAllocMemArray complication.
Ok, I missed that second "fReserve += fReserve / 4;" line. I'll switch to SkTDArray. https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/712223002/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:119: static bool draw_state_is_opaque(const GrDrawState& drawState) { On 2014/11/11 18:39:07, egdaniel wrote: > I don't believe this function fully captures what your trying to do. For example > there is no check to make sure there is no coverage, or if we are reading the > Dst and blending in fragment code. We will add a call on GrDrawState, > isUnblended(), that should capture the functionality that you are looking for > here. Ok, so while we wait for isUnblended can I capture all the corner cases from this file? For example by ensuring dstCopy is null in onDrawPaths? I'm a bit hazy the concept of coverage, but I believe it wouldn't be used with nvpr, right?
https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:127: return drawState.willEffectReadDstColor(); My understanding is that this should work. srcAlphaWillBeOne() takes coverage into account, and willEffectReadDstColor() should cover the case where a shader reads the dst directly for blending. But it would be great if I can get a confirmation.
On 2014/11/11 22:34:40, Chris Dalton wrote: > https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... > File src/gpu/GrInOrderDrawBuffer.cpp (right): > > https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... > src/gpu/GrInOrderDrawBuffer.cpp:127: return drawState.willEffectReadDstColor(); > My understanding is that this should work. srcAlphaWillBeOne() takes coverage > into account, and willEffectReadDstColor() should cover the case where a shader > reads the dst directly for blending. > > But it would be great if I can get a confirmation. You are on the right general track but there are some issues that are missed. First you also need to take into account the srcCoeff to make sure it is one. Next srcAlphaIsOne only takes coverage into account if we are coverage drawing which means we are putting the coverage stages directly into the color pipeline. What we really want to check is that the output of the coverage stages is solid. I've just landed this cl https://codereview.chromium.org/715873002/ with the function willBlendWithDst() that should cover your use case.
On 2014/11/11 23:03:50, egdaniel wrote: > On 2014/11/11 22:34:40, Chris Dalton wrote: > > > https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... > > File src/gpu/GrInOrderDrawBuffer.cpp (right): > > > > > https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... > > src/gpu/GrInOrderDrawBuffer.cpp:127: return > drawState.willEffectReadDstColor(); > > My understanding is that this should work. srcAlphaWillBeOne() takes coverage > > into account, and willEffectReadDstColor() should cover the case where a > shader > > reads the dst directly for blending. > > > > But it would be great if I can get a confirmation. > > You are on the right general track but there are some issues that are missed. > First you also need to take into account the srcCoeff to make sure it is one. > Next srcAlphaIsOne only takes coverage into account if we are coverage drawing > which means we are putting the coverage stages directly into the color pipeline. > What we really want to check is that the output of the coverage stages is solid. > > I've just landed this cl https://codereview.chromium.org/715873002/ with the > function willBlendWithDst() that should cover your use case. Wow! That was fast! I'll use your new query. I do have one question still. You mentioned verifying that the src coefficient is one, and I see that in the new patch, but I'm wondering if it's good enough just to make sure the src coefficient doesn't use dst alpha? In other words, "I don't care what you multiply the src color by, as long at it doesn't use the dst."
On 2014/11/11 23:09:47, Chris Dalton wrote: > On 2014/11/11 23:03:50, egdaniel wrote: > > On 2014/11/11 22:34:40, Chris Dalton wrote: > > > > > > https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... > > > File src/gpu/GrInOrderDrawBuffer.cpp (right): > > > > > > > > > https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... > > > src/gpu/GrInOrderDrawBuffer.cpp:127: return > > drawState.willEffectReadDstColor(); > > > My understanding is that this should work. srcAlphaWillBeOne() takes > coverage > > > into account, and willEffectReadDstColor() should cover the case where a > > shader > > > reads the dst directly for blending. > > > > > > But it would be great if I can get a confirmation. > > > > You are on the right general track but there are some issues that are missed. > > First you also need to take into account the srcCoeff to make sure it is one. > > Next srcAlphaIsOne only takes coverage into account if we are coverage drawing > > which means we are putting the coverage stages directly into the color > pipeline. > > What we really want to check is that the output of the coverage stages is > solid. > > > > I've just landed this cl https://codereview.chromium.org/715873002/ with the > > function willBlendWithDst() that should cover your use case. > > Wow! That was fast! I'll use your new query. > > I do have one question still. You mentioned verifying that the src coefficient > is one, and I see that in the new patch, but I'm wondering if it's good enough > just to make sure the src coefficient doesn't use dst alpha? In other words, "I > don't care what you multiply the src color by, as long at it doesn't use the > dst." Well we definitely do have to make sure the src coeff is not one of the cases that looks at the dst (dst, 1 -dst, 1-dstA, etc). I suppose there are other blend coeffs, like constant, that maybe would work here, but they really would never appear in practice especially with dstCoeff also being zero. In general we use the coeff pair [1,0] as a sign we are not do blending (assuming also the read dst checks etc.). So in maybe you could argue this check is more restrictive but it will be correct 99.999% of use cases.
On 2014/11/11 23:38:05, egdaniel wrote: > On 2014/11/11 23:09:47, Chris Dalton wrote: > > On 2014/11/11 23:03:50, egdaniel wrote: > > > On 2014/11/11 22:34:40, Chris Dalton wrote: > > > > > > > > > > https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... > > > > File src/gpu/GrInOrderDrawBuffer.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/712223002/diff/60001/src/gpu/GrInOrderDrawBuf... > > > > src/gpu/GrInOrderDrawBuffer.cpp:127: return > > > drawState.willEffectReadDstColor(); > > > > My understanding is that this should work. srcAlphaWillBeOne() takes > > coverage > > > > into account, and willEffectReadDstColor() should cover the case where a > > > shader > > > > reads the dst directly for blending. > > > > > > > > But it would be great if I can get a confirmation. > > > > > > You are on the right general track but there are some issues that are > missed. > > > First you also need to take into account the srcCoeff to make sure it is > one. > > > Next srcAlphaIsOne only takes coverage into account if we are coverage > drawing > > > which means we are putting the coverage stages directly into the color > > pipeline. > > > What we really want to check is that the output of the coverage stages is > > solid. > > > > > > I've just landed this cl https://codereview.chromium.org/715873002/ with the > > > function willBlendWithDst() that should cover your use case. > > > > Wow! That was fast! I'll use your new query. > > > > I do have one question still. You mentioned verifying that the src coefficient > > is one, and I see that in the new patch, but I'm wondering if it's good enough > > just to make sure the src coefficient doesn't use dst alpha? In other words, > "I > > don't care what you multiply the src color by, as long at it doesn't use the > > dst." > > Well we definitely do have to make sure the src coeff is not one of the cases > that looks at the dst (dst, 1 -dst, 1-dstA, etc). I suppose there are other > blend coeffs, like constant, that maybe would work here, but they really would > never appear in practice especially with dstCoeff also being zero. In general we > use the coeff pair [1,0] as a sign we are not do blending (assuming also the > read dst checks etc.). So in maybe you could argue this check is more > restrictive but it will be correct 99.999% of use cases. Yes, it definitely does everything we need. Thanks for getting that in so fast. The latest patchset to this change uses willBlendWithDst().
https://codereview.chromium.org/712223002/diff/80001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.h (right): https://codereview.chromium.org/712223002/diff/80001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:58: GrGpu* dstGpu() const { return fDstGpu; } Does this need to be public? Otherwise looks good.
https://codereview.chromium.org/712223002/diff/80001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.h (right): https://codereview.chromium.org/712223002/diff/80001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:58: GrGpu* dstGpu() const { return fDstGpu; } On 2014/11/13 17:36:05, bsalomon wrote: > Does this need to be public? Otherwise looks good. Done.
lgtm
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/712223002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/3fc6a2fdb260aa85ad724d575afede2c00db111b |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
