Mir

examples/demo-shell/demo_renderer.cpp (+4/-2)
examples/demo-shell/demo_renderer.h (+1/-1)
examples/demo-shell/demo_shell.cpp (+9/-2)
include/platform/mir/graphics/gl_program_factory.h (+50/-0)
include/server/mir/compositor/gl_renderer.h (+6/-3)
include/server/mir/default_server_configuration.h (+3/-0)
include/server/mir/graphics/gl_program.h (+4/-4)
src/server/compositor/CMakeLists.txt (+0/-1)
src/server/compositor/default_configuration.cpp (+2/-2)
src/server/compositor/gl_renderer.cpp (+16/-14)
src/server/compositor/gl_renderer_factory.cpp (+9/-4)
src/server/compositor/gl_renderer_factory.h (+6/-10)
src/server/graphics/CMakeLists.txt (+2/-0)
src/server/graphics/default_configuration.cpp (+11/-0)
src/server/graphics/gl_program.cpp (+8/-8)
src/server/graphics/program_factory.cpp (+32/-0)
src/server/graphics/program_factory.h (+47/-0)
tests/unit-tests/compositor/test_gl_renderer.cpp (+17/-165)
tests/unit-tests/graphics/CMakeLists.txt (+1/-0)
tests/unit-tests/graphics/test_program_factory.cpp (+211/-0)

To merge this branch:

bzr merge lp://qastaging/~kdub/mir/gl-program-creation-factory

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alberto Aguirre (community)			Approve on 2014-04-22
Alexandros Frantzis (community)			Approve on 2014-04-22
Robert Carr (community)		2014-04-16	Needs Information on 2014-04-21
PS Jenkins bot (community)	continuous-integration		Approve on 2014-04-18
Review via email: mp+216218@code.qastaging.launchpad.net

Commit message

Add a factory that is capable of compiling a vertex shader and a fragment shader into a gl program.

I am doing this so that the android platform can create a gl program that it can use when the overlays fail. I don't intend to expose this program in the platform-independent code, that will continue to use GLRenderer.

A few things were weighed (do we /really/ need another factory?)...

1) calls relating to gl program/shader creation should be serialized which points to a common factory.
2) the GLRendererFactory is a class that is overridable by the user. (see the demo-shell). I didn't want to extend the GLRendererFactory to have a method for creating a separate android-only specific renderer. It seemed better to have a generic factory for making any gl program, and have GLRendererFactory and GLRenderer use that generic factory to accomplish its goals

Description of the change

Add a factory that is capable of compiling a vertex shader and a fragment shader into a gl program.

A few things were weighed (do we /really/ need another factory?)...

note 1:
I also made an attempt to split out testing the gl program setup from the GLRenderer use cases. This one of the oldest tests (and... a mess by 'modern' mir test standards), so it has some cruft that could still use some attention. We'll probably have to continue to improve this test in smaller steps in subsequent MPs.

note 2:
If HWC rejects a surface, we are expected to draw that surface with OpenGLES. We should be doing this in the android platform, so the platform-independent code doesn't have to deal with this.

note 3: (future planning)
I plan on giving the GLProgramFactory down to the android platform so it can make its own HWC gl program.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-17:

FAILED: Continuous integration, rev:1571
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1359/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1651/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1649/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1224/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1091/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1096/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1225/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1359/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-18:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1361/rebuild

review: Approve (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-04-21:

>> 557 + /*
>> 558 + * We need to serialize renderer creation because some GL calls used
>> 559 + * during renderer construction that create unique resource ids
>> 560 + * (e.g. glCreateProgram) are not thread-safe when the threads are
>> 561 + * have the same or shared EGL contexts.
>> 562 + */

Can you explain the synchronization requirements a little here? I got a little confused...it seems like this isn't sufficient.

If this can be called from multiple threads then something has to ensure the eglMakeCurrent is called from the right threads (and held for the duration of GL usage), right? So if something is doing this then the lock guard is not necessary, if nothing else is handling it then its not quite enough....

review: Needs Information

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-04-21:

> >> 557 + /*
> >> 558 + * We need to serialize renderer creation because some GL calls used
> >> 559 + * during renderer construction that create unique resource ids
> >> 560 + * (e.g. glCreateProgram) are not thread-safe when the threads are
> >> 561 + * have the same or shared EGL contexts.
> >> 562 + */
>
> Can you explain the synchronization requirements a little here? I got a little
> confused...it seems like this isn't sufficient.
>
> If this can be called from multiple threads then something has to ensure the
> eglMakeCurrent is called from the right threads (and held for the duration of
> GL usage), right? So if something is doing this then the lock guard is not
> necessary, if nothing else is handling it then its not quite enough....

I would agree that our handling of the context doesn't really model the requirements of using gl contexts and could be improved. (but I'm not looking to improve that in this MP) I'm trying to preserve the guards that are in place so android can make a shader program in addition to the default compositor.

I suppose this guard is fulfilling:
http://www.khronos.org/opengles/sdk/docs/man/xhtml/glCreateShader.xml
"Applications are responsible for providing the synchronization across API calls when objects are accessed from different execution threads."

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-22:

> If this can be called from multiple threads then something has to ensure the eglMakeCurrent
> is called from the right threads (and held for the duration of GL usage), right? So if
> something is doing this then the lock guard is not necessary, if nothing else is handling
> it then its not quite enough....

Not sure I follow, but let me try to explain the reason we need this. If two GL contexts are shared then objects in them live in the same "namespace". Creation of objects in this shared namespace is not thread-safe. So, imagine:

C1: EGL context 1
C2: EGL context 2 shared with context 1

T1: MakeCurrent(C1)
T2: MakeCurrent(C2)

Now if T1 and T2 run concurrently and call glCreateProgram() we have a potential race. What happened in our case was that both T1:glCreateProgram() and T2:glCreateProgram() occasionally returned the same program id, messing things up.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-22:

100 + GLProgramFactory() {};

= default;

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-04-22:

> 100 + GLProgramFactory() {};
> = default;

Looks good otherwise, so pre-approving to unblock.

review: Approve

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-04-22:

> > 100 + GLProgramFactory() {};
> > = default;
>
> Looks good otherwise, so pre-approving to unblock.

thanks! working on change now

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-04-22:

Wrong year?

review: Approve

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-04-22:

Indeed, fixed!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Emanuele Antonio Faraone

Gerry Boland

Kevin DuBois

Mir development team