Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308273011:
Handle zero-length encoded images gracefully during deserialization (Closed)
DescriptionHandle zero-length encoded images gracefully during deserialization
Image encoding may fail during serialization, resulting in zero-length
encoded data in the SKP.
Instead of invalidating the stream (and preventing deserialization of
the whole picture) we can instantiate placeholder images.
BUG=skia:4285
R=reed@google.com,robertphillips@google.com
Committed: https://skia.googlesource.com/skia/+/c3470340b6658dea7baa3ac90d3b716c0afd7051
Patch Set 1 #Patch Set 2 : extra tests #
Total comments: 2
Patch Set 3 : drop the checkerboard #Patch Set 4 : use an image generator #
Total comments: 2
Patch Set 5 : generator comments #
Total comments: 2
Messages
Total messages: 31 (10 generated)
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/1308273011/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308273011/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1308273011/diff/20001/src/core/SkPictureData.cpp File src/core/SkPictureData.cpp (right): https://codereview.chromium.org/1308273011/diff/20001/src/core/SkPictureData.... src/core/SkPictureData.cpp:472: return make_checkerboard_image(width, height); This seems odd to me, partly because its arbitrary drawing, partly because it can be arbitrarily big (ram), and partly because it hides the fact that there was a failure. I was thinking we should just return NULL (and perhaps also return width/height in case the caller wanted to know that).
https://codereview.chromium.org/1308273011/diff/20001/src/core/SkPictureData.cpp File src/core/SkPictureData.cpp (right): https://codereview.chromium.org/1308273011/diff/20001/src/core/SkPictureData.... src/core/SkPictureData.cpp:472: return make_checkerboard_image(width, height); On 2015/09/04 17:26:24, reed1 wrote: > This seems odd to me, partly because its arbitrary drawing, partly because it > can be arbitrarily big (ram), Agreed. > and partly because it hides the fact that there > was a failure. I was thinking we should just return NULL (and perhaps also > return width/height in case the caller wanted to know that). That's problematic because the caller (new_array_from_buffer) will immediately fail if it sees a null value (and delete all previous images too). In some sense, hiding the failure is exactly what we're trying to accomplish: should loading such an SKP succeed or not? If the answer is yes, then we're effectively hiding the failure at some level, and the question becomes how to best do that. One option is to allocate a picture-backed SkImage which draws nothing. It saves the RAM and also preserves the SkImage metadata (dimensions). WDYT?
On 2015/09/04 17:50:41, f(malita) wrote: > One option is to allocate a picture-backed SkImage which draws nothing. It > saves the RAM and also preserves the SkImage metadata (dimensions). WDYT? Updated, PTAL.
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/1308273011/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308273011/40001
Agreed, I don't want us to fail to load the SKP, but its not clear to me where in the stack of functions we want to inject the heuristic that a failed image should be one thing or another. If we don't want to return NULL, perhaps an "empty" generator is even simpler/clearer: one that always returns false for its queries (but it has a legit info). SkImage::NewFromGenerator(new empty_generator);
Perhaps an advantage over the picture one, is that this one won't commit any RAM ever, since it always "fails".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/04 18:04:39, reed1 wrote: > If we don't want to return NULL, perhaps an "empty" generator is even > simpler/clearer: one that always returns false for its queries (but it has a > legit info). > > SkImage::NewFromGenerator(new empty_generator); Ah, good idea. 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/patch-status/1308273011/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308273011/60001
lgtm w/ comment suggestion https://codereview.chromium.org/1308273011/diff/60001/src/core/SkPictureData.cpp File src/core/SkPictureData.cpp (right): https://codereview.chromium.org/1308273011/diff/60001/src/core/SkPictureData.... src/core/SkPictureData.cpp:439: class EmptyImageGenerator final : public SkImageGenerator { // This guy intentionally should always fail on all attempts to get its pixels, simulating a bad or empty codec stream
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
https://codereview.chromium.org/1308273011/diff/60001/src/core/SkPictureData.cpp File src/core/SkPictureData.cpp (right): https://codereview.chromium.org/1308273011/diff/60001/src/core/SkPictureData.... src/core/SkPictureData.cpp:439: class EmptyImageGenerator final : public SkImageGenerator { On 2015/09/04 18:17:27, reed1 wrote: > // This guy intentionally should always fail on all attempts to get its pixels, > simulating a bad or empty codec stream Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308273011/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308273011/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1308273011/#ps80001 (title: "generator comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308273011/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308273011/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/c3470340b6658dea7baa3ac90d3b716c0afd7051
Message was sent while issue was closed.
scroggo@google.com changed reviewers: + scroggo@google.com
Message was sent while issue was closed.
LGTM https://codereview.chromium.org/1308273011/diff/80001/include/core/SkPicture.h File include/core/SkPicture.h (right): https://codereview.chromium.org/1308273011/diff/80001/include/core/SkPicture.... include/core/SkPicture.h:106: * bitmaps and images in the picture. I like that we turn bitmaps into images! I was thinking though, that if we ever captured an SKP that was created with bitmaps, we might get difference performance numbers in our tests due to converting to images. Maybe that's okay though, especially since we are switching to image anyway. https://codereview.chromium.org/1308273011/diff/80001/src/core/SkPictureData.cpp File src/core/SkPictureData.cpp (right): https://codereview.chromium.org/1308273011/diff/80001/src/core/SkPictureData.... src/core/SkPictureData.cpp:439: // This generator intentionally should always fail on all attempts to get its pixels, FWIW, we used to show deserialize such failed encodes as red bitmaps, to show something went wrong. Not sure whether that's something we should be doing.
Message was sent while issue was closed.
red: yea, that's sort of where I was going when I wondered about the "heuristic" of drawing checker-board-vs-nothing-vs-? Perhaps in the future the runtime-options passed to picture_deserialize could take over that aspect. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
