Bug 129676 – [EFL] Replace GraphicsContext3D with Evas_GL in AcceleratedCompositingContext.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Hyowon Kim]

As the GraphicsContext3D instance is created by TextureMapperGL, 
this patch removes the uses of GraphicsContext3D for AcceleratedCompositingContext 
and adds an EFL specific context using Evas_GL.

[Hyowon Kim]

Created attachment 225831 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 225831 [details]
Patch

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

> Source/WebKit/efl/ChangeLog:3
> +        [EFL] Replace GraphicsContext3D with Evas_GL in AcceleratedCompositingContext.

Could you explain why we need to replace GraphicsContext3D with Evas_GL ? Any benefit ?

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:69
> +    int width = 0;

Please use Evas_Coord to be used by evas_object_geometry_get()

e.g. Evas_Coord width, height;

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44
> +    bool resize(const IntSize&);

Why should this function be public ?

[Hyowon Kim]

Created attachment 225851 [details]
Patch

[Hyowon Kim]

(In reply to comment #2)
> (From update of attachment 225831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225831&action=review
> 
> > Source/WebKit/efl/ChangeLog:3
> > +        [EFL] Replace GraphicsContext3D with Evas_GL in AcceleratedCompositingContext.
> 
> Could you explain why we need to replace GraphicsContext3D with Evas_GL ? Any benefit ?

TextureMapperGL has its own GraphicsContext3D created by  createForCurrentGLContext(). So AcceleratedCompositingContext doesn't need to create and set it to TextureMapperGL anymore.
But, we should do 'makeCurrentContext' by using a platform specific context before createForCurrentGLContext().
This patch removes GraphicsContext3D related codes in AcceleratedCompositingContext and adds EvasGLContext/Surface to it instead.

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:69
> > +    int width = 0;
> 
> Please use Evas_Coord to be used by evas_object_geometry_get()
> 
> e.g. Evas_Coord width, height;

Done.

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44
> > +    bool resize(const IntSize&);
> 
> Why should this function be public ?

If webview size is changed, we will have to resize the EvasGLSurface.

[Gyuyoung Kim]

Comment on attachment 225851 [details]
Patch

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

> TextureMapperGL has its own GraphicsContext3D created by  createForCurrentGLContext(). So AcceleratedCompositingContext doesn't need to create and set it to  TextureMapperGL anymore.
But, we should do 'makeCurrentContext' by using a platform specific context before createForCurrentGLContext().

It would be good if you write this reason to ChangeLog.


This patch removes GraphicsContext3D related codes in AcceleratedCompositingContext and adds EvasGLContext/Surface to it instead.

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44
> +    bool resize(const IntSize&);

> > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44
> > > +    bool resize(const IntSize&);
> > 
> > Why should this function be public ?
> 
> If webview size is changed, we will have to resize the EvasGLSurface.

I don't see well. So, you may call it from other places in future, right ?

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61
> +    OwnPtr<Evas_GL> m_evasGL;

Please use c++11 (e.g. std::unique_ptr<>) instead of OwnPtr.

[Hyowon Kim]

Created attachment 225944 [details]
Patch

[Hyowon Kim]

(In reply to comment #5)

> It would be good if you write this reason to ChangeLog.

Done. 

> > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44
> > > > +    bool resize(const IntSize&);
> > > 
> > > Why should this function be public ?
> > 
> > If webview size is changed, we will have to resize the EvasGLSurface.
> 
> I don't see well. So, you may call it from other places in future, right ?

Right.
 
> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61
> > +    OwnPtr<Evas_GL> m_evasGL;
> 
> Please use c++11 (e.g. std::unique_ptr<>) instead of OwnPtr.

I think we should file a new bug to replace OwnPtr<> with unique_ptr<> for EFL objects.
I can't use unique_ptr<> for Evas_GL right now, 
because we have to add a custom deleter like deleteOwnedPtr() for OwnPtr<>.
Fixing this is out of this patch's scope, don't you think?

[Hyowon Kim]

Created attachment 225946 [details]
Patch

[Ryuan Choi]

Comment on attachment 225946 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:65
> +    m_evasGLContext = WebKit::EvasGLContext::create(m_evasGL.get());

Hmm, at least, I think that we should fix it before landed this.

EvasGLContext is in WebCore/
I believe that EvasGLContext should be in WebCore namespace.

[Gyuyoung Kim]

Comment on attachment 225946 [details]
Patch

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

r- based on review comments.

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61
> +    OwnPtr<Evas_GL> m_evasGL;

OwnPtr is being replaced by C+11 (e.g. std::unique_ptr) So, it would be good if you start to use it in this patch.

Loot at the meta bug.

Convert OwnPtr and PassOwnPtr uses to std::unique_ptr
https://bugs.webkit.org/show_bug.cgi?id=128007

[Gyuyoung Kim]

(In reply to comment #7)

> > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61
> > > +    OwnPtr<Evas_GL> m_evasGL;
> > 
> > Please use c++11 (e.g. std::unique_ptr<>) instead of OwnPtr.
> 
> I think we should file a new bug to replace OwnPtr<> with unique_ptr<> for EFL objects.
> I can't use unique_ptr<> for Evas_GL right now, 
> because we have to add a custom deleter like deleteOwnedPtr() for OwnPtr<>.
> Fixing this is out of this patch's scope, don't you think?

Ah, I missed to read this comment. If there is additional work to use it, please file a new bug for it.

[Hyowon Kim]

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:65
> > +    m_evasGLContext = WebKit::EvasGLContext::create(m_evasGL.get());
> 
> Hmm, at least, I think that we should fix it before landed this.
> 
> EvasGLContext is in WebCore/
> I believe that EvasGLContext should be in WebCore namespace.

Good point. Done. #129797

[Hyowon Kim]

(In reply to comment #11)
> (In reply to comment #7)
> 
> > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61
> > > > +    OwnPtr<Evas_GL> m_evasGL;
> > > 
> > > Please use c++11 (e.g. std::unique_ptr<>) instead of OwnPtr.
> > 
> > I think we should file a new bug to replace OwnPtr<> with unique_ptr<> for EFL objects.
> > I can't use unique_ptr<> for Evas_GL right now, 
> > because we have to add a custom deleter like deleteOwnedPtr() for OwnPtr<>.
> > Fixing this is out of this patch's scope, don't you think?
> 
> Ah, I missed to read this comment. If there is additional work to use it, please file a new bug for it.

I've added another bug for it. bug 129853

[Hyowon Kim]

Created attachment 226087 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 226087 [details]
Patch

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

LGTM except for my small comment.

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44
> +    bool resize(const IntSize&);

I think it would be good to keep it as *private* until using by other classes.

[Ryuan Choi]

Committed r165314: <http://trac.webkit.org/changeset/165314>

[Ryuan Choi]

Comment on attachment 226087 [details]
Patch

clearing flags. I landed manually after moved resize() to private section like gyuyoung mentioned.