Issue 6884054: In SKP serialization, use existing encoded data. (Closed)
|
Created:
11 years, 9 months ago by Leon Modified:
11 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionIn SKP serialization, use existing encoded data.
If an SkBitmap has encoded data, write that during serialization
rather than reencoding it.
Add a test to ensure that this does not modify the output stream,
so the reader need not know the difference.
Committed: https://code.google.com/p/skia/source/detail?r=6732
Patch Set 1 #Patch Set 2 : Modified some comments. #
Total comments: 8
Patch Set 3 : #Patch Set 4 : Disallow having both a heap and encoder function. #
Total comments: 2
Patch Set 5 : Assert when trying to set both a heap and encoder function. #
MessagesTotal messages: 11
https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... File src/core/SkOrderedWriteBuffer.cpp (right): https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... src/core/SkOrderedWriteBuffer.cpp:146: if (fBitmapHeap != NULL) { Does this seem like the right decision tree? I think we probably do not want to use the encoded data if there's an SkBitmapHeap, which is there to avoid the flattening in the first place (typically). It seems like if a bitmap encoder was supplied we definitely want to use the existing encoded data (unless the data has a different type of encoding, and the client wants a specific type, but there doesn't seem to be a clean way to identify that at the moment).
Sign in to reply to this message.
Not sure I understand how the BitmapHeap is involved here. From a simplistic read of the code, if we have a heap && an encoder, we will encode w/o first asking the bitmap if it already had an encoded form. Is that accurate? https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... File src/core/SkOrderedWriteBuffer.cpp (right): https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... src/core/SkOrderedWriteBuffer.cpp:161: SkDynamicMemoryWStream stream; I wonder if our encoder could also return SkData, rather than a stream. If so, we could do some more code sharing w/ above. https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... src/core/SkOrderedWriteBuffer.cpp:177: if (encoded) { Hmmm, almost seems backwards to write the data first, and then width/height. Oh well.
Sign in to reply to this message.
https://codereview.appspot.com/6884054/diff/2001/tests/PictureTest.cpp File tests/PictureTest.cpp (right): https://codereview.appspot.com/6884054/diff/2001/tests/PictureTest.cpp#newcod... tests/PictureTest.cpp:334: SkASSERT(stream != NULL); Perhaps we should do this copyToData() lazily, on the first request in onRefEncodedData?
Sign in to reply to this message.
https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... File src/core/SkOrderedWriteBuffer.cpp (right): https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... src/core/SkOrderedWriteBuffer.cpp:145: // version. Only do this in the case where there is no SkBitmapHeap. comment doesn't seem to line up with the code. You say you only do this when there is no SkBitmapHeap, but the if statement below evals to true only when there is a SkBitmapHeap.
Sign in to reply to this message.
> Not sure I understand how the BitmapHeap is involved here. From a simplistic > read of the code, if we have a heap && an encoder, we will encode w/o first > asking the bitmap if it already had an encoded form. Is that accurate? Yes, although I'm not sure what use case would want to have both a heap and encoder. Maybe we should disallow having both? The following shows my imagined behavior: Encoder Heap Result yes yes Bizarre use case - perhaps we should disallow? yes no Use encoded data if available, or encode no yes Store in the heap no no Use encoded data if available, or flatten https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... File src/core/SkOrderedWriteBuffer.cpp (right): https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... src/core/SkOrderedWriteBuffer.cpp:145: // version. Only do this in the case where there is no SkBitmapHeap. D'oh! The comment states my intended behavior... https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffe... src/core/SkOrderedWriteBuffer.cpp:177: if (encoded) { On 2012/12/06 21:24:06, reed1 wrote: > Hmmm, almost seems backwards to write the data first, and then width/height. Oh > well. The reason I do it this way is because line 169 (stream.read) can fail, in which case the reader does not need the width/height. https://codereview.appspot.com/6884054/diff/2001/tests/PictureTest.cpp File tests/PictureTest.cpp (right): https://codereview.appspot.com/6884054/diff/2001/tests/PictureTest.cpp#newcod... tests/PictureTest.cpp:334: SkASSERT(stream != NULL); On 2012/12/06 21:25:27, reed1 wrote: > Perhaps we should do this copyToData() lazily, on the first request in > onRefEncodedData? We could, although this class is just for this test, so I don't see the benefit of optimizing it. In its current use, onRefEncodedData will always be called.
Sign in to reply to this message.
If you have both encoded data and a bitmap heap I agree that it should go into the heap. That way it prevents us from writing the bitmap into the stream more than 1x. However, we need to make it easy for the owner of the heap to be able to write out the encoded bitmap if/when they want to serialize the heap. On Thu, Dec 6, 2012 at 5:46 PM, <scroggo@google.com> wrote: > Not sure I understand how the BitmapHeap is involved here. From a >> > simplistic > >> read of the code, if we have a heap && an encoder, we will encode w/o >> > first > >> asking the bitmap if it already had an encoded form. Is that accurate? >> > > Yes, although I'm not sure what use case would want to have both a heap > and encoder. Maybe we should disallow having both? The following shows > my imagined behavior: > > Encoder Heap Result > yes yes Bizarre use case - perhaps we should disallow? > yes no Use encoded data if available, or encode > no yes Store in the heap > no no Use encoded data if available, or flatten > > > > https://codereview.appspot.**com/6884054/diff/2001/src/** > core/SkOrderedWriteBuffer.cpp<https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffer.cpp> > File src/core/SkOrderedWriteBuffer.**cpp (right): > > https://codereview.appspot.**com/6884054/diff/2001/src/** > core/SkOrderedWriteBuffer.cpp#**newcode145<https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffer.cpp#newcode145> > src/core/SkOrderedWriteBuffer.**cpp:145: // version. Only do this in the > case where there is no SkBitmapHeap. > D'oh! The comment states my intended behavior... > > > https://codereview.appspot.**com/6884054/diff/2001/src/** > core/SkOrderedWriteBuffer.cpp#**newcode177<https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffer.cpp#newcode177> > src/core/SkOrderedWriteBuffer.**cpp:177: if (encoded) { > On 2012/12/06 21:24:06, reed1 wrote: > >> Hmmm, almost seems backwards to write the data first, and then >> > width/height. Oh > >> well. >> > > The reason I do it this way is because line 169 (stream.read) can fail, > in which case the reader does not need the width/height. > > > https://codereview.appspot.**com/6884054/diff/2001/tests/**PictureTest.cpp<ht... > File tests/PictureTest.cpp (right): > > https://codereview.appspot.**com/6884054/diff/2001/tests/** > PictureTest.cpp#newcode334<https://codereview.appspot.com/6884054/diff/2001/tests/PictureTest.cpp#newcode334> > tests/PictureTest.cpp:334: SkASSERT(stream != NULL); > On 2012/12/06 21:25:27, reed1 wrote: > >> Perhaps we should do this copyToData() lazily, on the first request in >> onRefEncodedData? >> > > We could, although this class is just for this test, so I don't see the > benefit of optimizing it. In its current use, onRefEncodedData will > always be called. > > https://codereview.appspot.**com/6884054/<https://codereview.appspot.com/6884... >
Sign in to reply to this message.
On Fri, Dec 7, 2012 at 12:57 PM, Derek Sollenberger <djsollen@google.com>wrote: > If you have both encoded data and a bitmap heap I agree that it should go > into the heap. That way it prevents us from writing the bitmap into the > stream more than 1x. > Glad my instincts were correct. Is there a reason to support having both an encoder and a heap? I'm thinking I'll remove that as a possibility and see if that makes the code clearer. However, we need to make it easy for the owner of the heap to be able to > write out the encoded bitmap if/when they want to serialize the heap. > So long as the encoded bitmap is marked as immutable (which I think should always be the case when a bitmap is backed by encoded data), bitmaps from the heap will share the pixel ref, so they'll still have the encoded data. On Fri, Dec 7, 2012 at 12:57 PM, Derek Sollenberger <djsollen@google.com>wrote: > If you have both encoded data and a bitmap heap I agree that it should go > into the heap. That way it prevents us from writing the bitmap into the > stream more than 1x. However, we need to make it easy for the owner of the > heap to be able to write out the encoded bitmap if/when they want to > serialize the heap. > > > On Thu, Dec 6, 2012 at 5:46 PM, <scroggo@google.com> wrote: > >> Not sure I understand how the BitmapHeap is involved here. From a >>> >> simplistic >> >>> read of the code, if we have a heap && an encoder, we will encode w/o >>> >> first >> >>> asking the bitmap if it already had an encoded form. Is that accurate? >>> >> >> Yes, although I'm not sure what use case would want to have both a heap >> and encoder. Maybe we should disallow having both? The following shows >> my imagined behavior: >> >> Encoder Heap Result >> yes yes Bizarre use case - perhaps we should disallow? >> yes no Use encoded data if available, or encode >> no yes Store in the heap >> no no Use encoded data if available, or flatten >> >> >> >> https://codereview.appspot.**com/6884054/diff/2001/src/** >> core/SkOrderedWriteBuffer.cpp<https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffer.cpp> >> File src/core/SkOrderedWriteBuffer.**cpp (right): >> >> https://codereview.appspot.**com/6884054/diff/2001/src/** >> core/SkOrderedWriteBuffer.cpp#**newcode145<https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffer.cpp#newcode145> >> src/core/SkOrderedWriteBuffer.**cpp:145: // version. Only do this in the >> case where there is no SkBitmapHeap. >> D'oh! The comment states my intended behavior... >> >> >> https://codereview.appspot.**com/6884054/diff/2001/src/** >> core/SkOrderedWriteBuffer.cpp#**newcode177<https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffer.cpp#newcode177> >> src/core/SkOrderedWriteBuffer.**cpp:177: if (encoded) { >> On 2012/12/06 21:24:06, reed1 wrote: >> >>> Hmmm, almost seems backwards to write the data first, and then >>> >> width/height. Oh >> >>> well. >>> >> >> The reason I do it this way is because line 169 (stream.read) can fail, >> in which case the reader does not need the width/height. >> >> >> https://codereview.appspot.**com/6884054/diff/2001/tests/** >> PictureTest.cpp<https://codereview.appspot.com/6884054/diff/2001/tests/PictureTest.cpp> >> File tests/PictureTest.cpp (right): >> >> https://codereview.appspot.**com/6884054/diff/2001/tests/** >> PictureTest.cpp#newcode334<https://codereview.appspot.com/6884054/diff/2001/tests/PictureTest.cpp#newcode334> >> tests/PictureTest.cpp:334: SkASSERT(stream != NULL); >> On 2012/12/06 21:25:27, reed1 wrote: >> >>> Perhaps we should do this copyToData() lazily, on the first request in >>> onRefEncodedData? >>> >> >> We could, although this class is just for this test, so I don't see the >> benefit of optimizing it. In its current use, onRefEncodedData will >> always be called. >> >> https://codereview.appspot.**com/6884054/<https://codereview.appspot.com/6884... >> > > -- Leon Scroggins III scroggo@google.com
Sign in to reply to this message.
If there is a heap I think we should always default to the heap, since the owner of the heap can later choose to serialize the encoded data. So feel free to re-organize the code with that in mind. On Fri, Dec 7, 2012 at 1:13 PM, Leon Scroggins <scroggo@google.com> wrote: > > > On Fri, Dec 7, 2012 at 12:57 PM, Derek Sollenberger <djsollen@google.com>wrote: > >> If you have both encoded data and a bitmap heap I agree that it should go >> into the heap. That way it prevents us from writing the bitmap into the >> stream more than 1x. >> > > Glad my instincts were correct. Is there a reason to support having both > an encoder and a heap? I'm thinking I'll remove that as a possibility and > see if that makes the code clearer. > > However, we need to make it easy for the owner of the heap to be able to >> write out the encoded bitmap if/when they want to serialize the heap. >> > > So long as the encoded bitmap is marked as immutable (which I think should > always be the case when a bitmap is backed by encoded data), bitmaps from > the heap will share the pixel ref, so they'll still have the encoded data. > > > On Fri, Dec 7, 2012 at 12:57 PM, Derek Sollenberger <djsollen@google.com>wrote: > >> If you have both encoded data and a bitmap heap I agree that it should go >> into the heap. That way it prevents us from writing the bitmap into the >> stream more than 1x. However, we need to make it easy for the owner of the >> heap to be able to write out the encoded bitmap if/when they want to >> serialize the heap. >> >> >> On Thu, Dec 6, 2012 at 5:46 PM, <scroggo@google.com> wrote: >> >>> Not sure I understand how the BitmapHeap is involved here. From a >>>> >>> simplistic >>> >>>> read of the code, if we have a heap && an encoder, we will encode w/o >>>> >>> first >>> >>>> asking the bitmap if it already had an encoded form. Is that accurate? >>>> >>> >>> Yes, although I'm not sure what use case would want to have both a heap >>> and encoder. Maybe we should disallow having both? The following shows >>> my imagined behavior: >>> >>> Encoder Heap Result >>> yes yes Bizarre use case - perhaps we should disallow? >>> yes no Use encoded data if available, or encode >>> no yes Store in the heap >>> no no Use encoded data if available, or flatten >>> >>> >>> >>> https://codereview.appspot.**com/6884054/diff/2001/src/** >>> core/SkOrderedWriteBuffer.cpp<https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffer.cpp> >>> File src/core/SkOrderedWriteBuffer.**cpp (right): >>> >>> https://codereview.appspot.**com/6884054/diff/2001/src/** >>> core/SkOrderedWriteBuffer.cpp#**newcode145<https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffer.cpp#newcode145> >>> src/core/SkOrderedWriteBuffer.**cpp:145: // version. Only do this in the >>> case where there is no SkBitmapHeap. >>> D'oh! The comment states my intended behavior... >>> >>> >>> https://codereview.appspot.**com/6884054/diff/2001/src/** >>> core/SkOrderedWriteBuffer.cpp#**newcode177<https://codereview.appspot.com/6884054/diff/2001/src/core/SkOrderedWriteBuffer.cpp#newcode177> >>> src/core/SkOrderedWriteBuffer.**cpp:177: if (encoded) { >>> On 2012/12/06 21:24:06, reed1 wrote: >>> >>>> Hmmm, almost seems backwards to write the data first, and then >>>> >>> width/height. Oh >>> >>>> well. >>>> >>> >>> The reason I do it this way is because line 169 (stream.read) can fail, >>> in which case the reader does not need the width/height. >>> >>> >>> https://codereview.appspot.**com/6884054/diff/2001/tests/** >>> PictureTest.cpp<https://codereview.appspot.com/6884054/diff/2001/tests/PictureTest.cpp> >>> File tests/PictureTest.cpp (right): >>> >>> https://codereview.appspot.**com/6884054/diff/2001/tests/** >>> PictureTest.cpp#newcode334<https://codereview.appspot.com/6884054/diff/2001/tests/PictureTest.cpp#newcode334> >>> tests/PictureTest.cpp:334: SkASSERT(stream != NULL); >>> On 2012/12/06 21:25:27, reed1 wrote: >>> >>>> Perhaps we should do this copyToData() lazily, on the first request in >>>> onRefEncodedData? >>>> >>> >>> We could, although this class is just for this test, so I don't see the >>> benefit of optimizing it. In its current use, onRefEncodedData will >>> always be called. >>> >>> https://codereview.appspot.**com/6884054/<https://codereview.appspot.com/6884... >>> >> >> > > > -- > Leon Scroggins III > scroggo@google.com >
Sign in to reply to this message.
Uploaded a new patch. This patch ensures that you only have a heap or an encoder. I also moved the code around to make it more clear. The stream that is written is still the same, though, so there is no need to bump the picture version.
Sign in to reply to this message.
lgtm with one nit https://codereview.appspot.com/6884054/diff/13001/src/core/SkOrderedWriteBuff... File src/core/SkOrderedWriteBuffer.h (right): https://codereview.appspot.com/6884054/diff/13001/src/core/SkOrderedWriteBuff... src/core/SkOrderedWriteBuffer.h:91: * Incompatible with the SkBitmapHeap. If an encoder is set fBitmapHeap will be set to NULL. I lean towards firing an SkASSERT if they try this. In release mode we should still set it to NULL, but in theory I would prefer to make the caller have to do this manually so they know the implications. setBitmapHeap(NULL) setBitmapEncoder(foo)
Sign in to reply to this message.
https://codereview.appspot.com/6884054/diff/13001/src/core/SkOrderedWriteBuff... File src/core/SkOrderedWriteBuffer.h (right): https://codereview.appspot.com/6884054/diff/13001/src/core/SkOrderedWriteBuff... src/core/SkOrderedWriteBuffer.h:91: * Incompatible with the SkBitmapHeap. If an encoder is set fBitmapHeap will be set to NULL. On 2012/12/07 21:46:03, DerekS wrote: > I lean towards firing an SkASSERT if they try this. In release mode we should > still set it to NULL, but in theory I would prefer to make the caller have to do > this manually so they know the implications. > > setBitmapHeap(NULL) > setBitmapEncoder(foo) Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
