939655 - MediaStreamGraphImpl redefines LOG macro used in other .cpp files

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

This bug is spun off form Bug 939582 comment 2. It appears that MediaStreamGraphImpl redefines the LOG macro to do some logging in an inline function. This could have issues depending on when this file is included in a .cpp file since most of the media .cpp files also #define a LOG macro.

We should move the inline function that requires this in the .cpp file and remove the LOG redefinition.

Comment 1

[cajbir (:cajbir)]

Attachment: Fix

Removes the macro from the header file and puts the inline destructor that depended on it in the implementation file.

While doing this I notice that MediaStreamGraph's destructor is non-virtual but MediaStreamGraphImpl is created and passed around as a MediaStreamGraph pointer in places. Is this a problem? Is it ever deleted as a MediaStreamGraph pointer?

Comment 2

[cajbir (:cajbir)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=cd1b0c6ac650

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Double (:doublec) from comment #1)
> While doing this I notice that MediaStreamGraph's destructor is non-virtual
> but MediaStreamGraphImpl is created and passed around as a MediaStreamGraph
> pointer in places. Is this a problem? Is it ever deleted as a
> MediaStreamGraph pointer?

I think it's OK but we should probably make the destructor virtual just to be sure.

Comment 4

[cajbir (:cajbir)]

Attachment: fix

Fix for build error that showed in try server push in comment 2. Carried forward r+. New Try: https://tbpl.mozilla.org/?tree=Try&rev=6a29b57f3120

Comment 5

[cajbir (:cajbir)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> 
> I think it's OK but we should probably make the destructor virtual just to
> be sure.

Spun off as bug 939716.

Comment 6

[cajbir (:cajbir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dfb6c2488f9

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5dfb6c2488f9