Unlock partially decoded images after passing them to the ImageDecodingStore
Created attachment 190101 [details] Patch
Comment on attachment 190101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190101&action=review Please move locking logic of ImageDecoder into ImageDecodingStore. ImageFrameGenerator shouldn't know about the underlying, it should only know that when an entry is locked, all locked resources are ready to be used. > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:145 > + cachedDecoder->lockFrameBuffers(); You want to call this in ImageDecodingStore::lockCache() such that if lockFrameBuffers fails then a new decoder is created. > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:150 > + cachedDecoder->unlockFrameBuffers(); Move this to ImageDecodingStore as well. > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:178 > + decoder->unlockFrameBuffers(); Move this to ImageDecodingStore.
Comment on attachment 190101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190101&action=review >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:145 >> + cachedDecoder->lockFrameBuffers(); > > You want to call this in ImageDecodingStore::lockCache() such that if lockFrameBuffers fails then a new decoder is created. The lockFrameBuffers() will not fail here. We reach here only if ImageDecodingStore::lockCache() succeeds. Since that call will lock the DiscardablePixelRef, so lockFrameBuffers() will not fail. It only increments the pixelref lock count by 1. Maybe I should add a comment to mention that the FrameBuffer is already locked when this gets called, so we are safe to use the same decoder. >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:150 >> + cachedDecoder->unlockFrameBuffers(); > > Move this to ImageDecodingStore as well. same reason >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:178 >> + decoder->unlockFrameBuffers(); > > Move this to ImageDecodingStore. same reason, we are safe to call this here.
Comment on attachment 190101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190101&action=review >>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:145 >>> + cachedDecoder->lockFrameBuffers(); >> >> You want to call this in ImageDecodingStore::lockCache() such that if lockFrameBuffers fails then a new decoder is created. > > The lockFrameBuffers() will not fail here. We reach here only if ImageDecodingStore::lockCache() succeeds. Since that call will lock the DiscardablePixelRef, so lockFrameBuffers() will not fail. It only increments the pixelref lock count by 1. > Maybe I should add a comment to mention that the FrameBuffer is already locked when this gets called, so we are safe to use the same decoder. One more thing: ImageDecodingStore doesn't know the status of the decoding, while ImageFrameGenerator keeps track of the decoding status. If we lock the frameBuffer inside lockCache(), imageDecodingStore will have a problem to determine when to unlock the framebuffer. The next call can be either unlockCache, insertAndLockCache, or overwriteAndLockCache. Since this lock is associated with decoder, i think it should belong to ImageFrameGenerator, not the ImageDecodingStore. ideas?
Comment on attachment 190101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190101&action=review >>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:145 >>>> + cachedDecoder->lockFrameBuffers(); >>> >>> You want to call this in ImageDecodingStore::lockCache() such that if lockFrameBuffers fails then a new decoder is created. >> >> The lockFrameBuffers() will not fail here. We reach here only if ImageDecodingStore::lockCache() succeeds. Since that call will lock the DiscardablePixelRef, so lockFrameBuffers() will not fail. It only increments the pixelref lock count by 1. >> Maybe I should add a comment to mention that the FrameBuffer is already locked when this gets called, so we are safe to use the same decoder. > > One more thing: ImageDecodingStore doesn't know the status of the decoding, while ImageFrameGenerator keeps track of the decoding status. > If we lock the frameBuffer inside lockCache(), imageDecodingStore will have a problem to determine when to unlock the framebuffer. The next call can be either unlockCache, insertAndLockCache, or overwriteAndLockCache. > Since this lock is associated with decoder, i think it should belong to ImageFrameGenerator, not the ImageDecodingStore. ideas? This is probably okay for now for single frame images but won't work well for multiframe images. Please add a comment here to explain why this call is safe and that it only works for single frame images. I would even do this: bool frameBuffersLocked = cacheDecoder->lockFrameBuffers(); ASSERT_UNUSED(frameBuffersLocked, frameBuffersLocked); >>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:150 >>> + cachedDecoder->unlockFrameBuffers(); >> >> Move this to ImageDecodingStore as well. > > same reason Also add an explanation here why unlock is needed. The reason here is because ImageDecodingStore deletes the ImageDecoder if image is complete. >>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:178 >>> + decoder->unlockFrameBuffers(); >> >> Move this to ImageDecodingStore. > > same reason, we are safe to call this here. Same here, add explanation please. > Source/WebCore/platform/image-decoders/ImageDecoder.h:426 > + virtual void lockFrameBuffers() Can you change this to bool? It returns true if all frames are locked, false otherwise.
I'm okay to keep this change simple and leave the problems for multiframe images later. A few more changes and this is good to go! :)
Created attachment 190576 [details] Patch
Comment on attachment 190101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190101&action=review >>>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:145 >>>>> + cachedDecoder->lockFrameBuffers(); >>>> >>>> You want to call this in ImageDecodingStore::lockCache() such that if lockFrameBuffers fails then a new decoder is created. >>> >>> The lockFrameBuffers() will not fail here. We reach here only if ImageDecodingStore::lockCache() succeeds. Since that call will lock the DiscardablePixelRef, so lockFrameBuffers() will not fail. It only increments the pixelref lock count by 1. >>> Maybe I should add a comment to mention that the FrameBuffer is already locked when this gets called, so we are safe to use the same decoder. >> >> One more thing: ImageDecodingStore doesn't know the status of the decoding, while ImageFrameGenerator keeps track of the decoding status. >> If we lock the frameBuffer inside lockCache(), imageDecodingStore will have a problem to determine when to unlock the framebuffer. The next call can be either unlockCache, insertAndLockCache, or overwriteAndLockCache. >> Since this lock is associated with decoder, i think it should belong to ImageFrameGenerator, not the ImageDecodingStore. ideas? > > This is probably okay for now for single frame images but won't work well for multiframe images. > > Please add a comment here to explain why this call is safe and that it only works for single frame images. I would even do this: > > bool frameBuffersLocked = cacheDecoder->lockFrameBuffers(); > ASSERT_UNUSED(frameBuffersLocked, frameBuffersLocked); Done. >>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:150 >>>> + cachedDecoder->unlockFrameBuffers(); >>> >>> Move this to ImageDecodingStore as well. >> >> same reason > > Also add an explanation here why unlock is needed. The reason here is because ImageDecodingStore deletes the ImageDecoder if image is complete. Done. >>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:178 >>>> + decoder->unlockFrameBuffers(); >>> >>> Move this to ImageDecodingStore. >> >> same reason, we are safe to call this here. > > Same here, add explanation please. Done. >> Source/WebCore/platform/image-decoders/ImageDecoder.h:426 >> + virtual void lockFrameBuffers() > > Can you change this to bool? It returns true if all frames are locked, false otherwise. Done.
Comment on attachment 190576 [details] Patch LGTM.
Stephen, would you please take a look? Thanks
Comment on attachment 190576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190576&action=review Pretty much a rubber-stamp. r=me > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:157 > + // ImageDecodingStore should have deleted the decoder here. Nit: Is this something worth asserting in code of instead of in a comment, if possible?
Comment on attachment 190576 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190576&action=review >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:157 >> + // ImageDecodingStore should have deleted the decoder here. > > Nit: Is this something worth asserting in code of instead of in a comment, if possible? hmm... probably this is not a good place to put the assertion. The cachedDecoder here is a dumb pointer. In order to check whether the decoder is deleted, we have to call lockCache() again. But that would break the lock/unlock situation. An alternative is to put the assertion check into overwriteAndLockCache(). But the logic is already pretty straightforward inside that function.
Comment on attachment 190576 [details] Patch Clearing flags on attachment: 190576 Committed r144242: <http://trac.webkit.org/changeset/144242>
All reviewed patches have been landed. Closing bug.