Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1579323002:
Add SkSpecialImage & SkSpecialSurface classes (Closed)
|
Created:
4 years, 11 months ago by robertphillips Modified:
4 years, 10 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 & SkSpecialSurface classes
Initial classes.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1579323002
Committed: https://skia.googlesource.com/skia/+/b6c65e99956b867e24bd5bd68ae37673a9fd4b27
Patch Set 1 #Patch Set 2 : update #Patch Set 3 : Clean up #
Total comments: 12
Patch Set 4 : Address code review comments #
Total comments: 6
Patch Set 5 : update #Patch Set 6 : updated from conversation #
Total comments: 2
Patch Set 7 : update to ToT #Patch Set 8 : Someone removed SK_ALWAYSBREAK #Patch Set 9 : Fix other version issues (e.g., noGpu) #Patch Set 10 : Fix another no-gpu issue #
Messages
Total messages: 41 (16 generated)
Description was changed from ========== Add SkSpecialImage & SkSpecialSurface classes ========== to ========== Add SkSpecialImage & SkSpecialSurface classes GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add SkSpecialImage & SkSpecialSurface classes GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkSpecialImage & SkSpecialSurface classes Initial classes. 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, reed@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/1579323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579323002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... File src/core/SkSpecialImage.cpp (right): https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.cpp:13: class SkSpecialImage_Base : public SkSpecialImage { Wondering why the SpecialImage_Base/SpecialImage distinction for an internal class, esp w/ no public methods. What is being protected? https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:25: * - it can be backed by a GrTexture larger than its nominal bounds Should we generalize to allow a irect subset rather than w/h? Seems easy to do and it might be easier to do now rather than later once we add a lot of code that assumes TL is (0,0). https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:26: * - it can't be drawn tiled Need a restriction on filtering too (no MIPS).
https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:52: const void* peekPixels(SkImageInfo* info, size_t* rowBytes) const; nit: bool peekPixels(SkPixmap*) const; https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:61: static SkSpecialImage* New(int width, int height, const SkBitmap&); Does this ever copy the pixels from the bitmap? https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialSurfa... File src/core/SkSpecialSurface.h (right): https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialSurfa... src/core/SkSpecialSurface.h:54: static SkSpecialSurface* New(GrContext*, const GrSurfaceDesc&, const SkSurfaceProps* = nullptr); I suggest we make the names back-end specific, just for more clarity at the call-sites: e.g. NewGpu(...) NewRaster(...)
On 2016/01/13 21:39:17, bsalomon wrote: > https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... > File src/core/SkSpecialImage.cpp (right): > > https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... > src/core/SkSpecialImage.cpp:13: class SkSpecialImage_Base : public > SkSpecialImage { > Wondering why the SpecialImage_Base/SpecialImage distinction for an internal > class, esp w/ no public methods. What is being protected? BTW just wanted to be clear that I'm not opposed to the separation just wondering what the motivation is. > > https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage.h > File src/core/SkSpecialImage.h (right): > > https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... > src/core/SkSpecialImage.h:25: * - it can be backed by a GrTexture larger > than its nominal bounds > Should we generalize to allow a irect subset rather than w/h? Seems easy to do > and it might be easier to do now rather than later once we add a lot of code > that assumes TL is (0,0). > > https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... > src/core/SkSpecialImage.h:26: * - it can't be drawn tiled > Need a restriction on filtering too (no MIPS).
https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... File src/core/SkSpecialImage.cpp (right): https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.cpp:13: class SkSpecialImage_Base : public SkSpecialImage { On 2016/01/13 21:39:17, bsalomon wrote: > Wondering why the SpecialImage_Base/SpecialImage distinction for an internal > class, esp w/ no public methods. What is being protected? Mainly b.c. it was based on SkImage, which has it, and Mike & I were earlier talking about expanding the use of the pattern. https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:26: * - it can't be drawn tiled On 2016/01/13 21:39:17, bsalomon wrote: > Need a restriction on filtering too (no MIPS). Done.
New version - PTAL. https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:25: * - it can be backed by a GrTexture larger than its nominal bounds On 2016/01/13 21:39:17, bsalomon wrote: > Should we generalize to allow a irect subset rather than w/h? Seems easy to do > and it might be easier to do now rather than later once we add a lot of code > that assumes TL is (0,0). Done. https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:52: const void* peekPixels(SkImageInfo* info, size_t* rowBytes) const; On 2016/01/14 13:52:55, reed1 wrote: > nit: > > bool peekPixels(SkPixmap*) const; Done. https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:61: static SkSpecialImage* New(int width, int height, const SkBitmap&); No. https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialSurfa... File src/core/SkSpecialSurface.h (right): https://codereview.chromium.org/1579323002/diff/40001/src/core/SkSpecialSurfa... src/core/SkSpecialSurface.h:54: static SkSpecialSurface* New(GrContext*, const GrSurfaceDesc&, const SkSurfaceProps* = nullptr); On 2016/01/14 13:52:55, reed1 wrote: > I suggest we make the names back-end specific, just for more clarity at the > call-sites: > > e.g. > NewGpu(...) > NewRaster(...) Done.
https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:34: const SkIRect& activeRect() const { return fActiveRect; } width() height() still seems handy Speculating... Should active rects only be part of subclasses that must subset a larger thing? All the subclasses that can only hold cpu pixels can always adjust their internal state to point to the top left pixel. https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:52: Should we have peekTexture? Maybe peekTexture(SkIRect* activeRect) if above speculative suggestion is taken. https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:61: static SkSpecialImage* NewGpu(const SkIRect& activeRect, GrTexture*); NewFromTexture to match SkImage? wonder if subset is a more natural name for the rectangle.
https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage.h File src/core/SkSpecialImage.h (right): https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:34: const SkIRect& activeRect() const { return fActiveRect; } I readded width() & height(). I'm not sure re the other question. I think even if we have SkBitmap control its own subset we would still want SkSpecialImage to have a top-level subset() interface. So I believe we can defer resolution of the question until later. https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:52: There is a peekTexture below. I think we should always have a subset() call though so don't know if returning the subset would be useful. https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage... src/core/SkSpecialImage.h:61: static SkSpecialImage* NewGpu(const SkIRect& activeRect, GrTexture*); On 2016/01/14 20:09:10, bsalomon wrote: > NewFromTexture to match SkImage? > > wonder if subset is a more natural name for the rectangle. Done.
On 2016/01/15 13:12:47, robertphillips wrote: > https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage.h > File src/core/SkSpecialImage.h (right): > > https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage... > src/core/SkSpecialImage.h:34: const SkIRect& activeRect() const { return > fActiveRect; } > I readded width() & height(). > > I'm not sure re the other question. I think even if we have SkBitmap control its > own subset we would still want SkSpecialImage to have a top-level subset() > interface. So I believe we can defer resolution of the question until later. My thinking was that given an instance the base class, SkSpecialImage, it isn't clear what the subset() refers to. I suppose the peek methods add some clarity. > > https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage... > src/core/SkSpecialImage.h:52: > There is a peekTexture below. I think we should always have a subset() call > though so don't know if returning the subset would be useful. > > https://codereview.chromium.org/1579323002/diff/60001/src/core/SkSpecialImage... > src/core/SkSpecialImage.h:61: static SkSpecialImage* NewGpu(const SkIRect& > activeRect, GrTexture*); > On 2016/01/14 20:09:10, bsalomon wrote: > > NewFromTexture to match SkImage? > > > > wonder if subset is a more natural name for the rectangle. > > Done.
This version hides the subset, peekPixels & peekTexture entry points and adds a factory function for subset SkBitmap-backed special surfaces.
ping
question, otherwise lgtm https://codereview.chromium.org/1579323002/diff/100001/src/core/SkSpecialImag... File src/core/SkSpecialImage.cpp (right): https://codereview.chromium.org/1579323002/diff/100001/src/core/SkSpecialImag... src/core/SkSpecialImage.cpp:82: GrTexture* texture = fImage->getTexture(); Wondering about sample counts.. what do filters do today?
https://codereview.chromium.org/1579323002/diff/100001/src/core/SkSpecialImag... File src/core/SkSpecialImage.cpp (right): https://codereview.chromium.org/1579323002/diff/100001/src/core/SkSpecialImag... src/core/SkSpecialImage.cpp:82: GrTexture* texture = fImage->getTexture(); So we may have to add a 'sampleCnt' parameter to this method (like the SkSurface::NewRenderTarget call). Currently, a new SkDevice is created through the Proxy object. In the DeviceProxy subclass the 'createDevice' method is called which, in turn, calls 'onCreateDevice' on a wrapped device. For SkGpuDevice this replicates the sampleCnt from the wrapped SkGpuDevice.
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/1579323002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579323002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac10.8-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.8-Clang-...)
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/1579323002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579323002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
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/1579323002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579323002/160001
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/1579323002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579323002/180001
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 bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1579323002/#ps180001 (title: "Fix another no-gpu issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579323002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579323002/180001
Message was sent while issue was closed.
Description was changed from ========== Add SkSpecialImage & SkSpecialSurface classes Initial classes. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add SkSpecialImage & SkSpecialSurface classes Initial classes. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/b6c65e99956b867e24bd5bd68ae37673a9fd4b27 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/b6c65e99956b867e24bd5bd68ae37673a9fd4b27 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
