- Revision
- 294179
- Author
- wenson_hs...@apple.com
- Date
- 2022-05-13 15:20:39 -0700 (Fri, 13 May 2022)
Log Message
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:
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
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (294178 => 294179)
--- trunk/Source/WebCore/ChangeLog 2022-05-13 22:11:26 UTC (rev 294178)
+++ trunk/Source/WebCore/ChangeLog 2022-05-13 22:20:39 UTC (rev 294179)
@@ -1,3 +1,32 @@
+2022-05-13 Wenson Hsieh <wenson_hs...@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 <ape...@igalia.com>
Non-unified build broken in debug mode
Modified: trunk/Source/WebCore/page/ImageAnalysisQueue.cpp (294178 => 294179)
--- trunk/Source/WebCore/page/ImageAnalysisQueue.cpp 2022-05-13 22:11:26 UTC (rev 294178)
+++ trunk/Source/WebCore/page/ImageAnalysisQueue.cpp 2022-05-13 22:20:39 UTC (rev 294179)
@@ -64,10 +64,35 @@
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 = ""
+ 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 @@
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)
Modified: trunk/Source/WebCore/page/ImageAnalysisQueue.h (294178 => 294179)
--- trunk/Source/WebCore/page/ImageAnalysisQueue.h 2022-05-13 22:11:26 UTC (rev 294178)
+++ trunk/Source/WebCore/page/ImageAnalysisQueue.h 2022-05-13 22:20:39 UTC (rev 294179)
@@ -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 @@
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 };
Modified: trunk/Source/WebKit/ChangeLog (294178 => 294179)
--- trunk/Source/WebKit/ChangeLog 2022-05-13 22:11:26 UTC (rev 294178)
+++ trunk/Source/WebKit/ChangeLog 2022-05-13 22:20:39 UTC (rev 294179)
@@ -1,3 +1,21 @@
+2022-05-13 Wenson Hsieh <wenson_hs...@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 <akeer...@apple.com>
[iOS] Multiple visible find highlights when searching for text after beginning a "find from selection"
Modified: trunk/Source/WebKit/Platform/cocoa/ImageAnalysisUtilities.h (294178 => 294179)
--- trunk/Source/WebKit/Platform/cocoa/ImageAnalysisUtilities.h 2022-05-13 22:11:26 UTC (rev 294178)
+++ trunk/Source/WebKit/Platform/cocoa/ImageAnalysisUtilities.h 2022-05-13 22:20:39 UTC (rev 294179)
@@ -60,7 +60,7 @@
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);
Modified: trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm (294178 => 294179)
--- trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm 2022-05-13 22:11:26 UTC (rev 294178)
+++ trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm 2022-05-13 22:20:39 UTC (rev 294179)
@@ -266,7 +266,7 @@
#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
Modified: trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm (294178 => 294179)
--- trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2022-05-13 22:11:26 UTC (rev 294178)
+++ trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm 2022-05-13 22:20:39 UTC (rev 294179)
@@ -10825,7 +10825,7 @@
#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
Modified: trunk/Tools/ChangeLog (294178 => 294179)
--- trunk/Tools/ChangeLog 2022-05-13 22:11:26 UTC (rev 294178)
+++ trunk/Tools/ChangeLog 2022-05-13 22:20:39 UTC (rev 294179)
@@ -1,3 +1,17 @@
+2022-05-13 Wenson Hsieh <wenson_hs...@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 <achristen...@webkit.org>
Disable MediaLoading.CaptivePortalHLS API test
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm (294178 => 294179)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm 2022-05-13 22:11:26 UTC (rev 294178)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm 2022-05-13 22:20:39 UTC (rev 294179)
@@ -272,6 +272,19 @@
[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 = ''"];
+ [webView waitForImageAnalysisRequests:2];
+}
+
TEST(ImageAnalysisTests, AnalyzeDynamicallyLoadedImages)
{
auto requestSwizzler = makeImageAnalysisRequestSwizzler(processRequestWithResults);
