ImageAnalysisQueue should reanalyze image elements whose image source…

…s have changed

https://bugs.webkit.org/show_bug.cgi?id=240371
rdar://93175651

Reviewed by Tim Horton.

Currently, ImageAnalysisQueue maintains the set of all image elements that have been queued for analysis, and
avoids re-queueing such image elements. However, on some websites, this leads to stale analysis results being
shown in certain image elements that have changed image sources (and subsequently finished loaded the new image).
To address this, we introduce a mechanism to remember the latest image URL for image elements that have been
analyzed using the analysis queue; we only avoid reanalyzing these same image elements if the source URL is the
same as the source URL when we last analyzed it.

This allows us (for instance) to handle the scenario where a single image element periodically cycles between
different `src` attribute values.

Test: ImageAnalysisTests.AnalyzeImageAfterChangingSource

* page/ImageAnalysisQueue.cpp:
(WebCore::ImageAnalysisQueue::enqueueIfNeeded):

Also avoid trying to prematurely analyze images whose cached images only contain the null image. This caused
empty results to sometimes be incorrectly cached for some image elements, if analysis is triggered too early.

(WebCore::ImageAnalysisQueue::resumeProcessing):
* page/ImageAnalysisQueue.h:

To aid with debugging similar issues in the future, plumb the image URL through to
`requestImageAnalysisWithIdentifier`, which (if an engineering default is specified) will additionally reveal
the URL in system logs.

* Platform/cocoa/ImageAnalysisUtilities.h:
* UIProcess/Cocoa/WebViewImpl.mm:
(WebKit::WebViewImpl::requestTextRecognition):
* UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView requestTextRecognition:imageData:identifier:completionHandler:]):

Add an API test to exercise the scenario by verifying that the same image element is reanalyzed after setting
the `src` to a different image URL.

* TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:
(TestWebKitAPI::TEST):

Canonical link: https://commits.webkit.org/250546@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294179 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,32 @@
+2022-05-13  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        ImageAnalysisQueue should reanalyze image elements whose image sources have changed
+        https://bugs.webkit.org/show_bug.cgi?id=240371
+        rdar://93175651
+
+        Reviewed by Tim Horton.
+
+        Currently, ImageAnalysisQueue maintains the set of all image elements that have been queued for analysis, and
+        avoids re-queueing such image elements. However, on some websites, this leads to stale analysis results being
+        shown in certain image elements that have changed image sources (and subsequently finished loaded the new image).
+        To address this, we introduce a mechanism to remember the latest image URL for image elements that have been
+        analyzed using the analysis queue; we only avoid reanalyzing these same image elements if the source URL is the
+        same as the source URL when we last analyzed it.
+
+        This allows us (for instance) to handle the scenario where a single image element periodically cycles between
+        different `src` attribute values.
+
+        Test: ImageAnalysisTests.AnalyzeImageAfterChangingSource
+
+        * page/ImageAnalysisQueue.cpp:
+        (WebCore::ImageAnalysisQueue::enqueueIfNeeded):
+
+        Also avoid trying to prematurely analyze images whose cached images only contain the null image. This caused
+        empty results to sometimes be incorrectly cached for some image elements, if analysis is triggered too early.
+
+        (WebCore::ImageAnalysisQueue::resumeProcessing):
+        * page/ImageAnalysisQueue.h:
+
 2022-05-13  Adrian Perez de Castro  <aperez@igalia.com>
 
         Non-unified build broken in debug mode

Source/WebCore/page/ImageAnalysisQueue.cpp

@@ -64,10 +64,35 @@ void ImageAnalysisQueue::enqueueIfNeeded(HTMLImageElement& element)
     if (!cachedImage || cachedImage->errorOccurred())
         return;
 
+    auto* image = cachedImage->image();
+    if (!image || image->isNull())
+        return;
+
     if (renderer.size().width() < minimumWidthForAnalysis || renderer.size().height() < minimumHeightForAnalysis)
         return;
 
-    if (!m_queuedElements.add(element).isNewEntry)
+    bool shouldAddToQueue = [&] {
+        auto url = cachedImage->url();
+        auto iterator = m_queuedElements.find(element);
+        if (iterator == m_queuedElements.end()) {
+            m_queuedElements.add(element, url);
+            return true;
+        }
+
+        if (iterator->value == url)
+            return false;
+
+        iterator->value = url;
+
+        for (auto& entry : m_queue) {
+            if (entry.element == &element)
+                return false;
+        }
+
+        return true;
+    }();
+
+    if (!shouldAddToQueue)
         return;
 
     Ref view = renderer.view().frameView();
@@ -123,6 +148,10 @@ void ImageAnalysisQueue::resumeProcessing()
 
         m_pendingRequestCount++;
         m_page->resetTextRecognitionResult(*element);
+
+        if (auto* image = element->cachedImage(); image && !image->errorOccurred())
+            m_queuedElements.set(*element, image->url());
+
         auto allowSnapshots = m_identifier.isEmpty() ? TextRecognitionOptions::AllowSnapshots::Yes : TextRecognitionOptions::AllowSnapshots::No;
         m_page->chrome().client().requestTextRecognition(*element, { m_identifier, allowSnapshots }, [this, page = m_page] (auto&&) {
             if (!page || page->imageAnalysisQueueIfExists() != this)

Source/WebCore/page/ImageAnalysisQueue.h

@@ -29,7 +29,8 @@
 
 #include <wtf/FastMalloc.h>
 #include <wtf/PriorityQueue.h>
-#include <wtf/WeakHashSet.h>
+#include <wtf/URL.h>
+#include <wtf/WeakHashMap.h>
 #include <wtf/WeakPtr.h>
 
 namespace WebCore {
@@ -69,7 +70,7 @@ class ImageAnalysisQueue {
     String m_identifier;
     WeakPtr<Page> m_page;
     Timer m_resumeProcessingTimer;
-    WeakHashSet<HTMLImageElement> m_queuedElements;
+    WeakHashMap<HTMLImageElement, URL> m_queuedElements;
     PriorityQueue<Task, firstIsHigherPriority> m_queue;
     unsigned m_pendingRequestCount { 0 };
     unsigned m_currentTaskNumber { 0 };

Source/WebKit/ChangeLog

@@ -1,3 +1,21 @@
+2022-05-13  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        ImageAnalysisQueue should reanalyze image elements whose image sources have changed
+        https://bugs.webkit.org/show_bug.cgi?id=240371
+        rdar://93175651
+
+        Reviewed by Tim Horton.
+
+        To aid with debugging similar issues in the future, plumb the image URL through to
+        `requestImageAnalysisWithIdentifier`, which (if an engineering default is specified) will additionally reveal
+        the URL in system logs.
+
+        * Platform/cocoa/ImageAnalysisUtilities.h:
+        * UIProcess/Cocoa/WebViewImpl.mm:
+        (WebKit::WebViewImpl::requestTextRecognition):
+        * UIProcess/ios/WKContentViewInteraction.mm:
+        (-[WKContentView requestTextRecognition:imageData:identifier:completionHandler:]):
+
 2022-05-13  Aditya Keerthi  <akeerthi@apple.com>
 
         [iOS] Multiple visible find highlights when searching for text after beginning a "find from selection"

Source/WebKit/Platform/cocoa/ImageAnalysisUtilities.h

@@ -60,7 +60,7 @@ RetainPtr<CocoaImageAnalyzer> createImageAnalyzer();
 RetainPtr<CocoaImageAnalyzerRequest> createImageAnalyzerRequest(CGImageRef, VKAnalysisTypes);
 
 #if ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS)
-void requestImageAnalysisWithIdentifier(CocoaImageAnalyzer *, const String& identifier, CGImageRef, CompletionHandler<void(WebCore::TextRecognitionResult&&)>&&);
+void requestImageAnalysisWithIdentifier(CocoaImageAnalyzer *, NSURL *, const String& identifier, CGImageRef, CompletionHandler<void(WebCore::TextRecognitionResult&&)>&&);
 void requestImageAnalysisMarkup(CGImageRef, CompletionHandler<void(CGImageRef, CGRect)>&&);
 
 std::pair<RetainPtr<NSData>, RetainPtr<CFStringRef>> imageDataForCroppedImageResult(CGImageRef, const String& sourceMIMEType);

Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm

@@ -266,7 +266,7 @@ - (void)mouseExited:(NSEvent *)event
 
 #if ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS)
     if (!identifier.isEmpty())
-        return requestImageAnalysisWithIdentifier(ensureImageAnalyzer(), identifier, cgImage.get(), WTFMove(completion));
+        return requestImageAnalysisWithIdentifier(ensureImageAnalyzer(), imageURL, identifier, cgImage.get(), WTFMove(completion));
 #else
     UNUSED_PARAM(identifier);
 #endif

Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm

@@ -10825,7 +10825,7 @@ - (void)requestTextRecognition:(NSURL *)imageURL imageData:(const WebKit::Sharea
 
 #if ENABLE(IMAGE_ANALYSIS_ENHANCEMENTS)
     if (identifier.length)
-        return WebKit::requestImageAnalysisWithIdentifier(self.imageAnalyzer, identifier, cgImage.get(), WTFMove(completion));
+        return WebKit::requestImageAnalysisWithIdentifier(self.imageAnalyzer, imageURL, identifier, cgImage.get(), WTFMove(completion));
 #else
     UNUSED_PARAM(identifier);
 #endif

Tools/ChangeLog

@@ -1,3 +1,17 @@
+2022-05-13  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        ImageAnalysisQueue should reanalyze image elements whose image sources have changed
+        https://bugs.webkit.org/show_bug.cgi?id=240371
+        rdar://93175651
+
+        Reviewed by Tim Horton.
+
+        Add an API test to exercise the scenario by verifying that the same image element is reanalyzed after setting
+        the `src` to a different image URL.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:
+        (TestWebKitAPI::TEST):
+
 2022-05-13  Alex Christensen  <achristensen@webkit.org>
 
         Disable MediaLoading.CaptivePortalHLS API test

Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm

@@ -272,6 +272,19 @@ static void processRequestWithResults(id, SEL, VKImageAnalyzerRequest *request,
     [webView waitForImageAnalysisRequests:10];
 }
 
+TEST(ImageAnalysisTests, AnalyzeImageAfterChangingSource)
+{
+    auto requestSwizzler = makeImageAnalysisRequestSwizzler(processRequestWithResults);
+
+    auto webView = createWebViewWithTextRecognitionEnhancements();
+    [webView synchronouslyLoadTestPageNamed:@"image"];
+    [webView _startImageAnalysis:nil];
+    [webView waitForImageAnalysisRequests:1];
+
+    [webView objectByEvaluatingJavaScript:@"document.querySelector('img').src = 'icon.png'"];
+    [webView waitForImageAnalysisRequests:2];
+}
+
 TEST(ImageAnalysisTests, AnalyzeDynamicallyLoadedImages)
 {
     auto requestSwizzler = makeImageAnalysisRequestSwizzler(processRequestWithResults);