Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1772933002:
Switch SkImageSource image filter over to new onFilterImage interface (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@if-follow-on Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSwitch SkImageSource image filter over to new onFilterImage interface
This CL relies on: https://codereview.chromium.org/1762013002/ (Swap over to using SkImageFilter::filterImage instead of filterImageDeprecated)
TBR=bsalomon@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1772933002
Committed: https://skia.googlesource.com/skia/+/6ac97b7eb99c06107bb4536e1a888fce7837213a
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix no-GPU build & rename parameters #Patch Set 3 : Added comment #Patch Set 4 : update to ToT again #
Messages
Total messages: 31 (17 generated)
Description was changed from ========== Switch SkImageSource image filter over to new onFilterImage interface ========== to ========== Switch SkImageSource image filter 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 SkImageSource image filter over to new onFilterImage interface GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkImageSource image filter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1762013002/ (Swap over to using SkImageFilter::filterImage instead of filterImageDeprecated) 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, senorblanco@google.com
https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.cpp File src/core/SkSpecialImage.cpp (right): https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.cpp... src/core/SkSpecialImage.cpp:149: result->setPixelRef(new SkGrPixelRef(info, texture))->unref(); I think this will break the no-GPU build, and probably should be wrapped in an #if SK_SUPPORT_GPU. (and maybe getBitmap() should also be getBitmapDeprecated()?) https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.h#n... src/core/SkSpecialImage.h:55: static SkSpecialImage* NewFromImage(SkImageFilter::Proxy*, Could we instead add a NewFromImageDeprecated() which takes the Proxy, and leave the existing NewFromImage() in place? https://codereview.chromium.org/1772933002/diff/1/src/effects/SkImageSource.cpp File src/effects/SkImageSource.cpp (right): https://codereview.chromium.org/1772933002/diff/1/src/effects/SkImageSource.c... src/effects/SkImageSource.cpp:77: return SkSpecialImage::NewFromImage(src->internal_getProxy(), This seems unfortunate. I'm guessing we need the proxy here for downstream nodes which still use the legacy path? If we can't hide this ugly, could we make this a call to NewFromImageDeprecated() instead, and add a TODO so we remember to circle back and remove it?
https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.cpp File src/core/SkSpecialImage.cpp (right): https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.cpp... src/core/SkSpecialImage.cpp:149: result->setPixelRef(new SkGrPixelRef(info, texture))->unref(); On 2016/03/07 21:47:26, Stephen White wrote: > I think this will break the no-GPU build, and probably should be wrapped in an > #if SK_SUPPORT_GPU. > > (and maybe getBitmap() should also be getBitmapDeprecated()?) Done. I'll rename getBitmap in a separate CL though. https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.h#n... src/core/SkSpecialImage.h:55: static SkSpecialImage* NewFromImage(SkImageFilter::Proxy*, On 2016/03/07 21:47:26, Stephen White wrote: > Could we instead add a NewFromImageDeprecated() which takes the Proxy, and leave > the existing NewFromImage() in place? Well, it now matches the other two entry points. I initially thought I wouldn't need to carry along the Proxy for SkImage-backed SkSpecialImages but the non-clipped case in imagesource requires it. Once all the imagefilters are swapped over they proxy parameter will be removed from all three entry points. https://codereview.chromium.org/1772933002/diff/1/src/effects/SkImageSource.cpp File src/effects/SkImageSource.cpp (right): https://codereview.chromium.org/1772933002/diff/1/src/effects/SkImageSource.c... src/effects/SkImageSource.cpp:77: return SkSpecialImage::NewFromImage(src->internal_getProxy(), On 2016/03/07 21:47:26, Stephen White wrote: > This seems unfortunate. I'm guessing we need the proxy here for downstream nodes > which still use the legacy path? > > If we can't hide this ugly, could we make this a call to > NewFromImageDeprecated() instead, and add a TODO so we remember to circle back > and remove it? It is unfortunate but we do need it for downstream legacy nodes. If we did have a NewFromImage and NewFromImageDeprecated we should probably do the same to the other two NewFrom* entry points. I don't think we're in any danger of forgetting to remove the proxy when we're able to though.
LGTM https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1772933002/diff/1/src/core/SkSpecialImage.h#n... src/core/SkSpecialImage.h:55: static SkSpecialImage* NewFromImage(SkImageFilter::Proxy*, On 2016/03/08 16:45:29, robertphillips wrote: > On 2016/03/07 21:47:26, Stephen White wrote: > > Could we instead add a NewFromImageDeprecated() which takes the Proxy, and > leave > > the existing NewFromImage() in place? > > Well, it now matches the other two entry points. I initially thought I wouldn't > need to carry along the Proxy for SkImage-backed SkSpecialImages but the > non-clipped case in imagesource requires it. > > Once all the imagefilters are swapped over they proxy parameter will be removed > from all three entry points. Ahh, good point. https://codereview.chromium.org/1772933002/diff/1/src/effects/SkImageSource.cpp File src/effects/SkImageSource.cpp (right): https://codereview.chromium.org/1772933002/diff/1/src/effects/SkImageSource.c... src/effects/SkImageSource.cpp:77: return SkSpecialImage::NewFromImage(src->internal_getProxy(), On 2016/03/08 16:45:29, robertphillips wrote: > On 2016/03/07 21:47:26, Stephen White wrote: > > This seems unfortunate. I'm guessing we need the proxy here for downstream > nodes > > which still use the legacy path? > > > > If we can't hide this ugly, could we make this a call to > > NewFromImageDeprecated() instead, and add a TODO so we remember to circle back > > and remove it? > > It is unfortunate but we do need it for downstream legacy nodes. > > If we did have a NewFromImage and NewFromImageDeprecated we should probably do > the same to the other two NewFrom* entry points. I don't think we're in any > danger of forgetting to remove the proxy when we're able to though. 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/1772933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772933002/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/1772933002/#ps40001 (title: "Added comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772933002/40001
Description was changed from ========== Switch SkImageSource image filter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1762013002/ (Swap over to using SkImageFilter::filterImage instead of filterImageDeprecated) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkImageSource image filter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1762013002/ (Swap over to using SkImageFilter::filterImage instead of filterImageDeprecated) TBR=bsalomon@google.com 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
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772933002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
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/1772933002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772933002/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/1772933002/#ps60001 (title: "update to ToT again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772933002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772933002/60001
Message was sent while issue was closed.
Description was changed from ========== Switch SkImageSource image filter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1762013002/ (Swap over to using SkImageFilter::filterImage instead of filterImageDeprecated) TBR=bsalomon@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkImageSource image filter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1762013002/ (Swap over to using SkImageFilter::filterImage instead of filterImageDeprecated) TBR=bsalomon@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/+/6ac97b7eb99c06107bb4536e1a888fce7837213a ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/6ac97b7eb99c06107bb4536e1a888fce7837213a |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
