875556 - webRTC bandwidth telemetry

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Comment 1

[Maire Reavy [:mreavy]]

Reassigning to Jan-Ivar since Ben has gone back to school

Comment 2

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

Attachment: Part 1: definitions for WebRTC bandwidth telemetry

Comment 3

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

Attachment: Part 2: tweak internal RTCStatsQuery for re-use.

Prep RTCStatsQuery struct so I can snatch its reports for part 3.

Comment 4

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

Attachment: Part 3: telemetry for WebRTC bandwidth, stats-tweak approach.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8424355 [details] [diff] [review]
Part 2: tweak internal RTCStatsQuery for re-use.

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

I can't get this patch to apply cleanly, but this looks basically sane. Go ahead and unrot, and I can give it a closer look.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +183,4 @@
>      std::string error;
>  
> +  protected:
> +    nsAutoPtr<mozilla::dom::RTCStatsReportInternal> report;

Why protected?

Comment 6

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

(In reply to Byron Campen [:bwc] from comment #5)
> I can't get this patch to apply cleanly,

Did you apply them on-top of Bug 970685?

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> @@ +183,4 @@
> >      std::string error;
> >  
> > +  protected:
> > +    nsAutoPtr<mozilla::dom::RTCStatsReportInternal> report;
> 
> Why protected?

For RTCStatsQueryForTelemetry.TakeReport() in part 3.

Comment 7

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

Attachment: Part 2: tweak internal RTCStatsQuery for re-use (2)

Unbitrot.

Comment 8

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

Comment on attachment 8424357 [details] [diff] [review]
Part 3: telemetry for WebRTC bandwidth, stats-tweak approach.

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +275,5 @@
> +
> +static nsTArray<nsAutoPtr<RTCStatsReportInternal>>::index_type
> +FindId(const nsTArray<nsAutoPtr<RTCStatsReportInternal>>& aArray,
> +       const nsString &aId) {
> +  for (nsTArray<nsAutoPtr<RTCStatsReportInternal>>::index_type i = 0;

All I can say is... This c++ stuff is icky

@@ +316,1 @@
>          auto& s = array[i];

Very cute, this c++11 stuff.  Can't wait to see ehsan grep-and-replace on the entire tree...

Comment 9

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

Attachment: Part 3: telemetry for WebRTC bandwidth, stats-tweak approach (2)

(In reply to Randell Jesup [:jesup] from comment #8)
> > +static nsTArray<nsAutoPtr<RTCStatsReportInternal>>::index_type
> > +FindId(const nsTArray<nsAutoPtr<RTCStatsReportInternal>>& aArray,
> > +       const nsString &aId) {
> > +  for (nsTArray<nsAutoPtr<RTCStatsReportInternal>>::index_type i = 0;
> 
> All I can say is... This c++ stuff is icky

Hopefully less icky. YMMV.

Comment 10

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

Attachment: Part 3: telemetry for WebRTC bandwidth, stats-tweak approach (3) r=jesup

Forgot MOZILLA_INTERNAL_API - Carrying forward r=jesup.

Comment 11

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

Try - https://tbpl.mozilla.org/?tree=Try&rev=92f6458daf2c

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8427004 [details] [diff] [review]
Part 2: tweak internal RTCStatsQuery for re-use (2)

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

This looks functional, by why not just put TakeReport in RTCStatsQuery (or maybe even just make it an nsAutoPtr that is public)? This would actually be useful for storing the final stats reports (see http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.h#87 ).

Comment 13

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

Attachment: Part 2: tweak internal RTCStatsQuery to use nsAutoPtr for report, so it can be stolen

Simpler tweak that exposes report as nsAutoPtr, as suggested.

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8429386 [details] [diff] [review]
Part 2: tweak internal RTCStatsQuery to use nsAutoPtr for report, so it can be stolen

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

lgtm

Comment 15

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

Attachment: Part 3: telemetry for WebRTC bandwidth, stats-tweak approach (4) r=jesup

Rebased ontop of Part 2, which means some rote changes, but nothing substantive so I've carried forward r=jesup.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

These patches are bitrotted. Please rebase.

Comment 17

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

Patches appear to have landed already (wrong bug # in two of them, sorry):

http://hg.mozilla.org/mozilla-central/rev/7790127900af
http://hg.mozilla.org/mozilla-central/rev/78f0a65aac79
http://hg.mozilla.org/mozilla-central/rev/8267531afaad

Comment 18

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

See Bug 970685 comment 18.