[Webkit-unassigned] [Bug 106842] adding support for DiscardablePixelRef for caching lazily decoded images

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106842





--- Comment #68 from James Robinson <jamesr at chromium.org>  2013-01-25 16:38:55 PST ---
(From update of attachment 184824)
View in context: https://bugs.webkit.org/attachment.cgi?id=184824&action=review

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:89
> +    ASSERT(!m_lockedMemory);
> +
> +    if (!m_firstOnLock) {
> +        ASSERT(m_discardable);
> +        m_firstOnLock = true;
> +        m_lockedMemory = m_discardable->data();
> +    } else if (m_discardable->lock())
> +        m_lockedMemory = m_discardable->data();

this seems confused. what's the purpose of m_firstOnLock if we do the exact same thing (except for ASSERTS) in both branches?

> Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:104
> +    if (pixelRef && pixelRef->getURI() && !strncmp(pixelRef->getURI(), labelDiscardable, sizeof(labelDiscardable)))

SkString has equals() and operator== and several other utilities. use those, don't use strncmp

> Source/WebCore/platform/image-decoders/ImageDecoder.h:422
> +            if (m_frameBufferCache.isEmpty())
> +                m_frameBufferCache.resize(1);
> +            m_frameBufferCache[0].setMemoryAllocator(allocator);

I don't think this scheme will work for any multi-frame format, since frames in the cache with index > 0 won't have our allocator set.  It looks busted for gif/ico at least.  I guess if the worst case is that these frames won't use our allocator it's OK for now, but it needs a FIXME here at least pointing out the problem.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.