Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
167113003:
Add capture snapshot as data to SkWriter32, use it to optimise record->playback. (Closed)
|
Created:
6 years, 10 months ago by iancottrell Modified:
6 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionAdd capture snapshot as data to SkWriter32, use it to optimise record->playback.
This is a new way of implementing https://codereview.chromium.org/155863005/
It uses copy on write semantics to return a buffer without copying it, so that record -> playback does not need to copy the buffer.
BUG=skia:2125
Committed: http://code.google.com/p/skia/source/detail?r=13769
Patch Set 1 #Patch Set 2 : Add reset and comment #
Total comments: 16
Patch Set 3 : Review fixes #Patch Set 4 : Writer32 snapshot tests #Patch Set 5 : Mutex protect snapshot creation #
Total comments: 10
Patch Set 6 : Review fixes #Patch Set 7 : Adding thread saftey comment #Patch Set 8 : Adding an ASSERT for an undefined write to a snapshotted SkWriter32 #Patch Set 9 : Clarified the snapshotAsData api comment. #
Messages
Total messages: 30 (0 generated)
Caches are nice, but why are we caching the data, instead of having the client perform that check outside of our code? Seems simple enough since the only check we preform is to compare sizes. Should we mention that clients who called reserve() need to have fully written their data before making this call? https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... include/core/SkWriter32.h:237: * Multiple calls without intervening writes may return the same buffer, I like noting this possible optimization, but perhaps we should be very clear not to rely on it... "... may return the same buffer (SkData?), but this is not guaranteed." https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... include/core/SkWriter32.h:243: return const_cast<SkWriter32*>(this)->snapshotAsData(); Do we want a mutex guard around this, since we are mutating a const-object ? https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:81: if ((fSnapshot.get() != NULL) && (fSnapshot->size() != fUsed)) { I wonder if we should consider invalidating this cache explicitly whenever we grow our allocator, just to avoid the memory bloat of an unreachable SkData cache.
If we're going to cache, seems like we need to invalidate the cache if anyone calls overwriteAt()
Lets add a unittest for this guy...
https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:94: fData = buffer; What if someone does a readAt(), and the snapshot has been deleted by the client? Won't this buffer be pointing at bad-memory?
neat https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... include/core/SkWriter32.h:242: SkData* snapshotAsData() const { Why have both const and non-const versions? Why not const_cast inside the const method? Can you add a note that snapshotAsData isn't threadsafe? https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:93: // is external and size limited Have I got this right, that we're detaching and re-reffing the internal storage, treating it as completely full external storage in the future? That way, the external to internal copy-on-overflow doubles as copy-on-write? Can we simplify things further by unifying fExternal and fSnapshot? fExternal becomes an SkAutoTUnref<SkData>, and we use different subclasses of SkData to control whether or not fExternal holds a ref on the buffer?
On 2014/02/14 17:31:37, reed1 wrote: > If we're going to cache, seems like we need to invalidate the cache if anyone > calls overwriteAt() I don't think we can afford to do that, it would be way too expensive. I think we have to live with that as a danger if we want to share the buffer, and still have you able to keep using the record object after a playback object has been built from it. Probably it needs to be really clearly documented as caveats on the snapshotAsData function?
On 2014/02/14 17:32:04, reed1 wrote: > Lets add a unittest for this guy... Yes, definitely needs tests, but I wanted to see if the basic approach was even one we wanted before going any further.
https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... include/core/SkWriter32.h:237: * Multiple calls without intervening writes may return the same buffer, On 2014/02/14 17:31:08, reed1 wrote: > I like noting this possible optimization, but perhaps we should be very clear > not to rely on it... > > "... may return the same buffer (SkData?), but this is not guaranteed." Sure, I deliberately used the word may, but I agree that's probably too subtle. https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... include/core/SkWriter32.h:242: SkData* snapshotAsData() const { On 2014/02/14 17:42:18, mtklein wrote: > Why have both const and non-const versions? Why not const_cast inside the const > method? > > Can you add a note that snapshotAsData isn't threadsafe? Would we rather make it threadsafe? https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... include/core/SkWriter32.h:243: return const_cast<SkWriter32*>(this)->snapshotAsData(); On 2014/02/14 17:31:08, reed1 wrote: > Do we want a mutex guard around this, since we are mutating a const-object ? Maybe? This is an area where I just don't have enough Skia experience to make the call between documenting non threadsafe vs making threadsafe, if you could make a call with Mike and tell me that would be great. https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:81: if ((fSnapshot.get() != NULL) && (fSnapshot->size() != fUsed)) { On 2014/02/14 17:31:08, reed1 wrote: > I wonder if we should consider invalidating this cache explicitly whenever we > grow our allocator, just to avoid the memory bloat of an unreachable SkData > cache. Yes, I did consider that alternative. Basically we need to addref in here before we return, and make sure we unref sometime after we stop using it. We could easily add the unref in the external->internal path in the growth code. https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:93: // is external and size limited On 2014/02/14 17:42:18, mtklein wrote: > Have I got this right, that we're detaching and re-reffing the internal storage, > treating it as completely full external storage in the future? That way, the > external to internal copy-on-overflow doubles as copy-on-write? > Exactly, yes. > Can we simplify things further by unifying fExternal and fSnapshot? fExternal > becomes an SkAutoTUnref<SkData>, and we use different subclasses of SkData to > control whether or not fExternal holds a ref on the buffer? Yes, I guess I just need to use SkData::NewWithProc and hand it a release proc that does nothing. https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:94: fData = buffer; On 2014/02/14 17:35:27, reed1 wrote: > What if someone does a readAt(), and the snapshot has been deleted by the > client? Won't this buffer be pointing at bad-memory? no, so long as we hold a ref the buffer should be valid.
On 2014/02/14 18:28:36, iancottrell wrote: > https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.h > File include/core/SkWriter32.h (right): > > https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... > include/core/SkWriter32.h:237: * Multiple calls without intervening writes may > return the same buffer, > On 2014/02/14 17:31:08, reed1 wrote: > > I like noting this possible optimization, but perhaps we should be very clear > > not to rely on it... > > > > "... may return the same buffer (SkData?), but this is not guaranteed." > > Sure, I deliberately used the word may, but I agree that's probably too subtle. > > https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... > include/core/SkWriter32.h:242: SkData* snapshotAsData() const { > On 2014/02/14 17:42:18, mtklein wrote: > > Why have both const and non-const versions? Why not const_cast inside the > const > > method? > > > > Can you add a note that snapshotAsData isn't threadsafe? > > Would we rather make it threadsafe? > > https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... > include/core/SkWriter32.h:243: return > const_cast<SkWriter32*>(this)->snapshotAsData(); > On 2014/02/14 17:31:08, reed1 wrote: > > Do we want a mutex guard around this, since we are mutating a const-object ? > > Maybe? This is an area where I just don't have enough Skia experience to make > the call between documenting non threadsafe vs making threadsafe, if you could > make a call with Mike and tell me that would be great. > > https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp > File src/core/SkWriter32.cpp (right): > > https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... > src/core/SkWriter32.cpp:81: if ((fSnapshot.get() != NULL) && (fSnapshot->size() > != fUsed)) { > On 2014/02/14 17:31:08, reed1 wrote: > > I wonder if we should consider invalidating this cache explicitly whenever we > > grow our allocator, just to avoid the memory bloat of an unreachable SkData > > cache. > Yes, I did consider that alternative. > Basically we need to addref in here before we return, and make sure we unref > sometime after we stop using it. > We could easily add the unref in the external->internal path in the growth code. > > https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... > src/core/SkWriter32.cpp:93: // is external and size limited > On 2014/02/14 17:42:18, mtklein wrote: > > Have I got this right, that we're detaching and re-reffing the internal > storage, > > treating it as completely full external storage in the future? That way, the > > external to internal copy-on-overflow doubles as copy-on-write? > > > Exactly, yes. > > > Can we simplify things further by unifying fExternal and fSnapshot? fExternal > > becomes an SkAutoTUnref<SkData>, and we use different subclasses of SkData to > > control whether or not fExternal holds a ref on the buffer? > > Yes, I guess I just need to use SkData::NewWithProc and hand it a release proc > that does nothing. > > https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... > src/core/SkWriter32.cpp:94: fData = buffer; > On 2014/02/14 17:35:27, reed1 wrote: > > What if someone does a readAt(), and the snapshot has been deleted by the > > client? Won't this buffer be pointing at bad-memory? > > no, so long as we hold a ref the buffer should be valid. Ah, right -- thus we would be *required* to hold onto the old data/buffer... so that "cache" is a little more than that.
This LGTM once the Skia team answers the threadsafety question & the comment nits are fixed up.
https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:81: if ((fSnapshot.get() != NULL) && (fSnapshot->size() != fUsed)) { On 2014/02/14 18:28:36, iancottrell wrote: > On 2014/02/14 17:31:08, reed1 wrote: > > I wonder if we should consider invalidating this cache explicitly whenever we > > grow our allocator, just to avoid the memory bloat of an unreachable SkData > > cache. > Yes, I did consider that alternative. > Basically we need to addref in here before we return, and make sure we unref > sometime after we stop using it. > We could easily add the unref in the external->internal path in the growth code. I added the snapshot drop in the growth code, but it is worth keeping this test as well, as it will trigger even when the buffer did not grow but the space used in it did.
Also with added tests for basic behavior and COW behavior. https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/167113003/diff/30001/include/core/SkWriter32.... include/core/SkWriter32.h:243: return const_cast<SkWriter32*>(this)->snapshotAsData(); On 2014/02/14 18:28:36, iancottrell wrote: > On 2014/02/14 17:31:08, reed1 wrote: > > Do we want a mutex guard around this, since we are mutating a const-object ? > > Maybe? This is an area where I just don't have enough Skia experience to make > the call between documenting non threadsafe vs making threadsafe, if you could > make a call with Mike and tell me that would be great. Added a mutex guard in the implementation. https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:93: // is external and size limited On 2014/02/14 17:42:18, mtklein wrote: > Have I got this right, that we're detaching and re-reffing the internal storage, > treating it as completely full external storage in the future? That way, the > external to internal copy-on-overflow doubles as copy-on-write? > > Can we simplify things further by unifying fExternal and fSnapshot? fExternal > becomes an SkAutoTUnref<SkData>, and we use different subclasses of SkData to > control whether or not fExternal holds a ref on the buffer? I looked at doing this, and it turns out to be awkward and gain you nothing, as the case exists where making a snapshot causes you to have to copy the external buffer first, which means you need to be able to tell the difference.
Sorry, let this slip through the cracks. https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... src/core/SkWriter32.cpp:93: // is external and size limited On 2014/02/27 20:16:19, iancottrell wrote: > On 2014/02/14 17:42:18, mtklein wrote: > > Have I got this right, that we're detaching and re-reffing the internal > storage, > > treating it as completely full external storage in the future? That way, the > > external to internal copy-on-overflow doubles as copy-on-write? > > > > Can we simplify things further by unifying fExternal and fSnapshot? fExternal > > becomes an SkAutoTUnref<SkData>, and we use different subclasses of SkData to > > control whether or not fExternal holds a ref on the buffer? > > I looked at doing this, and it turns out to be awkward and gain you nothing, as > the case exists where making a snapshot causes you to have to copy the external > buffer first, which means you need to be able to tell the difference. Thanks for investigating. https://codereview.chromium.org/167113003/diff/250001/src/core/SkPicturePlayb... File src/core/SkPicturePlayback.cpp (right): https://codereview.chromium.org/167113003/diff/250001/src/core/SkPicturePlayb... src/core/SkPicturePlayback.cpp:88: fOpData = writer.snapshotAsData(); Moving this up to / merge this with the other part that deals with fOpData ~15 lines up? https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp... src/core/SkWriter32.cpp:81: SK_DECLARE_STATIC_MUTEX(gSnapshotMutex); I think I'd be happier if we declared this to be not thread safe and didn't have a mutex. The complicated thing is that we use "threadsafe" as a shorthand for "safe to be called as part of any sequence of method calls from any number of threads". We're ignoring all the other methods here; the only thing we're guaranteeing is that no two threads can ever call snapshotAsData() at the same time. That's pretty limited protection, to the point where I'd rather have none, documented as such. This is probably another case where threadsafety is going to be best dealt with at a higher level (e.g SkPicture{,Record,Playback}). https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp... src/core/SkWriter32.cpp:86: SkWriter32& self = *const_cast<SkWriter32*>(this); Maybe SkWriter32& self -> SkWriter32* mutable_this? https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp... src/core/SkWriter32.cpp:107: return SkRef(fSnapshot.get()); Can you tack on // Take an extra ref for the caller. ? I stumbled over this a second until I remembered the contract.
On 2014/03/06 18:58:06, mtklein wrote: > Sorry, let this slip through the cracks. > > https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp > File src/core/SkWriter32.cpp (right): > > https://codereview.chromium.org/167113003/diff/30001/src/core/SkWriter32.cpp#... > src/core/SkWriter32.cpp:93: // is external and size limited > On 2014/02/27 20:16:19, iancottrell wrote: > > On 2014/02/14 17:42:18, mtklein wrote: > > > Have I got this right, that we're detaching and re-reffing the internal > > storage, > > > treating it as completely full external storage in the future? That way, > the > > > external to internal copy-on-overflow doubles as copy-on-write? > > > > > > Can we simplify things further by unifying fExternal and fSnapshot? > fExternal > > > becomes an SkAutoTUnref<SkData>, and we use different subclasses of SkData > to > > > control whether or not fExternal holds a ref on the buffer? > > > > I looked at doing this, and it turns out to be awkward and gain you nothing, > as > > the case exists where making a snapshot causes you to have to copy the > external > > buffer first, which means you need to be able to tell the difference. > > Thanks for investigating. > > https://codereview.chromium.org/167113003/diff/250001/src/core/SkPicturePlayb... > File src/core/SkPicturePlayback.cpp (right): > > https://codereview.chromium.org/167113003/diff/250001/src/core/SkPicturePlayb... > src/core/SkPicturePlayback.cpp:88: fOpData = writer.snapshotAsData(); > Moving this up to / merge this with the other part that deals with fOpData ~15 > lines up? > > https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp > File src/core/SkWriter32.cpp (right): > > https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp... > src/core/SkWriter32.cpp:81: SK_DECLARE_STATIC_MUTEX(gSnapshotMutex); > I think I'd be happier if we declared this to be not thread safe and didn't have > a mutex. > > The complicated thing is that we use "threadsafe" as a shorthand for "safe to be > called as part of any sequence of method calls from any number of threads". > We're ignoring all the other methods here; the only thing we're guaranteeing is > that no two threads can ever call snapshotAsData() at the same time. That's > pretty limited protection, to the point where I'd rather have none, documented > as such. > > This is probably another case where threadsafety is going to be best dealt with > at a higher level (e.g SkPicture{,Record,Playback}). > > https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp... > src/core/SkWriter32.cpp:86: SkWriter32& self = *const_cast<SkWriter32*>(this); > Maybe SkWriter32& self -> SkWriter32* mutable_this? > > https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp... > src/core/SkWriter32.cpp:107: return SkRef(fSnapshot.get()); > Can you tack on // Take an extra ref for the caller. ? I stumbled over this a > second until I remembered the contract. FYI folks who are interested in performance, I noticed today while profiling Domink's new paint dictionary that the copy this CL eliminates is currently the biggest single use of time in recording. It looks like this CL could mean an ~8% speedup in recording. It's a big deal.
https://codereview.chromium.org/167113003/diff/250001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/167113003/diff/250001/include/core/SkWriter32... include/core/SkWriter32.h:240: * without an intervening append may. "may" -- does this mean future writes *will* appear in the returned buffer, or *will not*, or undefined?
separate question : can we move SkWriter32.h into src to hide it from our clients?
I think we can make SkWriter32 non public, can I try in a follow up CL? https://codereview.chromium.org/167113003/diff/250001/include/core/SkWriter32.h File include/core/SkWriter32.h (right): https://codereview.chromium.org/167113003/diff/250001/include/core/SkWriter32... include/core/SkWriter32.h:240: * without an intervening append may. On 2014/03/06 19:20:31, reed1 wrote: > "may" -- does this mean future writes *will* appear in the returned buffer, or > *will not*, or undefined? Undefined but predictable. For instance, if the current backing store was an external buffer, then the returned snapshot will be a copy of that, but the array will keep writing to the external store, so they won't appear. If it was the internal buffer, then we will not copy it, just return it, so changes would appear. This is all an implementation detail though. I did not want to use the word undefined, because that carries scary connotations to me. https://codereview.chromium.org/167113003/diff/250001/src/core/SkPicturePlayb... File src/core/SkPicturePlayback.cpp (right): https://codereview.chromium.org/167113003/diff/250001/src/core/SkPicturePlayb... src/core/SkPicturePlayback.cpp:88: fOpData = writer.snapshotAsData(); On 2014/03/06 18:58:08, mtklein wrote: > Moving this up to / merge this with the other part that deals with fOpData ~15 > lines up? Done. https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp File src/core/SkWriter32.cpp (right): https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp... src/core/SkWriter32.cpp:81: SK_DECLARE_STATIC_MUTEX(gSnapshotMutex); On 2014/03/06 18:58:08, mtklein wrote: > I think I'd be happier if we declared this to be not thread safe and didn't have > a mutex. > > The complicated thing is that we use "threadsafe" as a shorthand for "safe to be > called as part of any sequence of method calls from any number of threads". > We're ignoring all the other methods here; the only thing we're guaranteeing is > that no two threads can ever call snapshotAsData() at the same time. That's > pretty limited protection, to the point where I'd rather have none, documented > as such. > > This is probably another case where threadsafety is going to be best dealt with > at a higher level (e.g SkPicture{,Record,Playback}). Done. https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp... src/core/SkWriter32.cpp:86: SkWriter32& self = *const_cast<SkWriter32*>(this); On 2014/03/06 18:58:08, mtklein wrote: > Maybe SkWriter32& self -> SkWriter32* mutable_this? Done. https://codereview.chromium.org/167113003/diff/250001/src/core/SkWriter32.cpp... src/core/SkWriter32.cpp:107: return SkRef(fSnapshot.get()); On 2014/03/06 18:58:08, mtklein wrote: > Can you tack on // Take an extra ref for the caller. ? I stumbled over this a > second until I remembered the contract. Done.
Exactly because it is an implementation detail (that could change) I think we should make this very scary sounding. This seems like the sort of side-effect (the write appearing or not appearing in the snapshot buffer) that could bite us later, by a client (unknowingly perhaps) relying on this, and thus forcing our hand in the future. Can we make it defined? e.g. - assert if they write before appending after a detach? - force a copy if they write before appending after a detach? - other?
On 2014/03/07 13:44:07, reed1 wrote: > Exactly because it is an implementation detail (that could change) I think we > should make this very scary sounding. This seems like the sort of side-effect > (the write appearing or not appearing in the snapshot buffer) that could bite us > later, by a client (unknowingly perhaps) relying on this, and thus forcing our > hand in the future. > > Can we make it defined? e.g. > - assert if they write before appending after a detach? > - force a copy if they write before appending after a detach? > - other? There is no reasonable way I can think of to add any extra protection or detection without make writes much more expensive, or losing the advantages of sharing the buffer. Hopefully this is less of an issue if we are taking SkWriter32 out of the public API? The correct longer term fix IMHO is to fix the issues with SkPicture that mean we can't guarantee SkWriter32 is not written to after a playback is built, and then have the snapshotAsData() method take the buffer out from under the SKWriter32 so it cannot ever be written to again.
On 2014/03/07 13:56:40, ian_cottrell wrote: > On 2014/03/07 13:44:07, reed1 wrote: > > Exactly because it is an implementation detail (that could change) I think we > > should make this very scary sounding. This seems like the sort of side-effect > > (the write appearing or not appearing in the snapshot buffer) that could bite > us > > later, by a client (unknowingly perhaps) relying on this, and thus forcing our > > hand in the future. > > > > Can we make it defined? e.g. > > - assert if they write before appending after a detach? > > - force a copy if they write before appending after a detach? > > - other? > > There is no reasonable way I can think of to add any extra protection or > detection without make writes much more expensive, or losing the advantages of > sharing the buffer. > Hopefully this is less of an issue if we are taking SkWriter32 out of the public > API? > The correct longer term fix IMHO is to fix the issues with SkPicture that mean > we can't guarantee SkWriter32 is not written to after a playback is built, and > then have the snapshotAsData() method take the buffer out from under the > SKWriter32 so it cannot ever be written to again. Seems like we can add SK_DEBUGCODE that tracks this state: detach followed by write before append, and then assert (but only in debug build). I like your long-term eliminate-the-feature approach tool.
On 2014/03/07 14:02:35, reed1 wrote: > On 2014/03/07 13:56:40, ian_cottrell wrote: > > On 2014/03/07 13:44:07, reed1 wrote: > > > Exactly because it is an implementation detail (that could change) I think > we > > > should make this very scary sounding. This seems like the sort of > side-effect > > > (the write appearing or not appearing in the snapshot buffer) that could > bite > > us > > > later, by a client (unknowingly perhaps) relying on this, and thus forcing > our > > > hand in the future. > > > > > > Can we make it defined? e.g. > > > - assert if they write before appending after a detach? > > > - force a copy if they write before appending after a detach? > > > - other? > > > > There is no reasonable way I can think of to add any extra protection or > > detection without make writes much more expensive, or losing the advantages of > > sharing the buffer. > > Hopefully this is less of an issue if we are taking SkWriter32 out of the > public > > API? > > The correct longer term fix IMHO is to fix the issues with SkPicture that mean > > we can't guarantee SkWriter32 is not written to after a playback is built, and > > then have the snapshotAsData() method take the buffer out from under the > > SKWriter32 so it cannot ever be written to again. > > Seems like we can add SK_DEBUGCODE that tracks this state: detach followed by > write before append, and then assert (but only in debug build). > > I like your long-term eliminate-the-feature approach tool. Assert added.
Perhaps we should have a quick VC. I still believe the function should officially say that writes after detach are either supported, or unsupported.
On 2014/03/10 13:15:44, reed1 wrote: > Perhaps we should have a quick VC. I still believe the function should > officially say that writes after detach are either supported, or unsupported. I forgot to re-send this yesterday after I changed the comment.
lgtm perhaps we should file a bug now, reminding us that with future changes to picture, we may be able to remove this caveat, and always actually detach.
On 2014/03/12 13:17:12, reed1 wrote: > lgtm > > perhaps we should file a bug now, reminding us that with future changes to > picture, we may be able to remove this caveat, and always actually detach. Bug filed as https://code.google.com/p/skia/issues/detail?id=2289
The CQ bit was checked by iancottrell@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/iancottrell@google.com/167113003/330001
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/iancottrell@google.com/167113003/330001
Message was sent while issue was closed.
Change committed as 13769 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
