Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
821083002:
add newImage API (Closed)
Descriptionadd newImage API
BUG=skia:3277
related bug: skbug.com/3276
Committed: https://skia.googlesource.com/skia/+/f803da12cff1d9b6148fea319220351efebfd1e0
Patch Set 1 #Patch Set 2 : initial draft of API #
Total comments: 2
Patch Set 3 : tighten constraint on valid subset parameter #
Total comments: 3
Patch Set 4 : rebase #Patch Set 5 : default impl #Patch Set 6 : add gm #Patch Set 7 : add SkFilterQuality option #
Total comments: 12
Patch Set 8 : #Patch Set 9 : initial LEGACY flag for new enum #Patch Set 10 : update dox for unpremul->premul #Patch Set 11 : add gm #
Total comments: 3
Patch Set 12 : #
Total comments: 22
Patch Set 13 : updates to gms #Patch Set 14 : fix warning #Patch Set 15 : include SkRandom.h #
Total comments: 11
Patch Set 16 : reinforce non-zero requirement #Patch Set 17 : check when subset == bounds #
Messages
Total messages: 56 (15 generated)
reed@google.com changed reviewers: + bsalomon@google.com, djsollen@google.com, robertphillips@google.com, scroggo@google.com
https://codereview.chromium.org/821083002/diff/20001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/20001/include/core/SkImage.h#n... include/core/SkImage.h:138: SkImage* newImage(int newWidth, int newHeight, const SkIRect* subset = NULL) const; Should there be control over filtering mode/quality? How does subset interact with filtering? Are the denominators in the scale factors computed before or after subset is clipped to the src bounds? For example if the old image is 100 high, the newHeight is 200, the subrect goes from -50, 50 in y - what is the height of the returned image and does it have any padding/trans-black pixels?
https://codereview.chromium.org/821083002/diff/20001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/20001/include/core/SkImage.h#n... include/core/SkImage.h:138: SkImage* newImage(int newWidth, int newHeight, const SkIRect* subset = NULL) const; On 2014/12/22 21:31:06, bsalomon wrote: > Should there be control over filtering mode/quality? How does subset interact > with filtering? > > Are the denominators in the scale factors computed before or after subset is > clipped to the src bounds? For example if the old image is 100 high, the > newHeight is 200, the subrect goes from -50, 50 in y - what is the height of the > returned image and does it have any padding/trans-black pixels? Good question -- in my next rev. I'm proposing changing the API to require that subset be exactly that : a subset of the original bounds. less is more.
https://codereview.chromium.org/821083002/diff/40001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/40001/include/core/SkImage.h#n... include/core/SkImage.h:137: SkImage* newImage(int newWidth, int newHeight, const SkIRect* subset = NULL) const; For use by Android, it might be nice to be able to have an option to supply the memory for pixels - or, to let them know how much was used, if we can convince them to let us use a native allocation (see https://docs.google.com/document/d/1Axl4VtjxX5kAwX87mLFzni8Ii4Cnsr7BFMsaWY9vF...). (I suppose in that case, they can use SkImageInfo::minRowBytes().)
reed@chromium.org changed reviewers: + reed@chromium.org
Derek, Where is the investigation on annotating native allocations for the VM? https://codereview.chromium.org/821083002/diff/40001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/40001/include/core/SkImage.h#n... include/core/SkImage.h:137: SkImage* newImage(int newWidth, int newHeight, const SkIRect* subset = NULL) const; On 2014/12/22 22:08:27, scroggo wrote: > For use by Android, it might be nice to be able to have an option to supply the > memory for pixels - or, to let them know how much was used, if we can convince > them to let us use a native allocation (see > https://docs.google.com/document/d/1Axl4VtjxX5kAwX87mLFzni8Ii4Cnsr7BFMsaWY9vF...). > (I suppose in that case, they can use SkImageInfo::minRowBytes().) For GPU images, it is hard to see how to supply the memory. For CPU images, I guess we could add an optional allocator callback. It seems more uniform and cleaner if we can augment the VM so we can annotate ALL of our large allocations so that Android apps can know where their memory went (not just some images).
On 2014/12/22 21:39:48, reed1 wrote: > https://codereview.chromium.org/821083002/diff/20001/include/core/SkImage.h > File include/core/SkImage.h (right): > > https://codereview.chromium.org/821083002/diff/20001/include/core/SkImage.h#n... > include/core/SkImage.h:138: SkImage* newImage(int newWidth, int newHeight, const > SkIRect* subset = NULL) const; > On 2014/12/22 21:31:06, bsalomon wrote: > > Should there be control over filtering mode/quality? How does subset interact > > with filtering? > > > > Are the denominators in the scale factors computed before or after subset is > > clipped to the src bounds? For example if the old image is 100 high, the > > newHeight is 200, the subrect goes from -50, 50 in y - what is the height of > the > > returned image and does it have any padding/trans-black pixels? > > Good question -- in my next rev. I'm proposing changing the API to require that > subset be exactly that : a subset of the original bounds. less is more. Seems like a safe place to start. What about filtering? Undefined or should it take a filter level?
https://codereview.chromium.org/821083002/diff/40001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/40001/include/core/SkImage.h#n... include/core/SkImage.h:137: SkImage* newImage(int newWidth, int newHeight, const SkIRect* subset = NULL) const; On 2014/12/24 19:47:26, reed2 wrote: > On 2014/12/22 22:08:27, scroggo wrote: > > For use by Android, it might be nice to be able to have an option to supply > the > > memory for pixels - or, to let them know how much was used, if we can convince > > them to let us use a native allocation (see > > > https://docs.google.com/document/d/1Axl4VtjxX5kAwX87mLFzni8Ii4Cnsr7BFMsaWY9vF...). > > (I suppose in that case, they can use SkImageInfo::minRowBytes().) > > For GPU images, it is hard to see how to supply the memory. For CPU images, I > guess we could add an optional allocator callback. It seems more uniform and > cleaner if we can augment the VM so we can annotate ALL of our large allocations > so that Android apps can know where their memory went (not just some images). Agreed. The more I think about it, the more I think annotating native allocations is the right way to go. When I talked to Derek about it last week, his plan was to work with the VM team to get something added, but at the time he had not heard anything yet.
What to do about filter-quality when scaling? a. pick a quality b. take a parameter c. other?
add SkFilterQuality parameter (defaults to none)
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821083002/120001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-01-01 04:02 UTC
The CQ bit was unchecked by reed@google.com
https://codereview.chromium.org/821083002/diff/120001/include/core/SkFilterQu... File include/core/SkFilterQuality.h (right): https://codereview.chromium.org/821083002/diff/120001/include/core/SkFilterQu... include/core/SkFilterQuality.h:13: enum SkFilterQuality { Could you add a description of what this is for? https://codereview.chromium.org/821083002/diff/120001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/120001/include/core/SkImage.h#... include/core/SkImage.h:140: * (if the new dimensions == the src dimensions and any subset encloses the entire src). This comment seems to conflict with the code and the comment in the previous paragraph. This comment appears to state that using a "subset" that is bigger than this SkImage will return this SkImage (whereas we actually return NULL). https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h#... include/core/SkPaint.h:323: void setFilterQuality(SkFilterQuality quality) { Do we need both? (enums and methods) https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:134: opaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType); If this image is unpremul, does this work properly? And is it deliberate that the new SkImage is premul rather than unpremul?
https://codereview.chromium.org/821083002/diff/120001/include/core/SkFilterQu... File include/core/SkFilterQuality.h (right): https://codereview.chromium.org/821083002/diff/120001/include/core/SkFilterQu... include/core/SkFilterQuality.h:13: enum SkFilterQuality { On 2015/01/05 16:15:33, scroggo wrote: > Could you add a description of what this is for? Done. https://codereview.chromium.org/821083002/diff/120001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/120001/include/core/SkImage.h#... include/core/SkImage.h:140: * (if the new dimensions == the src dimensions and any subset encloses the entire src). On 2015/01/05 16:15:33, scroggo wrote: > This comment seems to conflict with the code and the comment in the previous > paragraph. This comment appears to state that using a "subset" that is bigger > than this SkImage will return this SkImage (whereas we actually return NULL). Done. https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h#... include/core/SkPaint.h:323: void setFilterQuality(SkFilterQuality quality) { On 2015/01/05 16:15:33, scroggo wrote: > Do we need both? (enums and methods) For now we need filterlevel, until we decided to update existing callers (which are many). https://code.google.com/p/skia/issues/detail?id=3284 https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:134: opaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType); On 2015/01/05 16:15:33, scroggo wrote: > If this image is unpremul, does this work properly? And is it deliberate that > the new SkImage is premul rather than unpremul? This is the "base" implementation, which may change the alphatype (but in theory now how the image actually draws). Future CLs (I hope) will be more optimal for specific subclasses, where we can avoid "drawing" to create the subset.
https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h#... include/core/SkPaint.h:323: void setFilterQuality(SkFilterQuality quality) { On 2015/01/05 21:58:14, reed1 wrote: > On 2015/01/05 16:15:33, scroggo wrote: > > Do we need both? (enums and methods) > > For now we need filterlevel, until we decided to update existing callers (which > are many). > https://code.google.com/p/skia/issues/detail?id=3284 Should we go ahead and use a #define to stage? https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:134: opaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType); On 2015/01/05 21:58:14, reed1 wrote: > On 2015/01/05 16:15:33, scroggo wrote: > > If this image is unpremul, does this work properly? And is it deliberate that > > the new SkImage is premul rather than unpremul? > > This is the "base" implementation, which may change the alphatype (but in theory > now how the image actually draws). Future CLs (I hope) will be more optimal for > specific subclasses, where we can avoid "drawing" to create the subset. +1 to the more optimal subclass overrides. In the meantime, won't this break for an unpremul image? (Or did you already add drawing unpremul sources?)
https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h#... include/core/SkPaint.h:323: void setFilterQuality(SkFilterQuality quality) { On 2015/01/05 22:12:02, scroggo wrote: > On 2015/01/05 21:58:14, reed1 wrote: > > On 2015/01/05 16:15:33, scroggo wrote: > > > Do we need both? (enums and methods) > > > > For now we need filterlevel, until we decided to update existing callers > (which > > are many). > > https://code.google.com/p/skia/issues/detail?id=3284 > > Should we go ahead and use a #define to stage? Started too fix all of skia internally, but it is very large, and will make it hard to separate reviewing that syntactic change from this functional change. I propose doing that in a follow-on CL. https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:134: opaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType); On 2015/01/05 22:12:02, scroggo wrote: > On 2015/01/05 21:58:14, reed1 wrote: > > On 2015/01/05 16:15:33, scroggo wrote: > > > If this image is unpremul, does this work properly? And is it deliberate > that > > > the new SkImage is premul rather than unpremul? > > > > This is the "base" implementation, which may change the alphatype (but in > theory > > now how the image actually draws). Future CLs (I hope) will be more optimal > for > > specific subclasses, where we can avoid "drawing" to create the subset. > > +1 to the more optimal subclass overrides. > > In the meantime, won't this break for an unpremul image? (Or did you already add > drawing unpremul sources?) unpremul : good point, have to think about that case...
On 2015/01/20 22:23:08, reed1 wrote: > https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h > File include/core/SkPaint.h (right): > > https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h#... > include/core/SkPaint.h:323: void setFilterQuality(SkFilterQuality quality) { > On 2015/01/05 22:12:02, scroggo wrote: > > On 2015/01/05 21:58:14, reed1 wrote: > > > On 2015/01/05 16:15:33, scroggo wrote: > > > > Do we need both? (enums and methods) > > > > > > For now we need filterlevel, until we decided to update existing callers > > (which > > > are many). > > > https://code.google.com/p/skia/issues/detail?id=3284 > > > > Should we go ahead and use a #define to stage? > > Started too fix all of skia internally, but it is very large, and will make it > hard to separate reviewing that syntactic change from this functional change. I > propose doing that in a follow-on CL. > > https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp > File src/image/SkImage.cpp (right): > > https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp#n... > src/image/SkImage.cpp:134: opaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType); > On 2015/01/05 22:12:02, scroggo wrote: > > On 2015/01/05 21:58:14, reed1 wrote: > > > On 2015/01/05 16:15:33, scroggo wrote: > > > > If this image is unpremul, does this work properly? And is it deliberate > > that > > > > the new SkImage is premul rather than unpremul? > > > > > > This is the "base" implementation, which may change the alphatype (but in > > theory > > > now how the image actually draws). Future CLs (I hope) will be more optimal > > for > > > specific subclasses, where we can avoid "drawing" to create the subset. > > > > +1 to the more optimal subclass overrides. > > > > In the meantime, won't this break for an unpremul image? (Or did you already > add > > drawing unpremul sources?) > > unpremul : good point, have to think about that case... I think initially unpremul should either fail, or magically turn into premul. I don't think the latter is completely unreasonable (tho not perfect), esp. since that is what would have to happen if the caller tried to resize via drawing today. What do you think?
On 2015/01/20 22:24:32, reed1 wrote: > On 2015/01/20 22:23:08, reed1 wrote: > > https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h > > File include/core/SkPaint.h (right): > > > > > https://codereview.chromium.org/821083002/diff/120001/include/core/SkPaint.h#... > > include/core/SkPaint.h:323: void setFilterQuality(SkFilterQuality quality) { > > On 2015/01/05 22:12:02, scroggo wrote: > > > On 2015/01/05 21:58:14, reed1 wrote: > > > > On 2015/01/05 16:15:33, scroggo wrote: > > > > > Do we need both? (enums and methods) > > > > > > > > For now we need filterlevel, until we decided to update existing callers > > > (which > > > > are many). > > > > https://code.google.com/p/skia/issues/detail?id=3284 > > > > > > Should we go ahead and use a #define to stage? > > > > Started too fix all of skia internally, but it is very large, and will make it > > hard to separate reviewing that syntactic change from this functional change. > I > > propose doing that in a follow-on CL. > > > > https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp > > File src/image/SkImage.cpp (right): > > > > > https://codereview.chromium.org/821083002/diff/120001/src/image/SkImage.cpp#n... > > src/image/SkImage.cpp:134: opaque ? kOpaque_SkAlphaType : > kPremul_SkAlphaType); > > On 2015/01/05 22:12:02, scroggo wrote: > > > On 2015/01/05 21:58:14, reed1 wrote: > > > > On 2015/01/05 16:15:33, scroggo wrote: > > > > > If this image is unpremul, does this work properly? And is it deliberate > > > that > > > > > the new SkImage is premul rather than unpremul? > > > > > > > > This is the "base" implementation, which may change the alphatype (but in > > > theory > > > > now how the image actually draws). Future CLs (I hope) will be more > optimal > > > for > > > > specific subclasses, where we can avoid "drawing" to create the subset. > > > > > > +1 to the more optimal subclass overrides. > > > > > > In the meantime, won't this break for an unpremul image? (Or did you already > > add > > > drawing unpremul sources?) > > > > unpremul : good point, have to think about that case... > > I think initially unpremul should either fail, or magically turn into premul. I > don't think the latter is completely unreasonable (tho not perfect), esp. since > that is what would have to happen if the caller tried to resize via drawing > today. What do you think? That sounds fine to me, so long as it's documented. (And by "fail", I suppose you mean return NULL?)
done. will add tests next.
add gm. PTAL
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821083002/200001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-01-22 03:18 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia (http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)
https://codereview.chromium.org/821083002/diff/200001/gm/image.cpp File gm/image.cpp (right): https://codereview.chromium.org/821083002/diff/200001/gm/image.cpp#newcode221 gm/image.cpp:221: SkRandom rand; nit: Maybe we shouldn't use SkRandom in gms until we figure out why it gives different results sometimes on some platforms? https://codereview.chromium.org/821083002/diff/200001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/200001/include/core/SkImage.h#... include/core/SkImage.h:148: * to premul (as required by the impl). The image should draw (nearly) identically, As discussed in person, it seems like this will *require* changing to premul (unless we break down and let SkImage know its SkImageInfo, or at least its SkAlphaType). https://codereview.chromium.org/821083002/diff/200001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/821083002/diff/200001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:149: const SkImageInfo info = SkImageInfo::Make(newWidth, newHeight, kN32_SkColorType, As discussed in person, we don't actually know the alpha type of this, so we don't know whether we can draw in software....
nits https://codereview.chromium.org/821083002/diff/220001/gm/drawbitmaprect.cpp File gm/drawbitmaprect.cpp (right): https://codereview.chromium.org/821083002/diff/220001/gm/drawbitmaprect.cpp#n... gm/drawbitmaprect.cpp:131: static const int kBmpSize = 2048; onOnceBeforeDraw for this ? https://codereview.chromium.org/821083002/diff/220001/gm/drawbitmaprect.cpp#n... gm/drawbitmaprect.cpp:222: DEF_GM( return new DrawBitmapRectGM(imageproc, "_imagerect"); ) no '-' ? https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp File gm/image.cpp (right): https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode207 gm/image.cpp:207: protected: one line ? https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode211 gm/image.cpp:211: one line ? https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode215 gm/image.cpp:215: drawIntoImage or make static (and then DrawIntoImage) ? https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode230 gm/image.cpp:230: makeImage or make static ? https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode240 gm/image.cpp:240: same https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode259 gm/image.cpp:259: same https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode266 gm/image.cpp:266: SkIRect subset = SkIRect::MakeLTRB(W/4, H/4, W/2, H/2); this-> ? https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode272 gm/image.cpp:272: SkAutoTUnref ? https://codereview.chromium.org/821083002/diff/220001/include/core/SkFilterQu... File include/core/SkFilterQuality.h (right): https://codereview.chromium.org/821083002/diff/220001/include/core/SkFilterQu... include/core/SkFilterQuality.h:20: kMedium_SkFilterQuality, //!< typically bilerp + mipmaps for down-scaling add "+ mipmaps for down-scaling" ?
https://codereview.chromium.org/821083002/diff/220001/gm/drawbitmaprect.cpp File gm/drawbitmaprect.cpp (right): https://codereview.chromium.org/821083002/diff/220001/gm/drawbitmaprect.cpp#n... gm/drawbitmaprect.cpp:131: static const int kBmpSize = 2048; On 2015/01/22 15:50:02, robertphillips wrote: > onOnceBeforeDraw for this ? Done. https://codereview.chromium.org/821083002/diff/220001/gm/drawbitmaprect.cpp#n... gm/drawbitmaprect.cpp:222: DEF_GM( return new DrawBitmapRectGM(imageproc, "_imagerect"); ) On 2015/01/22 15:50:02, robertphillips wrote: > no '-' ? Done. https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp File gm/image.cpp (right): https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode207 gm/image.cpp:207: protected: On 2015/01/22 15:50:02, robertphillips wrote: > one line ? Done. https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode211 gm/image.cpp:211: On 2015/01/22 15:50:02, robertphillips wrote: > one line ? Done. https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode215 gm/image.cpp:215: On 2015/01/22 15:50:02, robertphillips wrote: > drawIntoImage or make static (and then DrawIntoImage) ? Done. https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode230 gm/image.cpp:230: On 2015/01/22 15:50:02, robertphillips wrote: > makeImage or make static ? Done. https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode240 gm/image.cpp:240: On 2015/01/22 15:50:02, robertphillips wrote: > same Done. https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode259 gm/image.cpp:259: On 2015/01/22 15:50:02, robertphillips wrote: > same Done. https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode266 gm/image.cpp:266: SkIRect subset = SkIRect::MakeLTRB(W/4, H/4, W/2, H/2); On 2015/01/22 15:50:02, robertphillips wrote: > this-> ? Done. https://codereview.chromium.org/821083002/diff/220001/gm/image.cpp#newcode272 gm/image.cpp:272: On 2015/01/22 15:50:02, robertphillips wrote: > SkAutoTUnref ? Done. https://codereview.chromium.org/821083002/diff/220001/include/core/SkFilterQu... File include/core/SkFilterQuality.h (right): https://codereview.chromium.org/821083002/diff/220001/include/core/SkFilterQu... include/core/SkFilterQuality.h:20: kMedium_SkFilterQuality, //!< typically bilerp + mipmaps for down-scaling On 2015/01/22 15:50:02, robertphillips wrote: > add "+ mipmaps for down-scaling" ? Maybe. Not sure how much I really want to say anyway, since I don't want to actually promise anything specific yet.
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821083002/240001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-01-23 00:26 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia (http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)
The CQ bit was checked by reed@google.com
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821083002/280001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-01-23 01:39 UTC
https://codereview.chromium.org/821083002/diff/280001/gm/image.cpp File gm/image.cpp (right): https://codereview.chromium.org/821083002/diff/280001/gm/image.cpp#newcode213 gm/image.cpp:213: void drawIntoImage(SkCanvas* canvas) { Can all these functions be static? https://codereview.chromium.org/821083002/diff/280001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/280001/include/core/SkImage.h#... include/core/SkImage.h:152: SkImage* newImage(int newWidth, int newHeight, const SkIRect* subset = NULL, Why not take an SkImageInfo instead of a width/height? Right now we silently give them kN32 Maybe that's okay, but we should perhaps document it? Unless we think we might make a different decision later? That would also give them the option of specifying the alpha type. https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:99: if (newWidth < 0 || newHeight < 0) { Did you make a decision on whether to allow SkImages with width/height of zero? https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:106: if (NULL == subset && this->width() == newWidth && this->height() == newHeight) { What if the subset == this->dimensions? (Silly use case, I know...) https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage_Base.h File src/image/SkImage_Base.h (right): https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage_Base.... src/image/SkImage_Base.h:62: // newWidth > 0, newHeight > 0, subset either NULL or a proper subset of this bounds Doesn't match the impl, which only checks for < 0.
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-01-23 03:12 UTC
The CQ bit was unchecked by reed@google.com
https://codereview.chromium.org/821083002/diff/280001/gm/image.cpp File gm/image.cpp (right): https://codereview.chromium.org/821083002/diff/280001/gm/image.cpp#newcode213 gm/image.cpp:213: void drawIntoImage(SkCanvas* canvas) { On 2015/01/22 20:05:14, scroggo wrote: > Can all these functions be static? At the moment, yes, but earlier they used some internal state. It proved a little laborious to switch the naming convention back-n-forth each time that changed, so I'm voting for members until some future refactoring finds some value in changing back to static. https://codereview.chromium.org/821083002/diff/280001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/821083002/diff/280001/include/core/SkImage.h#... include/core/SkImage.h:152: SkImage* newImage(int newWidth, int newHeight, const SkIRect* subset = NULL, On 2015/01/22 20:05:14, scroggo wrote: > Why not take an SkImageInfo instead of a width/height? Right now we silently > give them kN32 Maybe that's okay, but we should perhaps document it? Unless we > think we might make a different decision later? That would also give them the > option of specifying the alpha type. Being conservative. Since we don't even reveal the colortype or alphatype today, it seemed premature to allow them to request something specific during resize. If we hit some user that really needs more control, we can revisit the api then. Is that ok? https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:99: if (newWidth < 0 || newHeight < 0) { On 2015/01/22 20:05:14, scroggo wrote: > Did you make a decision on whether to allow SkImages with width/height of zero? Good question... I'll give you an answer soon. https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:106: if (NULL == subset && this->width() == newWidth && this->height() == newHeight) { On 2015/01/22 20:05:14, scroggo wrote: > What if the subset == this->dimensions? > > (Silly use case, I know...) Ah, good catch. Will add that check. https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage_Base.h File src/image/SkImage_Base.h (right): https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage_Base.... src/image/SkImage_Base.h:62: // newWidth > 0, newHeight > 0, subset either NULL or a proper subset of this bounds On 2015/01/22 20:05:14, scroggo wrote: > Doesn't match the impl, which only checks for < 0. Acknowledged.
https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/821083002/diff/280001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:99: if (newWidth < 0 || newHeight < 0) { On 2015/01/22 20:05:14, scroggo wrote: > Did you make a decision on whether to allow SkImages with width/height of zero? Zero is illegal, will update this check.
ptal
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821083002/310009
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-01-23 04:05 UTC
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. Transient error: <urlopen error timed out>
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821083002/310009
Message was sent while issue was closed.
Committed patchset #17 (id:310009) as https://skia.googlesource.com/skia/+/f803da12cff1d9b6148fea319220351efebfd1e0 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
