Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1846313002:
Switch SkDisplacementImageFilter over to new onFilterImage interface (Closed)
|
Created:
4 years, 8 months ago by robertphillips Modified:
4 years, 8 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSwitch SkDisplacementImageFilter over to new onFilterImage interface
TBR=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1846313002
Committed: https://skia.googlesource.com/skia/+/e6277de4ebb1b00a15f8f3c7cd9d6027c6b021fe
Patch Set 1 #Patch Set 2 : update #
Total comments: 8
Patch Set 3 : Address code review comments #Patch Set 4 : Temp patch set for code review (won't compile) #Patch Set 5 : address code review comments #
Total comments: 4
Patch Set 6 : Address code review comments #Patch Set 7 : Move GrDisplacementMapEffect back up for compilation #Patch Set 8 : Update to ToT #Patch Set 9 : Fix oversight #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== Switch SkDisplacementImageFilter over to new onFilterImage interface ========== to ========== Switch SkDisplacementImageFilter over to new onFilterImage interface GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Switch SkDisplacementImageFilter over to new onFilterImage interface GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkDisplacementImageFilter over to new onFilterImage interface TBR=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
robertphillips@google.com changed reviewers: + reed@google.com, senorblanco@chromium.org, senorblanco@google.com
https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... File src/effects/SkDisplacementMapEffect.cpp (right): https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:215: class GrDisplacementMapEffect : public GrFragmentProcessor { Could you move this code back to where it was for review purposes? You can move it back in a followup patch. https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:318: GrTexture* displacement = displ->peekTexture(); Peeking usually implies that these could be null. Is that the case? If so, should we be checking for null (or asserting non-null) here? https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:380: SkBitmap dst2; Nit: it looks like this could actually be called dst also, without fear of conflict.
Patchset #4 (id:50001) has been deleted
https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... File src/effects/SkDisplacementMapEffect.cpp (right): https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:215: class GrDisplacementMapEffect : public GrFragmentProcessor { On 2016/04/01 19:17:42, Stephen White wrote: > Could you move this code back to where it was for review purposes? You can move > it back in a followup patch. Done. https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:318: GrTexture* displacement = displ->peekTexture(); On 2016/04/01 19:17:42, Stephen White wrote: > Peeking usually implies that these could be null. Is that the case? If so, > should we be checking for null (or asserting non-null) here? Done. The logic is a bit circuitous. If 'source' is backed by a texture then filterInput should fail (and return null) if either 'color' or 'displ' cannot be backed by a texture. https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:380: SkBitmap dst2; On 2016/04/01 19:17:42, Stephen White wrote: > Nit: it looks like this could actually be called dst also, without fear of > conflict. Done.
https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... File src/effects/SkDisplacementMapEffect.cpp (right): https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:318: GrTexture* displacement = displ->peekTexture(); On 2016/04/01 19:41:43, robertphillips wrote: > On 2016/04/01 19:17:42, Stephen White wrote: > > Peeking usually implies that these could be null. Is that the case? If so, > > should we be checking for null (or asserting non-null) here? > > Done. The logic is a bit circuitous. If 'source' is backed by a texture then > filterInput should fail (and return null) if either 'color' or 'displ' cannot be > backed by a texture. OK, so at this point can we assert that both colorTexture and displacement are non-null? (Sorry, I missed the assert above before). As in, SkASSERT(colorTexture && displacement)? Also could we rename displacement -> displTexture? It's nice to have the names reflect which are SkSpecialImages and which are textures.
https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... File src/effects/SkDisplacementMapEffect.cpp (right): https://codereview.chromium.org/1846313002/diff/20001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:318: GrTexture* displacement = displ->peekTexture(); On 2016/04/02 02:33:48, Stephen White wrote: > On 2016/04/01 19:41:43, robertphillips wrote: > > On 2016/04/01 19:17:42, Stephen White wrote: > > > Peeking usually implies that these could be null. Is that the case? If so, > > > should we be checking for null (or asserting non-null) here? > > > > Done. The logic is a bit circuitous. If 'source' is backed by a texture then > > filterInput should fail (and return null) if either 'color' or 'displ' cannot > be > > backed by a texture. > > OK, so at this point can we assert that both colorTexture and displacement are > non-null? (Sorry, I missed the assert above before). > > As in, SkASSERT(colorTexture && displacement)? > > Also could we rename displacement -> displTexture? It's nice to have the names > reflect which are SkSpecialImages and which are textures. Done.
LGTM w/nit https://codereview.chromium.org/1846313002/diff/90001/src/effects/SkDisplacem... File src/effects/SkDisplacementMapEffect.cpp (right): https://codereview.chromium.org/1846313002/diff/90001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:252: if (color->peekTexture()) { Nit: I didn't notice that you were using color->peekTexture() to switch to the GPU path (hence my confusion). I wonder if we should use source->peekTexture() instead (in all filters)? That should always be the case, since it's the infrastructure which sets up the source primitive. Then we could assert that all inputs have textures.
https://codereview.chromium.org/1846313002/diff/90001/src/effects/SkDisplacem... File src/effects/SkDisplacementMapEffect.cpp (right): https://codereview.chromium.org/1846313002/diff/90001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:321: SkBitmap dst2; Nit: "dst2" could be "dst"
https://codereview.chromium.org/1846313002/diff/90001/src/effects/SkDisplacem... File src/effects/SkDisplacementMapEffect.cpp (right): https://codereview.chromium.org/1846313002/diff/90001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:252: if (color->peekTexture()) { On 2016/04/04 16:55:14, Stephen White wrote: > Nit: I didn't notice that you were using color->peekTexture() to switch to the > GPU path (hence my confusion). I wonder if we should use source->peekTexture() > instead (in all filters)? That should always be the case, since it's the > infrastructure which sets up the source primitive. Then we could assert that all > inputs have textures. Done - here. I will post a separate patch to switch over the other ImageFilters. https://codereview.chromium.org/1846313002/diff/90001/src/effects/SkDisplacem... src/effects/SkDisplacementMapEffect.cpp:321: SkBitmap dst2; On 2016/04/04 16:56:55, Stephen White wrote: > Nit: "dst2" could be "dst" Done.
even moar LGTM
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846313002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846313002/130001
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 client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846313002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846313002/170001
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 robertphillips@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/1846313002/#ps170001 (title: "Fix oversight")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846313002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846313002/170001
Message was sent while issue was closed.
Description was changed from ========== Switch SkDisplacementImageFilter over to new onFilterImage interface TBR=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkDisplacementImageFilter over to new onFilterImage interface TBR=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e6277de4ebb1b00a15f8f3c7cd9d6027c6b021fe ==========
Message was sent while issue was closed.
Committed patchset #9 (id:170001) as https://skia.googlesource.com/skia/+/e6277de4ebb1b00a15f8f3c7cd9d6027c6b021fe |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
