There are problems with dumping pixel results since r166637.
Created attachment 228668 [details] Patch
Comment on attachment 228668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228668&action=review > Source/WebKit/efl/ewk/ewk_view.h:147 > +typedef struct _cairo_surface cairo_surface_t; In r164168, ryuan removed to open cairo data structures from ewk public APIs. Ryuan, how do you think about this patch ? > Source/WebKit/efl/ewk/ewk_view.h:2504 > + * Gets a surface for the image data of the given ewkView. Add a new line. And ewkView -> the view. > Source/WebKit/efl/ewk/ewk_view.h:2507 > + * @return a cairo_surface_t pointer to the newly created surface. Remove . at the end of line. > Source/WebKit/efl/ewk/ewk_view.h:2509 > +EAPI cairo_surface_t* ewk_view_image_data_get(const Evas_Object* o); Wrong * place.
(In reply to comment #2) > (From update of attachment 228668 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228668&action=review > > > Source/WebKit/efl/ewk/ewk_view.h:147 > > +typedef struct _cairo_surface cairo_surface_t; > > In r164168, ryuan removed to open cairo data structures from ewk public APIs. Ryuan, how do you think about this patch ? > After discussion with Ryuan, I've decided to remove a new API ewk_view_image_data_get() and then reimplement the ewk_view_screenshot_contents_get(). Thanks for your comment and Ryuan's valuable advice.
Created attachment 228732 [details] Patch
Comment on attachment 228732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228732&action=review > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:149 > +void AcceleratedCompositingContext::paintToCurrentGLContext(bool paintingMirrored) paintingMirrored parameter is just passed to compositeLayers from AcceleratedCompositingContext::extractImageData(). So, I think that patintToCurrentGLContext() doesn't need to have the paintingMirrored param. It looks a private boolean member variable is proper for this case. For instance, AcceleratedCompositingContext::extractImageData() { m_paintingMirrored = true; } void AcceleratedCompositingContext::compositeLayers() { m_textureMapper->beginPainting(m_paintingMirrored ? TextureMapper::PaintingMirrored : 0); } > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:155 > + evas_gl_api_get(m_evasGL.get())->glClearColor(1, 0.5, 1, 1); Any reason to set green value to 0.5 ?
Comment on attachment 228732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228732&action=review > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:197 > + paintToCurrentGLContext(true); As discussed, it will make the bug. We will not draw flipped contents on the m_compositingObject. > Source/WebKit/efl/ewk/ewk_view.cpp:4577 > - ewk_paint_context_scale(context, scale, scale); > - ewk_paint_context_translate(context, -1 * area->x, -1 * area->y); > - > - ewk_view_paint(priv, context, area); > - > - ewk_paint_context_restore(context); > - ewk_paint_context_free(context); > + priv->acceleratedCompositingContext->extractImageData(screenshotImage, WebCore::IntRect(*area)); As discussed, this API lost scale functionality with your path. But, I agree with you that the name of this API, `screenshot` is quite ambiguous. So, Let's just remove scale parameter and change the doxygen for developers not to make ambiguity.
(In reply to comment #5) > (From update of attachment 228732 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228732&action=review > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:149 > > +void AcceleratedCompositingContext::paintToCurrentGLContext(bool paintingMirrored) > > paintingMirrored parameter is just passed to compositeLayers from AcceleratedCompositingContext::extractImageData(). So, I think that patintToCurrentGLContext() doesn't need to have the paintingMirrored param. It looks a private boolean member variable is proper for this case. > > For instance, > > AcceleratedCompositingContext::extractImageData() { > m_paintingMirrored = true; > } > > void AcceleratedCompositingContext::compositeLayers() { > m_textureMapper->beginPainting(m_paintingMirrored ? TextureMapper::PaintingMirrored : 0); > } Okay, I'll think about it. > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:155 > > + evas_gl_api_get(m_evasGL.get())->glClearColor(1, 0.5, 1, 1); > > Any reason to set green value to 0.5 ? Sorry. My mistake.
(In reply to comment #6) > (From update of attachment 228732 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228732&action=review > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:197 > > + paintToCurrentGLContext(true); > > As discussed, it will make the bug. > > We will not draw flipped contents on the m_compositingObject. I'll fix it. > > Source/WebKit/efl/ewk/ewk_view.cpp:4577 > > - ewk_paint_context_scale(context, scale, scale); > > - ewk_paint_context_translate(context, -1 * area->x, -1 * area->y); > > - > > - ewk_view_paint(priv, context, area); > > - > > - ewk_paint_context_restore(context); > > - ewk_paint_context_free(context); > > + priv->acceleratedCompositingContext->extractImageData(screenshotImage, WebCore::IntRect(*area)); > > As discussed, this API lost scale functionality with your path. > But, I agree with you that the name of this API, `screenshot` is quite ambiguous. > So, Let's just remove scale parameter and change the doxygen for developers not to make ambiguity. Okay. I appreciate your explanation about our discussion. An image taken by a screenshot API should have visible content displayed on the webview. But the previous implementation of ewk_view_screenshot_contents_get() was intended to repaint the certain area of webpage and return the resultant image as Evas_object. In conclusion, I'll modify that this API works as expected. Thanks.
Created attachment 229051 [details] Patch
Comment on attachment 229051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229051&action=review It looks there is no critical issue on EFL side. However, this patch modify cario graphics back-end. I think folks of cairo graphics need to take a look this patch. > Source/WebCore/ChangeLog:11 > + This patch adds a new API for pixel dump AcceleratedCompositingContext::extractImageData() This description is a little unclear for me. Although I'm not sure whether I understand this description, I modify this description a little. "This patch adds new member functions to AcceleratedCompositingContext for supporting pixel dump. One of new functions is "AcceleratedCompositingContext::extractImageData()", which replaces deprecated function calls. Besides the extractImageData() is invoked by ewk_view_screenshot_contents_get() in order to take the visible content displayed on the EFL webview."
Comment on attachment 229051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229051&action=review > Source/WebKit/efl/ChangeLog:8 > + Painting and compositing paths of WebKit2-EFL were totally modified from r166768. WebKit/Efl
(In reply to comment #10) > (From update of attachment 229051 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229051&action=review > > It looks there is no critical issue on EFL side. However, this patch modify cario graphics back-end. I think folks of cairo graphics need to take a look this patch. This patch doesn't modify cairo backend side, just adds a util function, flipVertically(cairo_surface_t*). > > Source/WebCore/ChangeLog:11 > > + This patch adds a new API for pixel dump AcceleratedCompositingContext::extractImageData() > > This description is a little unclear for me. Although I'm not sure whether I understand this description, I modify this description a little. > > "This patch adds new member functions to AcceleratedCompositingContext for supporting pixel dump. One of new functions is "AcceleratedCompositingContext::extractImageData()", which replaces deprecated function calls. Besides the extractImageData() is invoked by ewk_view_screenshot_contents_get() in order to take the visible content displayed on the EFL webview." Thanks!
(In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 229051 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=229051&action=review > > > > It looks there is no critical issue on EFL side. However, this patch modify cario graphics back-end. I think folks of cairo graphics need to take a look this patch. > > This patch doesn't modify cairo backend side, just adds a util function, flipVertically(cairo_surface_t*). Yes, I pointed it out.
Created attachment 229192 [details] modify change logs
Created attachment 229254 [details] rebase patch
Comment on attachment 229254 [details] rebase patch View in context: https://bugs.webkit.org/attachment.cgi?id=229254&action=review > Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:262 > +void flipVertically(cairo_surface_t* surface) I think Martin can review this function.
CC'ing Martin, could you take a look changes in CairoUtilities.cpp ?
Comment on attachment 229254 [details] rebase patch View in context: https://bugs.webkit.org/attachment.cgi?id=229254&action=review Looks okay with a few small changes. >> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:262 >> +void flipVertically(cairo_surface_t* surface) > > I think Martin can review this function. Probably should be called flipImageSurfaceVertically. > Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:265 > + ASSERT(!size.isEmpty()); You probably want to ASSERT the that this is also an image surface.
Created attachment 229423 [details] Patch
(In reply to comment #18) > (From update of attachment 229254 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229254&action=review > > Looks okay with a few small changes. > > >> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:262 > >> +void flipVertically(cairo_surface_t* surface) > > > > I think Martin can review this function. > > Probably should be called flipImageSurfaceVertically. > Okay, done. > > Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:265 > > + ASSERT(!size.isEmpty()); > > You probably want to ASSERT the that this is also an image surface. I've added it. Thank you Martin. :)
Comment on attachment 229423 [details] Patch LGTM with Martin'review.
Comment on attachment 229423 [details] Patch Clearing flags on attachment: 229423 Committed r167342: <http://trac.webkit.org/changeset/167342>
All reviewed patches have been landed. Closing bug.