Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
141483004:
Optimization for image decoding using Skia discardable memory (Closed)
|
Created:
6 years, 11 months ago by Alpha Left Google Modified:
6 years, 10 months ago CC:
blink-reviews, jamesr, krit, dsinclair, jbroman, danakj, Rik, Stephen Chennney, pdr., rwlbuis, reed1 Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionOptimization for image decoding using Skia discardable memory
In the new Skia discardable memory path. We used to use heap memory to
decode. And then copy into the discardable memory allocated by Skia.
There's an opportunity for optimization. If we know that all data has been received
and the image is not a multi frame image. We can decode directly into
the memory given by Skia.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166887
Patch Set 1 : fix compile #Patch Set 2 : merged #Patch Set 3 : fixed layout tests #Patch Set 4 : fixed webkit_unit_tests on debug #
Total comments: 1
Patch Set 5 : merged #Patch Set 6 : SkBitmap::installPixels #
Total comments: 1
Patch Set 7 : return false #
Total comments: 1
Patch Set 8 : done #
Messages
Total messages: 20 (0 generated)
This patch is merged now and is good for review.
uhoh not ready. test failures..
Please review. This is ready now.
I'm not sure I understand what you mean by "the memory given by skia" in this case. Do you mean discardable memory that Skia has allocated? https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... Source/platform/graphics/ImageFrameGenerator.cpp:42: // Creates a SkPixelRef such that the memory for pixels is given by an external body. IIRC, the usual way to handle externally-managed memory in Skia is to call setPixels() on the SkBitmap. Is there a reason you can't use that functionality in this case?
On 2014/01/25 18:33:55, Stephen White wrote: > I'm not sure I understand what you mean by "the memory given by skia" in this > case. Do you mean discardable memory that Skia has allocated? Yes that's what I mean. > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > File Source/platform/graphics/ImageFrameGenerator.cpp (right): > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > Source/platform/graphics/ImageFrameGenerator.cpp:42: // Creates a SkPixelRef > such that the memory for pixels is given by an external body. > IIRC, the usual way to handle externally-managed memory in Skia is to call > setPixels() on the SkBitmap. Is there a reason you can't use that functionality > in this case? The SkBitmap that should wraps this external memory is created by the ImageDecoder. The memory is currently allocated in ImageFrame::setSize(). It can also use SkBitmap::Allocator. If I do the SkBitmap::setPixels() route I will need to add methods to ImageFrame and ImageDecoder. With the approach in this patch I don't need to change the decoders.
On 2014/02/10 20:50:58, Alpha wrote: > On 2014/01/25 18:33:55, Stephen White wrote: > > I'm not sure I understand what you mean by "the memory given by skia" in this > > case. Do you mean discardable memory that Skia has allocated? > > Yes that's what I mean. Would it make sense to change the comment to say "the discardable memory allocated by Skia", then? > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > File Source/platform/graphics/ImageFrameGenerator.cpp (right): > > > > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > Source/platform/graphics/ImageFrameGenerator.cpp:42: // Creates a SkPixelRef > > such that the memory for pixels is given by an external body. > > IIRC, the usual way to handle externally-managed memory in Skia is to call > > setPixels() on the SkBitmap. Is there a reason you can't use that > functionality > > in this case? > > The SkBitmap that should wraps this external memory is created by the > ImageDecoder. The memory is currently allocated in ImageFrame::setSize(). It can > also use SkBitmap::Allocator. If I do the SkBitmap::setPixels() route I will > need to add methods to ImageFrame and ImageDecoder. With the approach in this > patch I don't need to change the decoders. At the cost of yet another PixelRef subclass. I'm a little nervous about them, since the API is fairly low-level in Skia, and has changed a bit recently, making these dependencies require 3-sided patches. Mike, do you have an opinion here? Does Skia have any existing functionality for this?
On 2014/02/10 21:18:51, Stephen White wrote: > On 2014/02/10 20:50:58, Alpha wrote: > > On 2014/01/25 18:33:55, Stephen White wrote: > > > I'm not sure I understand what you mean by "the memory given by skia" in > this > > > case. Do you mean discardable memory that Skia has allocated? > > > > Yes that's what I mean. > > Would it make sense to change the comment to say "the discardable memory > allocated by Skia", then? > > > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > > File Source/platform/graphics/ImageFrameGenerator.cpp (right): > > > > > > > > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > > Source/platform/graphics/ImageFrameGenerator.cpp:42: // Creates a SkPixelRef > > > such that the memory for pixels is given by an external body. > > > IIRC, the usual way to handle externally-managed memory in Skia is to call > > > setPixels() on the SkBitmap. Is there a reason you can't use that > > functionality > > > in this case? > > > > The SkBitmap that should wraps this external memory is created by the > > ImageDecoder. The memory is currently allocated in ImageFrame::setSize(). It > can > > also use SkBitmap::Allocator. If I do the SkBitmap::setPixels() route I will > > need to add methods to ImageFrame and ImageDecoder. With the approach in this > > patch I don't need to change the decoders. > > At the cost of yet another PixelRef subclass. I'm a little nervous about them, > since > the API is fairly low-level in Skia, and has changed a bit recently, making > these > dependencies require 3-sided patches. > > Mike, do you have an opinion here? Does Skia have any existing functionality for > this? There's no PixelRef subclass. I'm just adding a new SkBitmap::Allocator subclass.
SkBitmap::installPixels(const SkImageInfo& info, void* pixels, size_t rb,
void (*releaseProc)(void* addr, void* context),
void* context)
Can you not just call this guy directly?
On 2014/02/10 21:22:50, Alpha wrote: > On 2014/02/10 21:18:51, Stephen White wrote: > > On 2014/02/10 20:50:58, Alpha wrote: > > > On 2014/01/25 18:33:55, Stephen White wrote: > > > > I'm not sure I understand what you mean by "the memory given by skia" in > > this > > > > case. Do you mean discardable memory that Skia has allocated? > > > > > > Yes that's what I mean. > > > > Would it make sense to change the comment to say "the discardable memory > > allocated by Skia", then? > > > > > > > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > > > File Source/platform/graphics/ImageFrameGenerator.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > > > Source/platform/graphics/ImageFrameGenerator.cpp:42: // Creates a > SkPixelRef > > > > such that the memory for pixels is given by an external body. > > > > IIRC, the usual way to handle externally-managed memory in Skia is to call > > > > setPixels() on the SkBitmap. Is there a reason you can't use that > > > functionality > > > > in this case? > > > > > > The SkBitmap that should wraps this external memory is created by the > > > ImageDecoder. The memory is currently allocated in ImageFrame::setSize(). It > > can > > > also use SkBitmap::Allocator. If I do the SkBitmap::setPixels() route I will > > > need to add methods to ImageFrame and ImageDecoder. With the approach in > this > > > patch I don't need to change the decoders. > > > > At the cost of yet another PixelRef subclass. I'm a little nervous about them, > > since > > the API is fairly low-level in Skia, and has changed a bit recently, making > > these > > dependencies require 3-sided patches. > > > > Mike, do you have an opinion here? Does Skia have any existing functionality > for > > this? > > There's no PixelRef subclass. I'm just adding a new SkBitmap::Allocator > subclass. Sorry about the confusing CL description. I forgot to update it. The code creates a ExternalMemoryPixelRefAllocator and then the SkBitmap is created by SkMallocPixelRef::NewDirect. I've removed the line that said ExternalMemoryPixelRef.
On 2014/02/10 21:22:50, Alpha wrote: > On 2014/02/10 21:18:51, Stephen White wrote: > > On 2014/02/10 20:50:58, Alpha wrote: > > > On 2014/01/25 18:33:55, Stephen White wrote: > > > > I'm not sure I understand what you mean by "the memory given by skia" in > > this > > > > case. Do you mean discardable memory that Skia has allocated? > > > > > > Yes that's what I mean. > > > > Would it make sense to change the comment to say "the discardable memory > > allocated by Skia", then? > > > > > > > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > > > File Source/platform/graphics/ImageFrameGenerator.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > > > Source/platform/graphics/ImageFrameGenerator.cpp:42: // Creates a > SkPixelRef > > > > such that the memory for pixels is given by an external body. > > > > IIRC, the usual way to handle externally-managed memory in Skia is to call > > > > setPixels() on the SkBitmap. Is there a reason you can't use that > > > functionality > > > > in this case? > > > > > > The SkBitmap that should wraps this external memory is created by the > > > ImageDecoder. The memory is currently allocated in ImageFrame::setSize(). It > > can > > > also use SkBitmap::Allocator. If I do the SkBitmap::setPixels() route I will > > > need to add methods to ImageFrame and ImageDecoder. With the approach in > this > > > patch I don't need to change the decoders. > > > > At the cost of yet another PixelRef subclass. I'm a little nervous about them, > > since > > the API is fairly low-level in Skia, and has changed a bit recently, making > > these > > dependencies require 3-sided patches. > > > > Mike, do you have an opinion here? Does Skia have any existing functionality > for > > this? > > There's no PixelRef subclass. I'm just adding a new SkBitmap::Allocator > subclass. Sorry about the confusing CL description. I forgot to update it. The code creates a ExternalMemoryPixelRefAllocator and then the SkBitmap is created by SkMallocPixelRef::NewDirect. I've removed the line that said ExternalMemoryPixelRef.
On 2014/02/10 21:26:36, Alpha wrote: > On 2014/02/10 21:22:50, Alpha wrote: > > On 2014/02/10 21:18:51, Stephen White wrote: > > > On 2014/02/10 20:50:58, Alpha wrote: > > > > On 2014/01/25 18:33:55, Stephen White wrote: > > > > > I'm not sure I understand what you mean by "the memory given by skia" in > > > this > > > > > case. Do you mean discardable memory that Skia has allocated? > > > > > > > > Yes that's what I mean. > > > > > > Would it make sense to change the comment to say "the discardable memory > > > allocated by Skia", then? > > > > > > > > > > > > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > > > > File Source/platform/graphics/ImageFrameGenerator.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/141483004/diff/220001/Source/platform/graphic... > > > > > Source/platform/graphics/ImageFrameGenerator.cpp:42: // Creates a > > SkPixelRef > > > > > such that the memory for pixels is given by an external body. > > > > > IIRC, the usual way to handle externally-managed memory in Skia is to > call > > > > > setPixels() on the SkBitmap. Is there a reason you can't use that > > > > functionality > > > > > in this case? > > > > > > > > The SkBitmap that should wraps this external memory is created by the > > > > ImageDecoder. The memory is currently allocated in ImageFrame::setSize(). > It > > > can > > > > also use SkBitmap::Allocator. If I do the SkBitmap::setPixels() route I > will > > > > need to add methods to ImageFrame and ImageDecoder. With the approach in > > this > > > > patch I don't need to change the decoders. > > > > > > At the cost of yet another PixelRef subclass. I'm a little nervous about > them, > > > since > > > the API is fairly low-level in Skia, and has changed a bit recently, making > > > these > > > dependencies require 3-sided patches. > > > > > > Mike, do you have an opinion here? Does Skia have any existing functionality > > for > > > this? > > > > There's no PixelRef subclass. I'm just adding a new SkBitmap::Allocator > > subclass. > > Sorry about the confusing CL description. I forgot to update it. The code > creates a ExternalMemoryPixelRefAllocator and then the SkBitmap is created by > SkMallocPixelRef::NewDirect. > > I've removed the line that said ExternalMemoryPixelRef. Ah, I misread the code. A new Allocator seems less fragile.
I've changed the code to use SkBitmap::installPixels. The use of allocator remains.
sgtm https://codereview.chromium.org/141483004/diff/380001/Source/platform/graphic... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/141483004/diff/380001/Source/platform/graphic... Source/platform/graphics/ImageFrameGenerator.cpp:62: dst->installPixels(m_info, m_pixels, m_rowBytes, 0, 0); return false if this guy fails.
On 2014/02/10 21:51:01, reed1 wrote: > sgtm > > https://codereview.chromium.org/141483004/diff/380001/Source/platform/graphic... > File Source/platform/graphics/ImageFrameGenerator.cpp (right): > > https://codereview.chromium.org/141483004/diff/380001/Source/platform/graphic... > Source/platform/graphics/ImageFrameGenerator.cpp:62: dst->installPixels(m_info, > m_pixels, m_rowBytes, 0, 0); > return false if this guy fails. Done.
Naming nit up to you, but it's possible others might be confused by the naming. LGTM https://codereview.chromium.org/141483004/diff/360002/Source/platform/graphic... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/141483004/diff/360002/Source/platform/graphic... Source/platform/graphics/ImageFrameGenerator.cpp:42: // Creates a SkPixelRef such that the memory for pixels is given by an external body. Naming nit: maybe this should just be ExternalMemoryAllocator, then? It doesn't deal with PixelRefs directly, which seems misleading.
On 2014/02/10 21:58:02, Stephen White wrote: > Naming nit up to you, but it's possible others might be confused by the naming. > > LGTM > > https://codereview.chromium.org/141483004/diff/360002/Source/platform/graphic... > File Source/platform/graphics/ImageFrameGenerator.cpp (right): > > https://codereview.chromium.org/141483004/diff/360002/Source/platform/graphic... > Source/platform/graphics/ImageFrameGenerator.cpp:42: // Creates a SkPixelRef > such that the memory for pixels is given by an external body. > Naming nit: maybe this should just be ExternalMemoryAllocator, then? It doesn't > deal with PixelRefs directly, which seems misleading. Done.
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/141483004/480001
Message was sent while issue was closed.
Change committed as 166887 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
