Closed Bug 984071 Opened 8 years ago Closed 8 years ago

Fix unused variable warnings in content/media/webaudio and webrtc

Categories

(Core :: WebRTC: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- wontfix
firefox31 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(3 files)

This patch rearranges some code to fix the following clang warnings when building a release build on OS X:

content/media/webaudio/blink/HRTFPanner.cpp:115:14 [-Wunused-variable] unused variable 'numInputChannels'                                                                   
content/media/webrtc/LoadManager.h:45:11 [-Wunused-private-field] private field 'mLastSystemLoad' is not used

content/media/webrtc/LoadMonitor.h:51:26 [-Wunused-private-field] private field 'mLoadUpdateInterval' is not used

content/media/webrtc/LoadMonitor.h:56:26 [-Wunused-private-field] private field 'mTicksPerInterval' is not used
Attachment #8391813 - Flags: review?(paul)
Comment on attachment 8391813 [details] [diff] [review]
fix-ununused-variable-warnings.patch

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

r+ for the HRTFPanner part, r? gcp for the LoadManager bits.
Attachment #8391813 - Flags: review?(paul)
Attachment #8391813 - Flags: review?(gpascutto)
Attachment #8391813 - Flags: review+
Comment on attachment 8391813 [details] [diff] [review]
fix-ununused-variable-warnings.patch

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

I would recommend a more proper fix, but if necessary I can live with this.

::: content/media/webrtc/LoadMonitor.cpp
@@ +55,5 @@
>      mProcessLoad(0.0f),
>      mLoadNotificationCallback(nullptr)
> +#if defined(ANDROID) || defined(LINUX)
> +    , mLoadUpdateInterval(aLoadUpdateInterval)
> +#endif

I don't entirely like this because you're replacing an unused field by a unused function argument.

Proper fix is probably to #ifdef LoadMonitor initialization in LoadManager and not compile it at all until someone adds Mac OS X support.
Attachment #8391813 - Flags: review?(gpascutto) → review+
(In reply to Gian-Carlo Pascutto (:gcp) from comment #2)
> I don't entirely like this because you're replacing an unused field by a
> unused function argument.
> 
> Proper fix is probably to #ifdef LoadMonitor initialization in LoadManager
> and not compile it at all until someone adds Mac OS X support.

I'll post a new patch with a fix more to your liking. :)
Duplicate of this bug: 987300
As noted in bug 987300, I'm getting build warnings on clang++ Linux for mTicksPerInterval being unused, when compiling LoadManager.cpp.

This is a bit odd, because mTicksPerInterval *is* used *in the file that defines its class's methods* -- LoadMONITOR.cpp. Not sure why Clang is thinking that it should be used in LoadManager.cpp.

This might be an artifact of unified builds, somehow.
{
In file included from /obj/content/media/webrtc/Unified_cpp_content_media_webrtc0.cpp:2:
In file included from /mozilla/content/media/webrtc/LoadManager.cpp:6:
In file included from /mozilla/content/media/webrtc/LoadManager.h:9:
Warning: -Wunused-private-field in /mozilla/content/media/webrtc/LoadMonitor.h: private field 'mTicksPerInterval' is not used
/mozilla/content/media/webrtc/LoadMonitor.h:56:26: warning: private field 'mTicksPerInterval' is not used [-Wunused-private-field]
    uint64_t             mTicksPerInterval;
                         ^
}
LoadMonitor is only implemented on Android and Linux, so AFAICT LoadManager has no purpose because it won't call FireCallbacks(). So skip allocating LoadManager (and compiling LoadManager.cpp) on platforms other than Android and Linux.
Attachment #8399034 - Flags: review?(gpascutto)
(In reply to Chris Peterson (:cpeterson) from comment #8)
> Created attachment 8399034 [details] [diff] [review]
> LoadManager.patch
> 
> LoadMonitor is only implemented on Android and Linux, so AFAICT LoadManager
> has no purpose because it won't call FireCallbacks(). So skip allocating
> LoadManager (and compiling LoadManager.cpp) on platforms other than Android
> and Linux.

I'll note we have someone working on getting load monitoring working on all platforms right now....  We can always back this out, of course.

We also have a bunch of patches in the queue for warning removal (esp in webrtc, but also some in other media sub-dirs)
(In reply to Randell Jesup [:jesup] from comment #9)
> I'll note we have someone working on getting load monitoring working on all
> platforms right now....  We can always back this out, of course.
> 
> We also have a bunch of patches in the queue for warning removal (esp in
> webrtc, but also some in other media sub-dirs)

I don't want to create extra work for anyone. Please r- this change if you feel it is unproductive. This patch is just a small part of my "war on warnings" campaign. :)
Comment on attachment 8399034 [details] [diff] [review]
LoadManager.patch

Let's hold off on this until the LoadManager work is done.

See bug 768968 for a lot of the work underway to clean up webrtc and media in general
Attachment #8399034 - Flags: review?(gpascutto) → review-
Sounds good. I'll watch bug 768968.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → WONTFIX
Comment on attachment 8399034 [details] [diff] [review]
LoadManager.patch

I think it's OK for this to go in, we can gradually remove it as Load Management gets implemented on more platforms, and it avoids dead code. (This should be no surprise as I recommended this approach to you earlier!)

jesup is the module owner, though :)
Attachment #8399034 - Flags: feedback+
I'll go with gcp's assessment here.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attachment #8399034 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/c83c52c332be
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
The following changeset is now in Firefox Nightly:

> c83c52c332be Bug 984071 - Part 2: Only create WebRTC LoadManager on Android and Linux. r=jesup

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
I'm still getting build warnings for mTicksPerInterval and mLastSystemLoad being unused, in my Linux desktop mozilla-central build (using clang 3.4).

$ touch content/media/webrtc/LoadM*r.h
$ ./mach build binaries
[...]
 0:05.06 In file included from /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/obj/content/media/webrtc/Unified_cpp_content_media_webrtc0.cpp:2:
 0:05.06 In file included from /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/content/media/webrtc/LoadManager.cpp:6:
 0:05.06 In file included from /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/content/media/webrtc/LoadManager.h:9:
 0:05.06 Warning: -Wunused-private-field in /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/content/media/webrtc/LoadMonitor.h: private field 'mTicksPerInterval' is not used
 0:05.06 /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/content/media/webrtc/LoadMonitor.h:56:26: warning: private field 'mTicksPerInterval' is not used [-Wunused-private-field]
 0:05.06     uint64_t             mTicksPerInterval;
 0:05.06                          ^
 0:05.06 In file included from /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/obj/content/media/webrtc/Unified_cpp_content_media_webrtc0.cpp:2:
 0:05.06 In file included from /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/content/media/webrtc/LoadManager.cpp:6:
 0:05.06 Warning: -Wunused-private-field in /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/content/media/webrtc/LoadManager.h: private field 'mLastSystemLoad' is not used
 0:05.06 /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/content/media/webrtc/LoadManager.h:45:11: warning: private field 'mLastSystemLoad' is not used [-Wunused-private-field]
 0:05.06     float mLastSystemLoad;
 0:05.06           ^
 0:05.35 2 warnings generated.

Reopening, as I don't think the fix that landed actually fixed this. (It just silenced the warning on Mac OS, AFAICT.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Alternately, I suppose we could leave this closed and un-dupe (i.e. reopen) bug 987300 for the remaining issues.
Let's just leave this reopened. I have a local fix I'm testing (on try).
LoadManager::mLastSystemLoad and LoadMonitor::mTicksPerInterval are actually unused on all platforms, not just OSX.
Attachment #8403021 - Flags: review?(gpascutto)
Attachment #8403021 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/mozilla-central/rev/0b1fdffd07e5
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.