Closed Bug 1230295 Opened 4 years ago Closed 4 years ago

Add telemetry to report when Ogg chaining is used

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

Details

Attachments

(1 file)

It would be useful to know how often Ogg chaining is used. We should have telemetry for this.
Bug 1230295 - Add telemetry to report when Ogg chaining is used. r?rillian,r?vladan
Attachment #8695456 - Flags: review?(vladan.bugzilla)
Attachment #8695456 - Flags: review?(giles)
Comment on attachment 8695456 [details]
MozReview Request: Bug 1230295 - Add telemetry to report when Ogg chaining is used. r?rillian,r?vladan

https://reviewboard.mozilla.org/r/27115/#review24521

::: dom/media/ogg/OggReader.h:257
(Diff revision 1)
> -  void SetChained(bool aIsChained);
> +  void SetChained();

Fair enough.

::: dom/media/ogg/OggReader.cpp:715
(Diff revision 1)
> +      mIsChained = true;

You always told me it was better to return early and save a nesting level.

```
if (mIsChained) {
  // Nothing to do.
  return;
}
mIsChanged = true;
```

::: dom/media/ogg/OggReader.cpp:723
(Diff revision 1)
>    mDecoder->DispatchSetMediaSeekable(false);

Make sure you're not bit-rotted by bug 1228939 here.

::: toolkit/components/telemetry/Histograms.json:6317
(Diff revision 1)
> +  },

This just counts how many chained streams we see. Don't we need to count the number of Ogg streams overall as well so we have a denominator here?

Thanks for investigating this. Yay data! r=me with comments addressed.
Attachment #8695456 - Flags: review?(giles)
https://reviewboard.mozilla.org/r/27115/#review24521

> You always told me it was better to return early and save a nesting level.
> 
> ```
> if (mIsChained) {
>   // Nothing to do.
>   return;
> }
> mIsChanged = true;
> ```

Touche! But if we returned early, we'd not call mDecoder->DispatchMediaSeekable(false), so I'd be introducing a behaviour change.

Having said that, it seems it's safe to remove duplicate calls to  mDecoder->DispatchSetMediaSeekable(false), so ok.

> This just counts how many chained streams we see. Don't we need to count the number of Ogg streams overall as well so we have a denominator here?

"flag" telemetry probes don't count duplicates, so this actually reports whether chaining was used at least once per session, where session is the time between browser start and end (I think).

Being able to count chained vs non chained so we can report X% of Ogg playbacks are chained is easy, I'll change the telemetry.
Attachment #8695456 - Flags: review+ → review?(giles)
Comment on attachment 8695456 [details]
MozReview Request: Bug 1230295 - Add telemetry to report when Ogg chaining is used. r?rillian,r?vladan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27115/diff/1-2/
Attachment #8695456 - Flags: review?(vladan.bugzilla) → review+
Comment on attachment 8695456 [details]
MozReview Request: Bug 1230295 - Add telemetry to report when Ogg chaining is used. r?rillian,r?vladan

https://reviewboard.mozilla.org/r/27115/#review24631
Comment on attachment 8695456 [details]
MozReview Request: Bug 1230295 - Add telemetry to report when Ogg chaining is used. r?rillian,r?vladan

https://reviewboard.mozilla.org/r/27115/#review24633

LGTM.

For my own education, why do you defer the log and telemetry calls to a main thread task? I thought both of those were thread safe.
Attachment #8695456 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #6)
> For my own education, why do you defer the log and telemetry calls to a main
> thread task? I thought both of those were thread safe.

IIRC, telemetry only works on the main thread, and I wanted the log message to only run when we're actaully doing telemetry, so if the event somehow fails to dispatch, we don't see the log message.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a141cbb4544e489cdde767f1fc97dde0a5d10e
Bug 1230295 - Add telemetry to report when Ogg chaining is used. r=rillian,r=vladan
https://hg.mozilla.org/mozilla-central/rev/b4a141cbb454
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Per follow-up discussion with vladan on irc. Telementry isn't thread-safe, although some code treats it as such. Crashes don't seem to be common, and the Accumulate() call is expected to become thread-safe in the next few weeks.
You need to log in before you can comment on or make changes to this bug.