Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1426753006:
SkResourceCache::GetAllocator() index8 and other color types handling (Closed)
|
Created:
5 years, 1 month ago by aleksandar.stojiljkovic Modified:
5 years, 1 month ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkResourceCache::GetAllocator() index8 and other color types handling
Unit tests added to check all color types in SkOneShotDiscardablePixelref
SkResourceCacheTest seems to have been allways using default Heap Allocator.
Fixed so that it uses private (not global that is) SkDiscardableMemory.
BUG=skia:4355
Committed: https://skia.googlesource.com/skia/+/07e2692da3971960f8ae8adcd78ccb4bae7ab446
Patch Set 1 #
Total comments: 18
Patch Set 2 : after review comments #
Total comments: 9
Patch Set 3 : review 2. processed comments #Patch Set 4 : #
Messages
Total messages: 19 (7 generated)
aleksandar.stojiljkovic@intel.com changed reviewers: + halcanary@chromium.org, reed@chromium.org
Interested in this one as working on gif animation optimization (Chromium 476531) and 565 support for jpegs in Chromium.
Description was changed from ========== SkResourceCache::GetAllocator() index8 and other color types handling Unit tests added to check all color types in SkOneShotDiscardablePixelref SkResourceCacheTest seems to have been allways using default Heap Allocator. Fixed so that it uses private (not global that is) SkDiscardableMemory. BUG=4355 ========== to ========== SkResourceCache::GetAllocator() index8 and other color types handling Unit tests added to check all color types in SkOneShotDiscardablePixelref SkResourceCacheTest seems to have been allways using default Heap Allocator. Fixed so that it uses private (not global that is) SkDiscardableMemory. BUG=skia:4355 ==========
reed@google.com changed reviewers: + reed@google.com
Can the blink codecs actually return colortables? https://codereview.chromium.org/1426753006/diff/1/src/core/SkResourceCache.cpp File src/core/SkResourceCache.cpp (right): https://codereview.chromium.org/1426753006/diff/1/src/core/SkResourceCache.cp... src/core/SkResourceCache.cpp:182: if (ctable == nullptr) { 1. nit: skia convention is either if (!ctable) { or if (nullptr == ctable) { 2. if we do return false, we will leak the discardable memory (allocated above). I know the old code had a similar problem/leak. Can we perform this check before we call the fFactory? https://codereview.chromium.org/1426753006/diff/1/src/core/SkResourceCache.cp... src/core/SkResourceCache.cpp:189: if ((unsigned)bitmap->colorType() > (unsigned)kLastEnum_SkColorType) { What is this actually testing, now that we're not restricted to just 32bit types? https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... File tests/CachedDecodingPixelRefTest.cpp (right): https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:166: static uint32_t Color() { return 0xff10345a; } // value choosen so that there is no loss when converting to to RGB565 and back nits: skia limits itself to 100 columns https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:166: static uint32_t Color() { return 0xff10345a; } // value choosen so that there is no loss when converting to to RGB565 and back I know the old code used uint32_t for this type, but w/ your extensions to the test, it is getting very confusion what this type is, specifically, is it SkColor or SkPMColor. Since your modifications at the bottom of this file now always call bm.getColor(x,y), I think we should declare this to be SkColor. To do that cleanly, we'll need to convert it to SkPMColor when we use it to write into our image buffer (both for kN32 and for the colortable entry. https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:175: return SkImageInfo::Make(TestImageGenerator::Width(), TestImageGenerator::Height(), colorType, 100 cols https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:209: SkDitherPixel32ToPixel16(TestImageGenerator::Color()), info.width()); why the dither conversion? Since you're repeating the same value for all pixels, it is a little confusing/unnecessary it seems. https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:270: SkColorType testColorTypes[] = { nit: const https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:276: check_pixelref(TestImageGenerator::kFailGetPixels_TestType, reporter, nullptr, testColorTypes[i]); 100 cols https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:303: SkColorType testColorTypes[] = { const https://codereview.chromium.org/1426753006/diff/1/tests/SkResourceCacheTest.cpp File tests/SkResourceCacheTest.cpp (right): https://codereview.chromium.org/1426753006/diff/1/tests/SkResourceCacheTest.c... tests/SkResourceCacheTest.cpp:193: SkColorType testTypes[] = { nit: const https://codereview.chromium.org/1426753006/diff/1/tests/SkResourceCacheTest.c... tests/SkResourceCacheTest.cpp:204: make_bitmap(&cachedBitmap, SkImageInfo::Make(5, 5, testTypes[i], kPremul_SkAlphaType), allocator); nits: 100 cols
reed@google.com changed reviewers: + scroggo@google.com
Fixing... https://codereview.chromium.org/1426753006/diff/1/src/core/SkResourceCache.cpp File src/core/SkResourceCache.cpp (right): https://codereview.chromium.org/1426753006/diff/1/src/core/SkResourceCache.cp... src/core/SkResourceCache.cpp:182: if (ctable == nullptr) { On 2015/11/09 14:14:19, reed1 wrote: > 1. nit: skia convention is either > > if (!ctable) { > or > if (nullptr == ctable) { > > 2. if we do return false, we will leak the discardable memory (allocated above). > I know the old code had a similar problem/leak. Can we perform this check before > we call the fFactory? Good point. Fixing. https://codereview.chromium.org/1426753006/diff/1/src/core/SkResourceCache.cp... src/core/SkResourceCache.cpp:189: if ((unsigned)bitmap->colorType() > (unsigned)kLastEnum_SkColorType) { On 2015/11/09 14:14:18, reed1 wrote: > What is this actually testing, now that we're not restricted to just 32bit > types? Argh, I was some time ago, as customer, misusing color types to plug GPU texture compression decoders. Similar check's there in SkMallocPixelRef.cpp is_valid(). This is internal code, not an API like SkMallocPixelRef and makes sense not to have it. https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... File tests/CachedDecodingPixelRefTest.cpp (right): https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:166: static uint32_t Color() { return 0xff10345a; } // value choosen so that there is no loss when converting to to RGB565 and back On 2015/11/09 14:14:19, reed1 wrote: > I know the old code used uint32_t for this type, but w/ your extensions to the > test, it is getting very confusion what this type is, specifically, is it > SkColor or SkPMColor. > > Since your modifications at the bottom of this file now always call > bm.getColor(x,y), I think we should declare this to be SkColor. To do that > cleanly, we'll need to convert it to SkPMColor when we use it to write into our > image buffer (both for kN32 and for the colortable entry. Acknowledged. https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:166: static uint32_t Color() { return 0xff10345a; } // value choosen so that there is no loss when converting to to RGB565 and back On 2015/11/09 14:14:19, reed1 wrote: > nits: skia limits itself to 100 columns Acknowledged. https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:175: return SkImageInfo::Make(TestImageGenerator::Width(), TestImageGenerator::Height(), colorType, On 2015/11/09 14:14:19, reed1 wrote: > 100 cols Acknowledged. https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:209: SkDitherPixel32ToPixel16(TestImageGenerator::Color()), info.width()); On 2015/11/09 14:14:19, reed1 wrote: > why the dither conversion? Since you're repeating the same value for all pixels, > it is a little confusing/unnecessary it seems. I just didn't know better: SkDitherPixel32ToPixel16 (though called diether) doesn't take surrounding pixels into account, but only does RGB888-RGB565 quantization and packing into 16 bits: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i... I wanted to call directly SkPackRGB16, but there is assert if components don't fit 5/6 bits.
https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... File tests/CachedDecodingPixelRefTest.cpp (right): https://codereview.chromium.org/1426753006/diff/1/tests/CachedDecodingPixelRe... tests/CachedDecodingPixelRefTest.cpp:209: SkDitherPixel32ToPixel16(TestImageGenerator::Color()), info.width()); On 2015/11/09 15:28:00, aleksandar.stojiljkovic wrote: > On 2015/11/09 14:14:19, reed1 wrote: > > why the dither conversion? Since you're repeating the same value for all > pixels, > > it is a little confusing/unnecessary it seems. > I just didn't know better: > SkDitherPixel32ToPixel16 (though called diether) doesn't take surrounding pixels > into account, but only does RGB888-RGB565 quantization and packing into 16 bits: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i... > > I wanted to call directly SkPackRGB16, but there is assert if components don't > fit 5/6 bits. That dither call is the compliment to SkPixel32ToPixel16(). and *always* packs with a different bias applied, so it can pair up with SkPixel32ToPixel16 (calling each alternatively). I think your code will read simpler if you just call SkPixel32ToPixel16().
update after review
halcanary@google.com changed reviewers: - halcanary@chromium.org
code lgtm, but will wait on Leon. Is there a plan for Blink's codecs to return ColorTables?
lgtm https://codereview.chromium.org/1426753006/diff/20001/src/core/SkResourceCach... File src/core/SkResourceCache.cpp (right): https://codereview.chromium.org/1426753006/diff/20001/src/core/SkResourceCach... src/core/SkResourceCache.cpp:82: SkOneShotDiscardablePixelRef(const SkImageInfo&, SkDiscardableMemory*, size_t rowBytes, SkColorTable* ctable); nit: over 100 chars. Also, the name "ctable" adds no extra information, so I think this is just fine as "SkColorTable*" https://codereview.chromium.org/1426753006/diff/20001/src/core/SkResourceCach... src/core/SkResourceCache.cpp:96: SkColorTable* fCTable; Can you add a comment about ownership of the color table? (Sometimes we do not take a reference as in SkPixmap, but here you do.) https://codereview.chromium.org/1426753006/diff/20001/src/core/SkResourceCach... src/core/SkResourceCache.cpp:190: bitmap->setPixelRef(new SkOneShotDiscardablePixelRef(info, dm, bitmap->rowBytes(), ctable))->unref(); nit: over 100 chars https://codereview.chromium.org/1426753006/diff/20001/tests/CachedDecodingPix... File tests/CachedDecodingPixelRefTest.cpp (right): https://codereview.chromium.org/1426753006/diff/20001/tests/CachedDecodingPix... tests/CachedDecodingPixelRefTest.cpp:195: case kN32_SkColorType: nit: in Skia, we indent the case statement from switch https://codereview.chromium.org/1426753006/diff/20001/tests/CachedDecodingPix... tests/CachedDecodingPixelRefTest.cpp:279: for (size_t i = 0; i < SK_ARRAY_COUNT(testColorTypes); ++i) { nit: we can now use range based for loops. This is a lot easier to read as: for (SkColorType testColorType : testColorTypes) { or something like that https://codereview.chromium.org/1426753006/diff/20001/tests/SkResourceCacheTe... File tests/SkResourceCacheTest.cpp (right): https://codereview.chromium.org/1426753006/diff/20001/tests/SkResourceCacheTe... tests/SkResourceCacheTest.cpp:27: bitmap->tryAllocPixels(allocator, ctable); I think this should be allocPixels, to match the others (so we'll throw instead of silently failing). https://codereview.chromium.org/1426753006/diff/20001/tests/SkResourceCacheTe... tests/SkResourceCacheTest.cpp:197: kARGB_4444_SkColorType, Are we supporting 4444? https://codereview.chromium.org/1426753006/diff/20001/tests/SkResourceCacheTe... tests/SkResourceCacheTest.cpp:254: cache.reset(new SkResourceCache(byteLimit)); nit: Why not just put this on the stack? (same down below)
Updated after Leon's review. I don't have trybot and commit rights to repository; need to ask a committer to submit it after review. Thanks. >Is there a plan for Blink's codecs to return ColorTables? I understand Leon needs to answer about Blink Index8 & Gif plans - some time ago we discussed about feasibility, he gave me some guideline and I'll work with his help on fixing animated gif performance issues that way. https://codereview.chromium.org/1426753006/diff/20001/tests/SkResourceCacheTe... File tests/SkResourceCacheTest.cpp (right): https://codereview.chromium.org/1426753006/diff/20001/tests/SkResourceCacheTe... tests/SkResourceCacheTest.cpp:254: cache.reset(new SkResourceCache(byteLimit)); On 2015/11/09 20:18:09, scroggo wrote: > nit: Why not just put this on the stack? (same down below) Good point - refactored the purpose out. fixed.
On 2015/11/09 22:19:48, aleksandar.stojiljkovic wrote: > Updated after Leon's review. > > I don't have trybot and commit rights to repository; need to ask a committer to > submit it after review. Thanks. Actually, I think you're good to go. For Skia, you just need lgtm from a committer, and then you can click the little checkbox that says "Commit". Intel is already in the AUTHORS file (https://skia.googlesource.com/skia/+/master/AUTHORS), so I think that covers you. > > >Is there a plan for Blink's codecs to return ColorTables? > I understand Leon needs to answer about Blink Index8 & Gif plans - some time ago > we discussed about feasibility, he gave me some guideline and I'll work with his > help on fixing animated gif performance issues that way. I think it's worth investigating. But it does mean a big change to the way GIFImageDecoder works. If you have a GIF image with two frames A and B: - A uses 256 colors - B uses 256 other colors, and has a transparent region so that frame A shows through GIFImageDecoder will decode A into an SkBitmap (part of an ImageFrame). To decode B, it will copy the first SkBitmap into a new one, then it will decode B into it. When it comes time to draw frame B, we just draw the second SkBitmap. We cannot do that using index8, but we could instead decode two independent frames (i.e. no copying) and then draw them both at draw time. I think this method will be slower if we need to draw B multiple times, but it could also save a lot of memory, depending on the size of the image. It will also require some reworking of the way ImageDecoder works. > > https://codereview.chromium.org/1426753006/diff/20001/tests/SkResourceCacheTe... > File tests/SkResourceCacheTest.cpp (right): > > https://codereview.chromium.org/1426753006/diff/20001/tests/SkResourceCacheTe... > tests/SkResourceCacheTest.cpp:254: cache.reset(new SkResourceCache(byteLimit)); > On 2015/11/09 20:18:09, scroggo wrote: > > nit: Why not just put this on the stack? (same down below) > Good point - refactored the purpose out. fixed.
The CQ bit was checked by aleksandar.stojiljkovic@intel.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/1426753006/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426753006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426753006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/07e2692da3971960f8ae8adcd78ccb4bae7ab446 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
