Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
138063005:
Serialization of SkPictureImageFilter (Closed)
|
Created:
6 years, 11 months ago by sugoi1 Modified:
6 years, 10 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionSerialization of SkPictureImageFilter
BUG=skia:
Committed: http://code.google.com/p/skia/source/detail?r=13347
Committed: http://code.google.com/p/skia/source/detail?r=13354
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update to ToT #Patch Set 3 : Code ifdefed out #
Total comments: 9
Patch Set 4 : Follow up on comments #
Total comments: 2
Patch Set 5 : Added comments and changed PICTURE_VERSION #Patch Set 6 : Updated past revert point, will re-land tonight #
Messages
Total messages: 33 (0 generated)
This cl doesn't do much except fill in the blanks for the serialization of SkPictureImageFilter. I wouldn't commit this change as is since there must be some missing pieces (Like SkValidatingReadBuffer::readTypeface() not having been implemented yet). I need to test this first to find all other issues. This is where I have a problem. Any suggestions about how to test this ? Maybe this is simpler than I think, but it seems like creating a random SkPicture in a fuzzer that can actually cover all possible cases would be pretty hard. I'm open to all ideas at this point.
On 2014/01/15 13:32:38, sugoi wrote: > This cl doesn't do much except fill in the blanks for the serialization of > SkPictureImageFilter. I wouldn't commit this change as is since there must be > some missing pieces (Like SkValidatingReadBuffer::readTypeface() not having been > implemented yet). I need to test this first to find all other issues. > > This is where I have a problem. Any suggestions about how to test this ? Maybe > this is simpler than I think, but it seems like creating a random SkPicture in a > fuzzer that can actually cover all possible cases would be pretty hard. I'm open > to all ideas at this point. Lets document the state of the buffer (to the caller) in the case when we return NULL (i.e. the buffer's position may have been moved, etc.)
Since SkPictureImageFilter is already used in Chrome, I don't think we should land this until we have thorough fuzzer coverage. Is it possible to build the fuzzer with different flags from Chrome? If so, perhaps we should hide the serialization (in SkPictureImageFilter) behind a compile-time flag, and turn it on only in the fuzzer for now. https://codereview.chromium.org/138063005/diff/1/samplecode/SampleFilterFuzz.cpp File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/138063005/diff/1/samplecode/SampleFilterFuzz.... samplecode/SampleFilterFuzz.cpp:262: filter = new SkBitmapSource(make_bitmap(), make_rect(), make_rect()); This should probably be landed separately from this change. We should also exercise the other (1-parameter) constructor as well, in order to get full coverage. Perhaps 50% randomly. https://codereview.chromium.org/138063005/diff/1/src/core/SkPicture.cpp File src/core/SkPicture.cpp (right): https://codereview.chromium.org/138063005/diff/1/src/core/SkPicture.cpp#newco... src/core/SkPicture.cpp:316: #ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V16_AND_ALL_OTHER_INSTANCES_TOO Considering that this will be the first time that pictures are serialized this way, is this necessary? Is there a way to get an older version of a picture into this new code?
On 2014/01/15 13:56:52, reed1 wrote: > On 2014/01/15 13:32:38, sugoi wrote: > > This cl doesn't do much except fill in the blanks for the serialization of > > SkPictureImageFilter. I wouldn't commit this change as is since there must be > > some missing pieces (Like SkValidatingReadBuffer::readTypeface() not having been > > implemented yet). I need to test this first to find all other issues. > > > > This is where I have a problem. Any suggestions about how to test this ? Maybe > > this is simpler than I think, but it seems like creating a random SkPicture in a > > fuzzer that can actually cover all possible cases would be pretty hard. I'm open > > to all ideas at this point. > > Lets document the state of the buffer (to the caller) in the case when we return > NULL (i.e. the buffer's position may have been moved, etc.) Did you have a particular function in mind when you wrote this?
On 2014/01/15 14:59:01, Stephen White wrote: > Since SkPictureImageFilter is already used in Chrome, I don't think we should land this until we have thorough fuzzer coverage. I agree, I put this cl up for review in order to get suggestions on how to test this properly (see my initial comment) > Is it possible to build the fuzzer with different flags from Chrome? If so, perhaps we should hide the serialization (in SkPictureImageFilter) behind a compile-time flag, and turn it on only in the fuzzer for now. Sure, that can be done, but my problem is to generate random SkPicture objects that cover all possibilities, as much as possible. https://codereview.chromium.org/138063005/diff/1/samplecode/SampleFilterFuzz.cpp File samplecode/SampleFilterFuzz.cpp (right): https://codereview.chromium.org/138063005/diff/1/samplecode/SampleFilterFuzz.... samplecode/SampleFilterFuzz.cpp:262: filter = new SkBitmapSource(make_bitmap(), make_rect(), make_rect()); On 2014/01/15 14:59:02, Stephen White wrote: > This should probably be landed separately from this change. > > We should also exercise the other (1-parameter) constructor as well, in order to > get full coverage. Perhaps 50% randomly. Done. (see https://codereview.chromium.org/139613002/) https://codereview.chromium.org/138063005/diff/1/src/core/SkPicture.cpp File src/core/SkPicture.cpp (right): https://codereview.chromium.org/138063005/diff/1/src/core/SkPicture.cpp#newco... src/core/SkPicture.cpp:316: #ifndef DELETE_THIS_CODE_WHEN_SKPS_ARE_REBUILT_AT_V16_AND_ALL_OTHER_INSTANCES_TOO On 2014/01/15 14:59:02, Stephen White wrote: > Considering that this will be the first time that pictures are serialized this > way, is this necessary? Is there a way to get an older version of a picture into > this new code? Done.
Updated the code. If it's acceptable, I'd like to use SkPicture::CreateFromBuffer() for fuzzing (in a new fuzzer I'll create specifically for SkPicture, separate from the ImageFilter fuzzer). Any code that could use the new SkPicture::CreateFromBuffer() function should be currently ifdefed out.
This looks ok to me, but I'll leave for others. https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicturePlayb... File src/core/SkPicturePlayback.cpp (right): https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicturePlayb... src/core/SkPicturePlayback.cpp:445: // Write some of our data into a writebuffer Not sure I understand what "some of our data" means here. https://codereview.chromium.org/138063005/diff/160001/src/effects/SkPictureIm... File src/effects/SkPictureImageFilter.cpp (right): https://codereview.chromium.org/138063005/diff/160001/src/effects/SkPictureIm... src/effects/SkPictureImageFilter.cpp:49: buffer.writeBool(hasPicture); Maybe we should just writeBool(false) (and readBool() above) in an #else, so the SKP format is compatible when we turn on serialization.
https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicturePlayb... File src/core/SkPicturePlayback.cpp (right): https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicturePlayb... src/core/SkPicturePlayback.cpp:445: // Write some of our data into a writebuffer On 2014/02/05 16:44:39, Stephen White wrote: > Not sure I understand what "some of our data" means here. That's a copy-paste from SkPicturePlayback::serialize(). I'll rephrase it. https://codereview.chromium.org/138063005/diff/160001/src/effects/SkPictureIm... File src/effects/SkPictureImageFilter.cpp (right): https://codereview.chromium.org/138063005/diff/160001/src/effects/SkPictureIm... src/effects/SkPictureImageFilter.cpp:49: buffer.writeBool(hasPicture); On 2014/02/05 16:44:39, Stephen White wrote: > Maybe we should just writeBool(false) (and readBool() above) in an #else, so the SKP format is compatible when we turn on serialization. Good idea, I'll do that.
drive by comment https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicture.cpp File src/core/SkPicture.cpp (right): https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicture.cpp#... src/core/SkPicture.cpp:404: Is there some way to share more of this code (e.g., createHeader)?
https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.h File include/core/SkPicture.h (right): https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.... include/core/SkPicture.h:77: static SkPicture* CreateFromBuffer(SkReadBuffer&); Does this not need/want the same optional parameters as CreateFromStream?
https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.h File include/core/SkPicture.h (right): https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.... include/core/SkPicture.h:77: static SkPicture* CreateFromBuffer(SkReadBuffer&); On 2014/02/05 21:05:06, reed1 wrote: > Does this not need/want the same optional parameters as CreateFromStream? I'm assuming you're talking about "InstallPixelRefProc proc = &SkImageDecoder::DecodeMemory". So, this parameter is passed down like this : SkPicture::CreateFromStream() down to SkPicturePlayback::CreateFromStream() down to SkPicturePlayback::parseStream() down to SkPicturePlayback::parseStreamTag() down to SkReadBuffer::setBitmapDecoder() (Inside the PICT_BUFFER_SIZE_TAG case) Currently, the same sequence (if you replace "Stream" by "Buffer" in the previous list of function names) does not encode/decode bitmaps, it just saves the memory as is in the buffer, I believe, by using SkBitmap::flatten() so there's no current need for a decoder (The same way SkPicturePlayback::serialize() uses an encoder and SkPicturePlayback::flatten() does not) https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicture.cpp File src/core/SkPicture.cpp (right): https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicture.cpp#... src/core/SkPicture.cpp:404: On 2014/02/05 18:02:33, robertphillips wrote: > Is there some way to share more of this code (e.g., createHeader)? Done.
On 2014/02/05 21:33:05, sugoi1 wrote: > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.h > File include/core/SkPicture.h (right): > > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.... > include/core/SkPicture.h:77: static SkPicture* CreateFromBuffer(SkReadBuffer&); > On 2014/02/05 21:05:06, reed1 wrote: > > Does this not need/want the same optional parameters as CreateFromStream? > > I'm assuming you're talking about "InstallPixelRefProc proc = > &SkImageDecoder::DecodeMemory". So, this parameter is passed down like this : > > SkPicture::CreateFromStream() down to > SkPicturePlayback::CreateFromStream() down to > SkPicturePlayback::parseStream() down to > SkPicturePlayback::parseStreamTag() down to > SkReadBuffer::setBitmapDecoder() (Inside the PICT_BUFFER_SIZE_TAG case) > > Currently, the same sequence (if you replace "Stream" by "Buffer" in the > previous list of function names) does not encode/decode bitmaps, it just saves > the memory as is in the buffer, I believe, by using SkBitmap::flatten() so > there's no current need for a decoder (The same way > SkPicturePlayback::serialize() uses an encoder and SkPicturePlayback::flatten() > does not) Excellent. I should have checked. > > https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicture.cpp > File src/core/SkPicture.cpp (right): > > https://codereview.chromium.org/138063005/diff/160001/src/core/SkPicture.cpp#... > src/core/SkPicture.cpp:404: > On 2014/02/05 18:02:33, robertphillips wrote: > > Is there some way to share more of this code (e.g., createHeader)? > > Done.
API lgtm
https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.h File include/core/SkPicture.h (right): https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.... include/core/SkPicture.h:77: static SkPicture* CreateFromBuffer(SkReadBuffer&); On 2014/02/05 21:33:06, sugoi1 wrote: > On 2014/02/05 21:05:06, reed1 wrote: > > Does this not need/want the same optional parameters as CreateFromStream? > > I'm assuming you're talking about "InstallPixelRefProc proc = > &SkImageDecoder::DecodeMemory". So, this parameter is passed down like this : > > SkPicture::CreateFromStream() down to > SkPicturePlayback::CreateFromStream() down to > SkPicturePlayback::parseStream() down to > SkPicturePlayback::parseStreamTag() down to > SkReadBuffer::setBitmapDecoder() (Inside the PICT_BUFFER_SIZE_TAG case) > > Currently, the same sequence (if you replace "Stream" by "Buffer" in the > previous list of function names) does not encode/decode bitmaps, it just saves > the memory as is in the buffer, I believe, by using SkBitmap::flatten() so > there's no current need for a decoder (The same way > SkPicturePlayback::serialize() uses an encoder and SkPicturePlayback::flatten() > does not) This is almost true. In the case of a subclass of SkPixelRef() that implements onRefEncodedData(), the encoded data is written instead of flattening the SkBitmap (see SkWriteBuffer::writeBitmap()), and in general, such an SkPixelRef cannot be flattened normally (see SkCachingPixelRef for an example). When unflattening, you'll still need to decode images held in these types of SkPixelRefs somehow.
On 2014/02/05 22:20:10, scroggo wrote: > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.h > File include/core/SkPicture.h (right): > > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.... > include/core/SkPicture.h:77: static SkPicture* CreateFromBuffer(SkReadBuffer&); > On 2014/02/05 21:33:06, sugoi1 wrote: > > On 2014/02/05 21:05:06, reed1 wrote: > > > Does this not need/want the same optional parameters as CreateFromStream? > > > > I'm assuming you're talking about "InstallPixelRefProc proc = > > &SkImageDecoder::DecodeMemory". So, this parameter is passed down like this : > > > > SkPicture::CreateFromStream() down to > > SkPicturePlayback::CreateFromStream() down to > > SkPicturePlayback::parseStream() down to > > SkPicturePlayback::parseStreamTag() down to > > SkReadBuffer::setBitmapDecoder() (Inside the PICT_BUFFER_SIZE_TAG case) > > > > Currently, the same sequence (if you replace "Stream" by "Buffer" in the > > previous list of function names) does not encode/decode bitmaps, it just saves > > the memory as is in the buffer, I believe, by using SkBitmap::flatten() so > > there's no current need for a decoder (The same way > > SkPicturePlayback::serialize() uses an encoder and > SkPicturePlayback::flatten() > > does not) > > This is almost true. In the case of a subclass of SkPixelRef() that implements > onRefEncodedData(), the encoded data is written instead of flattening the > SkBitmap (see SkWriteBuffer::writeBitmap()), and in general, such an SkPixelRef > cannot be flattened normally (see SkCachingPixelRef for an example). > > When unflattening, you'll still need to decode images held in these types of > SkPixelRefs somehow. AFAIK, only 3 classes implement onRefEncodedData() and these are SkCachingPixelRef, SkDiscardablePixelRef and LazyDecodingPixelRef (that last one's in Webkit), but all 3 classes are declared as SK_DECLARE_UNFLATTENABLE_OBJECT(), so this problem may exist someday, but I don't think it's an issue today.
On 2014/02/06 15:19:04, sugoi1 wrote: > On 2014/02/05 22:20:10, scroggo wrote: > > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.h > > File include/core/SkPicture.h (right): > > > > > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.... > > include/core/SkPicture.h:77: static SkPicture* > CreateFromBuffer(SkReadBuffer&); > > On 2014/02/05 21:33:06, sugoi1 wrote: > > > On 2014/02/05 21:05:06, reed1 wrote: > > > > Does this not need/want the same optional parameters as CreateFromStream? > > > > > > I'm assuming you're talking about "InstallPixelRefProc proc = > > > &SkImageDecoder::DecodeMemory". So, this parameter is passed down like this > : > > > > > > SkPicture::CreateFromStream() down to > > > SkPicturePlayback::CreateFromStream() down to > > > SkPicturePlayback::parseStream() down to > > > SkPicturePlayback::parseStreamTag() down to > > > SkReadBuffer::setBitmapDecoder() (Inside the PICT_BUFFER_SIZE_TAG case) > > > > > > Currently, the same sequence (if you replace "Stream" by "Buffer" in the > > > previous list of function names) does not encode/decode bitmaps, it just > saves > > > the memory as is in the buffer, I believe, by using SkBitmap::flatten() so > > > there's no current need for a decoder (The same way > > > SkPicturePlayback::serialize() uses an encoder and > > SkPicturePlayback::flatten() > > > does not) > > > > This is almost true. In the case of a subclass of SkPixelRef() that implements > > onRefEncodedData(), the encoded data is written instead of flattening the > > SkBitmap (see SkWriteBuffer::writeBitmap()), and in general, such an > SkPixelRef > > cannot be flattened normally (see SkCachingPixelRef for an example). > > > > When unflattening, you'll still need to decode images held in these types of > > SkPixelRefs somehow. > > AFAIK, only 3 classes implement onRefEncodedData() and these are > SkCachingPixelRef, SkDiscardablePixelRef and LazyDecodingPixelRef (that last > one's in Webkit), but all 3 classes are declared as > SK_DECLARE_UNFLATTENABLE_OBJECT(), so this problem may exist someday, but I > don't think it's an issue today. Right. What SK_DECLARE_UNFLATTENABLE_OBJECT() is hinting at is that when you attempt to flatten them, they won't be flattened, and therefore they won't be recreated. As for when it is going to be a problem, my understanding is that our goal is to make more images lazy decoded (using these classes - in particular the ones in Skia, which are meant to be a replacement for LazyDecodingPixelRef). This will be a problem as soon as anyone tries to call CreateFromBuffer on an SkReadBuffer that has one of these classes encoded in it, without setting an InstallPixelRefProc first. Since CreateFromBuffer takes the SkReadBuffer as a parameter, we could leave it up to the caller to supply the InstallPixelRefProc before calling the function. If the main way we get to this function is from a CreateFromStream call (which had the option of setting the InstallPixelRefProc), this may be the right approach. If we go that route, please add a comment that they should call SkReadBuffer::setBitmapDecoder or they may miss some bitmaps (since it's a public function that a client could call otherwise). We might also want a similar comment on SkPictureImageFilter's SkReadBuffer constructor. https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... File src/effects/SkPictureImageFilter.cpp (right): https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... src/effects/SkPictureImageFilter.cpp:56: buffer.writeBool(false); This will require a change to the PICTURE_VERSION.
On 2014/02/06 15:52:46, scroggo wrote: > On 2014/02/06 15:19:04, sugoi1 wrote: > > On 2014/02/05 22:20:10, scroggo wrote: > > > > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.h > > > File include/core/SkPicture.h (right): > > > > > > > > > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.... > > > include/core/SkPicture.h:77: static SkPicture* > > CreateFromBuffer(SkReadBuffer&); > > > On 2014/02/05 21:33:06, sugoi1 wrote: > > > > On 2014/02/05 21:05:06, reed1 wrote: > > > > > Does this not need/want the same optional parameters as > CreateFromStream? > > > > > > > > I'm assuming you're talking about "InstallPixelRefProc proc = > > > > &SkImageDecoder::DecodeMemory". So, this parameter is passed down like > this > > : > > > > > > > > SkPicture::CreateFromStream() down to > > > > SkPicturePlayback::CreateFromStream() down to > > > > SkPicturePlayback::parseStream() down to > > > > SkPicturePlayback::parseStreamTag() down to > > > > SkReadBuffer::setBitmapDecoder() (Inside the PICT_BUFFER_SIZE_TAG case) > > > > > > > > Currently, the same sequence (if you replace "Stream" by "Buffer" in the > > > > previous list of function names) does not encode/decode bitmaps, it just > > saves > > > > the memory as is in the buffer, I believe, by using SkBitmap::flatten() so > > > > there's no current need for a decoder (The same way > > > > SkPicturePlayback::serialize() uses an encoder and > > > SkPicturePlayback::flatten() > > > > does not) > > > > > > This is almost true. In the case of a subclass of SkPixelRef() that > implements > > > onRefEncodedData(), the encoded data is written instead of flattening the > > > SkBitmap (see SkWriteBuffer::writeBitmap()), and in general, such an > > SkPixelRef > > > cannot be flattened normally (see SkCachingPixelRef for an example). > > > > > > When unflattening, you'll still need to decode images held in these types of > > > SkPixelRefs somehow. > > > > AFAIK, only 3 classes implement onRefEncodedData() and these are > > SkCachingPixelRef, SkDiscardablePixelRef and LazyDecodingPixelRef (that last > > one's in Webkit), but all 3 classes are declared as > > SK_DECLARE_UNFLATTENABLE_OBJECT(), so this problem may exist someday, but I > > don't think it's an issue today. > > Right. What SK_DECLARE_UNFLATTENABLE_OBJECT() is hinting at is that when you > attempt to flatten them, they won't be flattened, and therefore they won't be > recreated. > > As for when it is going to be a problem, my understanding is that our goal is to > make more images lazy decoded (using these classes - in particular the ones in > Skia, which are meant to be a replacement for LazyDecodingPixelRef). This will > be a problem as soon as anyone tries to call CreateFromBuffer on an SkReadBuffer > that has one of these classes encoded in it, without setting an > InstallPixelRefProc first. > > Since CreateFromBuffer takes the SkReadBuffer as a parameter, we could leave it > up to the caller to supply the InstallPixelRefProc before calling the function. > If the main way we get to this function is from a CreateFromStream call (which > had the option of setting the InstallPixelRefProc), this may be the right > approach. If we go that route, please add a comment that they should call > SkReadBuffer::setBitmapDecoder or they may miss some bitmaps (since it's a > public function that a client could call otherwise). We might also want a > similar comment on SkPictureImageFilter's SkReadBuffer constructor. > > https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... > File src/effects/SkPictureImageFilter.cpp (right): > > https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... > src/effects/SkPictureImageFilter.cpp:56: buffer.writeBool(false); > This will require a change to the PICTURE_VERSION. Orthogonal to this, there's also the security issue of whether we want to be decoding bitmaps in the browser (or even GPU) process. Image decoders have historically been a pretty fertile source of exploits, which is why we've usually restricted their decoding to the renderer process, where they can be sandboxed. IIRC we do this even for images requested by (decoded for) the browser process, such as favicons.
On 2014/02/06 16:13:54, Stephen White wrote: > On 2014/02/06 15:52:46, scroggo wrote: > > On 2014/02/06 15:19:04, sugoi1 wrote: > > > On 2014/02/05 22:20:10, scroggo wrote: > > > > > > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.h > > > > File include/core/SkPicture.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/138063005/diff/160001/include/core/SkPicture.... > > > > include/core/SkPicture.h:77: static SkPicture* > > > CreateFromBuffer(SkReadBuffer&); > > > > On 2014/02/05 21:33:06, sugoi1 wrote: > > > > > On 2014/02/05 21:05:06, reed1 wrote: > > > > > > Does this not need/want the same optional parameters as > > CreateFromStream? > > > > > > > > > > I'm assuming you're talking about "InstallPixelRefProc proc = > > > > > &SkImageDecoder::DecodeMemory". So, this parameter is passed down like > > this > > > : > > > > > > > > > > SkPicture::CreateFromStream() down to > > > > > SkPicturePlayback::CreateFromStream() down to > > > > > SkPicturePlayback::parseStream() down to > > > > > SkPicturePlayback::parseStreamTag() down to > > > > > SkReadBuffer::setBitmapDecoder() (Inside the PICT_BUFFER_SIZE_TAG case) > > > > > > > > > > Currently, the same sequence (if you replace "Stream" by "Buffer" in the > > > > > previous list of function names) does not encode/decode bitmaps, it just > > > saves > > > > > the memory as is in the buffer, I believe, by using SkBitmap::flatten() > so > > > > > there's no current need for a decoder (The same way > > > > > SkPicturePlayback::serialize() uses an encoder and > > > > SkPicturePlayback::flatten() > > > > > does not) > > > > > > > > This is almost true. In the case of a subclass of SkPixelRef() that > > implements > > > > onRefEncodedData(), the encoded data is written instead of flattening the > > > > SkBitmap (see SkWriteBuffer::writeBitmap()), and in general, such an > > > SkPixelRef > > > > cannot be flattened normally (see SkCachingPixelRef for an example). > > > > > > > > When unflattening, you'll still need to decode images held in these types > of > > > > SkPixelRefs somehow. > > > > > > AFAIK, only 3 classes implement onRefEncodedData() and these are > > > SkCachingPixelRef, SkDiscardablePixelRef and LazyDecodingPixelRef (that last > > > one's in Webkit), but all 3 classes are declared as > > > SK_DECLARE_UNFLATTENABLE_OBJECT(), so this problem may exist someday, but I > > > don't think it's an issue today. > > > > Right. What SK_DECLARE_UNFLATTENABLE_OBJECT() is hinting at is that when you > > attempt to flatten them, they won't be flattened, and therefore they won't be > > recreated. > > > > As for when it is going to be a problem, my understanding is that our goal is > to > > make more images lazy decoded (using these classes - in particular the ones in > > Skia, which are meant to be a replacement for LazyDecodingPixelRef). This will > > be a problem as soon as anyone tries to call CreateFromBuffer on an > SkReadBuffer > > that has one of these classes encoded in it, without setting an > > InstallPixelRefProc first. > > > > Since CreateFromBuffer takes the SkReadBuffer as a parameter, we could leave > it > > up to the caller to supply the InstallPixelRefProc before calling the > function. > > If the main way we get to this function is from a CreateFromStream call (which > > had the option of setting the InstallPixelRefProc), this may be the right > > approach. If we go that route, please add a comment that they should call > > SkReadBuffer::setBitmapDecoder or they may miss some bitmaps (since it's a > > public function that a client could call otherwise). We might also want a > > similar comment on SkPictureImageFilter's SkReadBuffer constructor. > > > > > https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... > > File src/effects/SkPictureImageFilter.cpp (right): > > > > > https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... > > src/effects/SkPictureImageFilter.cpp:56: buffer.writeBool(false); > > This will require a change to the PICTURE_VERSION. > > Orthogonal to this, there's also the security issue of whether we > want to be decoding bitmaps in the browser (or even GPU) process. > Image decoders have historically been a pretty fertile source of > exploits, which is why we've usually restricted their decoding to the > renderer process, where they can be sandboxed. IIRC we do this even > for images requested by (decoded for) the browser process, such as > favicons. This came up in an earlier meeting here. I think we want to start a project to *enable* decoding in the GPU process, even if its for a subset of formats (e.g. webp). I know that will mean hardening that code, but that seems like a useful thing to do.
https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... File src/effects/SkPictureImageFilter.cpp (right): https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... src/effects/SkPictureImageFilter.cpp:56: buffer.writeBool(false); On 2014/02/06 15:52:47, scroggo wrote: > This will require a change to the PICTURE_VERSION. Done.
On 2014/02/06 16:37:39, sugoi1 wrote: > https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... > File src/effects/SkPictureImageFilter.cpp (right): > > https://codereview.chromium.org/138063005/diff/250001/src/effects/SkPictureIm... > src/effects/SkPictureImageFilter.cpp:56: buffer.writeBool(false); > On 2014/02/06 15:52:47, scroggo wrote: > > This will require a change to the PICTURE_VERSION. > > Done. lgtm
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/138063005/350001
Message was sent while issue was closed.
Change committed as 13347
Message was sent while issue was closed.
This was reverted in r13348 due to a bump in the skp file format.
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/138063005/550001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch. svn: Commit failed (details follow): svn: MKACTIVITY of '/svn/!svn/act/06602e26-1c3e-455f-a9d8-06578961b664': authorization failed: Could not authenticate to server: rejected Basic challenge (https://skia.googlecode.com)
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/138063005/550001
Message was sent while issue was closed.
Change committed as 13354
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/149283003/ by sugoi@chromium.org. The reason for reverting is: SKP bot failed.
Message was sent while issue was closed.
Reverted in r13356 (https://codereview.chromium.org/153583007/) due to SKPs rebuild bot failures. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
