Bug 167096

Summary: Move TextureMapper-specific logic out of GraphicsContext3DPrivate
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, gyuyoung.kim, mcatanzaro, ossy, pvollan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Patch for landing
none
Patch for landing none

Description Zan Dobersek 2017-01-16 10:40:10 PST
Move TextureMapper-specific logic out of GraphicsContext3DPrivate
Comment 1 Zan Dobersek 2017-01-16 10:42:33 PST
Created attachment 298971 [details]
WIP
Comment 2 WebKit Commit Bot 2017-01-16 10:46:04 PST
Attachment 298971 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zan Dobersek 2017-01-16 10:46:24 PST
(In reply to comment #1)
> Created attachment 298971 [details]
> WIP

Based on proposal from https://bugs.webkit.org/show_bug.cgi?id=166968#c4.

Alex, I need some input -- this basically moves the GraphicsContext3DPrivate logic into a TextureMapper-specific class, leaving the former to only work with a GLContext object on non-TextureMapper ports that use GraphicsContext3DCairo.

Is this the desirable way of getting PlatformLayer interface out of the GraphicsContext3DPrivate class? Can the GraphicsContext3DPrivate class be potentially removed?
Comment 4 Alex Christensen 2017-01-17 10:22:11 PST
I think this is a step in the right direction.  I think GraphicsContext3DPrivate is an unnecessary and excessive layer of abstraction.
Comment 5 Zan Dobersek 2017-01-20 09:54:23 PST
Created attachment 299350 [details]
Patch
Comment 6 Zan Dobersek 2017-01-20 09:55:02 PST
(In reply to comment #5)
> Created attachment 299350 [details]
> Patch

I'm uploading this up for review, but it might still need some additional tweaking for the EFL port.
Comment 7 Michael Catanzaro 2017-01-28 12:44:15 PST
(In reply to comment #4)
> I think this is a step in the right direction.  I think
> GraphicsContext3DPrivate is an unnecessary and excessive layer of
> abstraction.

Do you want to do the formal review?
Comment 8 Alex Christensen 2017-01-30 17:51:34 PST
Comment on attachment 299350 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:103
> +#if USE(TEXTURE_MAPPER)
> +    m_texmapLayer = std::make_unique<TextureMapperGC3DPlatformLayer>(*this, renderStyle);
> +#else
> +    m_private = std::make_unique<GraphicsContext3DPrivate>(this, renderStyle);
> +#endif

Are there platforms that use Cairo but not TEXTURE_MAPPER?  Maybe we could remove even more code.
Comment 9 Zan Dobersek 2017-01-31 04:59:36 PST
(In reply to comment #8)
> Comment on attachment 299350 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299350&action=review
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:103
> > +#if USE(TEXTURE_MAPPER)
> > +    m_texmapLayer = std::make_unique<TextureMapperGC3DPlatformLayer>(*this, renderStyle);
> > +#else
> > +    m_private = std::make_unique<GraphicsContext3DPrivate>(this, renderStyle);
> > +#endif
> 
> Are there platforms that use Cairo but not TEXTURE_MAPPER?  Maybe we could
> remove even more code.

I would gladly remove it, but the EFL port still uses m_private and even implements a separate GraphicsContext3DPrivate class.

I can polish this patch enough that it builds for the EFL port, and land it, but I won't have time to modify anything further on the EFL side of things.
Comment 10 Alex Christensen 2017-02-01 12:51:19 PST
Ok, but I think it's probably a bad idea to wait for people to move EFL code to where it ought to be.  Nobody is working on architecture improvements on EFL.
Comment 11 Zan Dobersek 2017-02-03 23:03:38 PST
Created attachment 300605 [details]
Patch for landing
Comment 12 Zan Dobersek 2017-02-03 23:55:53 PST
Created attachment 300606 [details]
Patch for landing
Comment 13 Zan Dobersek 2017-02-04 13:03:39 PST
(In reply to comment #12)
> Created attachment 300606 [details]
> Patch for landing

I'll be landing this patch tomorrow, now that it builds on all platforms.

I didn't go and rip out GraphicsContext3DPrivate completely since it's still used by PLATFORM(WIN) && !USE(CAIRO) code here:
https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp#L376

Mac's GraphicsContext3DPrivate is no-op and could be removed easily. The PLATFORM(WIN) && !USE(CAIRO) configuration will need a replacement. I'm CC-ing EFL people if they want to take a look, but at minimum the m_private member could be made EFL-specific.
Comment 14 Zan Dobersek 2017-02-05 09:22:34 PST
(In reply to comment #13)
> I'm CC-ing EFL people if they want to take a look, but at minimum the
> m_private member could be made EFL-specific.
Comment 15 Zan Dobersek 2017-02-05 09:24:16 PST
Comment on attachment 300606 [details]
Patch for landing

Clearing flags on attachment: 300606

Committed r211681: <http://trac.webkit.org/changeset/211681>
Comment 16 Zan Dobersek 2017-02-05 09:24:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Csaba Osztrogonác 2017-02-06 00:42:16 PST
(In reply to comment #15)
> Comment on attachment 300606 [details]
> Patch for landing
> 
> Clearing flags on attachment: 300606
> 
> Committed r211681: <http://trac.webkit.org/changeset/211681>

FYI, it broke the WinCairo build:

..\..\Source\WebCore\platform\graphics\texmap\TextureMapperGC3DPlatformLayer.cpp(110): error C2039: 'glBindFramebuffer': is not a member of '`global namespace''
..\..\Source\WebCore\platform\graphics\texmap\TextureMapperGC3DPlatformLayer.cpp(110): error C3861: 'glBindFramebuffer': identifier not found
ninja: build stopped: subcommand failed.
Comment 18 Alex Christensen 2017-02-06 00:53:30 PST
r211718