Closed Bug 1402334 Opened 7 years ago Closed 7 years ago

Fix use of duplicate symbols in signaling code which prevents us from using unified build

Categories

(Core :: WebRTC, enhancement, P3)

Product:
Core
Component:
WebRTC
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

Details

Attachments

(5 files, 2 obsolete files)

Bug 1402334 - Build media-conduit and peerconnection using unified build;
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1402334 - Add log level test to CSFLog;
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1402334 - Build mediapipeline using unified build;
59 bytes, text/x-review-board-request
drno
: review+
Details
Fix static analysis warning
1.63 KB, patch
drno
: review+
Details | Diff | Splinter Review
Fix bustage v2
2.33 KB, patch
jesup
: review+
Details | Diff | Splinter Review 
The culprits are: logTag in peerconnection and sdp and getLogModule in mediapipeline. Fixing this will allow us to build these files using unified build which should speed things up.
Status: NEW → ASSIGNED
Comment on attachment 8914880 [details]
Bug 1402334 - Build media-conduit and peerconnection using unified build;

https://reviewboard.mozilla.org/r/185800/#review191192

r+, but....

I'd prefer something that used CPP magic to avoid some of this junk
static const char pciLogTag "..."
#define LOGTAG pciLogTag
...
CSFLogDebug(LOGTAG, ....)

Even better (though more work) would be to define macros like
#define LogDebug(...) CSFLogDebug(LOGTAG, ...)
(though that syntax doesn't work, and it may be that defining them to do this is a pain, or requires LOG(("xxxxxx", args)) type of macro)
Attachment #8914880 - Flags: review?(rjesup) → review+
Comment on attachment 8914881 [details]
Bug 1402334 - Add log level test to CSFLog;

https://reviewboard.mozilla.org/r/185802/#review191232

::: media/webrtc/signaling/src/common/browser_logging/CSFLog.h:41
(Diff revision 1)
>  #endif
>  ;
>  
>  void CSFLogV( CSFLogLevel priority, const char* sourceFile, int sourceLine, const char* tag , const char* format, va_list args);
>  
> +int CSFLogTestLevel(CSFLogLevel priority);

Not a big fan of the name. Test made me think of a unit test first. But since it's only exposing the underlying test function I guess it's okay.
Attachment #8914881 - Flags: review?(drno) → review+
Comment on attachment 8914882 [details]
Bug 1402334 - Build mediapipeline using unified build;

https://reviewboard.mozilla.org/r/185804/#review191242

::: media/webrtc/signaling/src/mediapipeline/RtpLogger.cpp
(Diff revision 1)
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Original author: nohlmeier@mozilla.com
>  
>  #include "RtpLogger.h"
> -#include "logging.h"

No '#include "CSFLog.h"' needed here?

::: media/webrtc/signaling/src/mediapipeline/moz.build:20
(Diff revision 1)
>      '/media/webrtc/trunk',
>      '/netwerk/srtp/src/crypto/include',
>      '/netwerk/srtp/src/include',
>  ]
>  
>  # Duplicate definition of getLogModule

Does this comment still apply?
Attachment #8914882 - Flags: review?(drno) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4eb36d1710ca
Build media-conduit and peerconnection using unified build; r=jesup
https://hg.mozilla.org/integration/autoland/rev/de7bd38908da
Add log level test to CSFLog; r=drno
https://hg.mozilla.org/integration/autoland/rev/978b0400ac3b
Build mediapipeline using unified build; r=drno
Backed out for bustage at media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:609: 'logTag' was not declared in this scope:

https://hg.mozilla.org/integration/autoland/rev/1d9045285aa9a481f4d115153991178a8485fc53
https://hg.mozilla.org/integration/autoland/rev/0435ae6ad52982e974037f74db0d87e1333f8770
https://hg.mozilla.org/integration/autoland/rev/f2040cc62aba75bc1fef1533474c8b957e44e4dd

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=978b0400ac3b1bdbb074c3a759144caec58ba90c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=134953015&repo=autoland

[task 2017-10-04T17:10:23.975Z] 17:10:23     INFO -  In file included from /builds/worker/workspace/build/src/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:5:0,
[task 2017-10-04T17:10:23.976Z] 17:10:23     INFO -                   from /builds/worker/workspace/build/src/obj-firefox/media/webrtc/signaling/src/media-conduit/Unified_cpp_src_media-conduit0.cpp:2:
[task 2017-10-04T17:10:23.976Z] 17:10:23     INFO -  /builds/worker/workspace/build/src/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp: In member function 'virtual mozilla::MediaConduitErrorCode mozilla::WebrtcAudioConduit::EnableMIDExtension(bool, uint8_t)':
[task 2017-10-04T17:10:23.976Z] 17:10:23     INFO -  /builds/worker/workspace/build/src/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:609:15: error: 'logTag' was not declared in this scope
[task 2017-10-04T17:10:23.976Z] 17:10:23     INFO -     CSFLogDebug(logTag,  "%s %d %d ", __FUNCTION__, enabled, id);
[task 2017-10-04T17:10:23.976Z] 17:10:23     INFO -                 ^
[task 2017-10-04T17:10:23.976Z] 17:10:23     INFO -  /builds/worker/workspace/build/src/media/webrtc/signaling/src/common/browser_logging/CSFLog.h:24:84: note: in definition of macro 'CSFLogDebug'
[task 2017-10-04T17:10:23.977Z] 17:10:23     INFO -   #define CSFLogDebug(tag , format, ...) CSFLog(CSF_LOG_DEBUG, __FILE__ , __LINE__ , tag , format , ## __VA_ARGS__ )
[task 2017-10-04T17:10:23.977Z] 17:10:23     INFO -                                                                                      ^~~
[task 2017-10-04T17:10:23.977Z] 17:10:23     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1075: recipe for target 'Unified_cpp_src_media-conduit0.o' failed
[task 2017-10-04T17:10:23.977Z] 17:10:23     INFO -  gmake[5]: *** [Unified_cpp_src_media-conduit0.o] Error 1
Flags: needinfo?(dminor)
Sorry, looks like another use of logTag got added while I was working on this. I'll do a manual rebase and build on top of inbound and try again.
Flags: needinfo?(dminor)
To be safe, I'll wait until tomorrow morning after the autoland repo has been merged back to inbound, as the new use of logTag only seems to be there.
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/807ccf63a12f
Build media-conduit and peerconnection using unified build; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/0712e0bd9e12
Add log level test to CSFLog; r=drno
https://hg.mozilla.org/integration/mozilla-inbound/rev/808190cb16d0
Build mediapipeline using unified build; r=drno
Depends on: 1408582
Hi Dan

I see the build error in my macbook with macos 10.11.6. Should I check my building environment?

 0:23.80 In file included from /Users/seanlee/Projects/mozilla/src/gecko/obj-x86_64-apple-darwin15.6.0/media/webrtc/signaling/src/media-conduit/Unified_cpp_src_media-conduit0.cpp:29:
 0:23.80 /Users/seanlee/Projects/mozilla/src/gecko/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:34:10: fatal error: 'webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h' file not found
 0:23.80 #include "webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h"
 0:23.80          ^

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #16)
> https://hg.mozilla.org/mozilla-central/rev/807ccf63a12f
I check out to the above commit's parent[1], and the error is gone.

[1] https://hg.mozilla.org/mozilla-central/rev/eab45e291bc0
Flags: needinfo?(dminor)
I'm guessing that the switch to unified build has somehow broken this #ifdef:
#if defined(MAC_OS_X_VERSION_10_8) && \
  (MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_8)
// XXX not available in Mac 10.7 SDK
#include "webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h"
#endif

I'm not sure why that would happen, I'll try to duplicate on my macbook. Sorry for the inconvenience.
Attached patch Fix static analysis warning (obsolete) — — Splinter Review 

        Attachment #8918834 -
        Flags: review?(drno)

    
    

    
  
(In reply to Dan Minor [:dminor] from comment #18)
> I'm guessing that the switch to unified build has somehow broken this #ifdef:
> #if defined(MAC_OS_X_VERSION_10_8) && \
>   (MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_8)
> // XXX not available in Mac 10.7 SDK
> #include "webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h"
> #endif
> 
> I'm not sure why that would happen, I'll try to duplicate on my macbook.
> Sorry for the inconvenience.

Note that local builds may be different that automation builds, depending on what SDK you have installed.  We *just* switched automation to 10.11 SDK I believe from 10.7 (in the last few weeks).

    
    

    
  
Comment on attachment 8918834 [details] [diff] [review]
Fix static analysis warning

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

Concern about the perf impact (at a micro level)

::: media/webrtc/signaling/src/common/browser_logging/CSFLog.cpp
@@ +42,5 @@
>    printf("%s\n:",tag);
>    vprintf(format, args);
>  #else
>  
> +  mozilla::LogLevel level = CSFLogLevelToMozLogLevel(priority);

Wouldn't = static_cast<mozilla::LogLevel>(static_cast<unsigned int>(priority)) bypass this without introducing a function?  Admittedly a hopefully-inlined function you can hope the compiler makes into an array access -- but it will still have to add branches to *every* use of CSFLog.

Or better, get rid of the separate logging levels and typedef CSFLogLevel to mozilla::LogLevel, and #define the CSF_LOG_* entries to LogLevel entries.  This would make it dependent on LogLevel/Logging.h --- but it is already at the cpp level, this would just add a dependency at the .h level.

    
    

    
  
(In reply to Randell Jesup [:jesup] from comment #21)
> Comment on attachment 8918834 [details] [diff] [review]
> Fix static analysis warning
> 
> Review of attachment 8918834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Concern about the perf impact (at a micro level)
> 
> ::: media/webrtc/signaling/src/common/browser_logging/CSFLog.cpp
> @@ +42,5 @@
> >    printf("%s\n:",tag);
> >    vprintf(format, args);
> >  #else
> >  
> > +  mozilla::LogLevel level = CSFLogLevelToMozLogLevel(priority);
> 
> Wouldn't = static_cast<mozilla::LogLevel>(static_cast<unsigned
> int>(priority)) bypass this without introducing a function?  Admittedly a
> hopefully-inlined function you can hope the compiler makes into an array
> access -- but it will still have to add branches to *every* use of CSFLog.
> 
> Or better, get rid of the separate logging levels and typedef CSFLogLevel to
> mozilla::LogLevel, and #define the CSF_LOG_* entries to LogLevel entries. 
> This would make it dependent on LogLevel/Logging.h --- but it is already at
> the cpp level, this would just add a dependency at the .h level.

We include this header from C code, so unfortunately typedefing CSFLogLevel to mozilla::LogLevel won't work. I can try the double cast and see if that is enough to fool the static analysis.

    
    

    
  
(In reply to Sean Lee [:seanlee][:weilonge] from comment #17)
> Hi Dan
> 
> I see the build error in my macbook with macos 10.11.6. Should I check my
> building environment?
> 
>  0:23.80 In file included from
> /Users/seanlee/Projects/mozilla/src/gecko/obj-x86_64-apple-darwin15.6.0/
> media/webrtc/signaling/src/media-conduit/Unified_cpp_src_media-conduit0.cpp:
> 29:
>  0:23.80
> /Users/seanlee/Projects/mozilla/src/gecko/media/webrtc/signaling/src/media-
> conduit/VideoConduit.cpp:34:10: fatal error:
> 'webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h' file not found
>  0:23.80 #include
> "webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h"
>  0:23.80          ^
> 
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #16)
> > https://hg.mozilla.org/mozilla-central/rev/807ccf63a12f
> I check out to the above commit's parent[1], and the error is gone.
> 
> [1] https://hg.mozilla.org/mozilla-central/rev/eab45e291bc0


Hi Sean,

I'm not able to reproduce this problem on my macbook, but I had to go through a large build environment update prior to being able to build. As Randell said, we now require the 10.11 SDK to build in automation, which is probably the update I had to apply on my machine. Maybe try updating your build environment and see if the problem persists?

Sorry,

Dan
Flags: needinfo?(dminor)

    
    

    
  


  

        Attachment #8918834 -
        Attachment is obsolete: true

        Attachment #8918834 -
        Flags: review?(drno)

        Attachment #8918940 -
        Flags: review?(drno)

    
    

    
  
I cannot build after landing this too when I use 10.11 SDK.

Maybe, before landing this, comment #18 condition isn't built.  And if building it, #include "webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h" is always error.  (correct path is webrtc/common_video/include/corevideo_frame_buffer.h)

    
    

    
  
And VideoFrame class has no nativeHandle member.  Maybe, it is frame.video_frame_bufer()->native_handle() instead.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix bustage (obsolete)
        — 
          — Splinter Review
      

    


  
3 issues
- include path (webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h) is invalid.
- webrtc namespace is missing
- VideoFrame class has no nativeHandle member.

I think that this condition's code isn't built unfortunately until using unified build.

        Attachment #8919175 -
        Flags: review?(rjesup)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix bustage v2
        — 
          — Splinter Review
      

    


  

        Attachment #8919175 -
        Attachment is obsolete: true

        Attachment #8919175 -
        Flags: review?(rjesup)

        Attachment #8919183 -
        Flags: review?(rjesup)

    
    

    
  
If we use MAC_OS_X_VERSION_10_8, it requires AvailabilityMacros.h.

    
    

    
  
(In reply to Makoto Kato [:m_kato] from comment #27)
> Created attachment 8919175 [details] [diff] [review]
> Fix bustage
> 
> 3 issues
> - include path (webrtc/sdk/objc/Framework/Classes/corevideo_frame_buffer.h)
> is invalid.
> - webrtc namespace is missing
> - VideoFrame class has no nativeHandle member.
> 
> I think that this condition's code isn't built unfortunately until using
> unified build.

Thank you for putting together a patch for this.

    
    

    
  
Comment on attachment 8919183 [details] [diff] [review]
Fix bustage v2

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

r+ with needed ifdefs

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +28,5 @@
>  #include "webrtc/modules/video_coding/codecs/vp9/include/vp9.h"
>  #include "webrtc/common_video/include/video_frame_buffer.h"
>  #include "webrtc/api/video/i420_buffer.h"
> +
> +#include <AvailabilityMacros.h>

Is this really available on Windows and Linux/etc?  I think this needs an ifdef XP_MACOSX or some such

        Attachment #8919183 -
        Flags: review?(rjesup) → review+

    
    

    
      
  

    
    

    
  
(In reply to Randell Jesup [:jesup] from comment #31)
> Comment on attachment 8919183 [details] [diff] [review]
>
> Is this really available on Windows and Linux/etc?  I think this needs an
> ifdef XP_MACOSX or some such

Thanks, I forget it for non-Mac.  I add #ifdef WEBRTC_MAC

    
    

    
  
Comment on attachment 8918940 [details] [diff] [review]
Fix static analysis warning

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

Not pretty, but should do the job.

        Attachment #8918940 -
        Flags: review?(drno) → review+

    
    

    
  
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7077cf15e4
Fix static analysis warning in CSFLog.cpp; r=drno

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: