Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1813813002:
Add SkSpecialImage::makeTextureImage entry point & update filterInput (Closed)
|
Created:
4 years, 9 months ago by robertphillips Modified:
4 years, 9 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd SkSpecialImage::makeTextureImage entry point
This is calved off of: https://codereview.chromium.org/1785643003 (Switch SkBlurImageFilter over to new onFilterImage interface)
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1813813002
Committed: https://skia.googlesource.com/skia/+/05849018c85403a34b88819db1c4bcf713b70a2b
Committed: https://skia.googlesource.com/skia/+/83c17fa56b23159166394cb3feb431ffafbbab48
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address code review issues #Patch Set 3 : update to ToT #Patch Set 4 : Fix possible nullptr dereference #
Messages
Total messages: 32 (17 generated)
Description was changed from ========== Add SkSpecialImage::makeTextureImage entry point ========== to ========== Add SkSpecialImage::makeTextureImage entry point GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add SkSpecialImage::makeTextureImage entry point GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkSpecialImage::makeTextureImage entry point This is calved off of: https://codereview.chromium.org/1785643003 (Switch SkBlurImageFilter over to new onFilterImage interface) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
robertphillips@google.com changed reviewers: + bsalomon@google.com
robertphillips@google.com changed reviewers: + senorblanco@chromium.org, senorblanco@google.com
Looks good overall; just some questions. https://codereview.chromium.org/1813813002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1813813002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:598: if (src->peekTexture() && !result->peekTexture()) { This is also done in filterInputGPU[Deprecated](). Is that path going away in the new world? Also, shouldn't this all be wrapped in #if SK_SUPPORT_GPU? https://codereview.chromium.org/1813813002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:600: // image filters that don't support GPU in all cases The only one I can think of off the top of my head is SkImageSource. Are there others? https://codereview.chromium.org/1813813002/diff/1/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1813813002/diff/1/src/core/SkSpecialImage.h#n... src/core/SkSpecialImage.h:17: #include "SkImageInfo.h" I don't see new references to this class in this patch. Is this just fixing a previous missing dependency? https://codereview.chromium.org/1813813002/diff/1/src/core/SkSpecialImage.h#n... src/core/SkSpecialImage.h:26: class SkPixmap; Same here -- is this necessary?
https://codereview.chromium.org/1813813002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1813813002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:598: if (src->peekTexture() && !result->peekTexture()) { On 2016/03/17 17:10:08, Stephen White wrote: > This is also done in filterInputGPU[Deprecated](). Is that path going away in > the new world? Right - filterInputGPU[Deprecated] should go away. Hopefully, this instance will also go away and just be replaced with an assert. > Also, shouldn't this all be wrapped in #if SK_SUPPORT_GPU? Done. https://codereview.chromium.org/1813813002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:600: // image filters that don't support GPU in all cases On 2016/03/17 17:10:08, Stephen White wrote: > The only one I can think of off the top of my head is SkImageSource. Are there > others? I believe the SkXfermodeImageFilter balks at cropRects. Some other asFragmentProcessor calls can also fail. https://codereview.chromium.org/1813813002/diff/1/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1813813002/diff/1/src/core/SkSpecialImage.h#n... src/core/SkSpecialImage.h:17: #include "SkImageInfo.h" On 2016/03/17 17:10:08, Stephen White wrote: > I don't see new references to this class in this patch. Is this just fixing a > previous missing dependency? It is, unfortunately, for SkAlphaType - used in the NewFromGpu call. https://codereview.chromium.org/1813813002/diff/1/src/core/SkSpecialImage.h#n... src/core/SkSpecialImage.h:26: class SkPixmap; On 2016/03/17 17:10:08, Stephen White wrote: > Same here -- is this necessary? Yes - it is used in the NewFromPixmap entry point.
LGTM https://codereview.chromium.org/1813813002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1813813002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:600: // image filters that don't support GPU in all cases On 2016/03/17 21:01:47, robertphillips wrote: > On 2016/03/17 17:10:08, Stephen White wrote: > > The only one I can think of off the top of my head is SkImageSource. Are there > > others? > > I believe the SkXfermodeImageFilter balks at cropRects. I think you're misunderstanding what canFilterImageGPU() means. SkXfermodeImageFilter falls back to the generic (two-pass) drawing if there's a crop rect. It doesn't mean it won't produce a texture-backed result, or that it won't process on the GPU, it just means it'll use generic Skia calls to do so. > Some other asFragmentProcessor calls can also fail. The only ImageFilter I could find that could fail in asFragmentProcessor() is SkAlphaThresholdFilter, which can fail to allocate a mask texture. We should probably not be dropping back to software in that case, but just aborting processing altogether. What I'm really getting at is, if there are filters which are not processing on the GPU (given a texture-backed input), I'd like to know. For now, doing the upload in filterInput() is fine. But if it's only SkImageSource that needs it, perhaps we could move the upload code there, and just assert that all results are texture-backed if src is texture-backed. That could be in another patch, of course.
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/1813813002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813813002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
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/1813813002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813813002/40001
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/1813813002/#ps40001 (title: "update to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1813813002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813813002/40001
Message was sent while issue was closed.
Description was changed from ========== Add SkSpecialImage::makeTextureImage entry point This is calved off of: https://codereview.chromium.org/1785643003 (Switch SkBlurImageFilter over to new onFilterImage interface) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkSpecialImage::makeTextureImage entry point This is calved off of: https://codereview.chromium.org/1785643003 (Switch SkBlurImageFilter over to new onFilterImage interface) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/05849018c85403a34b88819db1c4bcf713b70a2b ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/05849018c85403a34b88819db1c4bcf713b70a2b
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1811973005/ by bungeman@google.com. The reason for reverting is: Suspected cause of layout test css3/filters/effect-reference-tile-hw.htmlto crash. https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/... It could also be the previous change (but it appears this one depends on that one). .
Message was sent while issue was closed.
Description was changed from ========== Add SkSpecialImage::makeTextureImage entry point This is calved off of: https://codereview.chromium.org/1785643003 (Switch SkBlurImageFilter over to new onFilterImage interface) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/05849018c85403a34b88819db1c4bcf713b70a2b ========== to ========== Add SkSpecialImage::makeTextureImage entry point This is calved off of: https://codereview.chromium.org/1785643003 (Switch SkBlurImageFilter over to new onFilterImage interface) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/05849018c85403a34b88819db1c4bcf713b70a2b ==========
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/1813813002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813813002/60001
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/1813813002/#ps60001 (title: "Fix possible nullptr dereference")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1813813002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813813002/60001
Message was sent while issue was closed.
Description was changed from ========== Add SkSpecialImage::makeTextureImage entry point This is calved off of: https://codereview.chromium.org/1785643003 (Switch SkBlurImageFilter over to new onFilterImage interface) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/05849018c85403a34b88819db1c4bcf713b70a2b ========== to ========== Add SkSpecialImage::makeTextureImage entry point This is calved off of: https://codereview.chromium.org/1785643003 (Switch SkBlurImageFilter over to new onFilterImage interface) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/05849018c85403a34b88819db1c4bcf713b70a2b Committed: https://skia.googlesource.com/skia/+/83c17fa56b23159166394cb3feb431ffafbbab48 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/83c17fa56b23159166394cb3feb431ffafbbab48 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
