Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2062703003:
Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader (Closed)
DescriptionAbstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader
Will not run until after landing https://codereview.chromium.org/2106893003/
This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003
Committed: https://skia.googlesource.com/skia/+/3f58cd0eb8bf4499c4f179aa6111d7e005809df8
Committed: https://skia.googlesource.com/skia/+/e6347ad7459a52f42241fd5c48638725d9f11b40
Patch Set 1 #Patch Set 2 : Deleted out-of-date comments #Patch Set 3 : Small style fix #Patch Set 4 : Got rid of VLAs #Patch Set 5 : rebase #Patch Set 6 : Rebased, now allocates LightingShaderContext deps on the heap #
Total comments: 1
Patch Set 7 : rebase #
Total comments: 3
Patch Set 8 : Rebase #Patch Set 9 : diffuseFP is now in series with lightingFP #Patch Set 10 : rebase #
Total comments: 4
Patch Set 11 : Changed to SkAssertResult to avoid Release warning #Patch Set 12 : Nullptr returns now propagate through fragment processor creation chain #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 40 (17 generated)
Description was changed from ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader BUG=skia: ========== to ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ==========
Description was changed from ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ========== to ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ==========
dvonbeck@google.com changed reviewers: + egdaniel@google.com
https://codereview.chromium.org/2062703003/diff/100001/src/core/SkLightingSha... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2062703003/diff/100001/src/core/SkLightingSha... src/core/SkLightingShader.cpp:461: void* heapAllocated = malloc(heapRequired); // Freed on ~LightingShaderContext() malloc and free. Should I be using some SkMalloc or something?
rebase
egdaniel@google.com changed reviewers: + robertphillips@google.com
+rob so he can start looking at changes https://codereview.chromium.org/2062703003/diff/120001/src/core/SkLightingSha... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2062703003/diff/120001/src/core/SkLightingSha... src/core/SkLightingShader.cpp:276: new LightingFP(std::move(diffuseFP), std::move(normalFP), fLights)); Should diffuseFP actually be a child of LightingFP? Seems like we could us eGrFragmentProcessor::RunInSeries(...) here since the diffuse color is simply the input color into the lighting shader. The actual code generated will be the same, but it also means we can use the same LightingFP code for the case of having a diffuseShader or just using the paint.
https://codereview.chromium.org/2062703003/diff/120001/src/core/SkLightingSha... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2062703003/diff/120001/src/core/SkLightingSha... src/core/SkLightingShader.cpp:276: new LightingFP(std::move(diffuseFP), std::move(normalFP), fLights)); On 2016/06/27 13:55:39, egdaniel wrote: > Should diffuseFP actually be a child of LightingFP? Seems like we could us > eGrFragmentProcessor::RunInSeries(...) here since the diffuse color is simply > the input color into the lighting shader. The actual code generated will be the > same, but it also means we can use the same LightingFP code for the case of > having a diffuseShader or just using the paint. That makes a lot of sense. I'll refactor it that way.
Description was changed from ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ========== to ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader DO NOT LAND. Will not run until Issue 2106893003 has landed. This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ==========
Description was changed from ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader DO NOT LAND. Will not run until Issue 2106893003 has landed. This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ========== to ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader DO NOT LAND. Will not run until after landing https://codereview.chromium.org/2106893003/ This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ==========
https://codereview.chromium.org/2062703003/diff/120001/src/core/SkLightingSha... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2062703003/diff/120001/src/core/SkLightingSha... src/core/SkLightingShader.cpp:276: new LightingFP(std::move(diffuseFP), std::move(normalFP), fLights)); On 2016/06/27 15:15:45, dvonbeck wrote: > On 2016/06/27 13:55:39, egdaniel wrote: > > Should diffuseFP actually be a child of LightingFP? Seems like we could us > > eGrFragmentProcessor::RunInSeries(...) here since the diffuse color is simply > > the input color into the lighting shader. The actual code generated will be > the > > same, but it also means we can use the same LightingFP code for the case of > > having a diffuseShader or just using the paint. > > That makes a lot of sense. I'll refactor it that way. Done.
lgtm
Description was changed from ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader DO NOT LAND. Will not run until after landing https://codereview.chromium.org/2106893003/ This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ========== to ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader Will not run until after landing https://codereview.chromium.org/2106893003/ This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ==========
The CQ bit was checked by dvonbeck@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
https://codereview.chromium.org/2062703003/diff/180001/src/core/SkLightingSha... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2062703003/diff/180001/src/core/SkLightingSha... src/core/SkLightingShader.cpp:397: SkASSERT(!hasLocalMatrix); CQ's release bots are failing to compile here because since SkASSERT is defined as empty, hasLocalMatrix goes unused. Should I do SkASSERT_RELEASE instead? If the buffer has a local matrix encoded, execution is bound to fail somewhere in the function anyway. I guess I could also do SkASSERT(buf.readBool()) (As a side note, I feel like the unflattening code should have a way of propagating the read buffer to the superclasses so they can handle their members themselves. That's just an opinion though.)
https://codereview.chromium.org/2062703003/diff/180001/src/core/SkLightingSha... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2062703003/diff/180001/src/core/SkLightingSha... src/core/SkLightingShader.cpp:397: SkASSERT(!hasLocalMatrix); On 2016/07/06 21:21:02, dvonbeck wrote: > CQ's release bots are failing to compile here because since SkASSERT is defined > as empty, hasLocalMatrix goes unused. Should I do SkASSERT_RELEASE instead? If > the buffer has a local matrix encoded, execution is bound to fail somewhere in > the function anyway. I guess I could also do SkASSERT(buf.readBool()) > > (As a side note, I feel like the unflattening code should have a way of > propagating the read buffer to the superclasses so they can handle their members > themselves. That's just an opinion though.) P.S.: Just realized SkASSERT(buf.readBool()) would result in incorrect behavior, so that's out of the table.
https://codereview.chromium.org/2062703003/diff/180001/src/core/SkLightingSha... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2062703003/diff/180001/src/core/SkLightingSha... src/core/SkLightingShader.cpp:397: SkASSERT(!hasLocalMatrix); On 2016/07/06 21:22:35, dvonbeck wrote: > On 2016/07/06 21:21:02, dvonbeck wrote: > > CQ's release bots are failing to compile here because since SkASSERT is > defined > > as empty, hasLocalMatrix goes unused. Should I do SkASSERT_RELEASE instead? If > > the buffer has a local matrix encoded, execution is bound to fail somewhere in > > the function anyway. I guess I could also do SkASSERT(buf.readBool()) > > > > (As a side note, I feel like the unflattening code should have a way of > > propagating the read buffer to the superclasses so they can handle their > members > > themselves. That's just an opinion though.) > > P.S.: Just realized SkASSERT(buf.readBool()) would result in incorrect behavior, > so that's out of the table. SkAssertResult ?
https://codereview.chromium.org/2062703003/diff/180001/src/core/SkLightingSha... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2062703003/diff/180001/src/core/SkLightingSha... src/core/SkLightingShader.cpp:397: SkASSERT(!hasLocalMatrix); On 2016/07/06 21:56:43, robertphillips wrote: > On 2016/07/06 21:22:35, dvonbeck wrote: > > On 2016/07/06 21:21:02, dvonbeck wrote: > > > CQ's release bots are failing to compile here because since SkASSERT is > > defined > > > as empty, hasLocalMatrix goes unused. Should I do SkASSERT_RELEASE instead? > If > > > the buffer has a local matrix encoded, execution is bound to fail somewhere > in > > > the function anyway. I guess I could also do SkASSERT(buf.readBool()) > > > > > > (As a side note, I feel like the unflattening code should have a way of > > > propagating the read buffer to the superclasses so they can handle their > > members > > > themselves. That's just an opinion though.) > > > > P.S.: Just realized SkASSERT(buf.readBool()) would result in incorrect > behavior, > > so that's out of the table. > > SkAssertResult ? Done.
The CQ bit was checked by dvonbeck@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2062703003/#ps200001 (title: "Changed to SkAssertResult to avoid Release warning")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader Will not run until after landing https://codereview.chromium.org/2106893003/ This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 ========== to ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader Will not run until after landing https://codereview.chromium.org/2106893003/ This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 Committed: https://skia.googlesource.com/skia/+/3f58cd0eb8bf4499c4f179aa6111d7e005809df8 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://skia.googlesource.com/skia/+/3f58cd0eb8bf4499c4f179aa6111d7e005809df8
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2137513002/ by rmistry@google.com. The reason for reverting is: Causing the Test-Ubuntu-GCC-ShuttleA-GPU-GTX550Ti-x86_64-Release-Valgrind builder to fail. Started failing at this build: https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-S... Snippet from build log: ==3016== Invalid read of size 4 ==3016== at 0xCD1901: GrFragmentProcessor::registerChildProcessor(sk_sp<GrFragmentProcessor>) (SkTArray.h:163) ==3016== by 0xA58B47: _Z10sk_make_spI11NormalMapFPI5sk_spI19GrFragmentProcessorERK8SkMatrixEES1_IT_EDpOT0_ (SkNormalSource.cpp:84) ==3016== by 0xA582E8: NormalMapSourceImpl::asFragmentProcessor(GrContext*, SkMatrix const&, SkMatrix const*, SkFilterQuality, SkSourceGammaTreatment) const (SkNormalSource.cpp:187) ==3016== by 0xA0ADB5: SkLightingShaderImpl::asFragmentProcessor(GrContext*, SkMatrix const&, SkMatrix const*, SkFilterQuality, SkSourceGammaTreatment) const (SkLightingShader.cpp:268) ==3016== by 0xDA17E6: skpaint_to_grpaint_impl(GrContext*, SkPaint const&, SkMatrix const&, sk_sp<GrFragmentProcessor>*, SkXfermode::Mode*, bool, bool, GrPaint*) (SkGr.cpp:552) ==3016== by 0xDA4532: SkPaintToGrPaint(GrContext*, SkPaint const&, SkMatrix const&, bool, GrPaint*) (SkGr.cpp:668) ==3016== by 0xD99B51: SkGpuDevice::drawRect(SkDraw const&, SkRect const&, SkPaint const&) (SkGpuDevice.cpp:534) ==3016== by 0x9DDB1C: SkCanvas::onDrawRect(SkRect const&, SkPaint const&) (SkCanvas.cpp:2148) ==3016== by 0x9DBF40: SkCanvas::drawRect(SkRect const&, SkPaint const&) (SkCanvas.cpp:1919) ==3016== by 0x8D2A55: skiagm::LightingShaderGM::onDraw(SkCanvas*) (lightingshader.cpp:110) ==3016== by 0x625659: skiagm::GM::drawContent(SkCanvas*) (gm.cpp:32) ==3016== by 0x6256AD: skiagm::GM::draw(SkCanvas*) (gm.cpp:24) ==3016== by 0x61B747: DM::GMSrc::draw(SkCanvas*) const (DMSrcSink.cpp:61) ==3016== by 0x61F37B: DM::GPUSink::draw(DM::Src const&, SkBitmap*, SkWStream*, SkString*) const (DMSrcSink.cpp:1115) ==3016== by 0x619261: dm_main() (DM.cpp:1016) ==3016== by 0x619841: main (DM.cpp:1409) ==3016== Address 0xf4 is not stack'd, malloc'd or (recently) free'd ==3016== Caught signal 11 [Segmentation fault], was running: gpu gm lightingshader .
Message was sent while issue was closed.
Description was changed from ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader Will not run until after landing https://codereview.chromium.org/2106893003/ This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 Committed: https://skia.googlesource.com/skia/+/3f58cd0eb8bf4499c4f179aa6111d7e005809df8 ========== to ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader Will not run until after landing https://codereview.chromium.org/2106893003/ This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 Committed: https://skia.googlesource.com/skia/+/3f58cd0eb8bf4499c4f179aa6111d7e005809df8 ==========
This fixes the valgrind problem (on my computer, at least). Please LMK if I should land.
lgtm
The CQ bit was checked by dvonbeck@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader Will not run until after landing https://codereview.chromium.org/2106893003/ This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 Committed: https://skia.googlesource.com/skia/+/3f58cd0eb8bf4499c4f179aa6111d7e005809df8 ========== to ========== Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader Will not run until after landing https://codereview.chromium.org/2106893003/ This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 Committed: https://skia.googlesource.com/skia/+/3f58cd0eb8bf4499c4f179aa6111d7e005809df8 Committed: https://skia.googlesource.com/skia/+/e6347ad7459a52f42241fd5c48638725d9f11b40 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://skia.googlesource.com/skia/+/e6347ad7459a52f42241fd5c48638725d9f11b40 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
