Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1879643003:
Switch AlphaThresholdFilter 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 AlphaThresholdFilter over to new onFilterImage interface
This CL also alters the raster path in two ways:
it now respects the sRGB/linear distinction of its input
it now respects the clip
TBR=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1879643003
Committed: https://skia.googlesource.com/skia/+/af9b8c804643952d5ff3deed62f1355319b72f72
Patch Set 1 #Patch Set 2 : clean up #Patch Set 3 : clean up #
Total comments: 22
Patch Set 4 : Address code review comments #
Total comments: 4
Patch Set 5 : Fix serialize-8888 config (deserialize crop rect) #
Messages
Total messages: 27 (12 generated)
Description was changed from ========== Switch AlphaThresholdFilter over to new onFilterImage interface ========== to ========== Switch AlphaThresholdFilter 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 AlphaThresholdFilter over to new onFilterImage interface GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch AlphaThresholdFilter over to new onFilterImage interface This CL also alters the raster path in two ways: it now respects the sRGB/linear distinction of its input it now respects the clip 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
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/1879643003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1879643003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1879643003/diff/40001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1879643003/diff/40001/include/core/SkImageFil... include/core/SkImageFilter.h:425: * intersecting the initial bounds with "dstBounds", to ensure that we never Nit: extra indentation before "intersecting" https://codereview.chromium.org/1879643003/diff/40001/include/core/SkImageFil... include/core/SkImageFilter.h:427: * future). (To here.) https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... File src/effects/SkAlphaThresholdFilter.cpp (right): https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:35: GrTexture* createMaskTexture(GrContext*, const SkMatrix&, const SkIRect& bounds) const; Should this return an sk_sp<GrTexture> instead? https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:315: drawContext->clear(nullptr, 0x0, true); Add a reference to the "clear may not be necessary" bug, if applicable? https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:348: if (!this->applyCropRect(ctx, inputBounds, &bounds)) { Given that there's no create param to add a crop rect, I wouldn't bother with applying it. Or, add a test which exercises it. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:357: if (!inputTexture) { Please assert this is non-null instead. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:374: // SRGBTODO: handle sRGB here Is there a bug for that? https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:412: dst); Kind of a shame the GPU code got so much longer in this version. I guess that's a side-effect of taking the axe to asFragmentProcessor().
api lgtm
https://codereview.chromium.org/1879643003/diff/40001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1879643003/diff/40001/include/core/SkImageFil... include/core/SkImageFilter.h:425: * intersecting the initial bounds with "dstBounds", to ensure that we never On 2016/04/11 20:48:20, Stephen White wrote: > Nit: extra indentation before "intersecting" I think that's just a code review artifact. It lines up in the code. https://codereview.chromium.org/1879643003/diff/40001/include/core/SkImageFil... include/core/SkImageFilter.h:427: * future). On 2016/04/11 20:48:20, Stephen White wrote: > (To here.) Not sure what this means. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... File src/effects/SkAlphaThresholdFilter.cpp (right): https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:35: GrTexture* createMaskTexture(GrContext*, const SkMatrix&, const SkIRect& bounds) const; On 2016/04/11 20:48:20, Stephen White wrote: > Should this return an sk_sp<GrTexture> instead? Done. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:315: drawContext->clear(nullptr, 0x0, true); On 2016/04/11 20:48:20, Stephen White wrote: > Add a reference to the "clear may not be necessary" bug, if applicable? This one is necessary since we don't know where the rects will be drawn in the mask. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:348: if (!this->applyCropRect(ctx, inputBounds, &bounds)) { On 2016/04/11 20:48:20, Stephen White wrote: > Given that there's no create param to add a crop rect, I wouldn't bother with > applying it. > > Or, add a test which exercises it. applyCropRect also does interesting things with the clip. The GPU path was applying it and thus doing less work than the raster path. I have also added a parameter to the Make method and a GM to test it. Although it is unlikely to be used in practice, I prefer the uniformity/consistency. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:357: if (!inputTexture) { On 2016/04/11 20:48:20, Stephen White wrote: > Please assert this is non-null instead. Done. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:374: // SRGBTODO: handle sRGB here On 2016/04/11 20:48:20, Stephen White wrote: > Is there a bug for that? This is Brian O.'s TODO. I believe he is just tracking them as text strings. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:412: dst); On 2016/04/11 20:48:20, Stephen White wrote: > Kind of a shame the GPU code got so much longer in this version. I guess that's > a side-effect of taking the axe to asFragmentProcessor(). I've added a SkImageFilter::DrawWithFP helper function that should rein this in a bit (and be reusable in the Magnifier and MatrixConvolution ImageFilters).
https://codereview.chromium.org/1879643003/diff/40001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1879643003/diff/40001/include/core/SkImageFil... include/core/SkImageFilter.h:425: * intersecting the initial bounds with "dstBounds", to ensure that we never On 2016/04/12 15:36:38, robertphillips wrote: > On 2016/04/11 20:48:20, Stephen White wrote: > > Nit: extra indentation before "intersecting" > > I think that's just a code review artifact. It lines up in the code. You're right! Never mind. https://codereview.chromium.org/1879643003/diff/40001/include/core/SkImageFil... include/core/SkImageFilter.h:427: * future). On 2016/04/12 15:36:38, robertphillips wrote: > On 2016/04/11 20:48:20, Stephen White wrote: > > (To here.) > > Not sure what this means. The end of the indentation weirdness above; ignore. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... File src/effects/SkAlphaThresholdFilter.cpp (right): https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:315: drawContext->clear(nullptr, 0x0, true); On 2016/04/12 15:36:39, robertphillips wrote: > On 2016/04/11 20:48:20, Stephen White wrote: > > Add a reference to the "clear may not be necessary" bug, if applicable? > > This one is necessary since we don't know where the rects will be drawn in the > mask. Good point. Perhaps we could do a pass unioning all the rects first, and then clear with that as a mask. Anyway, not important. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:348: if (!this->applyCropRect(ctx, inputBounds, &bounds)) { On 2016/04/12 15:36:39, robertphillips wrote: > On 2016/04/11 20:48:20, Stephen White wrote: > > Given that there's no create param to add a crop rect, I wouldn't bother with > > applying it. > > > > Or, add a test which exercises it. > > applyCropRect also does interesting things with the clip. The GPU path was > applying it and thus doing less work than the raster path. > > I have also added a parameter to the Make method and a GM to test it. Although > it is unlikely to be used in practice, I prefer the uniformity/consistency. Thanks! https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:374: // SRGBTODO: handle sRGB here On 2016/04/12 15:36:39, robertphillips wrote: > On 2016/04/11 20:48:20, Stephen White wrote: > > Is there a bug for that? > > This is Brian O.'s TODO. I believe he is just tracking them as text strings. Acknowledged. https://codereview.chromium.org/1879643003/diff/40001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:412: dst); On 2016/04/12 15:36:39, robertphillips wrote: > On 2016/04/11 20:48:20, Stephen White wrote: > > Kind of a shame the GPU code got so much longer in this version. I guess > that's > > a side-effect of taking the axe to asFragmentProcessor(). > > I've added a SkImageFilter::DrawWithFP helper function that should rein this in > a bit (and be reusable in the Magnifier and MatrixConvolution ImageFilters). Acknowledged.
LGTM https://codereview.chromium.org/1879643003/diff/60001/src/effects/SkAlphaThre... File src/effects/SkAlphaThresholdFilter.cpp (right): https://codereview.chromium.org/1879643003/diff/60001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:66: std::move(input), Nit: std::move() here seems to be overkill. Unless I'm missing some other reason, I seriously doubt refcount churn is going to be a bottleneck in this codepath. https://codereview.chromium.org/1879643003/diff/60001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:387: return DrawWithFP(context, std::move(fp), bounds, source->internal_getProxy()); Much nicer, thanks! (Again, I'm not a fan with peppering the code with std::move(), but I won't block this for it.)
https://codereview.chromium.org/1879643003/diff/60001/src/effects/SkAlphaThre... File src/effects/SkAlphaThresholdFilter.cpp (right): https://codereview.chromium.org/1879643003/diff/60001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:66: std::move(input), On 2016/04/12 16:34:32, Stephen White wrote: > Nit: std::move() here seems to be overkill. Unless I'm missing some other > reason, I seriously doubt refcount churn is going to be a bottleneck in this > codepath. Yeah - this seems to be SOP for now. There is some discussion locally about the utility of it & other difficulties that might arise from using std::move whenever possible. It seems pretty innocuous in this instance/use case. https://codereview.chromium.org/1879643003/diff/60001/src/effects/SkAlphaThre... src/effects/SkAlphaThresholdFilter.cpp:387: return DrawWithFP(context, std::move(fp), bounds, source->internal_getProxy()); On 2016/04/12 16:34:32, Stephen White wrote: > Much nicer, thanks! > > (Again, I'm not a fan with peppering the code with std::move(), but I won't > block this for it.) Acknowledged.
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/1879643003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1879643003/60001
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/1879643003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1879643003/80001
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 reed@google.com, senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/1879643003/#ps80001 (title: "Fix serialize-8888 config (deserialize crop rect)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1879643003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1879643003/80001
Message was sent while issue was closed.
Description was changed from ========== Switch AlphaThresholdFilter over to new onFilterImage interface This CL also alters the raster path in two ways: it now respects the sRGB/linear distinction of its input it now respects the clip TBR=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch AlphaThresholdFilter over to new onFilterImage interface This CL also alters the raster path in two ways: it now respects the sRGB/linear distinction of its input it now respects the clip 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/+/af9b8c804643952d5ff3deed62f1355319b72f72 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/af9b8c804643952d5ff3deed62f1355319b72f72 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
