Last Comment Bug 816112 - Video fixups suggested by justin
: Video fixups suggested by justin
Status: RESOLVED FIXED
[WebRTC],[blocking-webrtc-interop],[b...
:
Product: Core
Classification: Components
Component: WebRTC (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla20
Assigned To: Jan-Ivar Bruaroey [:jib]
: Jason Smith [:jsmith]
Mentors:
: 783701 817437 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-28 08:25 PST by Eric Rescorla (:ekr)
Modified: 2012-12-09 11:45 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Video fixups suggested bu justin (6.53 KB, patch)
2012-11-28 08:25 PST, Eric Rescorla (:ekr)
rjesup: review+
Details | Diff | Splinter Review
Justin's settings with error handling in a working patch. (10.04 KB, patch)
2012-12-05 22:16 PST, Jan-Ivar Bruaroey [:jib]
rjesup: review+
Details | Diff | Splinter Review
Justin's settings with error handling in a working patch. r=jesup (10.04 KB, patch)
2012-12-08 15:19 PST, Jan-Ivar Bruaroey [:jib]
no flags Details | Diff | Splinter Review
Justin's settings with error handling in a working patch. r=jesup (10.04 KB, patch)
2012-12-08 15:53 PST, Jan-Ivar Bruaroey [:jib]
jib: review+
rjesup: checkin+
Details | Diff | Splinter Review

Description Eric Rescorla (:ekr) 2012-11-28 08:25:26 PST

    
Comment 1 Eric Rescorla (:ekr) 2012-11-28 08:25:31 PST
Created attachment 686122 [details] [diff] [review]
Video fixups suggested bu justin
Comment 2 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-11-28 15:25:31 PST
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(ekr@rtfm.com): 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(ekr@rtfm.com): 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 3 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-11-28 20:05:22 PST
*** Bug 783701 has been marked as a duplicate of this bug. ***
Comment 4 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-11-28 20:06:59 PST
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 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-11-28 20:09:45 PST
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(ekr@rtfm.com): Cleanup

Please check for errors too - see patch in duped bug
Comment 6 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-12-03 23:16:11 PST
*** Bug 817437 has been marked as a duplicate of this bug. ***
Comment 7 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-12-03 23:18:08 PST
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) 2012-12-04 06:34:10 PST
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] 2012-12-05 22:16:54 PST
Created attachment 689094 [details] [diff] [review]
Justin's settings with error handling in a working patch.
Comment 10 Jan-Ivar Bruaroey [:jib] 2012-12-05 22:25:23 PST
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 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-12-07 09:20:26 PST
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 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-12-08 10:31:33 PST
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] 2012-12-08 15:19:09 PST
Created attachment 690144 [details] [diff] [review]
Justin's settings with error handling in a working patch. r=jesup
Comment 14 Jan-Ivar Bruaroey [:jib] 2012-12-08 15:47:13 PST
I've opened Bug 819719 to cover negotiation.
Comment 15 Jan-Ivar Bruaroey [:jib] 2012-12-08 15:53:07 PST
Created attachment 690147 [details] [diff] [review]
Justin's settings with error handling in a working patch. r=jesup

Whoops, wrong msg in hg. Fixed.
Comment 16 Jan-Ivar Bruaroey [:jib] 2012-12-08 15:54:58 PST
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 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-12-08 22:33:53 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd521c3118f

Note You need to log in before you can comment on or make changes to this bug.