Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1757983002:
Add SkSpecialImage-based methods to SkImageFilter (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-based methods to SkImageFilter
This is calved off of https://codereview.chromium.org/1695823002/ (Get OffsetImageFilter really working with SkSpecialImages)
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1757983002
Committed: https://skia.googlesource.com/skia/+/eaf086e3ce1b8351a8cd01762ca5144254bddbc4
Patch Set 1 #
Total comments: 7
Patch Set 2 : update #
Total comments: 2
Patch Set 3 : Remove erroneous mapContext #
Messages
Total messages: 26 (9 generated)
Description was changed from ========== Add SkSpecialImage-based methods to SkImageFilter ========== to ========== Add SkSpecialImage-based methods to SkImageFilter GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add SkSpecialImage-based methods to SkImageFilter GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkSpecialImage-based methods to SkImageFilter This is calved off of https://codereview.chromium.org/1695823002/ (Get OffsetImageFilter really working with SkSpecialImages) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
robertphillips@google.com changed reviewers: + senorblanco@chromium.org
reed@google.com changed reviewers: + reed@google.com
api lgtm
https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:222: const SkIRect srcSubset = fUsesSrcInput ? src->subset() : SkIRect::MakeWH(0, 0); Could you explain the subset stuff? Is this the result of extractSubset() in a SkImage/SkSurface world? Also, are there tests which exercise the caching of a filter results with and without subset? https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:473: if (!canvas) { When can a surface's canvas be null? Could this be an assert instead?
https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:222: const SkIRect srcSubset = fUsesSrcInput ? src->subset() : SkIRect::MakeWH(0, 0); The subset stuff is intended to allow SpecialSurfaces and Images to be used for atlases. It can currently be generated from a subsetted SkBitmap (or such a bitmap wrapped in an SkImage). The ImageFilterCache_* tests do test caching with non-upper-left aligned subsets. https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:473: if (!canvas) { Done - getCanvas can return null if a SpecialImage has been snapped from the SpecialSurface. In this instance it should be fine to just assert.
https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:222: const SkIRect srcSubset = fUsesSrcInput ? src->subset() : SkIRect::MakeWH(0, 0); On 2016/03/04 16:26:13, robertphillips wrote: > The subset stuff is intended to allow SpecialSurfaces and Images to be used for > atlases. It can currently be generated from a subsetted SkBitmap (or such a > bitmap wrapped in an SkImage). How do filters and atlasing interact? > The ImageFilterCache_* tests do test caching with non-upper-left aligned > subsets. Great, thanks.
https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:222: const SkIRect srcSubset = fUsesSrcInput ? src->subset() : SkIRect::MakeWH(0, 0); > How do filters and atlasing interact? All speculative right now but presumably they could act as sources just like normal. As targets they would have be the target where the draw finally ends up (so the canvas that is being rendered to through an appropriate clip). I don't think they could simultaneously act as a source and target.
https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1757983002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:222: const SkIRect srcSubset = fUsesSrcInput ? src->subset() : SkIRect::MakeWH(0, 0); On 2016/03/04 16:58:22, robertphillips wrote: > > How do filters and atlasing interact? > > All speculative right now but presumably they could act as sources just like > normal. What Skia API calls would need to be made to do this? To be clear, what I'm really asking is, is this complexity needed today, or can we keep it out for now and add it in when the need arises? > As targets they would have be the target where the draw finally ends up > (so the canvas that is being rendered to through an appropriate clip). I don't think that could happen, since the final draw to canvas is always performed by the device, not by a filter. > I don't think they could simultaneously act as a source and target.
> What Skia API calls would need to be made to do this? To be clear, what I'm > really asking is, is this complexity needed today, or can we keep it out for now > and add it in when the need arises? You would simply create several (possibly transient) SkSpecialSurfaces that wrap the same RenderTarget (via SkSpecialSurface::NewFromTexture). > > As targets they would have be the target where the draw finally ends up > > (so the canvas that is being rendered to through an appropriate clip). > > I don't think that could happen, since the final draw to canvas is always > performed by the device, not by a filter. Do you mean the simultaneous use as a src and dest? If so, you would wrap the same GrTexture in both a SkSpecialSurface and an SkImage (via a subsetted SkBitmap) and then draw to the SkSpecialSurface's canvas.
On 2016/03/04 17:11:48, robertphillips wrote: > > What Skia API calls would need to be made to do this? To be clear, what I'm > > really asking is, is this complexity needed today, or can we keep it out for > now > > and add it in when the need arises? > > You would simply create several (possibly transient) SkSpecialSurfaces that wrap > the same RenderTarget (via SkSpecialSurface::NewFromTexture). What I meant was, what series of Skia calls would break if we didn't have subset support in filters. But from talking offline, it seems it's more that subsets provide support for atlasing in the layer hoister, which is currently disabled. > > > As targets they would have be the target where the draw finally ends up > > > (so the canvas that is being rendered to through an appropriate clip). > > > > I don't think that could happen, since the final draw to canvas is always > > performed by the device, not by a filter. > > Do you mean the simultaneous use as a src and dest? If so, you would wrap the > same GrTexture in both a SkSpecialSurface and an SkImage (via a subsetted > SkBitmap) and then draw to the SkSpecialSurface's canvas. Since these are all internal or Ganesh calls, I'm still wondering what Skia use cases this breaks or improves.
(Ignore my previous comments about the subset; I now understand that it's the subset of the source primitive that we're talking about here, which may have been atlased.) https://codereview.chromium.org/1757983002/diff/20001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1757983002/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:342: this->mapContext(ctx), &result, offset)) { Are you sure about the mapContext() call here? This was previously only called in filterInput(), when we were transitioning from a downstream node to an upstream node and the context needed to be adjusted to compensate for the current node. It shouldn't be necessary here.
https://codereview.chromium.org/1757983002/diff/20001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1757983002/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:342: this->mapContext(ctx), &result, offset)) { On 2016/03/04 19:49:31, Stephen White wrote: > Are you sure about the mapContext() call here? This was previously only called > in filterInput(), when we were transitioning from a downstream node to an > upstream node and the context needed to be adjusted to compensate for the > current node. It shouldn't be necessary here. Done.
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/1757983002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757983002/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 reed@google.com Link to the patchset: https://codereview.chromium.org/1757983002/#ps40001 (title: "Remove erroneous mapContext")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757983002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757983002/40001
Message was sent while issue was closed.
Description was changed from ========== Add SkSpecialImage-based methods to SkImageFilter This is calved off of https://codereview.chromium.org/1695823002/ (Get OffsetImageFilter really working with SkSpecialImages) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkSpecialImage-based methods to SkImageFilter This is calved off of https://codereview.chromium.org/1695823002/ (Get OffsetImageFilter really working with SkSpecialImages) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/eaf086e3ce1b8351a8cd01762ca5144254bddbc4 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/eaf086e3ce1b8351a8cd01762ca5144254bddbc4 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
