Closed
Bug 938970
Opened 11 years ago
Closed 11 years ago
Build gfx/layers in unified mode
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: bjacob, Assigned: bjacob)
References
Details
(Whiteboard: [qa-])
Attachments
(6 files)
(deleted),
patch
|
mattwoodrow
:
review+
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
Reviewer note: you'll want to check that the static functions that were defined in multiple cpp files, and that are unified by this patch, were actually identical... have lots of fun!
https://tbpl.mozilla.org/?tree=Try&rev=e38d2a88d0ea
R? Matt for gfx/layers code changes, Ehsan for build-system changes.
Attachment #832709 -
Flags: review?(matt.woodrow)
Attachment #832709 -
Flags: review?(ehsan)
Updated•11 years ago
|
Attachment #832709 -
Attachment is patch: true
Attachment #832709 -
Flags: review?(ehsan) → review+
Updated•11 years ago
|
Attachment #832709 -
Flags: review?(matt.woodrow) → review+
Comment 1•11 years ago
|
||
ISurfaceAllocator.cpp error C2872: “SharedMemory”: ambiguous symbol
d:\develop\mozilla\central\gfx\layers\d3d9\LayerManagerD3D9Shaders.h(56) : error
C2370: “LayerQuadVS”: redefinition; different storage class
same to sLayerManagerCount
d:/develop/mozilla/central/gfx/layers\d3d11/CompositorD3D11.cpp(29) : error C201
1: “mozilla::layers::Vertex”:“struct”type redefinition
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Re: comment 1: thanks for the findings, that means that my above try push in comment 2 is going to fail, here is another try push on windows that might succeed: it takes the affected windows-specific source files out of UNIFIED_SOURCES back into regular SOURCES.
https://tbpl.mozilla.org/?tree=Try&rev=83adf89b4c74
Comment 4•11 years ago
|
||
(In reply to Benoit Jacob [:bjacob] from comment #3)
> Re: comment 1: thanks for the findings, that means that my above try push in
> comment 2 is going to fail, here is another try push on windows that might
> succeed: it takes the affected windows-specific source files out of
> UNIFIED_SOURCES back into regular SOURCES.
>
> https://tbpl.mozilla.org/?tree=Try&rev=83adf89b4c74
I forget to mention that I locally remove the opengl code from building. So the try might secceed. But in future, as the code change, the problem can appear.
Assignee | ||
Comment 5•11 years ago
|
||
And another iteration on Mac:
https://tbpl.mozilla.org/?tree=Try&rev=38b22976f3dc
Assignee | ||
Comment 6•11 years ago
|
||
One more for the road on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=0d71800dab1c
Assignee | ||
Comment 7•11 years ago
|
||
We're actually still far away from being able to land. The Mac failures are serious:
In file included from Unified_cpp_gfx_layers1.cpp:15:
In file included from ../../../../gfx/layers/client/CompositableClient.cpp:11:
In file included from ../../dist/include/mozilla/layers/TextureClientOGL.h:17:
In file included from ../../dist/include/mozilla/gfx/MacIOSurface.h:10:
In file included from /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/QuartzCore.framework/Headers/QuartzCore.h:7:
In file included from /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/QuartzCore.framework/Headers/CoreVideo.h:1:
In file included from /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CoreVideo.h:20:
In file included from /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CVReturn.h:21:
In file included from /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CVBase.h:26:
In file included from /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreFoundation.framework/Headers/CFBase.h:48:
/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreServices.framework/Headers/../Frameworks/CarbonCore.framework/Headers/MacTypes.h:501:16: error: reference to 'Point' is ambiguous
typedef struct Point Point;
^
/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreServices.framework/Headers/../Frameworks/CarbonCore.framework/Headers/MacTypes.h:497:8: note: candidate found by name lookup is 'Point'
struct Point {
^
../../dist/include/mozilla/gfx/Point.h:64:34: note: candidate found by name lookup is 'mozilla::gfx::Point'
typedef PointTyped<UnknownUnits> Point;
^
The problem here is that MacIOSurface.h is included in a few headers: TextureClientOGL.h and TextureHostOGL.h and so this problem is not just contained in one or two cpp files.
I think that the right way to fix this is to minimize the number of cpp files that have to include MacIOSurface.h by stopping including it in headers. Then we can move these cpp files back into SOURCES (not UNIFIED_SOURCES).
Assignee | ||
Comment 8•11 years ago
|
||
New try, new hope:
https://tbpl.mozilla.org/?tree=Try&rev=5e58135fb405
Assignee | ||
Comment 9•11 years ago
|
||
woohoohoo!! green! Thanks a bunch to Josh&BenWa for the Mac builds and to zhoubcfan@ for the above tips!
Assignee | ||
Comment 10•11 years ago
|
||
Carrying forward r=ehsan,mattwoodrow
Attachment #8333601 -
Flags: review+
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8333603 -
Flags: review?(bas)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8333604 -
Flags: review?(bas)
Updated•11 years ago
|
Attachment #8333604 -
Flags: review?(bas) → review+
Updated•11 years ago
|
Attachment #8333603 -
Flags: review?(bas) → review+
Assignee | ||
Comment 13•11 years ago
|
||
Indeed, Mac system headers define Size and Point types in the root namespace that collide with our mozilla::gfx:: types as soon as we do |using namespace mozilla::gfx;|.
Since we do that in a lot of files, the easiest solution for now is to just minimize the number of cpp files that have to include these Mac headers, and keep these files out of UNIFIED_SOURCES for now.
Attachment #8333611 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #8333612 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•11 years ago
|
Attachment #8333603 -
Attachment description: 1/3. Some d3d tweaks needed to build with UNIFIED_SOURCES → 3/5. Some d3d tweaks needed to build with UNIFIED_SOURCES
Assignee | ||
Updated•11 years ago
|
Attachment #8333604 -
Attachment description: 2/3. Some gralloc-related tweaks required to build with UNIFIED_SOURCES → 4/5. Some gralloc-related tweaks required to build with UNIFIED_SOURCES
Assignee | ||
Updated•11 years ago
|
Attachment #8333601 -
Attachment description: 3/3. Switch gfx/layers to UNIFIED_SOURCES → 5/5. Switch gfx/layers to UNIFIED_SOURCES
Updated•11 years ago
|
Attachment #8333611 -
Flags: review?(matt.woodrow) → review+
Updated•11 years ago
|
Attachment #8333612 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 15•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/116329598a64
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c60b3513c790
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8931a0bb421
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/08add9705bc1
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/56045f7c969d
Target Milestone: --- → mozilla28
Assignee | ||
Updated•11 years ago
|
Summary: Switch gfx/layers to UNIFIED_SOURCES → Build gfx/layers in unified mode
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/116329598a64
https://hg.mozilla.org/mozilla-central/rev/c60b3513c790
https://hg.mozilla.org/mozilla-central/rev/e8931a0bb421
https://hg.mozilla.org/mozilla-central/rev/08add9705bc1
https://hg.mozilla.org/mozilla-central/rev/56045f7c969d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•