816112 - Video fixups suggested by justin

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Video fixups suggested bu justin

Comment 2

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

Comment on attachment 686122 [details] [diff] [review]
Video fixups suggested bu justin

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

r+ if you want to commit it; but try some local calls first, and also at least check what happens if you call an older browser without these (so we know)

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +137,5 @@
>    }
>  
> +  if( !(mPtrRTP = webrtc::ViERTP_RTCP::GetInterface(mVideoEngine)))
> +  {
> +    CSFLogError(logTag, "%s Unable to create video engine ", __FUNCTION__);

Please change the log message

@@ +194,5 @@
>  
>  
> +  // Set up some parameters, per juberti
> +  // TODO([e-mail]): Cleanup
> +  mPtrViENetwork->SetMTU(mChannel, 1200);

1200: hmmpf, that's low.  I'd like to know why they're using that.  I used to use >1400

@@ +197,5 @@
> +  // TODO([e-mail]): Cleanup
> +  mPtrViENetwork->SetMTU(mChannel, 1200);
> +  mPtrRTP->SetRTCPStatus(mChannel, webrtc::kRtcpCompound_RFC4585);
> +  mPtrRTP->SetKeyFrameRequestMethod(mChannel, webrtc::kViEKeyFrameRequestPliRtcp);
> +  mPtrRTP->SetNACKStatus(mChannel, true);

I'm a little skeptical of this, but ok.  Depends on what it really implies.

Comment 4

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

My comments from the bug I duped against this:

  Note that these *really* should come as a result of SDP negotiation, and the default keyframe mode should be periodic iframes, or *maybe* (just maybe) RFC 2302 FIRs in RTCP instead of RTP.

Comment 5

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

Comment on attachment 686122 [details] [diff] [review]
Video fixups suggested bu justin

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

r+ if you want to commit it; but try some local calls first, and also at least check what happens if you call an older browser without these (so we know)

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +193,5 @@
>    }
>  
>  
> +  // Set up some parameters, per juberti
> +  // TODO([e-mail]): Cleanup

Please check for errors too - see patch in duped bug

Comment 7

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

From bug 817437: 

We need to analyze which of these are right, which need to be in SDP, etc.

See also:
https://docs.google.com/a/rtfm.com/spreadsheet/ccc?key=0Aksfg6M2xJgudDNrVUNfa0EtQUpadWtPMVRBTUd5OFE#gid=0

Comment 8

[Eric Rescorla (:ekr)]

I am not suggesting we land this as-is. It's just what I already have. Think of it as notes.

Comment 9

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

Attachment: Justin's settings with error handling in a working patch.

Comment 10

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

Remaining issue:
> Note that these *really* should come as a result of SDP negotiation, and the
> default keyframe mode should be periodic iframes, or *maybe* (just maybe) RFC 2302
> FIRs in RTCP instead of RTP.

Let me know how you want it.

Comment 11

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

Comment on attachment 689094 [details] [diff] [review]
Justin's settings with error handling in a working patch.

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

This covers the basic items from my patch and ekr's but we should review the spreadsheet as well

Have you tried the basic loopack video call with this?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +192,5 @@
>      CSFLogError(logTag, "%s Failed to added external renderer ", __FUNCTION__);
>      return kMediaConduitInvalidRenderer;
>    }
>  
> +    

trailing spaces, and also at the end of this change block

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +183,5 @@
>    MediaConduitErrorCode ValidateCodecConfig(const VideoCodecConfig* codecInfo, bool send) const;
>  
>    //Utility function to dump recv codec database
>    void DumpCodecDB() const;
> + 

don't add a spurious trailing space

Comment 12

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

Ok, we want to check these in ASAP.  We need to open a new bug to negotiate these (see comment 4) and when that lands change the defaults.  We should test calls between an old browser and one with this landed. I've made loopback calls, they look good.

Comment 13

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

Attachment: Justin's settings with error handling in a working patch. r=jesup

Comment 14

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

I've opened Bug 819719 to cover negotiation.

Comment 15

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

Attachment: Justin's settings with error handling in a working patch. r=jesup

Whoops, wrong msg in hg. Fixed.

Comment 16

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

Comment on attachment 690147 [details] [diff] [review]
Justin's settings with error handling in a working patch. r=jesup

Carrying forward r+ from Randell.

Comment 17

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

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

Comment 18

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/2bd521c3118f