Create unified GL Wrapper

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bas, Assigned: bas)

Tracking

Trunk
mozilla1.9.3a5
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
We currently have 2 GL wrappers, one in Layers and one in WebGL. We should create a single wrapper inside thebes which can be used by all code in the tree.
(Assignee)

Comment 1

8 years ago
Created attachment 440377 [details] [diff] [review]
Part 1: Add Unified GL Wrapper
[Checkin: Comment 6]
Attachment #440377 - Flags: review?(vladimir)
This is mostly ok, with a few comments:

- WGLLibrary should be a LibrarySymbolLoader.  No point in manually calling getprocaddress for everything, especially when there are EXT/ARB versions of things.

- There's a lot of incomplete stuff -- e.g. createpbuffer needs to take a lot of context config info, but I guess that can be fleshed out afterwards.

this is probably fine to check in though, and we can expand it as needed.
(Assignee)

Comment 3

8 years ago
Created attachment 441211 [details] [diff] [review]
Part 2: Make Layers use new Unified OpenGL

This will make layers use the new Unified OpenGL architecture.
Attachment #441211 - Flags: review?(vladimir)
(Assignee)

Comment 4

8 years ago
Created attachment 441621 [details] [diff] [review]
Part 2: Make Layers use new Unified OpenGL v2
[Checkin: Comment 6]

Updated according to IRC discussion.
Attachment #441211 - Attachment is obsolete: true
Attachment #441621 - Flags: review?(vladimir)
Attachment #441211 - Flags: review?(vladimir)
Created attachment 441707 [details] [diff] [review]
OSX backend, and relevant cocoa widget changes to enable usage

The pixel buffer options will almost definitely need adjusting. Just setting the accelerated flags appears to work for now.
Attachment #441707 - Flags: review?(vladimir)
Bas checked in these patches. Lets do followup work in new bugs.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
FTR I checked in a couple of bustage fixes for non-libxul builds, because the patch added publicly exported functions but didn't have THEBES_API on them.

http://hg.mozilla.org/mozilla-central/rev/0723bab9f15d
http://hg.mozilla.org/mozilla-central/rev/29a6a85fab8e

Comment 8

8 years ago
Static Thunderbird and SeaMonkey builds on Linux are still busted from this landing, see e.g. http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1272381729.1272382167.28813.gz#err0

Comment 9

8 years ago
Oh, and the bustage on shared SeaMonkey and Thunderbird builds on Windows seems to be from this as well, see http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1272387772.1272388191.16315.gz and http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1272381159.1272386635.12140.gz for examples.
(Assignee)

Comment 10

8 years ago
One way to fix the windows bustage as far as I can see would be to move layers into thebes.dll, but I'm not sure if that's desirable? Otherwise we'd have to export the symbols from thebes.dll. Roc or Ted have any thoughts on this?
I don't really care what you do with non-libxul builds, since we don't ship Firefox like that. We haven't unsupported it yet because Thunderbird and SeaMonkey can't build in the libxul configuration, so they can only do static or shared, and they build shared builds in order to build with tests. I'd suggest doing the right thing for libxul builds, and then doing whatever hackery you need to make shared builds work.
I think moving layers into thebes.dll would be OK if it helps.
I've been thinking over the last day or so that building layers thebes and maybe ycbcr would be best linked as one lib in shared configuration. Probably a topic for another bug: could we even go as far as all dirs in gfx as one lib?
(Assignee)

Comment 14

8 years ago
(In reply to comment #13)
> I've been thinking over the last day or so that building layers thebes and
> maybe ycbcr would be best linked as one lib in shared configuration. Probably a
> topic for another bug: could we even go as far as all dirs in gfx as one lib?

I can live with that, gfx isn't that big, no advantage to having a seperate tiny Layers DLL as far as I can see.

Comment 15

8 years ago
I'd love to tell you non-libxul is bogus for 1.9.3, but as hard as I'm pushing for it, I only believe it once we're there. ;-)

In the mean time, we have a busted Linux static and Windows shared builds on SeaMonkey and a red and basically closed tree one week before cutting an Alpha, you can imagine how uncomfortable I feel with that as a project manager...

So, as long as you can fix it fast, I'm very happy with any way you can fix it (unfortunately I have no clue about C++ code myself).
(Assignee)

Comment 16

8 years ago
So fundamentally the problem here is that Layers is re-using THEBES_API. THEBES_API is declared as dllexport or dllimport depending on whether IMPL_THEBES is defined. However THEBES_API is ment to be used for elements in the thebes module. Since we use it in layers, we cannot import anything from thebes, since we will __declspec(dllimport) it.

There's two solutions as far as I can see:

1. Define LAYERS_API and IMPL_LAYERS
2. Change compile order and include layers in thebes.dll
(Assignee)

Comment 17

8 years ago
Created attachment 442011 [details] [diff] [review]
Include layers in thebes module

This should fix buildbustage on non-libxul builds my moving layers into the thebes module.
Attachment #442011 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 18

8 years ago
Created attachment 442022 [details] [diff] [review]
Include layers in thebes module v2
[Checkin: Comment 22]

After clobbering and try server builds coming back it turns out previous patch had issues. The new patch changes around more and forces thebes.dll to export the dllexport interfaces from layers using gfxDllDeps.cpp.
Attachment #442011 - Attachment is obsolete: true
Attachment #442022 - Flags: review?(ted.mielczarek)
Attachment #442011 - Flags: review?(ted.mielczarek)
Comment on attachment 442022 [details] [diff] [review]
Include layers in thebes module v2
[Checkin: Comment 22]

>diff --git a/gfx/thebes/src/gfxDllDeps.cpp b/gfx/thebes/src/gfxDllDeps.cpp
>new file mode 100644
>--- /dev/null
>+++ b/gfx/thebes/src/gfxDllDeps.cpp
>@@ -0,0 +1,4 @@
>+#include "Layers.h"
>+#include "LayerManagerOGL.h"
>+#include "BasicLayers.h"
>+#include "ImageLayers.h"
WARNING - MS-DOS LINE ENDINGS!

Comment 20

8 years ago
(In reply to comment #16)
> 2. Change compile order and include layers in thebes.dll

I'm very much for that, probably should include ycbcr as well (can go for a followup) as we needed some ugly hack there recently to make it compile everywhere in static builds.
(Assignee)

Comment 21

8 years ago
(In reply to comment #19)
> (From update of attachment 442022 [details] [diff] [review])
> >diff --git a/gfx/thebes/src/gfxDllDeps.cpp b/gfx/thebes/src/gfxDllDeps.cpp
> >new file mode 100644
> >--- /dev/null
> >+++ b/gfx/thebes/src/gfxDllDeps.cpp
> >@@ -0,0 +1,4 @@
> >+#include "Layers.h"
> >+#include "LayerManagerOGL.h"
> >+#include "BasicLayers.h"
> >+#include "ImageLayers.h"
> WARNING - MS-DOS LINE ENDINGS!

Thanks! Will fix.

The patch passes try btw, so as soon as there's a green light from Ted we can land this.
Blocks: 562311
Attachment #442022 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 23

8 years ago
Created attachment 442237 [details] [diff] [review]
Build src/thebes after thebes
[Checkin: See comment 25]

src/thebes has dependencies on thebes, since we moved src to building before thebes. Because of the new dependencies thebes has on src. We need src/thebes to build after thebes now. We probably should restructure all this, but this should work as a quick fix to get all non-libxul builds going again.
Attachment #442237 - Flags: review?(ted.mielczarek)
Comment on attachment 442237 [details] [diff] [review]
Build src/thebes after thebes
[Checkin: See comment 25]

-DIRS        = thebes
+DIRS        = $(NULL)

Just remove this line.
Attachment #442237 - Flags: review?(ted.mielczarek) → review+

Updated

8 years ago
Depends on: 562664

Updated

8 years ago
Depends on: 564026
Attachment #442237 - Attachment description: Build src/thebes after thebes → Build src/thebes after thebes [Checkin: See comment 25]
Attachment #442022 - Attachment description: Include layers in thebes module v2 → Include layers in thebes module v2 [Checkin: Comment 22]
Attachment #440377 - Attachment description: Part 1: Add Unified GL Wrapper → Part 1: Add Unified GL Wrapper [Checkin: Comment 6]
Attachment #441621 - Attachment description: Part 2: Make Layers use new Unified OpenGL v2 → Part 2: Make Layers use new Unified OpenGL v2 [Checkin: Comment 6]
Attachment #441707 - Attachment description: OSX backend, and relevant cocoa widget changes to enable usage → OSX backend, and relevant cocoa widget changes to enable usage [Checkin: Comment 6]
Attachment #441707 - Attachment description: OSX backend, and relevant cocoa widget changes to enable usage [Checkin: Comment 6] → OSX backend, and relevant cocoa widget changes to enable usage
Blocks: 564606
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.