Move image decoding WorkQueue to a separate class named ImageFrameWorkQueue #23318

shallawa · 2024-01-26T19:06:33Z

`d20aed3`

Move image decoding WorkQueue to a separate class named ImageFrameWorkQueue
https://bugs.webkit.org/show_bug.cgi?id=268184
rdar://121677190

Reviewed by NOBODY (OOPS!).

ImageSource will ask ImageFrameWorkQueue to decode the image frames. Once the
image frame is decoded, ImageSource will be notifed to cache the decoded frame
and tell the observer to repaint its renderer.

Alos ImageFrameWorkQueue will answer whether a frame is being decoded or not.

* Source/WebCore/Headers.cmake:
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/loader/cache/CachedImage.cpp:
(WebCore::CachedImage::removeAllClientsWaitingForAsyncDecoding):
* Source/WebCore/platform/graphics/BitmapImage.cpp:
(WebCore::BitmapImage::~BitmapImage):
(WebCore::BitmapImage::destroyDecodedData):
(WebCore::BitmapImage::draw):
(WebCore::BitmapImage::canDestroyDecodedData):
(WebCore::BitmapImage::internalStartAnimation):
(WebCore::BitmapImage::advanceAnimation):
(WebCore::BitmapImage::internalAdvanceAnimation):
(WebCore::BitmapImage::stopAnimation):
(WebCore::BitmapImage::decode):
(WebCore::BitmapImage::imageFrameAvailableAtIndex):
* Source/WebCore/platform/graphics/BitmapImage.h:
* Source/WebCore/platform/graphics/ImageFrameWorkQueue.cpp: Added.
(WebCore::ImageFrameWorkQueue::create):
(WebCore::ImageFrameWorkQueue::ImageFrameWorkQueue):
(WebCore::ImageFrameWorkQueue::requestQueue):
(WebCore::ImageFrameWorkQueue::start):
(WebCore::ImageFrameWorkQueue::dispatch):
(WebCore::ImageFrameWorkQueue::stop):
(WebCore::ImageFrameWorkQueue::isPendingDecodingAtIndex const):
(WebCore::ImageFrameWorkQueue::dump const):
* Source/WebCore/platform/graphics/ImageFrameWorkQueue.h: Added.
(WebCore::ImageFrameWorkQueue::isIdle const):
(WebCore::ImageFrameWorkQueue::setMinimumDecodingDurationForTesting):
(WebCore::ImageFrameWorkQueue::decodeQueue):
(WebCore::ImageFrameWorkQueue::minimumDecodingDurationForTesting const):
* Source/WebCore/platform/graphics/ImageSource.cpp:
(WebCore::ImageSource::~ImageSource):
(WebCore::ImageSource::ensureDecoderAvailable):
(WebCore::ImageSource::cacheNativeImageAtIndex):
(WebCore::ImageSource::cacheNativeImageAtIndexAsync):
(WebCore::ImageSource::clearFrameAtIndex):
(WebCore::ImageSource::workQueue):
(WebCore::ImageSource::requestFrameAsyncDecodingAtIndex):
(WebCore::ImageSource::stopWorkQueue):
(WebCore::ImageSource::setMinimumDecodingDurationForTesting):
(WebCore::ImageSource::frameAtIndexCacheIfNeeded):
(WebCore::ImageSource::sourceUTF8 const):
(WebCore::ImageSource::frameIsBeingDecodedAndIsCompatibleWithOptionsAtIndex):
(WebCore::ImageSource::cachePlatformImageAtIndex): Deleted.
(WebCore::ImageSource::cachePlatformImageAtIndexAsync): Deleted.
(WebCore::ImageSource::decodingQueue): Deleted.
(WebCore::ImageSource::frameRequestQueue): Deleted.
(WebCore::ImageSource::startAsyncDecodingQueue): Deleted.
(WebCore::ImageSource::isAsyncDecodingQueueIdle const): Deleted.
(WebCore::ImageSource::stopAsyncDecodingQueue): Deleted.
* Source/WebCore/platform/graphics/ImageSource.h:
* Source/WebCore/testing/Internals.cpp:
(WebCore::Internals::setImageFrameDecodingDuration):

d20aed3

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-01-26T20:15:21Z

EWS run on previous version of this PR (hash 44c7b71)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

…kQueue https://bugs.webkit.org/show_bug.cgi?id=268184 rdar://121677190 Reviewed by NOBODY (OOPS!). ImageSource will ask ImageFrameWorkQueue to decode the image frames. Once the image frame is decoded, ImageSource will be notifed to cache the decoded frame and tell the observer to repaint its renderer. Alos ImageFrameWorkQueue will answer whether a frame is being decoded or not. * Source/WebCore/Headers.cmake: * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/loader/cache/CachedImage.cpp: (WebCore::CachedImage::removeAllClientsWaitingForAsyncDecoding): * Source/WebCore/platform/graphics/BitmapImage.cpp: (WebCore::BitmapImage::~BitmapImage): (WebCore::BitmapImage::destroyDecodedData): (WebCore::BitmapImage::draw): (WebCore::BitmapImage::canDestroyDecodedData): (WebCore::BitmapImage::internalStartAnimation): (WebCore::BitmapImage::advanceAnimation): (WebCore::BitmapImage::internalAdvanceAnimation): (WebCore::BitmapImage::stopAnimation): (WebCore::BitmapImage::decode): (WebCore::BitmapImage::imageFrameAvailableAtIndex): * Source/WebCore/platform/graphics/BitmapImage.h: * Source/WebCore/platform/graphics/ImageFrameWorkQueue.cpp: Added. (WebCore::ImageFrameWorkQueue::create): (WebCore::ImageFrameWorkQueue::ImageFrameWorkQueue): (WebCore::ImageFrameWorkQueue::requestQueue): (WebCore::ImageFrameWorkQueue::start): (WebCore::ImageFrameWorkQueue::dispatch): (WebCore::ImageFrameWorkQueue::stop): (WebCore::ImageFrameWorkQueue::isPendingDecodingAtIndex const): (WebCore::ImageFrameWorkQueue::dump const): * Source/WebCore/platform/graphics/ImageFrameWorkQueue.h: Added. (WebCore::ImageFrameWorkQueue::isIdle const): (WebCore::ImageFrameWorkQueue::setMinimumDecodingDurationForTesting): (WebCore::ImageFrameWorkQueue::decodeQueue): (WebCore::ImageFrameWorkQueue::minimumDecodingDurationForTesting const): * Source/WebCore/platform/graphics/ImageSource.cpp: (WebCore::ImageSource::~ImageSource): (WebCore::ImageSource::ensureDecoderAvailable): (WebCore::ImageSource::cacheNativeImageAtIndex): (WebCore::ImageSource::cacheNativeImageAtIndexAsync): (WebCore::ImageSource::clearFrameAtIndex): (WebCore::ImageSource::workQueue): (WebCore::ImageSource::requestFrameAsyncDecodingAtIndex): (WebCore::ImageSource::stopWorkQueue): (WebCore::ImageSource::setMinimumDecodingDurationForTesting): (WebCore::ImageSource::frameAtIndexCacheIfNeeded): (WebCore::ImageSource::sourceUTF8 const): (WebCore::ImageSource::frameIsBeingDecodedAndIsCompatibleWithOptionsAtIndex): (WebCore::ImageSource::cachePlatformImageAtIndex): Deleted. (WebCore::ImageSource::cachePlatformImageAtIndexAsync): Deleted. (WebCore::ImageSource::decodingQueue): Deleted. (WebCore::ImageSource::frameRequestQueue): Deleted. (WebCore::ImageSource::startAsyncDecodingQueue): Deleted. (WebCore::ImageSource::isAsyncDecodingQueueIdle const): Deleted. (WebCore::ImageSource::stopAsyncDecodingQueue): Deleted. * Source/WebCore/platform/graphics/ImageSource.h: * Source/WebCore/testing/Internals.cpp: (WebCore::Internals::setImageFrameDecodingDuration):

webkit-early-warning-system · 2024-02-06T10:10:28Z

EWS run on current version of this PR (hash d20aed3)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

cdumez · 2024-02-06T18:49:21Z

Source/WebCore/platform/graphics/BitmapImage.cpp

    ASSERT((!index && !m_currentFrame) || !canAnimate());
-    if (m_source->isAsyncDecodingQueueIdle())
-        m_source->stopAsyncDecodingQueue();
+    if (m_source->isWorkQueueIdle())


We should hold a Ref to the source on the stack:

if (Ref source = m_source; source->isWorkQueueIdle()) source->stopWorkQueue();

Will be fixed in #20916 but in ImageFrameProvider::stopDecodingWorkQueue().

cdumez · 2024-02-06T18:50:15Z

Source/WebCore/platform/graphics/BitmapImage.h


-    bool shouldUseAsyncDecodingForTesting() const { return m_source->frameDecodingDurationForTesting() > 0_s; }
-    void setFrameDecodingDurationForTesting(Seconds duration) { m_source->setFrameDecodingDurationForTesting(duration); }
+    void setMinimumDecodingDurationForTesting(Seconds duration) { m_source->setMinimumDecodingDurationForTesting(duration); }


Ref { m_source }->setMinimumDecodingDurationForTesting(duration) per our new smart pointer guidelines.

You can also add a protectedSource() getter which returns a Ref<> if you want to make this look nicer.

Will be fixed in #20916.

cdumez · 2024-02-06T18:50:58Z

Source/WebCore/platform/graphics/BitmapImage.h

    void setAsyncDecodingEnabledForTesting(bool enabled) { m_asyncDecodingEnabledForTesting = enabled; }
    bool isAsyncDecodingEnabledForTesting() const { return m_asyncDecodingEnabledForTesting; }
-    void stopAsyncDecodingQueue() { m_source->stopAsyncDecodingQueue(); }
+    void stopWorkQueue() { m_source->stopWorkQueue(); }


Will be fixed in #20916.

cdumez · 2024-02-06T18:53:31Z

Source/WebCore/platform/graphics/ImageFrameWorkQueue.cpp

+    if (m_workQueue)
+        return;
+
+    ASSERT(isMainThread());


Would be nice to move this above, before the early return.

Will be fixed in #20916.

cdumez · 2024-02-06T18:55:26Z

Source/WebCore/platform/graphics/ImageFrameWorkQueue.cpp

+            if (minimumDecodingDuration > 0_s)
+                startingTime = MonotonicTime::now();
+
+            auto platformImage = decoder->createFrameImageAtIndex(request.index, request.subsamplingLevel, request.options);


Please do not obfuscate smart pointers with auto, use RefPtr

Will be fixed in #20916.

cdumez · 2024-02-06T18:59:44Z

Source/WebCore/platform/graphics/ImageSource.cpp

 {
-    if (hasAsyncDecodingQueue() || !isDecoderAvailable())
-        return;
+    if (!m_workQueue)


Is this always called on the same thread? If not, we have a thread-safety issue here and would need a Lock.

Yes this is called only on the main thread. In #20916. ImageFrameProvider will owns the ImageFrameWorkQueue.

cdumez · 2024-02-06T19:00:44Z

Source/WebCore/platform/graphics/ImageSource.cpp

-    });
+    LOG(Images, "ImageSource::%s - %p - url: %s. Decoding work queue will be stopped.", __FUNCTION__, this, sourceUTF8());
+
+    m_workQueue->stop();


You need a Ref/RefPtr on the stack before you call stop(), e.g. RefPtr { m_workQueue }->stop() or protectedWorkQueue()->stop()

This will be fixed in #20916 but in ImageFrameProvider since it will own the ImageFrameWorkQueue.

cdumez · 2024-02-06T19:01:00Z

Source/WebCore/platform/graphics/ImageSource.cpp


        // We have to perform synchronous image decoding in this code.
-        auto platformImage = m_decoder->createFrameImageAtIndex(index, subsamplingLevelValue, decodingOptions);
+        auto platformImage = m_decoder->createFrameImageAtIndex(index, currentSubsamplingLevel, decodingOptions);


Please don't hide smart pointers with auto.

Will be fixed in #20916. All auto platformImage = ... are replaced by `PlatformImagePtr platformImage = ...

cdumez · 2024-02-06T19:01:05Z

Source/WebCore/platform/graphics/ImageSource.cpp

        // We have to perform synchronous image decoding in this code.
-        auto platformImage = m_decoder->createFrameImageAtIndex(index, subsamplingLevelValue, decodingOptions);
+        auto platformImage = m_decoder->createFrameImageAtIndex(index, currentSubsamplingLevel, decodingOptions);
+        auto nativeImage = NativeImage::create(WTFMove(platformImage));


Will be fixed in #20916. All auto nativeImage = ... are replaced by RefPtr nativeImage = ...

cdumez · 2024-02-06T19:01:54Z

Source/WebCore/platform/graphics/ImageFrameWorkQueue.cpp

+
+    m_workQueue->dispatch([protectedThis = Ref { *this }, protectedWorkQueue = Ref { *m_workQueue }, protectedSource = Ref { source() }] () mutable {
+        Request request;
+        while (protectedThis->requestQueue().dequeue(request)) {


I need to look into the implementation of this requestQueue(). I assume it does locking?

I see, it's a SynchronizedFixedQueue which does indeed Lock internally. All good.

shallawa · 2024-02-07T22:50:19Z

The plan is to submit the whole BitmapImage refactor PR #20916 as is without split. All the comments here will be addressed #20916.

shallawa requested a review from cdumez as a code owner January 26, 2024 19:06

shallawa self-assigned this Jan 26, 2024

shallawa added the Images For bugs in image handling. label Jan 26, 2024

shallawa requested review from heycam and smfr January 27, 2024 01:03

shallawa force-pushed the eng/Move-image-decoding-WorkQueue-to-a-separate-class-named-ImageFrameWorkQueue branch from 44c7b71 to d20aed3 Compare February 6, 2024 10:03

shallawa mentioned this pull request Feb 6, 2024

Decouple BitmapImage sources and main functions into separate classes #20916

Merged

cdumez reviewed Feb 6, 2024

View reviewed changes

shallawa closed this Feb 7, 2024

shallawa deleted the eng/Move-image-decoding-WorkQueue-to-a-separate-class-named-ImageFrameWorkQueue branch March 21, 2024 17:13

Move image decoding WorkQueue to a separate class named ImageFrameWorkQueue #23318

Move image decoding WorkQueue to a separate class named ImageFrameWorkQueue #23318

Uh oh!

Conversation

shallawa commented Jan 26, 2024 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shallawa commented Feb 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shallawa commented Jan 26, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Jan 26, 2024 •

edited

Loading

webkit-early-warning-system commented Feb 6, 2024 •

edited

Loading