Make LazyDecodingPixelRef inherit from skia::LazyPixelRef so that cc thread can access it
Created attachment 176559 [details] Patch
Is Skia rolled already?
The skia change i made is in rev 168995, and webkit now is using 169895. So I assume the change has rolled already. (In reply to comment #2) > Is Skia rolled already?
Comment on attachment 176559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176559&action=review > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:94 > + m_mutex.lock(); Use MutexLocker lock(m_mutex). The mutex just needs to protect access to m_frameGenerator and m_scaledSize. We don't absolutely need a mutex here because they don't mutate but just for safety.
Created attachment 176565 [details] Patch
(In reply to comment #4) > (From update of attachment 176559 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176559&action=review > > > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:94 > > + m_mutex.lock(); > > Use MutexLocker lock(m_mutex). The mutex just needs to protect access to m_frameGenerator and m_scaledSize. We don't absolutely need a mutex here because they don't mutate but just for safety. Done
Comment on attachment 176565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176565&action=review > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:95 > + // TODO(hclam): check if only a particular rect is available in image cache. Whoops also WebKit doesn't use TODO(XXX), it's just TODO. Sorry I didn't point it out earlier.
Created attachment 176569 [details] Patch
(In reply to comment #7) > (From update of attachment 176565 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176565&action=review > > > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:95 > > + // TODO(hclam): check if only a particular rect is available in image cache. > > Whoops also WebKit doesn't use TODO(XXX), it's just TODO. Sorry I didn't point it out earlier. ah.... Sorry, missed that. Fixed now
lgtm from me by the way.
Comment on attachment 176569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176569&action=review OK. r=me > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:103 > + lockPixels(); > + unlockPixels(); Not sure I really get this, since you're going to have to lock again to use them, but I'm sure you know that. :)
Comment on attachment 176569 [details] Patch Clearing flags on attachment: 176569 Committed r136167: <http://trac.webkit.org/changeset/136167>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 103701
Hi, Stephen, Can you help me recommit this patch? A fix for the previous build fix is submitted in r170542 The current DEPS roll is at 171019 now. Thanks
Comment on attachment 176569 [details] Patch OK. r=me
Comment on attachment 176569 [details] Patch Clearing flags on attachment: 176569 Committed r136630: <http://trac.webkit.org/changeset/136630>
Reverted r136630 for reason: Still breaking Chrome/Win Debug builds Committed r136728: <http://trac.webkit.org/changeset/136728>
Comment on attachment 176569 [details] Patch Hi, Stephen, sorry about this, but can you approve this again. Thanks. We believe the windows dbg bot failure is caused by that the lazy_pixel_ref.h is not built into the dll. So we split the file into a .cc and .h file. The fix is committed in r171375. And DEPS has rolled to r171376
(In reply to comment #20) > (From update of attachment 176569 [details]) > Hi, Stephen, sorry about this, but can you approve this again. Thanks. > > We believe the windows dbg bot failure is caused by that the lazy_pixel_ref.h is not built into the dll. So we split the file into a .cc and .h file. The fix is committed in r171375. And DEPS has rolled to r171376 Have you built windows debug (component build) locally?
(In reply to comment #21) > (In reply to comment #20) > > (From update of attachment 176569 [details] [details]) > > Hi, Stephen, sorry about this, but can you approve this again. Thanks. > > > > We believe the windows dbg bot failure is caused by that the lazy_pixel_ref.h is not built into the dll. So we split the file into a .cc and .h file. The fix is committed in r171375. And DEPS has rolled to r171376 > > Have you built windows debug (component build) locally? And if you aren't set up to do this, you can send a tryjob to the chromium win trybot (which is debug and component) to verify. Please do one or the other before attempting to land.
Hi, Stephen. Yes, I tried our new patch on windows dbg build, and it compiles fine now.
Comment on attachment 176569 [details] Patch Great, thanks. r=me
Comment on attachment 176569 [details] Patch Clearing flags on attachment: 176569 Committed r136891: <http://trac.webkit.org/changeset/136891>
Comment on attachment 176569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176569&action=review > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:97 > + return ImageDecodingStore::instance()->lockCompleteCache(m_frameGenerator.get(), m_scaledSize); I don't think this is entirely correct, this method is used as a hint to see of the cache is there. So the code should be: const ScaledImageFragment* cachedImage = ...lockCompleteCache(); if (!cachedImage) return false; ... unlockCache(cachedImage); return true; Can you make another change to fix this?
(In reply to comment #27) > (From update of attachment 176569 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176569&action=review > > > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:97 > > + return ImageDecodingStore::instance()->lockCompleteCache(m_frameGenerator.get(), m_scaledSize); > > I don't think this is entirely correct, this method is used as a hint to see of the cache is there. So the code should be: > > const ScaledImageFragment* cachedImage = ...lockCompleteCache(); > if (!cachedImage) > return false; > ... unlockCache(cachedImage); > return true; > > Can you make another change to fix this? If you don't do this it wil leak.. :(