-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[WPE][WebDriver] Support taking the screenshots from the UIProcess #51535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WPE][WebDriver] Support taking the screenshots from the UIProcess #51535
Conversation
|
EWS run on previous version of this PR (hash 92d0a63) |
92d0a63 to
69dceed
Compare
|
EWS run on previous version of this PR (hash 69dceed) |
69dceed to
7d9bd90
Compare
|
EWS run on previous version of this PR (hash 7d9bd90) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be endif, we want to fail the same way when building with the new api but the old one is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use SkData::bytesSpan()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I'll also refactor this funtion to use the base64EncodedPNGData(SkImage&) defined above and use byteSpan there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this async version, the previous call to callAfterNextPresentationUpdate should ensure we have a pending buffer ready, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, using m_commitedBuffer synchronously seems to be working fine. Thanks for the suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have to call ViewSnapshotStore::singleton().didAddImageToSnapshot(*this) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And ViewSnapshotStore::singleton().willRemoveImageFromSnapshot(*this) here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably use m_image->imageInfo().computeMinByteSize()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try to not use this async approach and get the snapshot after the call after next presenation update using the pending buffer if there's any or committed buffer otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it because it relied on taking the screenshots synchronously. I'll test this approach, reading the last m_commitedBuffer, so we can avoid this guard.
7d9bd90 to
6b86043
Compare
|
EWS run on previous version of this PR (hash 6b86043) |
6b86043 to
284ef69
Compare
|
EWS run on previous version of this PR (hash 284ef69) |
284ef69 to
4f47248
Compare
|
EWS run on previous version of this PR (hash 4f47248) |
4f47248 to
b2563f6
Compare
|
EWS run on previous version of this PR (hash b2563f6) |
|
@carlosgcampos I'm afraid the Maybe |
| #elif PLATFORM(WPE) && USE(SKIA) | ||
| Function<void(WebPageProxy&, std::optional<WebCore::IntRect>&&, CommandCallback<String>&&)> takeViewSnapshot = [](WebPageProxy& page, std::optional<WebCore::IntRect>&& rect, CommandCallback<String>&& callback) { | ||
| // FIXME Now that we're adding a takeViewSnapshotAsync, should we still wait for the next presentation update? | ||
| page.callAfterNextPresentationUpdate([page = Ref { page }, rect = WTFMove(rect), callback = WTFMove(callback)] () mutable { | ||
| auto snapshot = page->takeViewSnapshotWithErrorDetail(WTFMove(rect)); | ||
| ASYNC_FAIL_WITH_PREDEFINED_ERROR_AND_DETAILS_IF(!snapshot, InternalError, snapshot.error()); | ||
|
|
||
| std::optional<String> base64EncodedData = platformGetBase64EncodedPNGData(snapshot.value().get()); | ||
| ASYNC_FAIL_WITH_PREDEFINED_ERROR_AND_DETAILS_IF(!base64EncodedData, InternalError, "Failed to extract base64 data from screenshot."_s); | ||
|
|
||
| callback(base64EncodedData.value()); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a leftover from previous patch? or is there any difference with the cocoa/gtk implementation? Can't use use the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was related to the takeViewSnapshotWithErrorDetail approach. I've updated the patch to make WPE use the same RefPtr in its PageClient instead of Expected for now, and reuse this same callback.
In a follow-up commit for https://webkit.org/b/300271, I'll switch the return type in the PageClient/WebPageProxy for all ports to forward the error.
| if (!m_committedBuffer) [[unlikely]] | ||
| return makeUnexpected("No commited buffer to create snapshot from"_s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have a committed buffer, but there's a pending one, we should use the pending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. But should we do any kind of check regarding the fences to read from the pending buffer?
Source/WebKit/UIProcess/PageClient.h
Outdated
| #if PLATFORM(WPE) && USE(SKIA) | ||
| virtual Expected<Ref<ViewSnapshot>, String> takeViewSnapshotWithErrorDetail(std::optional<WebCore::IntRect>&&) = 0; | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does wpe need the error details and other ports don't?
| // FIXME log error | ||
| return nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we add a specific method for WPE to get the error details and then we don't use them? I think it's better to use the existing method and in a follow up change it to return the error message but for all the ports (and actually use it)
| if (!pageClient) | ||
| return makeUnexpected("No PageClient available to handle snapshot request"_s); | ||
|
|
||
| return pageClient->takeViewSnapshotWithErrorDetail(WTFMove(clipRect)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I see it's used in this case. I still don't see why this is wpe specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the previous reply, this was postponed to a follow-up commit, in https//webkit.org/b/300271
b2563f6 to
965e9ff
Compare
|
EWS run on current version of this PR (hash 965e9ff) |
https://bugs.webkit.org/show_bug.cgi?id=299732 Reviewed by Carlos Garcia Campos. Currently, WPE's implentation of taking screenshots rely on asking the root `RenderLayer` on the WebProcess to paint its content. While it worked fine for most elements, this approach could lead to differences from the final composited frame that we send back to the UIProcess, especially for some kinds of accelerated content, like animated 3D CSS filters. On top of that, the recent ANGLE bump in 295416@main included a stricter check on ReadPixels that affected taking screenshots of WebGL scenes on WebGL 1.0 environments, as the GraphicsContextGLAngle used it to read the WebGL scenes contents for this code path. To address these issues, this commits adds support for taking screenshots on the UIProcess, by reading the last buffer committed to the system. * Source/WebKit/Shared/WebBackForwardListItem.h: * Source/WebKit/SourcesWPE.txt: * Source/WebKit/UIProcess/API/wpe/PageClientImpl.cpp: (WebKit::PageClientImpl::takeViewSnapshot): * Source/WebKit/UIProcess/API/wpe/PageClientImpl.h: * Source/WebKit/UIProcess/API/wpe/WPEWebViewPlatform.cpp: (WKWPE::ViewPlatform::takeViewSnapshot): * Source/WebKit/UIProcess/API/wpe/WPEWebViewPlatform.h: * Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp: (WebKit::WebAutomationSession::takeScreenshot): * Source/WebKit/UIProcess/Automation/skia/WebAutomationSessionSkia.cpp: (WebKit::base64EncodedPNGData): (WebKit::WebAutomationSession::platformGetBase64EncodedPNGData): * Source/WebKit/UIProcess/PageClient.h: * Source/WebKit/UIProcess/ViewSnapshotStore.cpp: * Source/WebKit/UIProcess/ViewSnapshotStore.h: (WebKit::ViewSnapshot::image const): * Source/WebKit/UIProcess/WebPageProxy.cpp: * Source/WebKit/UIProcess/WebPageProxy.h: * Source/WebKit/UIProcess/skia/ViewSnapshotSkia.cpp: Copied from Source/WebKit/UIProcess/Automation/skia/WebAutomationSessionSkia.cpp. (WebKit::ViewSnapshot::create): (WebKit::ViewSnapshot::ViewSnapshot): (WebKit::ViewSnapshot::hasImage const): (WebKit::ViewSnapshot::clearImage): (WebKit::ViewSnapshot::estimatedImageSizeInBytes const): (WebKit::ViewSnapshot::size const): * Source/WebKit/UIProcess/wpe/AcceleratedBackingStore.cpp: (WebKit::getImageInfoFromBuffer): (WebKit::saveBufferSnapshot): (WebKit::AcceleratedBackingStore::takeSnapshot): * Source/WebKit/UIProcess/wpe/AcceleratedBackingStore.h: * WebDriverTests/TestExpectations.json: Canonical link: https://commits.webkit.org/301948@main
965e9ff to
50c1d97
Compare
|
Committed 301948@main (50c1d97): https://commits.webkit.org/301948@main Reviewed commits have been landed. Closing PR #51535 and removing active labels. |
50c1d97
965e9ff