1006641 - Measure and report the audio stream creation time

Status: RESOLVED FIXED

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

Description

[Paul Adenot (:padenot)]

We need to have figures for how long an audio stream takes to create. We also need to know whether this number is for the first stream, or any other stream, because the first stream is likely to have the IPC setup overhead (depending on the platform).

This is a good candidate for telemetry probes.

Comment 1

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

Attachment: add telemetry for AudioStream open times

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8434077 [details] [diff] [review]
add telemetry for AudioStream open times

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

r=me with nits.

::: content/media/AudioStream.cpp
@@ +75,5 @@
>  }
>  
> +/*static*/ bool AudioStream::GetFirstStream()
> +{
> +  StaticMutexAutoLock lock(sMutex);

Why not define sFirstStream here instead of exposing it in class scope?

  static bool sFirstStream = true;

@@ +78,5 @@
> +{
> +  StaticMutexAutoLock lock(sMutex);
> +  bool result = sFirstStream;
> +  sFirstStream = false;
> +  return result;

.

@@ +506,5 @@
>        return NS_ERROR_FAILURE;
>      }
>    }
>  
> +  if (!mStartTime.IsNull()) {

Can mStartTime ever be null here?

@@ +513,5 @@
> +         (uint32_t) timeDelta.ToMilliseconds()));
> +		Telemetry::Accumulate(mIsFirst ? Telemetry::AUDIOSTREAM_FIRST_OPEN :
> +                          Telemetry::AUDIOSTREAM_LATER_OPEN, timeDelta.ToMilliseconds());
> +  }
> +		 

whitespace

::: content/media/AudioStream.h
@@ +429,5 @@
>    // This mutex protects the static members below.
>    static StaticMutex sMutex;
>    static cubeb* sCubebContext;
>  
> +  static bool sFirstStream;

Don't need both sFirstStream and GetFirstStream

::: toolkit/components/telemetry/Histograms.json
@@ +49,5 @@
>      "expires_in_version": "never",
>      "kind": "boolean",
>      "description": "Application reputation query count (both local and remote)"
>    },
> +  "AUDIOSTREAM_FIRST_OPEN": {

Do we want to use _MS in the name? I find descriptions absent in about:telemetry at least and I personally find this pattern helpful, though I see it's not used consistently throughout. Just an idea.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/504404fcc7dc

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/504404fcc7dc