Closed Bug 998179 Opened 10 years ago Closed 10 years ago

content/media/MediaStreamGraph.cpp:1286:28: warning: variable 'sampleRate' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- unaffected
firefox31 --- affected

People

(Reporter: cpeterson, Assigned: padenot)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Paul: Your MediaStreamGraph patch added the following clang -Wsometimes-uninitialized warning. For a trivial warning, I would include a patch, but it's not obvious to me what should happen if mStreams.Length() == 0 or none of the mStream entries return a non-null AudioNodeStream.


mozilla/inbound/content/media/MediaStreamGraph.cpp:1286:28: warning: variable 'sampleRate' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]
      for (uint32_t i = 0; i < mStreams.Length(); ++i) {
                           ^~~~~~~~~~~~~~~~~~~~~
mozilla/inbound/content/media/MediaStreamGraph.cpp:1299:31: note: uninitialized use occurs here
      RoundUpToNextAudioBlock(sampleRate, mCurrentTime + MillisecondsToMediaTime(AUDIO_TARGET_MS));
                              ^~~~~~~~~~
mozilla/inbound/content/media/MediaStreamGraph.cpp:1286:28: note: remove the condition if it is always true
      for (uint32_t i = 0; i < mStreams.Length(); ++i) {
                           ^~~~~~~~~~~~~~~~~~~~~
/mozilla/inbound/content/media/MediaStreamGraph.cpp:1283:25: note: initialize the variable 'sampleRate' to silence this warning
    TrackRate sampleRate;
                        ^
                         = 0
Paul: this clang -Wsometimes-uninitialized warning is a regression from your patch for bug 997229.
Flags: needinfo?(paul)
Keywords: regression
Yes, I've got this fixed locally, I'll attach my patch here, sorry about that.
Flags: needinfo?(paul)
For some reason, the first time we run the graph, in very rare occasion, we can
have zero streams, and this logic breaks. I'm not sure if this is intended, but
I needed to this make sure the mochitests don't crash while testing the msg
refactor. This only happens when using offline graphs.

It's pretty hard to repro, and I haven't investigated why we don't have a stream
on the first run although the previous assertion about the fact that we should
not start a stream without messages holds for some reason.
Attachment #8410359 - Flags: review?(roc)
Comment on attachment 8410359 [details] [diff] [review]
Use the sample rate passed to the OfflineAudioContext constructor in MediaStreamGraph::CreateOfflineInstance. r=

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

::: content/media/MediaStreamGraph.cpp
@@ +1266,3 @@
>      } else {
>        sampleRate = IdealAudioRate();
>      }

Get rid of 'sampleRate' and just use mSampleRate directly. In the constructor, initialize mSampleRate to IdealAudioRate if necessary.
Attachment #8410359 - Flags: review?(roc) → review-
Blocks: 998557
It was a bit more involved to do this the clean way, but it's nicer.
Attachment #8411139 - Flags: review?(roc)
Attachment #8410359 - Attachment is obsolete: true
Comment on attachment 8411139 [details] [diff] [review]
Use the sample rate passed to the OfflineAudioContext constructor in MediaStreamGraph::CreateOfflineInstance. r=

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

::: content/media/MediaStreamGraphImpl.h
@@ +524,5 @@
>     */
>    GraphTime mEndTime;
> +
> +  /**
> +   * Sample rate at which this offline graph runs. For real time graphs, this is

"at which this graph runs", since it's valid for non-offline
Attachment #8411139 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/1f37af745467
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.