Bug 131265 - [EFL] Fix problems with the pixel dump.
Summary: [EFL] Fix problems with the pixel dump.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Hyowon Kim
URL:
Keywords:
Depends on:
Blocks: 79766 131266
  Show dependency treegraph
 
Reported: 2014-04-05 01:39 PDT by Hyowon Kim
Modified: 2014-04-30 04:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (11.26 KB, patch)
2014-04-05 05:14 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (9.31 KB, patch)
2014-04-07 06:49 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (21.85 KB, patch)
2014-04-10 08:49 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
modify change logs (22.25 KB, patch)
2014-04-11 18:45 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
rebase patch (22.34 KB, patch)
2014-04-13 18:50 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (22.46 KB, patch)
2014-04-15 18:56 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hyowon Kim 2014-04-05 01:39:29 PDT
There are problems with dumping pixel results since r166637.
Comment 1 Hyowon Kim 2014-04-05 05:14:08 PDT
Created attachment 228668 [details]
Patch
Comment 2 Gyuyoung Kim 2014-04-06 03:38:49 PDT
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.
Comment 3 Hyowon Kim 2014-04-06 22:31:06 PDT
(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.
Comment 4 Hyowon Kim 2014-04-07 06:49:06 PDT
Created attachment 228732 [details]
Patch
Comment 5 Gyuyoung Kim 2014-04-07 17:46:57 PDT
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 6 Ryuan Choi 2014-04-07 18:48:02 PDT
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.
Comment 7 Hyowon Kim 2014-04-07 21:21:12 PDT
(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.
Comment 8 Hyowon Kim 2014-04-07 21:55:40 PDT
(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.
Comment 9 Hyowon Kim 2014-04-10 08:49:50 PDT
Created attachment 229051 [details]
Patch
Comment 10 Gyuyoung Kim 2014-04-11 00:05:56 PDT
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 11 Ryuan Choi 2014-04-11 00:13:10 PDT
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
Comment 12 Hyowon Kim 2014-04-11 01:03:18 PDT
(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!
Comment 13 Gyuyoung Kim 2014-04-11 01:13:56 PDT
(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.
Comment 14 Hyowon Kim 2014-04-11 18:45:18 PDT
Created attachment 229192 [details]
modify change logs
Comment 15 Hyowon Kim 2014-04-13 18:50:48 PDT
Created attachment 229254 [details]
rebase patch
Comment 16 Gyuyoung Kim 2014-04-14 23:47:42 PDT
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.
Comment 17 Gyuyoung Kim 2014-04-14 23:48:58 PDT
CC'ing Martin, could you take a look changes in CairoUtilities.cpp ?
Comment 18 Martin Robinson 2014-04-15 05:46:15 PDT
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.
Comment 19 Hyowon Kim 2014-04-15 18:56:11 PDT
Created attachment 229423 [details]
Patch
Comment 20 Hyowon Kim 2014-04-15 18:58:47 PDT
(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 21 Gyuyoung Kim 2014-04-15 20:15:06 PDT
Comment on attachment 229423 [details]
Patch

LGTM with Martin'review.
Comment 22 WebKit Commit Bot 2014-04-15 20:45:55 PDT
Comment on attachment 229423 [details]
Patch

Clearing flags on attachment: 229423

Committed r167342: <http://trac.webkit.org/changeset/167342>
Comment 23 WebKit Commit Bot 2014-04-15 20:46:02 PDT
All reviewed patches have been landed.  Closing bug.