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

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

Attachment: fix-ununused-variable-warnings.patch

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

Comment 1

[Paul Adenot (:padenot)]

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.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 3

[Chris Peterson [:cpeterson]]

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

Comment 4

[Chris Peterson [:cpeterson]]

HRTFPanner fix without LoadManager:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ffb7a5bdcc

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/24ffb7a5bdcc

Comment 7

[Daniel Holbert [:dholbert]]

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;
                         ^
}

Comment 8

[Chris Peterson [:cpeterson]]

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

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 10

[Chris Peterson [:cpeterson]]

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

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 12

[Chris Peterson [:cpeterson]]

Sounds good. I'll watch bug 768968.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

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

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

I'll go with gcp's assessment here.

Comment 15

[Chris Peterson [:cpeterson]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c83c52c332be

Comment 17

[Firefox Commit Bot]

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

Comment 18

[Daniel Holbert [:dholbert]]

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

Comment 19

[Daniel Holbert [:dholbert]]

Alternately, I suppose we could leave this closed and un-dupe (i.e. reopen) bug 987300 for the remaining issues.

Comment 20

[Chris Peterson [:cpeterson]]

Let's just leave this reopened. I have a local fix I'm testing (on try).

Comment 21

[Chris Peterson [:cpeterson]]

Attachment: 984071_part-2-fix-linux-warnings.patch

LoadManager::mLastSystemLoad and LoadMonitor::mTicksPerInterval are actually unused on all platforms, not just OSX.

Comment 22

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b1fdffd07e5

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b1fdffd07e5