Closed Bug 939582 Opened 7 years ago Closed 7 years ago

Build content/media in unified mode

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

No description provided.
Assignee: nobody → ehsan
Component: General → Video/Audio
Attachment #8333544 - Flags: review?(roc)
Blocks: unified
Comment on attachment 8333544 [details] [diff] [review]
Build content/media in unified mode

>+#ifdef LOG
>+#undef LOG
>+#endif

I'm not a fan of having to add this at the top of every .cpp file before defining LOG. It seems to me that unified builds have the potential to cause a number of issues involving name clashes from things that are expected to be local to a .cpp file. One example is anonymous namespaces and/or static definitions. Code in .cpp files expects this to be local to the compilation unit but unified build extends this past the point that the developer expects a compilation unit to be. An anonymous namespace is now extended to the entire set of unified build files. The potential for clashing #define's like the way we use LOG is another issue.

I think we should refactor the way we use local LOG macros in media code if we are going to do unified build changes. Maybe raise an bug to deal with this if you go through with these changes.


>diff --git a/content/media/MediaStreamGraphImpl.h b/content/media/MediaStreamGraphImpl.h
>index c7d2e87..a709024 100644
>--- a/content/media/MediaStreamGraphImpl.h
>+++ b/content/media/MediaStreamGraphImpl.h
>@@ -14,16 +14,20 @@
> #include "nsIRunnable.h"
> #include "Latency.h"
> 
> namespace mozilla {
> 
> template <typename T>
> class LinkedList;
> 
>+#ifdef LOG
>+#undef LOG
>+#endif
>+
> #ifdef PR_LOGGING
> extern PRLogModuleInfo* gMediaStreamGraphLog;
> #define LOG(type, msg) PR_LOG(gMediaStreamGraphLog, type, msg)
> #else
> #define LOG(type, msg)
> #endif
> 
> /**

This seems to have the potential for  issues depending on where MediaStreamGraphImpl.h is included. Any file that #includes it will have the LOG defined to use gMediaStreamGraphLog. A user could use LOG in a header file after this is included without being aware it's logging to the wrong module. The '#ifdef PR_LOGGING' should be removed from the header. Code in the header that uses it should either expand it manually or have that code moved to non-inline. This LOG should be added to a .cpp where it's needed or the code refactored to do logging in a more sane way.
Attachment #8333544 - Flags: feedback-
Comment on attachment 8333544 [details] [diff] [review]
Build content/media in unified mode

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

I agree with everything doublec said, especially the LOG macro issues.

f-
Attachment #8333544 - Flags: review-
Attachment #8333544 - Flags: review- → feedback-
I'm not sure what you're suggesting that we should do, Chris.  Are you suggesting to not do unified builds of this code?  If not, please let me know what you'd like to see here.

The MediaStreamGraphImpl.h's usage of the LOG macro is not really a problem created by this patch, and can be solved by moving the destructor out of line.
Flags: needinfo?(chris.double)
I would prefer not having the media code unified. If it must be unified then I would like the LOG macro usage cleaned up first so that we don't need the #ifdef/#undef boilerplate. I realise that the LOG usage problem wasn't created by this patch but its unfortunate usage is made worse by unification in my opinion and should be fixed before this is done.
Flags: needinfo?(chris.double)
OK :(
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
[16:46]	roc	doublec: how about if we switched the various uses of LOG to use CACHE_LOG, DECODER_LOG, RESOURCE_LOG, GRAPH_LOG, RTSP_LOG and VTT_LOG and then enabled unified builds? Would that be satisfactory?
[17:00]	doublec	roc: sure, something like that sounds reasonable.
[17:01]	        roc: although the saving over the expanded PR_LOG becomes less

I think that means we can reopen this bug.
Chris, with comment 7 in mind, would you be OK with doing unified builds of this code?  I don't want to write another patch if not.  :-)
Flags: needinfo?(chris.double)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> Chris, with comment 7 in mind, would you be OK with doing unified builds of
> this code?  I don't want to write another patch if not.  :-)

Yes.
Flags: needinfo?(chris.double)
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attachment #8333544 - Attachment is obsolete: true
Attachment #8333544 - Flags: review?(roc)
Comment on attachment 8335667 [details] [diff] [review]
Part 1: Rework the NSPR logging in content/media to use different macro names for different logs; r=roc

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

This patch is very boring, it removes a bunch of macros and renames a bunch of other ones.  Almost nothing interesting to see.  With this patch applied, there is no more LOG macro in content/media (excluding the sub-directories of course.)
Attachment #8335667 - Flags: review?(roc)
Comment on attachment 8335668 [details] [diff] [review]
Part 2: Build content/media in unified mode; r=roc

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

This is the meat.
Attachment #8335668 - Flags: review?(roc)
Comment on attachment 8335667 [details] [diff] [review]
Part 1: Rework the NSPR logging in content/media to use different macro names for different logs; r=roc

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

looks good to me but we need Chris's approval.
Attachment #8335667 - Flags: review?(roc)
Attachment #8335667 - Flags: review?(chris.double)
Attachment #8335667 - Flags: feedback+
Comment on attachment 8335668 [details] [diff] [review]
Part 2: Build content/media in unified mode; r=roc

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

looks good to me but we need Chris's approval.
Attachment #8335668 - Flags: review?(roc)
Attachment #8335668 - Flags: review?(chris.double)
Attachment #8335668 - Flags: feedback+
Attachment #8335667 - Flags: review?(chris.double) → review+
Attachment #8335668 - Flags: review?(chris.double) → review+
https://hg.mozilla.org/mozilla-central/rev/47e13023b4b7
https://hg.mozilla.org/mozilla-central/rev/a4262c9397d9
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.