Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155063002:
use special-image for imagefilters and save/restore layer (Closed)
Descriptionuse special-image for imagefilters and save/restore layer
add special virtuals to device, in preparation for using them instead of bitmap for imagefilters
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2155933002
patch from issue 2155933002 at patchset 20001 (http://crrev.com/2155933002#ps20001)
use specialimages instead of bitmaps for imagefiltering
Committed: https://skia.googlesource.com/skia/+/a2217ef965e57fdbbf989989e7ec1f2c04f62d39
Patch Set 1 #Patch Set 2 : fix restore #Patch Set 3 : rebase #
Total comments: 11
Patch Set 4 : merge #Patch Set 5 : rebase #Patch Set 6 : check for null-special #Patch Set 7 : move compatible-with-filtering test into special factories #Patch Set 8 : gpu fix, remove srgb check #Patch Set 9 : rebase #Patch Set 10 : can't assert in snapSpecial (for backdrops) #
Total comments: 6
Patch Set 11 : address comments #
Messages
Total messages: 42 (31 generated)
The CQ bit was checked by reed@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...)
reed@google.com changed reviewers: + bsalomon@google.com, robertphillips@google.com
https://codereview.chromium.org/2155063002/diff/40001/src/core/SkBitmapDevice... File src/core/SkBitmapDevice.cpp (right): https://codereview.chromium.org/2155063002/diff/40001/src/core/SkBitmapDevice... src/core/SkBitmapDevice.cpp:390: tmpUnfiltered.setImageFilter(nullptr); getROPixels ? https://codereview.chromium.org/2155063002/diff/40001/src/core/SkBitmapDevice... src/core/SkBitmapDevice.cpp:395: } else { getROPixels ? https://codereview.chromium.org/2155063002/diff/40001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2155063002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1461: SkIPoint pos = { x - iter.getX(), y - iter.getY() }; Why not just call drawSpecial in both cases ? https://codereview.chromium.org/2155063002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:2334: const SkPaint& pnt = looper.paint(); ditch drawAsSprite and just use special here ? https://codereview.chromium.org/2155063002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:2411: special = this->getDevice()->makeSpecial(bitmap); I think we don't need the following if block https://codereview.chromium.org/2155063002/diff/40001/src/image/SkImage_Base.h File src/image/SkImage_Base.h (right): https://codereview.chromium.org/2155063002/diff/40001/src/image/SkImage_Base.... src/image/SkImage_Base.h:70: virtual bool canBeImageFiltered() const { Make this live ?
https://codereview.chromium.org/2155063002/diff/40001/src/core/SkBitmapDevice... File src/core/SkBitmapDevice.cpp (right): https://codereview.chromium.org/2155063002/diff/40001/src/core/SkBitmapDevice... src/core/SkBitmapDevice.cpp:390: tmpUnfiltered.setImageFilter(nullptr); On 2016/07/18 17:01:58, robertphillips wrote: > getROPixels ? Done. https://codereview.chromium.org/2155063002/diff/40001/src/core/SkBitmapDevice... src/core/SkBitmapDevice.cpp:395: } else { On 2016/07/18 17:01:58, robertphillips wrote: > getROPixels ? Done. https://codereview.chromium.org/2155063002/diff/40001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2155063002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1461: SkIPoint pos = { x - iter.getX(), y - iter.getY() }; On 2016/07/18 17:01:58, robertphillips wrote: > Why not just call drawSpecial in both cases ? PDFDevice doesn't return a special (he will have returned a raster device if there was a filter). https://codereview.chromium.org/2155063002/diff/40001/src/image/SkImage_Base.h File src/image/SkImage_Base.h (right): https://codereview.chromium.org/2155063002/diff/40001/src/image/SkImage_Base.... src/image/SkImage_Base.h:70: virtual bool canBeImageFiltered() const { On 2016/07/18 17:01:58, robertphillips wrote: > Make this live ? Done.
The CQ bit was checked by reed@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...
https://codereview.chromium.org/2145343005/ (Recast draw_filter_into_device as drawDevice)
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 reed@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-Debug-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by reed@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...
https://codereview.chromium.org/2155063002/diff/40001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2155063002/diff/40001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:2334: const SkPaint& pnt = looper.paint(); On 2016/07/18 17:01:58, robertphillips wrote: > ditch drawAsSprite and just use special here ? Done.
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-Debug-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by reed@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-Debug-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
ptal
The CQ bit was checked by reed@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.
https://codereview.chromium.org/2155063002/diff/180001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2155063002/diff/180001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:2318: const SkPaint& pnt = looper.paint(); Do you think we actually need the "pnt.getImageFilter" check? It is kind of odd in that we could create the special image and then not end up using it. https://codereview.chromium.org/2155063002/diff/180001/src/core/SkSpecialImag... File src/core/SkSpecialImage.cpp (right): https://codereview.chromium.org/2155063002/diff/180001/src/core/SkSpecialImag... src/core/SkSpecialImage.cpp:20: // The raster-specific image filters are written specifically for N32 ? https://codereview.chromium.org/2155063002/diff/180001/src/core/SkSpecialImag... src/core/SkSpecialImage.cpp:321: GPU-backed SkImages should work fine https://codereview.chromium.org/2155063002/diff/180001/src/core/SkSpecialImag... src/core/SkSpecialImage.cpp:424: SkBitmap tmpStorage; for -> force
lgtm if you would like to land as-is
Will try to address the image-wrapped version in a follow-up CL https://codereview.chromium.org/2155063002/diff/180001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2155063002/diff/180001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:2318: const SkPaint& pnt = looper.paint(); On 2016/07/20 12:20:05, robertphillips wrote: > Do you think we actually need the "pnt.getImageFilter" check? > It is kind of odd in that we could create the special image and then not end up > using it. Done. https://codereview.chromium.org/2155063002/diff/180001/src/core/SkSpecialImag... File src/core/SkSpecialImage.cpp (right): https://codereview.chromium.org/2155063002/diff/180001/src/core/SkSpecialImag... src/core/SkSpecialImage.cpp:424: SkBitmap tmpStorage; On 2016/07/20 12:20:05, robertphillips wrote: > for -> force Done.
The CQ bit was checked by reed@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 reed@google.com
The CQ bit was checked by reed@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/2155063002/#ps200001 (title: "address comments")
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 ========== use special-image for imagefilters and save/restore layer add special virtuals to device, in preparation for using them instead of bitmap for imagefilters BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2155933002 patch from issue 2155933002 at patchset 20001 (http://crrev.com/2155933002#ps20001) use specialimages instead of bitmaps for imagefiltering ========== to ========== use special-image for imagefilters and save/restore layer add special virtuals to device, in preparation for using them instead of bitmap for imagefilters BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2155933002 patch from issue 2155933002 at patchset 20001 (http://crrev.com/2155933002#ps20001) use specialimages instead of bitmaps for imagefiltering Committed: https://skia.googlesource.com/skia/+/a2217ef965e57fdbbf989989e7ec1f2c04f62d39 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://skia.googlesource.com/skia/+/a2217ef965e57fdbbf989989e7ec1f2c04f62d39 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
