(no longer active) Assignee
	Description • 11 years ago

      No description provided.

	cajbir (:cajbir)
	Comment 2 • 11 years ago

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.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 3 • 11 years ago

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-

	(no longer active) Assignee
	Comment 4 • 11 years ago

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.

	cajbir (:cajbir)
	Comment 5 • 11 years ago

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.

	(no longer active) Assignee
	Comment 6 • 11 years ago

OK :(

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 11 years ago

[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.

	(no longer active) Assignee
	Comment 8 • 11 years ago

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.  :-)

	cajbir (:cajbir)
	Comment 9 • 11 years ago

(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.

	(no longer active) Assignee
	Comment 12 • 11 years ago

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.)

	(no longer active) Assignee
	Comment 13 • 11 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 11 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 15 • 11 years ago

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.

	(no longer active) Assignee
	Comment 16 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/af472e590088
https://hg.mozilla.org/integration/mozilla-inbound/rev/2079c828645a

	(no longer active) Assignee
	Comment 17 • 11 years ago

The patches had rotten :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf4cc082531

	(no longer active) Assignee
	Comment 18 • 11 years ago

Trying again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/47e13023b4b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4262c9397d9

	Carsten Book [:Tomcat]
	Comment 19 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/47e13023b4b7
https://hg.mozilla.org/mozilla-central/rev/a4262c9397d9