Summary: | [EFL][WK2] 3D pixel tests are failing | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||||||||||||||||||||
Component: | Tools / Tests | Assignee: | Yael <yael> | ||||||||||||||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, cgarcia, d-r, gustavo, gyuyoung.kim, jussi.kukkonen, kenneth, laszlo.gombos, mcatanzaro, mrobinson, noam, ostap73, philn, rakuco, webkit.review.bot, xan.lopez, zeno | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Yael
2012-11-20 13:48:03 PST
Created attachment 175510 [details]
Patch
Generate a snapshot of the view in the UI process instead of the web process.
We have to use Texture Mapper in order to correctly paint 3D transforms etc.
Since I could not find an efl based API that will give me a snapshot, and I cannot use both evas and openGl API in the same file, I created a new file, in which I use openGL to get a snapshot.
Comment on attachment 175510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175510&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:45 > + return viewImpl->getSnapshot(); takeShapshot? > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:978 > + } Is this right with those ifdefs? (In reply to comment #2) > (From update of attachment 175510 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175510&action=review > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:45 > > + return viewImpl->getSnapshot(); > > takeShapshot? > ok > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:978 > > + } > > Is this right with those ifdefs? yes. both '{' and '}' are inside #if USE(ACCELERATED_COMPOSITING) Comment on attachment 175510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175510&action=review > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:24 > +#include <GL/gl.h> Unfortunately this will not work for opengl ES. For GL ES2 it will be GLES2/gl2.h, for GL ES1 it will be GLES/gl.h I think there should be single place for this, may be OpenGLShims.h ? (In reply to comment #4) > (From update of attachment 175510 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175510&action=review > > > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:24 > > +#include <GL/gl.h> > > Unfortunately this will not work for opengl ES. > For GL ES2 it will be GLES2/gl2.h, for GL ES1 it will be GLES/gl.h > > I think there should be single place for this, may be OpenGLShims.h ? I got a tip from Carsten Haitzler on the enlightement mailing list about some ecore_x API that I can use. I will give that a try first. Created attachment 175907 [details] Patch Address comment #2 and comment #4. XShmGetImage is failing to grab the image, so I resorted to glReadPixels, which is what the Qt port does. Attachment 175907 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:64: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 175907 [details] Patch Attachment 175907 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14981481 Created attachment 175910 [details]
Patch
Forgot to remove one of the X11 headers. sorry :(
Attachment 175910 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:122: One space before end of line comments [whitespace/comments] [5]
Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:122: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 2 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 175911 [details]
Patch
Comment on attachment 175911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175911&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1034 > + delete buffer; delete[] buffer ? > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:34 > + unsigned char* buffer = new unsigned char[width * height * 4]; allocated with operator new[]. > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:39 > + unsigned* buf = (unsigned*)buffer; Please void C style cast > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:41 > + for (int i = 0; i < height / 2; i++) { we usually prefer preincrement when possible > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:42 > + for (int j = 0; j < width; j++) { ditto > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:138 > + RefPtr<cairo_surface_t> surface = WKImageCreateCairoSurface(WKViewGetSnapshot(toAPI(m_view))); You need to adopt. > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:147 > + return adoptWK(WKImageCreateFromCairoSurface(surface.get(), options)); I did not understand why we converted to a cairo surface and then back to a WKImageRef. Why not return WKViewGetSnapshot(toAPI(m_view)) directly? > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:85 > + cairo_t* context = cairo_create(surface); Smart pointer? > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:121 > + cairo_surface_t* surface = 0; Smart pointer? (In reply to comment #12) Thank you for your comments :) > (From update of attachment 175911 [details]) > we usually prefer preincrement when possible > Can you please tell me which coding guideline says this? > I did not understand why we converted to a cairo surface and then back to a WKImageRef. Why not return WKViewGetSnapshot(toAPI(m_view)) directly? I didn't know if it is ok to return a cairo type in our API. If that is ok, then there is no need for the multiple conversions. Comment on attachment 175911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175911&action=review >> Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:41 >> + for (int i = 0; i < height / 2; i++) { > > we usually prefer preincrement when possible It is not documented in the WebKit style guidelines document but Benjamin Poulain commented about this about on one of my patches: https://bugs.webkit.org/show_bug.cgi?id=101257#c22 I also landed a cleanup patch regarding this in: https://bugs.webkit.org/show_bug.cgi?id=101859 While this makes no difference for primitive types, it usually does for complex types. Therefore, it is good practice to use prefix operator whenever possible. >> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:147 >> + return adoptWK(WKImageCreateFromCairoSurface(surface.get(), options)); > > I did not understand why we converted to a cairo surface and then back to a WKImageRef. Why not return WKViewGetSnapshot(toAPI(m_view)) directly? What I mean is WKViewGetSnapshot() returns a WKImageRef and this function returns a WKImageRef. Why not return the output of WKViewGetSnapshot() directly? Instead you are creating a surface using WKImageCreateCairoSurface() from the WKImage, then converting the surface back to a WKImage. I did not understand the reason for this. (In reply to comment #12) > (From update of attachment 175911 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175911&action=review > > > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:85 > > + cairo_t* context = cairo_create(surface); > > Smart pointer? > > > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:121 > > + cairo_surface_t* surface = 0; > > Smart pointer? TestInvocationEfl.cpp is almost a copy of TestInvocationCairo.cpp, with some modifications specific to EFL. I was hoping to get some comments regarding if people prefer that the file is split, or to add the EFL specific bits to TestInvocationCairo.cpp. I think your comments are result of copy/pasted code from TestInvocationCairo.cpp. Adding a few GTK guys to see if they have an opinion regarding the addition of EFL specific code in TestInvocationCairo.cpp (https://bugs.webkit.org/show_bug.cgi?id=102833#c15). Created attachment 176066 [details] Patch Address comment #12. Comment on attachment 176066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176066&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1035 > + delete[] buffer; > + return image; I prefer newline before return > Source/WebKit2/UIProcess/API/efl/SnapshotGL.cpp:37 > + // Now flip it Bad comment and ends without punctuation mark > Source/WebKit2/UIProcess/API/efl/SnapshotGL.h:21 > +#ifndef SnapshotGL_h > +#define SnapshotGL_h what about SnapshotImageGL ? Created attachment 176113 [details] Patch Address comment #18. Comment on attachment 176113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176113&action=review > Source/WebKit2/PlatformEfl.cmake:45 > + UIProcess/API/efl/SnapshotImageGL.cpp Wrong alphabetical order. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1026 > + return WKImageCreateFromCairoSurface(surface.get(), 0); If you don't use local variable, I think you can remove "{", "}" in this function. return WKImageCreateFromCairoSurface(createSurfaceForImage(sd->image).get(), 0); Or, because you define *surface* variable two places, I think you can declare it first on 1022 line. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1030 > + Evas_Native_Surface* ns = evas_object_image_native_surface_get(sd->image); I prefer to use full name. e.g) naviveSurface. > Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:21 > +#if USE(ACCELERATED_COMPOSITING) IMO, it would be better if you move this macro to above #if USE(OPENGL_ES_2) Created attachment 176279 [details] Patch Address comment #20. Comment on attachment 176279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176279&action=review > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:45 > +void computeMD5HashStringForCairoSurface(cairo_surface_t* surface, char hashString[33]) There seems to be quite a bit of duplication with TestInvocationCairo.cpp. I really wish we could avoid that. Personally, I would add #ifdefs in TestInvocationCairo.cpp to git EFL port's need. This would avoid the duplication. Any other opinion on this? (In reply to comment #22) > (From update of attachment 176279 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176279&action=review > > > Tools/WebKitTestRunner/efl/TestInvocationEfl.cpp:45 > > +void computeMD5HashStringForCairoSurface(cairo_surface_t* surface, char hashString[33]) > > There seems to be quite a bit of duplication with TestInvocationCairo.cpp. I really wish we could avoid that. > > Personally, I would add #ifdefs in TestInvocationCairo.cpp to git EFL port's need. This would avoid the duplication. > Any other opinion on this? That sounds like the proper thing to do indeed Created attachment 176329 [details]
Patch
Removed TestInvocationEFL.cpp
Looks like I cannot include WebCore headers in webkit-gtk, so I cannot use RefCairoPtr in common code. Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo_surface>(_cairo_surface*)' Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo>(_cairo*)' Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo>(_cairo*)' Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo_surface>(_cairo_surface*)' collect2: error: ld returned 1 exit status make[1]: *** [Programs/WebKitTestRunner] Error 1 make[1]: Leaving directory `/home/phil/gst/jhbuild/makes/WebKit/WebKitBuild/Release' make: *** [all] Error 2 Failed to build WebKit using 'make'! (In reply to comment #25) > Looks like I cannot include WebCore headers in webkit-gtk, so I cannot use RefCairoPtr in common code. > > Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo_surface>(_cairo_surface*)' > Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo>(_cairo*)' > Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo>(_cairo*)' > Tools/WebKitTestRunner/cairo/Programs_WebKitTestRunner-TestInvocationCairo.o:TestInvocationCairo.cpp:function WTR::TestInvocation::dumpPixelsAndCompareWithExpected(OpaqueWKImage const*, OpaqueWKArray const*): error: undefined reference to 'void WTF::derefIfNotNull<_cairo_surface>(_cairo_surface*)' > collect2: error: ld returned 1 exit status > make[1]: *** [Programs/WebKitTestRunner] Error 1 > make[1]: Leaving directory `/home/phil/gst/jhbuild/makes/WebKit/WebKitBuild/Release' > make: *** [all] Error 2 > > Failed to build WebKit using 'make'! This would probably fix it: diff --git a/Tools/WebKitTestRunner/GNUmakefile.am b/Tools/WebKitTestRunner/GNUmakefile.am index 3706003..b1dfb98 100644 --- a/Tools/WebKitTestRunner/GNUmakefile.am +++ b/Tools/WebKitTestRunner/GNUmakefile.am @@ -52,6 +52,7 @@ Programs_WebKitTestRunner_CFLAGS = $(global_cflags) Programs_WebKitTestRunner_LDADD = \ libjavascriptcoregtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la \ + libWebCoreGtk.la \ libwebkit2gtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la \ $(GLOBALDEPS_LIBS) \ $(CAIRO_LIBS) \ Unless there is a specific reason not to have WebKitTestRunner link against WebCore library? Created attachment 176336 [details]
Patch
Removed smart pointer usage from TestInvocationCairo.cpp.
They break the GTK build.
Comment on attachment 176336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176336&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1026 > + return WKImageCreateFromCairoSurface(createSurfaceForImage(sd->image).get(), 0); > +#if USE(ACCELERATED_COMPOSITING) newline after return please > Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:6 > +/* > + Copyright (C) 2012 Intel Corporation. All rights reserved. > + > + This library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Library General Public > + License as published by the Free Software Foundation; either why not * ? and why not BSD license? I think we prefer that > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:110 > +void TestInvocation::forceRepaintDoneCallback(WKErrorRef, void *context) didFinishRepaint would be a nicer name > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:112 > + static_cast<TestInvocation*>(context)->m_gotRepaint = true; gotRepaint is a terrible name. why not m_didRepaint > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:136 > + > + if (m_gotRepaint) > + surface = WKImageCreateCairoSurface(TestController::shared().mainWebView()->windowSnapshotImage().get()); > + else { > + m_error = true; > + m_errorMessage = "Timed out waiting for repaint\n"; > + m_webProcessIsUnrensponsive = true; > + return; > + } no. if (!m_didRepaint) { ... return; } cairo_surface_t surface = WKImageCreateCairoSurface(TestController::shared().mainWebView()->windowSnapshotImage().get()); > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:132 > + Ecore_Evas* ee = ecore_evas_ecore_evas_get(evas_object_evas_get(m_view)); > + > + ASSERT(ee); no newline before ASSERT, it belong to ee (In reply to comment #28) > (From update of attachment 176336 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176336&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:1026 > > + return WKImageCreateFromCairoSurface(createSurfaceForImage(sd->image).get(), 0); > > +#if USE(ACCELERATED_COMPOSITING) > > newline after return please > ok > > Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:6 > > +/* > > + Copyright (C) 2012 Intel Corporation. All rights reserved. > > + > > + This library is free software; you can redistribute it and/or > > + modify it under the terms of the GNU Library General Public > > + License as published by the Free Software Foundation; either > > why not * ? and why not BSD license? I think we prefer that > I copied the license of EwkViewImpl, I'll change that. > > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:110 > > +void TestInvocation::forceRepaintDoneCallback(WKErrorRef, void *context) > > didFinishRepaint would be a nicer name > The functions names are not introduced in this patch. I can rename them as a followup > > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:112 > > + static_cast<TestInvocation*>(context)->m_gotRepaint = true; > > gotRepaint is a terrible name. why not m_didRepaint > copy/paste from Qt :) I'll rename > > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:136 > > + > > + if (m_gotRepaint) > > + surface = WKImageCreateCairoSurface(TestController::shared().mainWebView()->windowSnapshotImage().get()); > > + else { > > + m_error = true; > > + m_errorMessage = "Timed out waiting for repaint\n"; > > + m_webProcessIsUnrensponsive = true; > > + return; > > + } > > no. > > if (!m_didRepaint) { > ... > return; > } > same as before, copy/paste from Qt :) Will change. > cairo_surface_t surface = WKImageCreateCairoSurface(TestController::shared().mainWebView()->windowSnapshotImage().get()); > > > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:132 > > + Ecore_Evas* ee = ecore_evas_ecore_evas_get(evas_object_evas_get(m_view)); > > + > > + ASSERT(ee); > > no newline before ASSERT, it belong to ee ok Created attachment 176347 [details] Patch Address part of comment #28. Comment on attachment 176347 [details] Patch Clearing flags on attachment: 176347 Committed r135935: <http://trac.webkit.org/changeset/135935> All reviewed patches have been landed. Closing bug. This patch seems to have caused 6 failures: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6416 We have blurry text on scaled layers and a few other issues. (In reply to comment #33) > This patch seems to have caused 6 failures: > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6416 > > We have blurry text on scaled layers and a few other issues. There are over 100 tests that have become flaky ([ IMAGE PASS ]) because of this Not sure how it is related but fast/dom/shadow/input-with-validation.html seems to have started crashing reliably after this commit on WK2 debug (In reply to comment #35) > Not sure how it is related but fast/dom/shadow/input-with-validation.html seems to have started crashing reliably after this commit on WK2 debug We have these bugs to track the crash: https://bugs.webkit.org/show_bug.cgi?id=98026 https://bugs.webkit.org/show_bug.cgi?id=99886 Causes masive reftests flakiness, this needs to be solved urgently. (In reply to comment #37) > Causes masive reftests flakiness, this needs to be solved urgently. Unfortunately, many of the failures are real failures. I was looking today e.g. the css/sticky results and the actual results match what I see in MiniBrowser. Which means we have bugs that need to be fixed. (In reply to comment #37) > Causes masive reftests flakiness, this needs to be solved urgently. Thanks to kbalazs, who helped me analyze some of the flaky tests. It seems that we have a bug in the way we draw the scrollbars. If you look at the expected results images, they are completely wrong in many tests. I will file a separate bug about that. (In reply to comment #39) > (In reply to comment #37) > > Causes masive reftests flakiness, this needs to be solved urgently. > > Thanks to kbalazs, who helped me analyze some of the flaky tests. It seems that we have a bug in the way we draw the scrollbars. If you look at the expected results images, they are completely wrong in many tests. I will file a separate bug about that. https://bugs.webkit.org/show_bug.cgi?id=103641 (In reply to comment #38) > (In reply to comment #37) > > Causes masive reftests flakiness, this needs to be solved urgently. > > Unfortunately, many of the failures are real failures. > I was looking today e.g. the css/sticky results and the actual results match what I see in MiniBrowser. Which means we have bugs that need to be fixed. Many tests don't seem that way. As an example (and this one seems common) fast/forms/input-first-letter-edit.html is just empty (totally white) when it fails. If you look at the test, it is highly unlikely to be a real failure: it's basically just <body> <input type="text" id="target" /> </body> with a bit of script that 'types' text into the input. I don't think that really renders as an empty page. Closing this bug because the EFL port has been removed from trunk. If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this. |