Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1487683004:
Create an SkCodecImageGenerator (Closed)
DescriptionCreate an SkCodecImageGenerator
BUG=skia:
patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001)
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1487683004
Committed: https://skia.googlesource.com/skia/+/e1102ce1d3d0895e840e756e155ec56b5a1a7540
Committed: https://skia.googlesource.com/skia/+/edd2dcf08224fc6c313610bfc9f9f8257e0e85ef
Patch Set 1 : #
Total comments: 1
Patch Set 2 : SkCodecImageGenerator - no tests #
Total comments: 3
Patch Set 3 : Replace SkImageDecoderGenerator with SkCodecImageGenerator #Patch Set 4 : Use Mike's gm to test SkCodecImageGenerator #Patch Set 5 : Test SkCodecImageGenerator in addition to the default #Patch Set 6 : Rebase #Patch Set 7 : Use SkCodecImageGenerator #
Total comments: 2
Patch Set 8 : Use public inheritance #Patch Set 9 : Rebase #Patch Set 10 : Add test in CodexTest, do not replace SkIDGenerator #
Total comments: 3
Patch Set 11 : Move to src/codec and codec.gyp #
Depends on Patchset: Messages
Total messages: 41 (19 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Initial attempt at a codec image generator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) ========== to ========== Initial attempt at a codec image generator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) ==========
Description was changed from ========== Initial attempt at a codec image generator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) ========== to ========== Initial attempt at a SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) ==========
msarett@google.com changed reviewers: + reed@google.com, scroggo@google.com
This didn't really upload as I had hoped - my changes and Mike's changes are both there. Luckily, we didn't modify any of the same files. https://codereview.chromium.org/1487683004/diff/20001/src/ports/SkImageGenera... File src/ports/SkImageGenerator_skia.cpp (left): https://codereview.chromium.org/1487683004/diff/20001/src/ports/SkImageGenera... src/ports/SkImageGenerator_skia.cpp:89: I'm not sure what the implications of removing this are.
Perhaps we can land this w/o my new entry-points, just by overriding onGetPixels for now. That way we can start to test it, and then I can modify it for the new (scaling) methods...
Patchset #2 (id:40001) has been deleted
Description was changed from ========== Initial attempt at a SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) ========== to ========== Add an a SkCodecImageGenerator This CL will compile the SkCodecImageGenerator, but it won't be used yet. In the next step, we will make SkImageGenerator::NewFromEncodedImpl() create an SkCodecImageGenerator. BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) ==========
On 2015/12/01 03:39:16, reed1 wrote: > Perhaps we can land this w/o my new entry-points, just by overriding onGetPixels > for now. That way we can start to test it, and then I can modify it for the new > (scaling) methods... SGTM. Here's the new version.
seems right. can we add a unittest or gm to exercise it?
On 2015/12/02 17:38:16, reed1 wrote: > seems right. > > can we add a unittest or gm to exercise it? I think we should use it wherever we currently use whichever generator uses SkImageDecoder. https://codereview.chromium.org/1487683004/diff/60001/gyp/core.gyp File gyp/core.gyp (right): https://codereview.chromium.org/1487683004/diff/60001/gyp/core.gyp#newcode21 gyp/core.gyp:21: '../include/codec', I thought that back in the day we did not make core depend on images, so it could be built without image decoders. This is why SkImageDecoder is in include/core instead of include/images. But looking down below, it appears that core does depend on images (at least the other headers?). Is that something we should consider here? https://codereview.chromium.org/1487683004/diff/60001/src/core/SkCodecImageGe... File src/core/SkCodecImageGenerator.cpp (right): https://codereview.chromium.org/1487683004/diff/60001/src/core/SkCodecImageGe... src/core/SkCodecImageGenerator.cpp:26: SkAutoTDelete<SkCodec> fCodec; // Owned nit: I think it's obvious that fCodec is owned, since it is held in an SkAutoTDelete.
Alright so now there are three options. Patch Set 2: SkCodecImageGenerator without tests Patch Set 3: Replace SkImageDecoderGenerator with SkCodecImageGenerator Patch Set 4: Borrow Mike's gm to test SkCodecImageGenerator I think Patch Set 3 will break things (though I haven't investigated yet). SkCodecImageGenerator doesn't implement onGetYUVPixels() (which SkImageDecoderGenerator did). Also I thought it was strange that SkImageGenerator_skia.cpp was compiled in images.gyp? Patch Set 3 moves it to codec.gyp. This may be related to Leon's comments about keeping image decoding out of core. Maybe we should move SkCodecImageGenerator there? Patch Set 4 might be easier in the short term. Or even better, tomorrow I can write my own GM rather than borrowing Mike's. https://codereview.chromium.org/1487683004/diff/60001/src/core/SkCodecImageGe... File src/core/SkCodecImageGenerator.cpp (right): https://codereview.chromium.org/1487683004/diff/60001/src/core/SkCodecImageGe... src/core/SkCodecImageGenerator.cpp:26: SkAutoTDelete<SkCodec> fCodec; // Owned On 2015/12/02 20:22:55, scroggo wrote: > nit: I think it's obvious that fCodec is owned, since it is held in an > SkAutoTDelete. I think so too. Removing the comment.
agreed it may be too early to divert the default SkImage factory to use Codecs... I'm fine with just a gm for now. I'd prefer we just extend the GM to *also* call your genrerator, rather than remove the default.
Works for me
Description was changed from ========== Add an a SkCodecImageGenerator This CL will compile the SkCodecImageGenerator, but it won't be used yet. In the next step, we will make SkImageGenerator::NewFromEncodedImpl() create an SkCodecImageGenerator. BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) ========== to ========== Add an a SkCodecImageGenerator This CL will compile the SkCodecImageGenerator, but it won't be used yet. In the next step, we will make SkImageGenerator::NewFromEncodedImpl() create an SkCodecImageGenerator. BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #7 (id:160001) has been deleted
Description was changed from ========== Add an a SkCodecImageGenerator This CL will compile the SkCodecImageGenerator, but it won't be used yet. In the next step, we will make SkImageGenerator::NewFromEncodedImpl() create an SkCodecImageGenerator. BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Replace SkIDGenerator with SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
I'm hoping to replace SkImageDecoderGenerator with SkCodecImageGenerator in this CL (see Patch Set 7). A new CL description now reflects this as well. This is not ready to land. Known remaining issues are: (1) Need to implement getYUVPlanes() for SkCodec. (2) KTXUnitTests fail. SkCodec does not support KTX. (3) gm/etc1bitmap.cpp seg faults. SkCodec does not support ETC. (4) all_bitmap_configs.cpp fails an assert on 4444. I think we are fine to remove the 4444 portion of this test. We might want to sync up on (2) and (3). I'm not really sure how we want to approach fixing these failures.
https://codereview.chromium.org/1487683004/diff/180001/src/core/SkCodecImageG... File src/core/SkCodecImageGenerator.h (right): https://codereview.chromium.org/1487683004/diff/180001/src/core/SkCodecImageG... src/core/SkCodecImageGenerator.h:12: class SkCodecImageGenerator : SkImageGenerator { not "public SkImageGenerator" ?
lgtm other than my "public" question
We still need to work out the failures before this can land. Also, it's worth noting that there is a DecodingImageGenerator (in addition to the SkImageDecoderGenerator that we delete here) that we will want to delete/replace as well. https://codereview.chromium.org/1487683004/diff/180001/src/core/SkCodecImageG... File src/core/SkCodecImageGenerator.h (right): https://codereview.chromium.org/1487683004/diff/180001/src/core/SkCodecImageG... src/core/SkCodecImageGenerator.h:12: class SkCodecImageGenerator : SkImageGenerator { On 2016/01/04 17:52:08, reed1 wrote: > not "public SkImageGenerator" ? "public" sgtm. Originally I decided to make everything as private as possible. But I think it makes sense for everyone to know that SkCodecImageGenerator inherits from SkImageGenerator.
On 2015/12/21 16:56:34, msarett wrote: > I'm hoping to replace SkImageDecoderGenerator with SkCodecImageGenerator in this > CL (see Patch Set 7). A new CL description now reflects this as well. > > This is not ready to land. Known remaining issues are: > (1) Need to implement getYUVPlanes() for SkCodec. > (2) KTXUnitTests fail. SkCodec does not support KTX. I think it's safe to remove the tests. We could add an SkCodec version, but I'm not aware of any demand. Maybe Robert has an opinion on this (since it was his intern who added this code, IIRC)? > (3) gm/etc1bitmap.cpp seg faults. SkCodec does not support ETC. We need to add this one. We had not been working on it, since Android was not using ETC, but Android now wants ETC support (see b/25335647 - I just added you to the CC list). We may want to hide it behind a flag (like I think the current one is) until that becomes more concrete. > (4) all_bitmap_configs.cpp fails an assert on 4444. I think we are fine to > remove the 4444 portion of this test. +1 > > We might want to sync up on (2) and (3). I'm not really sure how we want to > approach fixing these failures.
Patchset #9 (id:220001) has been deleted
PTAL I've added a test in CodexTest and removed the code that replaces SkImageDecoderGenerator with SkCodecImageGenerator. Originally, I did not intend to land this until we were ready to replace SkIDGenerator. But, we need this to test SkCodec::getYUVPixels() (and we need getYUVPixels() to replace SkIDGenerator.
> Replace SkIDGenerator with SkCodecImageGenerator Please update the commit message Other than that, and my concern about index8 in the test, lgtm https://codereview.chromium.org/1487683004/diff/260001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1487683004/diff/260001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:391: REPORTER_ASSERT(r, gen->getPixels(info, bm.getPixels(), bm.rowBytes())); Are these ever index8? If so, it seems like we've missed something?
Description was changed from ========== Replace SkIDGenerator with SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add an SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add an SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Create an SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1487683004/diff/260001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1487683004/diff/260001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:391: REPORTER_ASSERT(r, gen->getPixels(info, bm.getPixels(), bm.rowBytes())); On 2016/01/14 19:06:18, scroggo wrote: > Are these ever index8? If so, it seems like we've missed something? Up at the top, we force info to kN32. Partly I think for the convenience of not needing a color table, and partly to make it easier to compare digests.
https://codereview.chromium.org/1487683004/diff/260001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1487683004/diff/260001/tests/CodexTest.cpp#ne... tests/CodexTest.cpp:391: REPORTER_ASSERT(r, gen->getPixels(info, bm.getPixels(), bm.rowBytes())); On 2016/01/14 19:13:12, msarett wrote: > On 2016/01/14 19:06:18, scroggo wrote: > > Are these ever index8? If so, it seems like we've missed something? > > Up at the top, we force info to kN32. Partly I think for the convenience of not > needing a color table, and partly to make it easier to compare digests. Ah, great. Nvm then.
The CQ bit was checked by msarett@google.com
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/1487683004/#ps260001 (title: "Add test in CodexTest, do not replace SkIDGenerator")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1487683004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1487683004/260001
Message was sent while issue was closed.
Description was changed from ========== Create an SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Create an SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e1102ce1d3d0895e840e756e155ec56b5a1a7540 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://skia.googlesource.com/skia/+/e1102ce1d3d0895e840e756e155ec56b5a1a7540
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:260001) has been created in https://codereview.chromium.org/1582373003/ by msarett@google.com. The reason for reverting is: Core doesn't know about Codec..
Message was sent while issue was closed.
Description was changed from ========== Create an SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e1102ce1d3d0895e840e756e155ec56b5a1a7540 ========== to ========== Create an SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e1102ce1d3d0895e840e756e155ec56b5a1a7540 ==========
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1487683004/#ps280001 (title: "Move to src/codec and codec.gyp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1487683004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1487683004/280001
Message was sent while issue was closed.
Description was changed from ========== Create an SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e1102ce1d3d0895e840e756e155ec56b5a1a7540 ========== to ========== Create an SkCodecImageGenerator BUG=skia: patch from issue 1396323007 at patchset 120001 (http://crrev.com/1396323007#ps120001) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e1102ce1d3d0895e840e756e155ec56b5a1a7540 Committed: https://skia.googlesource.com/skia/+/edd2dcf08224fc6c313610bfc9f9f8257e0e85ef ==========
Message was sent while issue was closed.
Committed patchset #11 (id:280001) as https://skia.googlesource.com/skia/+/edd2dcf08224fc6c313610bfc9f9f8257e0e85ef |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
