Last Comment Bug 734784 - Clean up includes in content/media
: Clean up includes in content/media
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla14
Assigned To: Matthew Gregan [:kinetik]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-11 18:05 PDT by Matthew Gregan [:kinetik]
Modified: 2012-03-15 16:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch v0 (4.49 KB, patch)
2012-03-11 18:06 PDT, Matthew Gregan [:kinetik]
cpearce: review+
Details | Diff | Splinter Review
followup patch v0 (1007 bytes, patch)
2012-03-13 21:46 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
followup patch v1 (1005 bytes, patch)
2012-03-14 16:25 PDT, Matthew Gregan [:kinetik]
cpearce: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Matthew Gregan [:kinetik] 2012-03-11 18:05:08 PDT
I ran include-what-you-use (http://code.google.com/p/include-what-you-use/) over content/media while experimenting with clang builds.  I've only used it to remove unneeded headers for now (and have ignored some of the more extreme suggestions), and haven't bothered fixing the situations where we're depending on things included indirectly, as it can make the include list significantly larger.  There's some discussion in bug 634839 debating the merits of enlarging the include list to reduce dependencies.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5de423b0424f
Comment 1 Matthew Gregan [:kinetik] 2012-03-11 18:06:10 PDT
Created attachment 604811 [details] [diff] [review]
patch v0
Comment 2 Chris Pearce (:cpearce) 2012-03-11 18:07:27 PDT
Comment on attachment 604811 [details] [diff] [review]
patch v0

Review of attachment 604811 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Comment 3 Matthew Gregan [:kinetik] 2012-03-11 20:07:37 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/3c82a920d095
Comment 4 Matt Brubeck (:mbrubeck) 2012-03-12 13:38:17 PDT
https://hg.mozilla.org/mozilla-central/rev/3c82a920d095
Comment 5 Takanori MATSUURA 2012-03-13 08:14:55 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> https://hg.mozilla.org/mozilla-central/rev/3c82a920d095

After the checkin, generating libxul.so with system libvpx is failed on Fedora 16 x86_64.

I can build it successful with the changeset c77b00ff2c80 which is the parent of 3c82a920d095.


../../content/media/webm/nsWebMReader.o: In function `~nsWebMReader':
/foo/mozilla-central-3c82a920d095/content/medi/webm/nsWebMReader.cpp:164: undefined reference to `vpx_codec_destroy'
/usr/bin/ld: libxul.so: hidden symbol `vpx_codec_destroy' isn't defined
/usr/bin/ld: final link failed: Bad value
collect2: ld returned 1 exit status
make[5]: *** [libxul.so] Error 1
Comment 6 Takanori MATSUURA 2012-03-13 08:24:24 PDT
BTW, I suppose vpx/vpx_codec.h is double included because vpx/vpx_decoder.h includes it.
Comment 7 Matthew Gregan [:kinetik] 2012-03-13 21:41:12 PDT
At a glance this appears to be caused by me neglecting to add vpx/vpx_codec.h to config/system-headers, but addressing that does not solve the problem.

The symbol is correctly exported from the system libvpx.so:
    66: 0000000000005590   266 FUNC    GLOBAL DEFAULT   11 vpx_codec_control_

But with and without the system-headers fix, the symbol in nsWebMReader.o ends up being hidden:
   578: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN   UND vpx_codec_control_

The preprocessor confirms that the build is correctly generating a system_wrappers versions of vpx/vpx_codec.h and that it is included before the real vpx/vpx_codec.h:

# 1 "../../../dist/system_wrappers/vpx/vpx_codec.h" 1

# 2 "../../../dist/system_wrappers/vpx/vpx_codec.h" 3
#pragma GCC visibility push(default)
# 1 "/home/kinetik/third_party/libvpx-dist/include/vpx/vpx_codec.h" 1 3
# 40 "/home/kinetik/third_party/libvpx-dist/include/vpx/vpx_codec.h" 3

I must be missing something.
Comment 8 Matthew Gregan [:kinetik] 2012-03-13 21:46:01 PDT
Ted, do you have any suggestions on how to further debug this?
Comment 9 Matthew Gregan [:kinetik] 2012-03-13 21:46:39 PDT
Created attachment 605655 [details] [diff] [review]
followup patch v0

Takanori, does the attached patch fix the problem for you?  It partially reverts the change, by including vpx/vpx_decoder.h rather than vpx/vpx_codec.h in nsWebMReader.h.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2012-03-14 05:11:42 PDT
Is vpx_decoder.h including vpx_codec.h directly first? If that's happening the system wrapper version might not be getting included in time, so the visibility would be wrong.
Comment 11 Takanori MATSUURA 2012-03-14 05:21:44 PDT
(In reply to Matthew Gregan [:kinetik] from comment #9)
> Created attachment 605655 [details] [diff] [review]
> followup patch v0
> 
> Takanori, does the attached patch fix the problem for you?

Yes, the build is completed with the attachment 605655 [details] [diff] [review].
Comment 12 Matthew Gregan [:kinetik] 2012-03-14 15:52:24 PDT
(In reply to Ted Mielczarek [:ted] from comment #10)
> Is vpx_decoder.h including vpx_codec.h directly first? If that's happening
> the system wrapper version might not be getting included in time, so the
> visibility would be wrong.

The include order in nsWebMReader.cpp is:
nsWebMReader.h
  vpx_codec.h
vpx_decoder.h
  vpx_codec.h
vp8dx.h

Looking at the preprocessed output confirms that vpx_codec_control_ is correctly placed inside a visibility push(default) pragma produced by the system wrapper, but inspecting nsWebMReader.o with readelf reveals the symbol is still marked as hidden.

Today I ran into the same issue when trying to build the new GStreamer patch in bug 422540 after fixing the system-headers.  It turns out that ccache is the culprit.  In direct mode (the default), it hashes the source file and then uses the hash to find a manifest in the cache, which it reads the include file paths from and then hashes them--so it'll notice that the include files have changed, but *not* that the include file *paths* have changed.  Setting CCACHE_NODIRECT=1 in the environment forces it to use the older/slower mode where it hashes the preprocessed output, which works fine when system-headers changes.
Comment 13 Matthew Gregan [:kinetik] 2012-03-14 16:25:24 PDT
Created attachment 606019 [details] [diff] [review]
followup patch v1

Better fix--update system-headers to wrap vpx_codec.h.

We'll need this on aurora too, since the original patch landed just before the branch.
Comment 14 Matthew Gregan [:kinetik] 2012-03-14 18:05:56 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/049823935141
Comment 15 Matthew Gregan [:kinetik] 2012-03-14 18:07:42 PDT
Comment on attachment 606019 [details] [diff] [review]
followup patch v1

We need this on aurora since the original change landed just before the branch.

[Approval Request Comment]
Regression caused by (bug #): 734784
User impact if declined: Builds using the system libvpx will fail.
Testing completed (on m-c, etc.): Verified locally.
Risk to taking this patch (and alternatives if risky): None.  Trivial build system change.
String changes made by this patch: None.
Comment 16 Ted Mielczarek [:ted.mielczarek] 2012-03-15 04:50:36 PDT
Ah! Crazy. Good work tracking that down. :)
Comment 17 Marco Bonardo [::mak] 2012-03-15 08:25:32 PDT
https://hg.mozilla.org/mozilla-central/rev/049823935141
Comment 18 Alex Keybl [:akeybl] 2012-03-15 14:32:28 PDT
Comment on attachment 606019 [details] [diff] [review]
followup patch v1

[Triage Comment]
Build only change - approved for Aurora 13.
Comment 19 Matthew Gregan [:kinetik] 2012-03-15 16:11:20 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/d42b8ffc7ba0

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