Although applications should use opengl_XXX as evas backends to use weblgl, accelerated compositing and GL related features, We should have fallback mechanism for the applications which uses S/W backend. Now, Efl applications on linux use software_x11 as a default backend.
Created attachment 173465 [details] Patch Create a setting that allows applications to fallback to the software path of TextureMapper. The setting is only supported if TextureMapper was not created yet and does not allow to switch afterwards. (It asserts in debug and is ignored in release mode). Applications that want to use the software_x11 evas engine can do so if they set the new setting. Detection of the engine will be done in https://bugs.webkit.org/show_bug.cgi?id=101389 .
Attachment 173465 [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:96: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 173466 [details] Patch Fix style.
Comment on attachment 173466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173466&action=review > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:363 > +EAPI Eina_Bool ewk_settings_hardware_acceleration_set(Ewk_Settings *settings, RenderingMode mode); Weird name "hardware acceleration set == software" that is not really hardware accelerated. Why not rendering_mode_set(settings, OpenGL); In what way is this different than setting the engine? Tha is not clear to people
(In reply to comment #4) > (From update of attachment 173466 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173466&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:363 > > +EAPI Eina_Bool ewk_settings_hardware_acceleration_set(Ewk_Settings *settings, RenderingMode mode); > > Weird name "hardware acceleration set == software" that is not really hardware accelerated. > > Why not rendering_mode_set(settings, OpenGL); > ok. > In what way is this different than setting the engine? Tha is not clear to people I thought we wanted the engine to be transparent to WebKit, no?
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 173466 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=173466&action=review > > > > > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:363 > > > +EAPI Eina_Bool ewk_settings_hardware_acceleration_set(Ewk_Settings *settings, RenderingMode mode); > > > > Weird name "hardware acceleration set == software" that is not really hardware accelerated. > > > > Why not rendering_mode_set(settings, OpenGL); > > > ok. > > In what way is this different than setting the engine? Tha is not clear to people > I thought we wanted the engine to be transparent to WebKit, no? I could easily add a test for opengl engine, if that is what people prefer.
Created attachment 173482 [details] Patch Remove the setting and silently fallback to software rendering if opengl is not available. Also fixed some bugs like disable webGl and reset the software path after a web process crash.
Attachment 173482 [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/efl/PageClientBase.cpp:72: Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 173482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173482&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:87 > +cairo_t* createCairoContext(Ewk_View_Smart_Data* sd) > +{ I think chris made a class with similar methods > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:106 > +void freeCairoContext(cairo_t* graphicsContext) > +{ > + cairo_surface_t* surface = cairo_get_target(graphicsContext); > + cairo_destroy(graphicsContext); > + cairo_surface_finish(surface); > +} and there is no OwnPtr for this one? like there is for cairo_surface_t ?
Comment on attachment 173482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173482&action=review >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:87 >> +{ > > I think chris made a class with similar methods Source/WebCore/platform/graphics/efl/CairoUtilitiesEfl.h > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:97 > + cairo_t* graphicsContext = cairo_create(surface); Looks to me that cairo_surface_destroy() should be called on surface after this. Or even better, let's use a RefPtr<cairo_surface_t> earlier to avoid such problem. >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:106 >> +} > > and there is no OwnPtr for this one? like there is for cairo_surface_t ? There is RefPtr<cairo_t> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:363 > + cairo_t* graphicsContext = createCairoContext(sd); RefPtr ? > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:656 > + if (m_hardwareAccelerationEnabled) { I think we should use the return early approach like in the rest of WebKit. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:202 > + bool hardwareAccelerationEnabled() { return m_hardwareAccelerationEnabled; } method should be const
(In reply to comment #10) > (From update of attachment 173482 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173482&action=review > > >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:87 > >> +{ > > > > I think chris made a class with similar methods > > Source/WebCore/platform/graphics/efl/CairoUtilitiesEfl.h > Thanks for the tio, I'll look into that > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:97 > > + cairo_t* graphicsContext = cairo_create(surface); > > Looks to me that cairo_surface_destroy() should be called on surface after this. Or even better, let's use a RefPtr<cairo_surface_t> earlier to avoid such problem. > It is called in freeCairoContext() > >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:106 > >> +} > > > > and there is no OwnPtr for this one? like there is for cairo_surface_t ? > > There is RefPtr<cairo_t> > ok > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:363 > > + cairo_t* graphicsContext = createCairoContext(sd); > > RefPtr ? > ok > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:656 > > + if (m_hardwareAccelerationEnabled) { > > I think we should use the return early approach like in the rest of WebKit. > We still need to call setRenderActive. I'll see if I can move that call. > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:202 > > + bool hardwareAccelerationEnabled() { return m_hardwareAccelerationEnabled; } > > method should be const ok
(In reply to comment #9) > (From update of attachment 173482 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173482&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:87 > > +cairo_t* createCairoContext(Ewk_View_Smart_Data* sd) > > +{ > > I think chris made a class with similar methods > Thanks for the tip. Chris's methods don't exactly match what I need, so I will move the Cairo related code to there. > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:106 > > +void freeCairoContext(cairo_t* graphicsContext) > > +{ > > + cairo_surface_t* surface = cairo_get_target(graphicsContext); > > + cairo_destroy(graphicsContext); > > + cairo_surface_finish(surface); > > +} > > and there is no OwnPtr for this one? like there is for cairo_surface_t ? Will change that.
Created attachment 173525 [details] Patch Address comment #9 and comment #10.
Attachment 173525 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/UIProcess/efl/PageClientBase.cpp:72: Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 173525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173525&action=review >> Source/WebKit2/UIProcess/efl/PageClientBase.cpp:72 >> + PassOwnPtr<DrawingAreaProxy> drawingArea = DrawingAreaProxyImpl::create(m_viewImpl->page()); > > Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Yael, I think it should be OwnPtr<DrawingAreaProxy> drawingArea here and you have to do drawingArea.release() when return.
Comment on attachment 173482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173482&action=review >>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:97 >>> + cairo_t* graphicsContext = cairo_create(surface); >> >> Looks to me that cairo_surface_destroy() should be called on surface after this. Or even better, let's use a RefPtr<cairo_surface_t> earlier to avoid such problem. > > It is called in freeCairoContext() This is still not nice. The documentation for cairo_create() says: "This function references target, so you can immediately call cairo_surface_destroy() on it if you don't need to maintain a separate reference to it." We are not keeping a reference to the surface so we should undef it directly here. The cairo_surface_finish(surface) call in freeCairoContext will then become useless because cairo_destroy() will undef the surface and its refcount will hit 0. Also note that freeCairoContext() will probably go away when you switch to smart pointers.
Comment on attachment 173525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173525&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:345 > + RefPtr<cairo_t> graphicsContext = cairo_create(surface.get()); Missing adoptRef() it seems. BTW, should't we keep it as a class member? I don't think renderer->paintToGraphicsContext(graphicsContext.get()) will ref it. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:346 > + if (!graphicsContext) Useless NULL check, the doc says "This function never returns NULL. " >>> Source/WebKit2/UIProcess/efl/PageClientBase.cpp:72 >>> + PassOwnPtr<DrawingAreaProxy> drawingArea = DrawingAreaProxyImpl::create(m_viewImpl->page()); >> >> Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] > > Yael, I think it should be OwnPtr<DrawingAreaProxy> drawingArea here and you have to do drawingArea.release() when return. Right. > Source/WebCore/platform/graphics/efl/CairoUtilitiesEfl.cpp:109 > +PassRefPtr<cairo_surface_t> createSurfaceForImage(Evas_Object *image) Star on wrong side > Source/WebCore/platform/graphics/efl/CairoUtilitiesEfl.cpp:127 > + return surface; you are supposed to release() here I believe.
(In reply to comment #17) > (From update of attachment 173525 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173525&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:345 > > + RefPtr<cairo_t> graphicsContext = cairo_create(surface.get()); > > Missing adoptRef() it seems. BTW, should't we keep it as a class member? I don't think renderer->paintToGraphicsContext(graphicsContext.get()) will ref it. > adoptRef is oly needed if the create method return a raw pointer but it doesn't. The adoptRef is inside the create method. > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:346 > > + if (!graphicsContext) > > Useless NULL check, the doc says "This function never returns NULL. " > ok > >>> Source/WebKit2/UIProcess/efl/PageClientBase.cpp:72 > >>> + PassOwnPtr<DrawingAreaProxy> drawingArea = DrawingAreaProxyImpl::create(m_viewImpl->page()); > >> > >> Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] > > > > Yael, I think it should be OwnPtr<DrawingAreaProxy> drawingArea here and you have to do drawingArea.release() when return. > > Right. > ok > > Source/WebCore/platform/graphics/efl/CairoUtilitiesEfl.cpp:109 > > +PassRefPtr<cairo_surface_t> createSurfaceForImage(Evas_Object *image) > > Star on wrong side > ok > > Source/WebCore/platform/graphics/efl/CairoUtilitiesEfl.cpp:127 > > + return surface; > > you are supposed to release() here I believe. This method is not returning a raw pointer. It returns PassRefPtr, so I believe it is correct as it is now. Please refer to your method right above the new one. It does exactly the same.
Comment on attachment 173525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173525&action=review >>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:345 >>> + RefPtr<cairo_t> graphicsContext = cairo_create(surface.get()); >> >> Missing adoptRef() it seems. BTW, should't we keep it as a class member? I don't think renderer->paintToGraphicsContext(graphicsContext.get()) will ref it. > > adoptRef is oly needed if the create method return a raw pointer but it doesn't. The adoptRef is inside the create method. Well, cairo_create() is a cairo function so it returns a raw pointer, right? :) >>> Source/WebCore/platform/graphics/efl/CairoUtilitiesEfl.cpp:127 >>> + return surface; >> >> you are supposed to release() here I believe. > > This method is not returning a raw pointer. It returns PassRefPtr, so I believe it is correct as it is now. Please refer to your method right above the new one. It does exactly the same. release() does not return a raw pointer, it returns a PassRefPtr. If you don't call release() when returning here the ref count will be 2 instead of 1. I still believe this code is incorrect. You pass ownership to the caller so you should call release().
(In reply to comment #19) > (From update of attachment 173525 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173525&action=review > > >>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:345 > >>> + RefPtr<cairo_t> graphicsContext = cairo_create(surface.get()); > >> > >> Missing adoptRef() it seems. BTW, should't we keep it as a class member? I don't think renderer->paintToGraphicsContext(graphicsContext.get()) will ref it. > > > > adoptRef is oly needed if the create method return a raw pointer but it doesn't. The adoptRef is inside the create method. > > Well, cairo_create() is a cairo function so it returns a raw pointer, right? :) > I meant createSurfaceForImage, not cairo_create > >>> Source/WebCore/platform/graphics/efl/CairoUtilitiesEfl.cpp:127 > >>> + return surface; > >> > >> you are supposed to release() here I believe. > > > > This method is not returning a raw pointer. It returns PassRefPtr, so I believe it is correct as it is now. Please refer to your method right above the new one. It does exactly the same. > > release() does not return a raw pointer, it returns a PassRefPtr. If you don't call release() when returning here the ref count will be 2 instead of 1. I still believe this code is incorrect. You pass ownership to the caller so you should call release().
Comment on attachment 173525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173525&action=review >>>>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:345 >>>>> + RefPtr<cairo_t> graphicsContext = cairo_create(surface.get()); >>>> >>>> Missing adoptRef() it seems. BTW, should't we keep it as a class member? I don't think renderer->paintToGraphicsContext(graphicsContext.get()) will ref it. >>> >>> adoptRef is oly needed if the create method return a raw pointer but it doesn't. The adoptRef is inside the create method. >> >> Well, cairo_create() is a cairo function so it returns a raw pointer, right? :) > > I meant createSurfaceForImage, not cairo_create Ok, my comment is about the cairo_create() line so I still think the comment is valid and adoptRef() is missing. >>>>> Source/WebCore/platform/graphics/efl/CairoUtilitiesEfl.cpp:127 >>>>> + return surface; >>>> >>>> you are supposed to release() here I believe. >>> >>> This method is not returning a raw pointer. It returns PassRefPtr, so I believe it is correct as it is now. Please refer to your method right above the new one. It does exactly the same. >> >> release() does not return a raw pointer, it returns a PassRefPtr. If you don't call release() when returning here the ref count will be 2 instead of 1. I still believe this code is incorrect. You pass ownership to the caller so you should call release(). > > BTW, the methods above are not really mine, I merely moved existing functions to this new file I created. And you are right that createSurfaceForBackingStore() above is not releasing, which is wrong (we should fix that). Note that the evasObjectFromCairoImageSurface() method is correctly releasing though.
Created attachment 173634 [details] Patch
(In reply to comment #22) > Created an attachment (id=173634) [details] > Patch Address comment #15 and comment #17.
Comment on attachment 173634 [details] Patch LGTM.
Comment on attachment 173634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173634&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:131 > + , m_hardwareAccelerationEnabled(true) why not just isHardwareAccelerated(? It is not a setting right? > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:349 > + evas_object_image_data_update_add(sd->image, 0, 0, viewport.width(), viewport.height()); why is this 0, 0 ? when the above uses viewport.x() etc > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:104 > + // The acceleration mode can be set only before TextureMapper was created. *is*
(In reply to comment #25) > (From update of attachment 173634 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173634&action=review > > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:131 > > + , m_hardwareAccelerationEnabled(true) > > why not just isHardwareAccelerated(? It is not a setting right? > ok > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:349 > > + evas_object_image_data_update_add(sd->image, 0, 0, viewport.width(), viewport.height()); > > why is this 0, 0 ? when the above uses viewport.x() etc > When using openGl, the Gl surface is n the coordinates of the parent. i.e. it includes the URL bar. When not using a Gl surface, the coordinates are view coordinates. > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:104 > > + // The acceleration mode can be set only before TextureMapper was created. > > *is* ok
Created attachment 173651 [details] Patch for landing. Approved by Kenneth on irc: [10:40] <yael> kenneth_: Since the last issues are so minor, are you ready to r+ it? [10:41] <kenneth_> r=me [10:47] <yael> kenneth_: I think you need to add that to the bug ? [10:48] <kenneth_> not if you write it and cq+ :)
Comment on attachment 173651 [details] Patch for landing. Clearing flags on attachment: 173651 Committed r134247: <http://trac.webkit.org/changeset/134247>
All reviewed patches have been landed. Closing bug.