Bug 130072

Summary:

[EFL] Revise AcceleratedCompositingEfl implementation.

Product:

WebKit

Reporter:

Hyowon Kim <hw1008.kim>

Component:

WebKit EFL

Assignee:

Hyowon Kim <hw1008.kim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bunhere, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, ryuan.choi, sergio

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Linux

Bug Depends on:

Bug Blocks:

79766

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Hyowon Kim

Reported 2014-03-11 01:41:22 PDT

I think this would be the last bug to enable accelerated compositing on WK1 efl.

Attachments
Patch (10.33 KB, patch) 2014-03-11 02:07 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
Patch (10.68 KB, patch) 2014-03-11 18:51 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
Patch (11.71 KB, patch) 2014-03-12 03:32 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
Patch (11.92 KB, patch) 2014-03-12 04:25 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Hyowon Kim

Comment 1 2014-03-11 02:07:18 PDT

Created attachment 226399 [details] Patch

Ryuan Choi

Comment 2 2014-03-11 03:04:08 PDT

Comment on attachment 226399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226399&action=review > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:81 > + ewk_view_mark_for_sync(m_view); It's quite complex. What do you think about keeping a reference of compositingObject in AcceleratedCompositingContext ? > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:107 > + return m_rootGraphicsLayer && m_textureMapper; Does we have situation which one of them is valid ? > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:132 > + m_syncTimer.startOneShot(1.0 / 60.0); We'd better to mentione constant value like below const double frameRate = 60;

Hyowon Kim

Comment 3 2014-03-11 18:51:11 PDT

Created attachment 226463 [details] Patch

Hyowon Kim

Comment 4 2014-03-11 18:55:13 PDT

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:81 > > + ewk_view_mark_for_sync(m_view); > > It's quite complex. > > What do you think about keeping a reference of compositingObject in AcceleratedCompositingContext ? All sync requests will call ewk_view_mark_for_sync(). And compositingObject will be removed in next patches. > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:107 > > + return m_rootGraphicsLayer && m_textureMapper; > > Does we have situation which one of them is valid ? Yes, m_rootGraphicsLayer could be 0 while m_textureMapper is vaild. > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:132 > > + m_syncTimer.startOneShot(1.0 / 60.0); > > We'd better to mentione constant value like below > const double frameRate = 60; Done.

Hyowon Kim

Comment 5 2014-03-12 03:32:47 PDT

Created attachment 226490 [details] Patch

Hyowon Kim

Comment 6 2014-03-12 03:35:50 PDT

(In reply to comment #4) > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:81 > > > + ewk_view_mark_for_sync(m_view); > > > > It's quite complex. > > > > What do you think about keeping a reference of compositingObject in AcceleratedCompositingContext ? > > All sync requests will call ewk_view_mark_for_sync(). > And compositingObject will be removed in next patches. > ewkView and compositingObject will be passed to AcceleratedCompositing. Thank you for good comment.

Gyuyoung Kim

Comment 7 2014-03-12 03:59:29 PDT

Hyowon Kim

Comment 8 2014-03-12 04:05:11 PDT

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51 > > + bool enabled(); > > Public function name is too simple :( If it's moved to private, its name is ok?

Gyuyoung Kim

Comment 9 2014-03-12 04:06:19 PDT

Comment on attachment 226490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226490&action=review > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:37 > + OwnPtr<AcceleratedCompositingContext> context = adoptPtr(new AcceleratedCompositingContext(ewkView, compositingObject)); Can't we use c++11 std::unique_ptr ? >> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51 >> + bool enabled(); > > Public function name is too simple :( Ah. this name comes from super class. I see.

Ryuan Choi

Comment 10 2014-03-12 04:07:19 PDT

Comment on attachment 226490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226490&action=review > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:31 > +const double compositingFrameRate = 1.0 / 60.0; FrameRate is 60 :) >> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51 >> + bool enabled(); > > Public function name is too simple :( I agree. how about needComposite() or canComposite()?

Hyowon Kim

Comment 11 2014-03-12 04:25:17 PDT

Created attachment 226494 [details] Patch

Gyuyoung Kim

Comment 12 2014-03-12 04:27:12 PDT

Comment on attachment 226494 [details] Patch LGTM. Ryuan might have a final look before landing.

Hyowon Kim

Comment 13 2014-03-12 04:28:49 PDT

(In reply to comment #9) > (From update of attachment 226490 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226490&action=review > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:37 > > + OwnPtr<AcceleratedCompositingContext> context = adoptPtr(new AcceleratedCompositingContext(ewkView, compositingObject)); > > Can't we use c++11 std::unique_ptr ? Good. Done. > >> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51 > >> + bool enabled(); > > > > Public function name is too simple :( > > Ah. this name comes from super class. I see. enabled() has been replaced with canComposite().

Hyowon Kim

Comment 14 2014-03-12 04:31:34 PDT

(In reply to comment #10) > (From update of attachment 226490 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226490&action=review > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:31 > > +const double compositingFrameRate = 1.0 / 60.0; > > FrameRate is 60 :) Right. Done. > >> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:51 > >> + bool enabled(); > > > > Public function name is too simple :( > > I agree. how about needComposite() or canComposite()? We chose canCompsite(). :) Done.

Ryuan Choi

Comment 15 2014-03-12 21:47:00 PDT

Comment on attachment 226494 [details] Patch Clearing flags on attachment: 226494 Committed r165525: <http://trac.webkit.org/changeset/165525>

Ryuan Choi

Comment 16 2014-03-12 21:47:09 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.