Bug 131265 – [EFL] Fix problems with the pixel dump.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hyowon Kim]

There are problems with dumping pixel results since r166637.

[Hyowon Kim]

Created attachment 228668 [details]
Patch

[Gyuyoung Kim]

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.

[Hyowon Kim]

(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.

[Hyowon Kim]

Created attachment 228732 [details]
Patch

[Gyuyoung Kim]

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 ?

[Ryuan Choi]

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.

[Hyowon Kim]

(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.

[Hyowon Kim]

(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.

[Hyowon Kim]

Created attachment 229051 [details]
Patch

[Gyuyoung Kim]

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."

[Ryuan Choi]

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

[Hyowon Kim]

(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!

[Gyuyoung Kim]

(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.

[Hyowon Kim]

Created attachment 229192 [details]
modify change logs

[Hyowon Kim]

Created attachment 229254 [details]
rebase patch

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

CC'ing Martin, could you take a look changes in CairoUtilities.cpp ?

[Martin Robinson]

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.

[Hyowon Kim]

Created attachment 229423 [details]
Patch

[Hyowon Kim]

(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. :)

[Gyuyoung Kim]

Comment on attachment 229423 [details]
Patch

LGTM with Martin'review.

[WebKit Commit Bot]

Comment on attachment 229423 [details]
Patch

Clearing flags on attachment: 229423

Committed r167342: <http://trac.webkit.org/changeset/167342>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.