Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2115023002:
Store mipmap levels in deferred texture image (Closed)
DescriptionStore mipmap levels in deferred texture image
This is a follow-up to https://codereview.chromium.org/2034933003/ which
was reverted due to a memory leak.
When creating the deferred texture image, detect if using medium / high
quality. If so, generate and store mipmaps in the deferred texture
image.
When creating a texture from that be sure to read it back out.
BUG=578304
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2115023002
Committed: https://skia.googlesource.com/skia/+/d6113140f7ae8996f679ac6698a60fb8c1386da3
Patch Set 1 #Patch Set 2 : Adding the test back in (with a change to auto unref the proxy). #Patch Set 3 : It looks like maybe the leak is in respecting gamma. #Patch Set 4 : Did I leak a SkBitmap? #
Total comments: 1
Patch Set 5 : The memory leak seems to be fixed. Checking. #Patch Set 6 : Rebasing against fix. #Patch Set 7 : Merged in serialization of color space info. #
Total comments: 10
Patch Set 8 : Making TU function static (instead of unnamed namespace) + moving it closer to call site. Indenting… #Patch Set 9 : Merging DeferredTextureImage unit tests. Adding tests for low and high qualities. #
Total comments: 2
Patch Set 10 : Renaming static function to_use_underscores. #Patch Set 11 : Updating call sites. #
Messages
Total messages: 40 (25 generated)
Description was changed from ========== Store mipmap levels in deferred texture image This is a follow-up to https://codereview.chromium.org/2034933003/ which was reverted due to a memory leak. When creating the deferred texture image, detect if using medium / high quality. If so, generate and store mipmaps in the deferred texture image. When creating a texture from that be sure to read it back out. BUG=578304 ========== to ========== Store mipmap levels in deferred texture image This is a follow-up to https://codereview.chromium.org/2034933003/ which was reverted due to a memory leak. When creating the deferred texture image, detect if using medium / high quality. If so, generate and store mipmaps in the deferred texture image. When creating a texture from that be sure to read it back out. BUG=578304 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2115023002 ==========
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
cblume@chromium.org changed reviewers: + brianosman@google.com, bsalomon@google.com, ericrk@chromium.org
This is a fix for a patch that landed a while ago and was reverted. There was a memory leak in the unit test. The memory leak ended up being a SkBitmap which wasn't being released. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2115023002/diff/60001/gm/deferredtextureimage... File gm/deferredtextureimage.cpp (right): https://codereview.chromium.org/2115023002/diff/60001/gm/deferredtextureimage... gm/deferredtextureimage.cpp:99: void onOnceBeforeDraw() override { I should have added helpful info for the reviewers. The only difference between this patch and https://codereview.chromium.org/2034933003/ is here. I am now using GetResourceAsImage. Before, I had: if (!GetResourceAsBitmap("mandrill_512.png", &bitmap)) { bitmap.allocN32Pixels(1, 1); bitmap.eraseARGB(255, 255, 0, 0); // red == bad } fDecodedImage = SkImage::MakeFromBitmap(bitmap); So it loses the behavior of allocating a 1x1 red pixel when the resource isn't found. But I wasn't a huge fan of that anyway.
On 2016/07/22 16:53:50, cblume wrote: > https://codereview.chromium.org/2115023002/diff/60001/gm/deferredtextureimage... > File gm/deferredtextureimage.cpp (right): > > https://codereview.chromium.org/2115023002/diff/60001/gm/deferredtextureimage... > gm/deferredtextureimage.cpp:99: void onOnceBeforeDraw() override { > I should have added helpful info for the reviewers. > > The only difference between this patch and > https://codereview.chromium.org/2034933003/ is here. > I am now using GetResourceAsImage. > > Before, I had: > if (!GetResourceAsBitmap("mandrill_512.png", &bitmap)) { > bitmap.allocN32Pixels(1, 1); > bitmap.eraseARGB(255, 255, 0, 0); // red == bad > } > fDecodedImage = SkImage::MakeFromBitmap(bitmap); > > So it loses the behavior of allocating a 1x1 red pixel when the resource isn't > found. But I wasn't a huge fan of that anyway. How did the original code leak? I think I'm missing something.
On 2016/07/22 17:27:58, bsalomon wrote: > How did the original code leak? I think I'm missing something. Oh, actually I'm not sure anymore. Last night, I assumed I was missing a ->release() or some such. The ASAN bot is definitely red before and green after. But now my confidence is lower. Maybe there is something inside one of those functions being called, in which case it would be nice to actually find the cause (since it isn't a missing ->release() ). That would let us fix the real error.
On 2016/07/22 18:07:26, cblume wrote:
> On 2016/07/22 17:27:58, bsalomon wrote:
> > How did the original code leak? I think I'm missing something.
>
> Oh, actually I'm not sure anymore.
> Last night, I assumed I was missing a ->release() or some such.
>
> The ASAN bot is definitely red before and green after. But now my confidence
is
> lower.
>
> Maybe there is something inside one of those functions being called, in which
> case it would be nice to actually find the cause (since it isn't a missing
> ->release() ). That would let us fix the real error.
Looks like the real leak happens in SkColorSpace_Base::NewRGB.
I'm investigating.
Direct leak of 336 byte(s) in 3 object(s) allocated from:
#0 0x7f87464eb220 in operator new(unsigned long)
/b/work/skia/third_party/externals/llvm/out/../projects/compiler-rt/lib/asan/asan_new_delete.cc:62
#1 0x7f8748920923 in SkColorSpace_Base::NewRGB(SkColorSpace::GammaNamed,
SkMatrix44 const&)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/core/SkColorSpace.cpp:121:32
#2 0x7f8748921acf in SkColorSpace::NewRGB(SkColorSpace::GammaNamed,
SkMatrix44 const&)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/core/SkColorSpace.cpp:125:12
#3 0x7f87496dbc54 in read_color_space(png_struct_def*, png_info_def*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/codec/SkPngCodec.cpp:246:16
#4 0x7f87496df674 in read_header(SkStream*, SkPngChunkReader*, SkCodec**,
png_struct_def**, png_info_def**)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/codec/SkPngCodec.cpp:612:42
#5 0x7f87496e25d0 in SkPngCodec::NewFromStream(SkStream*, SkPngChunkReader*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/codec/SkPngCodec.cpp:810:9
#6 0x7f87496890f9 in SkCodec::NewFromStream(SkStream*, SkPngChunkReader*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/codec/SkCodec.cpp:91:16
#7 0x7f8749689684 in SkCodec::NewFromData(SkData*, SkPngChunkReader*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/codec/SkCodec.cpp:114:12
#8 0x7f87497185e4 in SkCodecImageGenerator::NewFromEncodedCodec(SkData*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/codec/SkCodecImageGenerator.cpp:11:22
#9 0x7f8749719d40 in SkImageGenerator::NewFromEncodedImpl(SkData*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/ports/SkImageGenerator_skia.cpp:12:12
#10 0x7f8748ac8ad4 in SkImageGenerator::NewFromEncoded(SkData*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../src/core/SkImageGenerator.cpp:218:12
#11 0x7f8748470aa3 in GetResourceAsBitmap(char const*, SkBitmap*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../tools/Resources.cpp:31:41
#12 0x7f8747bb4e6c in DeferredTextureImage_Medium::onOnceBeforeDraw()
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../gm/deferredtextureimage.cpp:101:14
#13 0x7f87465f4c74 in skiagm::GM::drawBackground(SkCanvas*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../gm/gm.cpp:38:9
#14 0x7f87465f4773 in skiagm::GM::draw(SkCanvas*)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../gm/gm.cpp:23:5
#15 0x7f874655009b in DM::GMSrc::draw(SkCanvas*) const
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../dm/DMSrcSink.cpp:65:5
#16 0x7f874657a2ed in DM::GPUSink::draw(DM::Src const&, SkBitmap*,
SkWStream*, SkString*) const
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../dm/DMSrcSink.cpp:1145:17
#17 0x7f8746514105 in Task::Run(Task const&)
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../dm/DM.cpp:1022:25
#18 0x7f87464f0f39 in dm_main()
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../dm/DM.cpp:1324:32
#19 0x7f87464f9f42 in main
/b/work/skia/out/Build-Ubuntu-GCC-x86_64-Debug-ASAN-Trybot/Debug/../../../dm/DM.cpp:1415:12
#20 0x7f8740701ec4 in __libc_start_main
/build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287
I did a bit of printf debugging and it seems like the unit test is always hitting the line: return sk_sp<SkColorSpace>(new SkColorSpace_Base(gammaNamed, toXYZD50, kUnknown_Named)); inside this overload of SkColorSpace_Base::NewRGB It is strange to me that this memory is leaking. It is immediately placed into a sk_sp. Is it possible that the sk_sp<SkColorSpace> is being added to a cache somewhere and so the shared object lives beyond this particular unit test? And maybe that cache isn't actually cleaned? If so, is there a flush() or some such to force the cache to be cleaned? Also, I think the 336 bytes in 3 objects is because 1 src * 3 sinks + 0 tests = 3 tasks. The printf debugging shows that line is called 3 times. Although, sizeof(SkColorSpace) is 88. sizeof(sk_sp<SkColorSpace>) is 8.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
It looks like the memory leaks are all cleared up. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2115023002/diff/120001/gm/deferredtextureimag... File gm/deferredtextureimage.cpp (right): https://codereview.chromium.org/2115023002/diff/120001/gm/deferredtextureimag... gm/deferredtextureimage.cpp:16: class DeferredTextureImage_Medium : public skiagm::GM { Should we replace the existing deferred_image_data GM with this one? Seems like they are overlapping quite a bit. This one uses a more sensible image. It seems like if we added an encoded image case here it would do everything the other GM does. https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:23: bool shouldUseMipMaps(const SkImage::DeferredTextureImageUsageParams & param) { For file-local functions we use static over anonymous namespace and use_hacker_naming. Also, can you move it down closer to its caller where MipMapLevelData and DeferredTextureImage are defined? https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:392: SkSourceGammaTreatment fGammaTreatment; Let's either line up all the members or make them all have one space between type and name https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:493: SkAlphaType at = this->isOpaque() ? kOpaque_SkAlphaType : kPremul_SkAlphaType; I believe SkImage_Gpu now stores an alpha type.
https://codereview.chromium.org/2115023002/diff/120001/gm/deferredtextureimag... File gm/deferredtextureimage.cpp (right): https://codereview.chromium.org/2115023002/diff/120001/gm/deferredtextureimag... gm/deferredtextureimage.cpp:16: class DeferredTextureImage_Medium : public skiagm::GM { On 2016/07/29 14:12:35, bsalomon wrote: > Should we replace the existing deferred_image_data GM with this one? Seems like > they are overlapping quite a bit. This one uses a more sensible image. It seems > like if we added an encoded image case here it would do everything the other GM > does. I was planning to integrate the two somehow. This test is specific to mipmaps, showing the tree. I would be happy to add the other qualities here and an encoded image. Although, since they test different things (this one is specifically testing that the mipmaps are generated and serialized and deserialized in that different code path) it would mostly just be merging the files, right? https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:23: bool shouldUseMipMaps(const SkImage::DeferredTextureImageUsageParams & param) { On 2016/07/29 14:12:35, bsalomon wrote: > For file-local functions we use static over anonymous namespace and > use_hacker_naming. Also, can you move it down closer to its caller where > MipMapLevelData and DeferredTextureImage are defined? Done. https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:392: SkSourceGammaTreatment fGammaTreatment; On 2016/07/29 14:12:35, bsalomon wrote: > Let's either line up all the members or make them all have one space between > type and name Done. https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:493: SkAlphaType at = this->isOpaque() ? kOpaque_SkAlphaType : kPremul_SkAlphaType; On 2016/07/29 14:12:35, bsalomon wrote: > I believe SkImage_Gpu now stores an alpha type. This code is inside SkImage::getDeferred..., not SkImage_Gpu::... Are all SkImages actually SkImage_Gpu when compiled a certain way? If so, I can cast.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2115023002/diff/120001/gm/deferredtextureimag... File gm/deferredtextureimage.cpp (right): https://codereview.chromium.org/2115023002/diff/120001/gm/deferredtextureimag... gm/deferredtextureimage.cpp:16: class DeferredTextureImage_Medium : public skiagm::GM { On 2016/07/29 20:14:13, cblume wrote: > On 2016/07/29 14:12:35, bsalomon wrote: > > Should we replace the existing deferred_image_data GM with this one? Seems > like > > they are overlapping quite a bit. This one uses a more sensible image. It > seems > > like if we added an encoded image case here it would do everything the other > GM > > does. > > I was planning to integrate the two somehow. > This test is specific to mipmaps, showing the tree. > I would be happy to add the other qualities here and an encoded image. Although, > since they test different things (this one is specifically testing that the > mipmaps are generated and serialized and deserialized in that different code > path) it would mostly just be merging the files, right? I merged the two and added unit tests for low / high qualities. The tests now use the mandrill image as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ rename request https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2115023002/diff/120001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:493: SkAlphaType at = this->isOpaque() ? kOpaque_SkAlphaType : kPremul_SkAlphaType; On 2016/07/29 20:14:13, cblume wrote: > On 2016/07/29 14:12:35, bsalomon wrote: > > I believe SkImage_Gpu now stores an alpha type. > > This code is inside SkImage::getDeferred..., not SkImage_Gpu::... > > Are all SkImages actually SkImage_Gpu when compiled a certain way? If so, I can > cast. Nope, my suggestion was wrong https://codereview.chromium.org/2115023002/diff/160001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2115023002/diff/160001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:388: static bool shouldUseMipMaps(const SkImage::DeferredTextureImageUsageParams & param) { can you rename this to should_use_mip_maps?
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2115023002/diff/160001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2115023002/diff/160001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:388: static bool shouldUseMipMaps(const SkImage::DeferredTextureImageUsageParams & param) { On 2016/08/09 19:02:08, bsalomon wrote: > can you rename this to should_use_mip_maps? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 cblume@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2115023002/#ps200001 (title: "Updating call sites.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Store mipmap levels in deferred texture image This is a follow-up to https://codereview.chromium.org/2034933003/ which was reverted due to a memory leak. When creating the deferred texture image, detect if using medium / high quality. If so, generate and store mipmaps in the deferred texture image. When creating a texture from that be sure to read it back out. BUG=578304 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2115023002 ========== to ========== Store mipmap levels in deferred texture image This is a follow-up to https://codereview.chromium.org/2034933003/ which was reverted due to a memory leak. When creating the deferred texture image, detect if using medium / high quality. If so, generate and store mipmaps in the deferred texture image. When creating a texture from that be sure to read it back out. BUG=578304 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2115023002 Committed: https://skia.googlesource.com/skia/+/d6113140f7ae8996f679ac6698a60fb8c1386da3 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://skia.googlesource.com/skia/+/d6113140f7ae8996f679ac6698a60fb8c1386da3
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2227323002/ by halcanary@google.com. The reason for reverting is: speculative revert: android dm crashes. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
