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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Chris Peterson [:cpeterson]]

Paul: this clang -Wsometimes-uninitialized warning is a regression from your patch for bug 997229.

Comment 3

[Paul Adenot (:padenot)]

Yes, I've got this fixed locally, I'll attach my patch here, sorry about that.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Use the sample rate passed to the OfflineAudioContext constructor in MediaStreamGraph::CreateOfflineInstance. r=

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.

Comment 5

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

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.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Use the sample rate passed to the OfflineAudioContext constructor in MediaStreamGraph::CreateOfflineInstance. r=

It was a bit more involved to do this the clean way, but it's nicer.

Comment 7

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

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

Comment 8

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f37af745467

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f37af745467