908695 - Provide the webRTC stats RTP packets/bytes-sent/received

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

Once bug 902003 gives us a basic getStats skeleton (which as of this writing should include basic packetsSent/Received, bytesSent/Received stats and little else), we should add more stats.

We should compile a list of most desirable stats in this bug.

In lieu of that list, a good starting point may be here: http://lists.w3.org/Archives/Public/public-webrtc/2013May/0053.html

Comment 1

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #0)
> Once bug 902003 gives us a basic getStats skeleton (which as of this writing
> should include basic packetsSent/Received, bytesSent/Received stats and
> little else), we should add more stats.

Slight change, patch for packets/bytes-sent/received now goes here.

> We should compile a list of most desirable stats in this bug.

New best reference is here http://www.w3.org/2011/04/webrtc/wiki/Stats

Comment 2

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

Attachment: Packets/Bytes-sent/received stats

Not a complete set of RTP stats, but a start. Still missing SSRC, jitter etc.

Try - https://tbpl.mozilla.org/?tree=Try&rev=20ec7249f958

Comment 3

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

Attachment: mozstats.html - updated test page

Comment 4

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

Try - https://tbpl.mozilla.org/?tree=Try&rev=20ec7249f958

Comment 5

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

Attachment: Packets/Bytes-sent/received stats (2)

Unbitrot.

Comment 6

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

Comment on attachment 8344167 [details] [diff] [review]
Packets/Bytes-sent/received stats (2)

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

r+, but we should really consider moving those stats off of int to something more definite.  really should be uint64_t - negative values are not sensical.  That can be a followup bug, however, though feel free to change it now if there are no outside impacts.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +139,1 @@
>    int rtcp_packets_received() const { return rtcp_packets_received_; }

We should consider making these something other than 'int'

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +497,5 @@
> +          break;
> +        }
> +        case MediaPipeline::RECEIVE: {
> +          RTCInboundRTPStreamStats s;
> +          s.mTimestamp.Construct(now);

hoist the common code out of the switch (s, Construct(now), etc)

@@ +503,5 @@
> +          s.mType.Construct(RTCStatsType::Inboundrtp);
> +          s.mSsrc.Construct(NS_LITERAL_STRING("123457"));
> +          s.mPacketsReceived.Construct(mp.rtp_packets_received());
> +          s.mBytesReceived.Construct(mp.rtp_bytes_received());
> +          inbound->AppendElement(s);

Move this to after the switch

Comment 7

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

(In reply to Randell Jesup [:jesup] from comment #6)
> r+, but we should really consider moving those stats off of int to something
> more definite.  really should be uint64_t - negative values are not
> sensical.

Thanks for catching that.

> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> @@ +139,1 @@
> >    int rtcp_packets_received() const { return rtcp_packets_received_; }
> 
> We should consider making these something other than 'int'

Yes, I'll make packet count be uint32_t and byte count uint64_t like http://www.w3.org/2011/04/webrtc/wiki/Stats says.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +497,5 @@
> > +          break;
> > +        }
> > +        case MediaPipeline::RECEIVE: {
> > +          RTCInboundRTPStreamStats s;
> > +          s.mTimestamp.Construct(now);
> 
> hoist the common code out of the switch (s, Construct(now), etc)

I tried, but since the types are case-specific, it was messy (? operator), and less robust: A common accident in near-duplicate code is mixing things like received vs. sent, and the way it is now the compiler catches this (no way to use wrong s by accident). I prefer that.

> @@ +503,5 @@
> > +          s.mType.Construct(RTCStatsType::Inboundrtp);
> > +          s.mSsrc.Construct(NS_LITERAL_STRING("123457"));
> > +          s.mPacketsReceived.Construct(mp.rtp_packets_received());
> > +          s.mBytesReceived.Construct(mp.rtp_bytes_received());
> > +          inbound->AppendElement(s);
> 
> Move this to after the switch

The target array (inbound vs. outbound) is case specific as well.

Comment 8

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

Hmm, I also broke the mediapipeline_unittest https://tbpl.mozilla.org/?tree=Try&rev=20ec7249f958

It's not immediately obvious to me, so I need to look at it.

[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from MediaPipelineTest
[ RUN      ] MediaPipelineTest.TestAudioSendNoMux
WARNING: no real random source present!
/Users/Jan/moz/mozilla-central/media/webrtc/signaling/test/mediapipeline_unittest.cpp:328: Failure
Expected: (p2_.GetAudioRtpCount()) >= (40), actual: 0 vs 40
Assertion failure: !stream_, at /Users/Jan/moz/mozilla-central/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:53

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
mozilla::MediaPipeline::~MediaPipeline (this=0x11116c900) at MediaPipeline.cpp:53
53	  MOZ_ASSERT(!stream_);  // Check that we have shut down already.
(gdb) bt
#0  mozilla::MediaPipeline::~MediaPipeline at MediaPipeline.cpp:53
#1  mozilla::MediaPipelineReceive::~MediaPipelineReceive at MediaPipeline.h:432
#2  mozilla::MediaPipelineReceiveAudio::~MediaPipelineReceiveAudio at MediaPipeline.h:460
#3  mozilla::MediaPipelineReceiveAudio::~MediaPipelineReceiveAudio at MediaPipeline.h:460
#4  mozilla::MediaPipelineReceiveAudio::~MediaPipelineReceiveAudio at MediaPipeline.h:460
#5  mozilla::MediaPipeline::Release at MediaPipeline.h:144
#6  mozilla::RefPtr<mozilla::MediaPipeline>::unref at RefPtr.h:203
#7  mozilla::RefPtr<mozilla::MediaPipeline>::~RefPtr at RefPtr.h:153
#8  mozilla::RefPtr<mozilla::MediaPipeline>::~RefPtr at RefPtr.h:153
#9  (anonymous namespace)::TestAgent::~TestAgent at media/webrtc/signaling/test/mediapipeline_unittest.cpp:105

Comment 9

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

It seems intermittent, and I can't understand how my changes could cause this to happen.

If someone who knows the MediaPipeline code better can take a look I would really appreciate it!

Comment 10

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

The crash appears to be that if the ASSERT_GE() fails, it exits without calling p1_.Stop()/p2_.Stop(), and there is a shutdown issue it appears.  Fancier setups where the destructors guarantee calling shutdown functions would sidestep this I thik

The real problem appears to be that rtp_packets_received_ is 0 - and this patch does touch that code.

Comment 11

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

The cause appears to be failure to Unprotect() the RTP packets.  You moved the "received" bumping to after Unprotect(), and we never get there.

I popped this patch off, and tested in GDB without it, and it also won't Unprotect() the data.  And if I move the increment of packets received to after Unprotect() (i.e. it doesn't get called, and the ASSERT_GE() fails) we get teh same shutdown crash.

Please file a bug against the unit test for the Unprotect() problem, assign to ekr (he can reassign if he wishes), ditto one that if the test fails it crashes on exit.

Add a disabling of the test for received packets in the pipeline test.  (comment out the ASSERT_GE()).  When the problem in the test is resolved, it can be re-enabled.  You have r+ from me to disable the ASSERT_GE(), since it wasn't actually receiving packets successfully in the test

Comment 12

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

Attachment: Packets/Bytes-sent/received stats (3)

- Rectified side-effect that caused mediapipeline_unittest crash.
- Made media-pipeline packet counter 32-bit and bytes counter 64-bit as suggested.

I didn't dare switch the MediaPipeline code from signed to unsigned for fear of introducing bugs (IMHO unsigned is just a source of bugs anyhow these 64-bit days).

Try - https://tbpl.mozilla.org/?tree=Try&rev=2ad1ee2c1976

Comment 13

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

If you could give it another look-over that would be great. Thanks for pointing me in the right direction.

Comment 14

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

(In reply to Randell Jesup [:jesup] from comment #11)
> The cause appears to be failure to Unprotect() the RTP packets.  You moved
> the "received" bumping to after Unprotect(), and we never get there.

Sorry I didn't see this comment before I posted. I found the same thing and I believe I've solved it by moving it back close to where it was before.

> Please file a bug against the unit test for the Unprotect() problem, assign
> to ekr (he can reassign if he wishes), ditto one that if the test fails it
> crashes on exit.
> 
> Add a disabling of the test for received packets in the pipeline test. 
> (comment out the ASSERT_GE()).  When the problem in the test is resolved, it
> can be re-enabled.  You have r+ from me to disable the ASSERT_GE(), since it
> wasn't actually receiving packets successfully in the test

Please take a look at the patch and let me know if you think this is still necessary.

Comment 15

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

Comment on attachment 8344245 [details] [diff] [review]
Packets/Bytes-sent/received stats (3)

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

r+ if you also disable the packets-received test in mediapipeline_unittests.cpp (per previous comment)

Also file the followup bugs (doesn't have to be today), and a followup to convert to unsigned (it's still the right thing to do, but no hurry).

Comment 16

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #14)
> (In reply to Randell Jesup [:jesup] from comment #11)
> > The cause appears to be failure to Unprotect() the RTP packets.  You moved
> > the "received" bumping to after Unprotect(), and we never get there.
> 
> Sorry I didn't see this comment before I posted. I found the same thing and
> I believe I've solved it by moving it back close to where it was before.

yes that should work (as in not fail), though it simply side-steps the failure in the unit tests.  The test *should* fail since it didn't receive packets it could decrypt - receiving unusable packets is not success.

I'd prefer to explicitly disable the test that isn't actually testing what it thinks it is, until it's fixed.

Please disable the test, and move the call back to where you had it (after you test the result of Unprotect()).

Comment 17

[Eric Rescorla (:ekr)]

I see what the problem is here: TransportLayerPrsock is coalescing the packets...

Comment 18

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

Attachment: Packets/Bytes-sent/received stats (4)

Done. I filed Bug 947663 for the unittest.

Carrying forward r+ from jesup.

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b1230494e0b

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b1230494e0b
https://hg.mozilla.org/mozilla-central/rev/7b0b527637dd

Comment 21

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

Attachment: Move gathering of RTP stats to main thread

Reading mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h#221 I realized all the rtp stats (currently) gathered are safe to read off main, so I moved gathering for rtp to main. Seemed simpler for now than prolonging life-cycles safely. We can always move it back.

Please re-open this issue only if you decide to take this patch. Thanks.

Try - https://tbpl.mozilla.org/?tree=Try&rev=82b2ddcca9cc

Comment 22

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

Oh and GetPipelineStats never fails. http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#474

Comment 23

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

Comment on attachment 8344502 [details] [diff] [review]
Move gathering of RTP stats to main thread

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

I assume the code only calls GetPipelineStats on MainThread (from what I can see)

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +481,1 @@
>    for (std::map<int, RefPtr<MediaPipeline> >::iterator it = mPipelines.begin();

This is safe from MainThread only, since mPipelines is modified on mainthread only.  Please add a MainThread assert in place of the STS assert.

Note that mPipelines is accessed (but not modified) on STS in shutdown (only).

Comment 24

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

Will do. Thanks.


After speaking with Byron, I'll modify this to shove the pipelines into a vector and move the gathering back to STS, so he can build on top of it.

Comment 25

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

Attachment: Collect pipelines on main, dispatch to STS for stats

Try - https://tbpl.mozilla.org/?tree=Try&rev=3a11490f0072

Comment 26

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

Attachment: Collect pipelines on main, dispatch to STS for stats (2)

Removed an unneeded typedef.

Comment 27

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

Comment on attachment 8345144 [details] [diff] [review]
Collect pipelines on main, dispatch to STS for stats (2)

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1256,2 @@
>                               internalStats,
> +                             pipelines,

How do you avoid the references you passed to STS being released there if timing is just wrong?  (I.e. if the mainthread releases all other references to the pipeline while STS has a ref, then STS releases?)

Two options: send the pipelines back to mainthread to drop the ref, or make sure pipelines can be safely released on any thread.  (if it's already safe to release them on STS, great!  If so, we may be very close to r+ depending on the other comment.)

Releasing from any thread (if some parts need to be mainthread-released) can be done with a outer threadsafe object and an inner mainthread object, and if the outer is destroyed on a thread other than main, it ejects the inner in a runnable to MainThread to die (if on mainthread, it just kills the inner).

The former solution is simpler generally (much).

@@ +1869,5 @@
> +        // TODO: Get SSRC
> +        // int channel = mp.Conduit()->GetChannel();
> +        s.mSsrc.Construct(NS_LITERAL_STRING("123457"));
> +        s.mPacketsSent.Construct(mp.rtp_packets_sent());
> +        s.mBytesSent.Construct(mp.rtp_bytes_sent());

Which thread can rtp_*_sent/received be safely touched?  Is it STS (if so, good)?  Are they the same?  Or do you need to lock?  Or (if they're mainthread, which they very likely aren't) you could gather first and then send to STS instead of the pipeline pointer.

Likely receive is STS, and send might be STS (probably, and good) or the GIPS thread that calls SendPacket (though it could likely be moved to STS if that's the case.

Please annotate the .h files with which threads they're safe to access from.  Perhaps we should also add a thread assert for accessing the values.

Comment 28

[Byron Campen [:bwc]]

Comment on attachment 8345144 [details] [diff] [review]
Collect pipelines on main, dispatch to STS for stats (2)

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

This is roughly what we should be doing. As for the lifecycle of MediaPipeline, it looks like we run some cleanup code on main (namely ShutdownMedia_m), then some cleanup code on STS (namely, ShutdownTransport_s) before finally destroying back on the main thread. As long as we do a sanity-check on main before we call GetStats_s, I'm pretty sure that the back-and-forth GetStats stuff will finish before any back-and-forth teardown stuff finishes. We could future-proof by throwing in a ref pointer on the return dispatch, though.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1214,5 @@
> +PushBackSelect(std::vector<RefPtr<MediaPipeline>>& aDst,
> +               const std::map<TrackID, RefPtr<mozilla::MediaPipeline>> & aSrc,
> +               TrackID aKey = 0) {
> +  auto it1 = aKey ? aSrc.find(aKey) : aSrc.begin();
> +  auto it2 = aKey ? it1 : aSrc.end();

For readability, suggest it1 => begin and it2 => end.

@@ +1215,5 @@
> +               const std::map<TrackID, RefPtr<mozilla::MediaPipeline>> & aSrc,
> +               TrackID aKey = 0) {
> +  auto it1 = aKey ? aSrc.find(aKey) : aSrc.begin();
> +  auto it2 = aKey ? it1 : aSrc.end();
> +  if (it1 != aSrc.end()) {

This only happens when it2 is also at the end, so the for loop should have us covered.

Comment 29

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

(In reply to Randell Jesup [:jesup] from comment #27)
> Comment on attachment 8345144 [details] [diff] [review]
> Collect pipelines on main, dispatch to STS for stats (2)
> 
> Review of attachment 8345144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1256,2 @@
> >                               internalStats,
> > +                             pipelines,
> 
> How do you avoid the references you passed to STS being released there if
> timing is just wrong?  (I.e. if the mainthread releases all other references
> to the pipeline while STS has a ref, then STS releases?)
> 
> Two options: send the pipelines back to mainthread to drop the ref, or make
> sure pipelines can be safely released on any thread.  (if it's already safe
> to release them on STS, great!  If so, we may be very close to r+ depending
> on the other comment.)

I see no ASSERT_ON_THREAD in ~MediaPipeline(). It seems to hit on main-thread from PeerConnectionMedia::SelfDestruct_m(), though lots of threads already touch MediaPipeline.

> Releasing from any thread (if some parts need to be mainthread-released) can
> be done with a outer threadsafe object and an inner mainthread object, and
> if the outer is destroyed on a thread other than main, it ejects the inner
> in a runnable to MainThread to die (if on mainthread, it just kills the
> inner).

Is there a rule for what needs to be mainthread-released? With our threading model wouldn't it be wise to ASSERT_ON_THREAD all of these cases?

> The former solution is simpler generally (much).

Yes that may be simpler than figuring out whether it is ok now. Though that potentially keeps pipelines alive even longer, and is the return dispatch guaranteed to make it back to main? When is the STS queue torn down? It might be better to piggyback on ShutdownMediaTransport_s().

> @@ +1869,5 @@
> > +        // TODO: Get SSRC
> > +        // int channel = mp.Conduit()->GetChannel();
> > +        s.mSsrc.Construct(NS_LITERAL_STRING("123457"));
> > +        s.mPacketsSent.Construct(mp.rtp_packets_sent());
> > +        s.mBytesSent.Construct(mp.rtp_bytes_sent());
> 
> Which thread can rtp_*_sent/received be safely touched?  Is it STS (if so,
> good)?  Are they the same?  Or do you need to lock?  Or (if they're
> mainthread, which they very likely aren't) you could gather first and then
> send to STS instead of the pipeline pointer.

My last patch read them on main, which the MediaPipeline threading-cheatsheet http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h#34 said was OK, but without a mutex this is dubious, and may not be true for all stats ever. So Byron and I agreed STS access is best.

> Please annotate the .h files with which threads they're safe to access from.
> Perhaps we should also add a thread assert for accessing the values.

Comment 30

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

Attachment: Collect pipelines on main, dispatch to STS for stats (3)

- Returns pipelines back to main to be freed.
- Fixed nits and iterator mistake.
- Added ASSERT_ON_THREAD(main_thread_) to ~MediaPipeline()

Try - https://tbpl.mozilla.org/?tree=Try&rev=0ee697f996b1

Comment 31

[Byron Campen [:bwc]]

Comment on attachment 8345606 [details] [diff] [review]
Collect pipelines on main, dispatch to STS for stats (3)

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

Seems about right.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1217,5 @@
> +  auto begin = aKey ? aSrc.find(aKey) : aSrc.begin();
> +  for (auto it = begin, end = (aKey && begin != aSrc.end())? ++begin : aSrc.end();
> +       it != end; ++it) {
> +    aDst.push_back(it->second);
> +  }

This simple concept is frustratingly hard to express in a simple way. If it were me, I'd just do if/else here, since the actual work is just a push_back, but that's just taste.

Comment 32

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

(In reply to Byron Campen [:bwc] from comment #31)
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1217,5 @@
> > +  auto begin = aKey ? aSrc.find(aKey) : aSrc.begin();
> > +  for (auto it = begin, end = (aKey && begin != aSrc.end())? ++begin : aSrc.end();
> > +       it != end; ++it) {
> > +    aDst.push_back(it->second);
> > +  }
> 
> This simple concept is frustratingly hard to express in a simple way. If it
> were me, I'd just do if/else here, since the actual work is just a
> push_back, but that's just taste.

I had something simpler in mind using std::next, but apparently, we don't support std::next. If-else looked complicated when I tried.

Comment 33

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

Thanks. The auto use in Comment 32 burned on Windows though:
c:/builds/moz2_slave/try-w32-0000000000000000000000/build/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp(1218) : error C3538: in a declarator-list 'auto' must always deduce to the same type
        could be 'std::_Tree_const_iterator<_Mytree>'
        with
        [
            _Mytree=std::_Tree_val<std::_Tmap_traits<mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>,std::less<base::Histogram::Sample>,std::allocator<std::pair<const mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>>>,false>>
        ]
        or       'std::_Tree_const_iterator<_Mytree>'
        with
        [
            _Mytree=std::_Tree_val<std::_Tmap_traits<mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>,std::less<base::Histogram::Sample>,std::allocator<std::pair<const mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>>>,false>>
        ]

I don't see a difference here, but hey. Will fix.

Comment 34

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

Attachment: Collect pipelines on main, dispatch to STS for stats (4)

Compiles on Windows. Carrying forward r+ fron jesup.

Try - https://tbpl.mozilla.org/?tree=Try&rev=00f625deb3d5

Comment 35

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

Only the second patch needs checking in. The first has landed already.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1733cf3b70e9

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1733cf3b70e9

Comment 38

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #33)
> Thanks. The auto use in Comment 32 burned on Windows though:
> c:/builds/moz2_slave/try-w32-0000000000000000000000/build/media/webrtc/
> signaling/src/peerconnection/PeerConnectionImpl.cpp(1218) : error C3538: in
> a declarator-list 'auto' must always deduce to the same type
>         could be 'std::_Tree_const_iterator<_Mytree>'
>         with
>         [
>            
> _Mytree=std::_Tree_val<std::_Tmap_traits<mozilla::TrackID,mozilla::
> RefPtr<mozilla::MediaPipeline>,std::less<base::Histogram::Sample>,std::
> allocator<std::pair<const
> mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>>>,false>>
>         ]
>         or       'std::_Tree_const_iterator<_Mytree>'
>         with
>         [
>            
> _Mytree=std::_Tree_val<std::_Tmap_traits<mozilla::TrackID,mozilla::
> RefPtr<mozilla::MediaPipeline>,std::less<base::Histogram::Sample>,std::
> allocator<std::pair<const
> mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>>>,false>>
>         ]
> 
> I don't see a difference here, but hey. Will fix.

   A little late, but FYI this is a compiler bug, probably due to the ternary operator being used: http://connect.microsoft.com/VisualStudio/feedback/details/728741

Comment 39

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

Yeah I figured. I avoided it by moving one of them out of the declarator list.

Comment 40

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

Closing as fixed. See Bug 950855 for getStats mochitest.