Bug 105803 – [EFL][WK2] Regression(138462) Sometimes garbage in snapshots

• Component: WebKit2
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

In EwkViewImpl::takeSnapshot surface created by cairo_image_surface_create_for_data is returned, but buffer is release on exit.
Doc for cairo_image_surface_create_for_data here http://www.cairographics.org/manual/cairo-Image-Surfaces.html#cairo-image-surface-create-for-data says:
"The output buffer must be kept around until the cairo_surface_t is destroyed or cairo_surface_finish() is called on the surface."

It just keeps pointer internally to the provided image buffer. The data from buffer is copied only in WKImageCreateFromCairoSurface.
Mostly it works because data is copied immediately after return from already deallocated memory block, but I don't think it is right.

[Viatcheslav Ostapenko]

Created attachment 180812 [details]
Patch

[Chris Dumez]

Comment on attachment 180812 [details]
Patch

Sadly you are right. Sorry I missed that. However, I really don't like that we are returning a WKType from our C++ API. How about using copyCairoImageSurface() from CairoUtilities.h instead so that we can keep returning a PassRefPtr<cairo_surface_t>?

[Viatcheslav Ostapenko]

(In reply to comment #2)
> (From update of attachment 180812 [details])
> Sadly you are right. Sorry I missed that. However, I really don't like that we are returning a WKType from our C++ API. How about using copyCairoImageSurface() from CairoUtilities.h instead so that we can keep returning a PassRefPtr<cairo_surface_t>?

Do you mean it should copy image data 2 times?

[Chris Dumez]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 180812 [details] [details])
> > Sadly you are right. Sorry I missed that. However, I really don't like that we are returning a WKType from our C++ API. How about using copyCairoImageSurface() from CairoUtilities.h instead so that we can keep returning a PassRefPtr<cairo_surface_t>?
> 
> Do you mean it should copy image data 2 times?

Right, it will copy the image data twice in the accelerated compositing case. But at least, we would respect layering and the method would be more directly reusable by our public API.

[Viatcheslav Ostapenko]

Created attachment 180819 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 180819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180819&action=review

Nice idea, I like it

> Source/WebKit2/ChangeLog:9
> +        from it create cairo surface 1st and use surface image internal buffer

I would add a comma after 'it'

[Viatcheslav Ostapenko]

Created attachment 180834 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180834 [details]
Patch for landing

Clearing flags on attachment: 180834

Committed r138518: <http://trac.webkit.org/changeset/138518>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 180834 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=180834&action=review

> Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:58
> +    return newSurface;

Shouldn't we call release() here?

[Viatcheslav Ostapenko]

Comment on attachment 180834 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=180834&action=review

>> Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:58
>> +    return newSurface;
> 
> Shouldn't we call release() here?

Not necessary. I see that a lot of code use it this way with release, but it works fine without it. newSurface object will be deallocated in any case, but reference to the pointer will be kept by PassRefPtr object.

[Chris Dumez]

Comment on attachment 180834 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=180834&action=review

>>> Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:58
>>> +    return newSurface;
>> 
>> Shouldn't we call release() here?
> 
> Not necessary. I see that a lot of code use it this way with release, but it works fine without it. newSurface object will be deallocated in any case, but reference to the pointer will be kept by PassRefPtr object.

I know it is not strictly required but it is still good practice to call release() here. By not calling release() you're uselessly ref'ing (when the PassRefPtr is constructed) and then unref'ing (when the RefPtr goes out of scope). I still believe it should be done properly.