Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2066903003:
Document SkSurface::MakeRaster's memory initialization (Closed)
DescriptionDocument SkSurface::MakeRaster's memory initialization
So clients don't go clearing w/ SK_ColorTRANSPARENT unnecessarily.
R=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2066903003
Committed: https://skia.googlesource.com/skia/+/03912f141fba6a5c17ac7e8fbe5998ac3834e29c
Patch Set 1 #
Total comments: 7
Patch Set 2 : review #
Messages
Total messages: 25 (9 generated)
Description was changed from ========== Document SkSurface::MakeRaster's memory initialization So clients don't go clearing w/ SK_ColorTRANSPARENT unnecessarily. R=reed@google.com ========== to ========== Document SkSurface::MakeRaster's memory initialization So clients don't go clearing w/ SK_ColorTRANSPARENT unnecessarily. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2066903003 ==========
Description was changed from ========== Document SkSurface::MakeRaster's memory initialization So clients don't go clearing w/ SK_ColorTRANSPARENT unnecessarily. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2066903003 ========== to ========== Document SkSurface::MakeRaster's memory initialization So clients don't go clearing w/ SK_ColorTRANSPARENT unnecessarily. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2066903003 ==========
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066903003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + bsalomon@google.com
brian, do we make this promise for gpu surfaces as well? https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... include/core/SkSurface.h:53: * zero-initialized, but respecting the specified rowBytes. If rowBytes==0, then a default Do we consistently make this promise for surfaces (gpu backed as well)? That if Skia allocates the buffer, it will *always* be initialized? If so, then lgtm
https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... include/core/SkSurface.h:53: * zero-initialized, but respecting the specified rowBytes. If rowBytes==0, then a default On 2016/06/15 15:30:35, reed1 wrote: > Do we consistently make this promise for surfaces (gpu backed as well)? That if > Skia allocates the buffer, it will *always* be initialized? If so, then lgtm Even if we only do this for raster, it's still worth documenting, no? (the CL only makes this promise for Make*Raster* currently)
On 2016/06/15 15:36:37, f(malita) wrote: > https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h > File include/core/SkSurface.h (right): > > https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... > include/core/SkSurface.h:53: * zero-initialized, but respecting the specified > rowBytes. If rowBytes==0, then a default > On 2016/06/15 15:30:35, reed1 wrote: > > Do we consistently make this promise for surfaces (gpu backed as well)? That > if > > Skia allocates the buffer, it will *always* be initialized? If so, then lgtm > > Even if we only do this for raster, it's still worth documenting, no? > > (the CL only makes this promise for Make*Raster* currently) Not sure, as we may not want to keep that promise. That is I guess my question: do we want to make this promise, and if the gpu isn't then perhaps that's evidence we shouldn't for cpu. Just wondering.
On 2016/06/15 15:38:00, reed1 wrote: > On 2016/06/15 15:36:37, f(malita) wrote: > > https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h > > File include/core/SkSurface.h (right): > > > > > https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... > > include/core/SkSurface.h:53: * zero-initialized, but respecting the specified > > rowBytes. If rowBytes==0, then a default > > On 2016/06/15 15:30:35, reed1 wrote: > > > Do we consistently make this promise for surfaces (gpu backed as well)? That > > if > > > Skia allocates the buffer, it will *always* be initialized? If so, then lgtm > > > > Even if we only do this for raster, it's still worth documenting, no? > > > > (the CL only makes this promise for Make*Raster* currently) > > Not sure, as we may not want to keep that promise. That is I guess my question: > do we want to make this promise, and if the gpu isn't then perhaps that's > evidence we shouldn't for cpu. Just wondering. Ah, got you. I see Chromium clients are split between assuming n32 surfaces are transparent-initialized and clearing them explicitly. The latter is unnecessary overhead currently, and if we ever stop clearing newly allocated surfaces we're going to break the former group. I think we should either stop clearing or commit to it - otherwise there is no way for clients to behave optimally.
https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... include/core/SkSurface.h:53: * zero-initialized, but respecting the specified rowBytes. If rowBytes==0, then a default On 2016/06/15 15:36:37, f(malita) wrote: > On 2016/06/15 15:30:35, reed1 wrote: > > Do we consistently make this promise for surfaces (gpu backed as well)? That > if > > Skia allocates the buffer, it will *always* be initialized? If so, then lgtm > > Even if we only do this for raster, it's still worth documenting, no? > > (the CL only makes this promise for Make*Raster* currently) I believe we do this for GPU as well.
On 2016/06/15 16:18:23, bsalomon wrote: > https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h > File include/core/SkSurface.h (right): > > https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... > include/core/SkSurface.h:53: * zero-initialized, but respecting the specified > rowBytes. If rowBytes==0, then a default > On 2016/06/15 15:36:37, f(malita) wrote: > > On 2016/06/15 15:30:35, reed1 wrote: > > > Do we consistently make this promise for surfaces (gpu backed as well)? That > > if > > > Skia allocates the buffer, it will *always* be initialized? If so, then lgtm > > > > Even if we only do this for raster, it's still worth documenting, no? > > > > (the CL only makes this promise for Make*Raster* currently) > > I believe we do this for GPU as well. Great. Are we good with landing this as is then, or should I expand the promise to all surface factories?
https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... include/core/SkSurface.h:34: * Create a new surface, using the specified pixels/rowbytes as its Lets document that do or do not zero-init this memory. https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... include/core/SkSurface.h:65: static sk_sp<SkSurface> MakeRaster(const SkImageInfo&, const SkSurfaceProps* = nullptr); Maybe we should have this guy be inline, so that its obvious that the above comment applies to us as well. e.g. { return MakeRaster(info, 0, props); }
https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h File include/core/SkSurface.h (right): https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... include/core/SkSurface.h:34: * Create a new surface, using the specified pixels/rowbytes as its On 2016/07/01 20:11:42, reed1 wrote: > Lets document that do or do not zero-init this memory. Done. https://codereview.chromium.org/2066903003/diff/1/include/core/SkSurface.h#ne... include/core/SkSurface.h:65: static sk_sp<SkSurface> MakeRaster(const SkImageInfo&, const SkSurfaceProps* = nullptr); On 2016/07/01 20:11:42, reed1 wrote: > Maybe we should have this guy be inline, so that its obvious that the above > comment applies to us as well. > > e.g. > { > return MakeRaster(info, 0, props); > } Done.
The CQ bit was checked by fmalita@chromium.org 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.
lgtm
The CQ bit was checked by fmalita@chromium.org
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 ========== Document SkSurface::MakeRaster's memory initialization So clients don't go clearing w/ SK_ColorTRANSPARENT unnecessarily. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2066903003 ========== to ========== Document SkSurface::MakeRaster's memory initialization So clients don't go clearing w/ SK_ColorTRANSPARENT unnecessarily. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2066903003 Committed: https://skia.googlesource.com/skia/+/03912f141fba6a5c17ac7e8fbe5998ac3834e29c ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/03912f141fba6a5c17ac7e8fbe5998ac3834e29c |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
