Use our own SkBitmap for which we manage the pixels instead of letting SkDevice construct a SkBitmap. This avoids an unnecessary memset otherwise done by SkDevice.
Created attachment 126651 [details] Patch
Verified that this works both on Linux and Windows.
*** Bug 77897 has been marked as a duplicate of this bug. ***
This seems a little extreme, can't we just ask skia not to memset() and promise to set every pixel ourselves?
(In reply to comment #4) > This seems a little extreme, can't we just ask skia not to memset() and promise to set every pixel ourselves? AFAIK, there's no proper way to do that. We could pass isOpaque=true to the SkDevice constructor to avoid the memset but even though that might not have any bad side-effects in our case, we would be exploiting implementation details..
This is really really a Skia API question, so hopefully Mike or Stephen can chime in. It seems excessive to require that in order to avoid a memset we have to take complete control over allocating and owning the memory for the bitmap. TextureManager::memoryUseBytes() is a best-effort estimate for resource tracking, we haven't depending on it being exact up until now.
(In reply to comment #6) > This is really really a Skia API question, so hopefully Mike or Stephen can chime in. It seems excessive to require that in order to avoid a memset we have to take complete control over allocating and owning the memory for the bitmap. TextureManager::memoryUseBytes() is a best-effort estimate for resource tracking, we haven't depending on it being exact up until now. Sure, if the Skia API allows us do this then we should just use that. As we already rely on the pixel format, row stride and hence also the size of the buffer to be of some specific values for our texture uploads to be correct I don't think it matters a huge deal if Skia owns the memory used for the bitmap or not. Probably more appropriate to use SkBitmap::ComputeSize instead of TextureManager::memoryUseBytes() in this patch. I'll fix that for now.
Created attachment 126658 [details] Patch Use SkBitmap::ComputeSize
(In reply to comment #6) > This is really really a Skia API question, so hopefully Mike or Stephen can chime in. It seems excessive to require that in order to avoid a memset we have to take complete control over allocating and owning the memory for the bitmap. TextureManager::memoryUseBytes() is a best-effort estimate for resource tracking, we haven't depending on it being exact up until now. I think what you want is SkBitmap::allocPixels(). This will let skia take care of the size calculation, allocation and lifetime for you, while still avoiding the memset. You could also keep the SkBitmap persistent between invocations of prepareRect() (which I assume are fairly frequent?) and avoid reallocating the bitmap if the size hasn't changed. This would avoid both the memset() and the malloc() if the size is the same.
(In reply to comment #9) > (In reply to comment #6) > > This is really really a Skia API question, so hopefully Mike or Stephen can chime in. It seems excessive to require that in order to avoid a memset we have to take complete control over allocating and owning the memory for the bitmap. TextureManager::memoryUseBytes() is a best-effort estimate for resource tracking, we haven't depending on it being exact up until now. > > I think what you want is SkBitmap::allocPixels(). This will let skia take care of the size calculation, allocation and lifetime for you, while still avoiding the memset. Thanks! that will do it. > > You could also keep the SkBitmap persistent between invocations of prepareRect() (which I assume are fairly frequent?) and avoid reallocating the bitmap if the size hasn't changed. This would avoid both the memset() and the malloc() if the size is the same. The malloc doesn't seem to cost much on the platforms where I've tested this so I'm avoiding that optimization for now as it might just a have negative effect on the memory pressure.
Created attachment 126682 [details] Patch Use allocPixels()
We used to preserve the buffer across frames for layers, but decided the memory cost wasn't worth it. I think our allocator should perform pretty well for these sorts of allocations.
We could potentially use stack memory for this after landing https://bugs.webkit.org/show_bug.cgi?id=72752. On the other hand we're also working on painting directly to shared memory.
Comment on attachment 126682 [details] Patch Nice! r=me
Comment on attachment 126682 [details] Patch Clearing flags on attachment: 126682 Committed r107515: <http://trac.webkit.org/changeset/107515>
All reviewed patches have been landed. Closing bug.
Alternatively, if you pass 'true' for the 4th (implicit) parameter to SkDevice's constructor, then you get two benefits: 1. I won't call memset, since you've told me the device is opaque 2. You've told me the device is opaque, so any blits using that bitmap will be faster, since it is known to be opaque.
(In reply to comment #17) > Alternatively, if you pass 'true' for the 4th (implicit) parameter to SkDevice's constructor, then you get two benefits: > 1. I won't call memset, since you've told me the device is opaque > 2. You've told me the device is opaque, so any blits using that bitmap will be faster, since it is known to be opaque. It's not always opaque though. Tile contents can be translucent.
(In reply to comment #17) > Alternatively, if you pass 'true' for the 4th (implicit) parameter to SkDevice's constructor, then you get two benefits: > 1. I won't call memset, since you've told me the device is opaque > 2. You've told me the device is opaque, so any blits using that bitmap will be faster, since it is known to be opaque. Presumably we can get this 2nd win by setting isOpaque on the bitmap appropriately? We already have information if the layer is fully opaque - at least for image layers. I can put together a simple CL for this.
(In reply to comment #19) > (In reply to comment #17) > > Alternatively, if you pass 'true' for the 4th (implicit) parameter to SkDevice's constructor, then you get two benefits: > > 1. I won't call memset, since you've told me the device is opaque > > 2. You've told me the device is opaque, so any blits using that bitmap will be faster, since it is known to be opaque. > > Presumably we can get this 2nd win by setting isOpaque on the bitmap appropriately? We already have information if the layer is fully opaque - at least for image layers. I can put together a simple CL for this. I don't think these bitmaps are ever used for blits.