Video fixups suggested by justin

RESOLVED FIXED in mozilla20

Status

()

Core
WebRTC
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ekr, Assigned: jib)

Tracking

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+][qa-])

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Created attachment 686122 [details] [diff] [review]
Video fixups suggested bu justin
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.
Attachment #686122 - Flags: review+

Updated

5 years ago
Duplicate of this bug: 783701
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.

Updated

5 years ago
Summary: Video fixups suggested bu justin → Video fixups suggested by justin
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

Updated

5 years ago
Duplicate of this bug: 817437
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
Assignee: ekr → jib
Component: WebRTC: Audio/Video → WebRTC
Priority: -- → P1
Whiteboard: [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+]
(Reporter)

Comment 8

5 years ago
I am not suggesting we land this as-is. It's just what I already have. Think of it as notes.
(Assignee)

Comment 9

5 years ago
Created attachment 689094 [details] [diff] [review]
Justin's settings with error handling in a working patch.
Attachment #686122 - Attachment is obsolete: true
Attachment #689094 - Flags: review?(rjesup)
(Assignee)

Comment 10

5 years ago
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 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
Attachment #689094 - Flags: review?(rjesup) → review+
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.
(Assignee)

Comment 13

5 years ago
Created attachment 690144 [details] [diff] [review]
Justin's settings with error handling in a working patch. r=jesup
Attachment #689094 - Attachment is obsolete: true
Attachment #690144 - Flags: checkin?(rjesup)
(Assignee)

Comment 14

5 years ago
I've opened Bug 819719 to cover negotiation.
(Assignee)

Updated

5 years ago
See Also: → bug 819719
(Assignee)

Comment 15

5 years ago
Created attachment 690147 [details] [diff] [review]
Justin's settings with error handling in a working patch. r=jesup

Whoops, wrong msg in hg. Fixed.
Attachment #690144 - Attachment is obsolete: true
Attachment #690144 - Flags: checkin?(rjesup)
Attachment #690147 - Flags: checkin?
(Assignee)

Updated

5 years ago
Attachment #690147 - Flags: checkin? → checkin?(rjesup)
(Assignee)

Comment 16

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

Carrying forward r+ from Randell.
Attachment #690147 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd521c3118f
Target Milestone: --- → mozilla20

Updated

5 years ago
Attachment #690147 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/2bd521c3118f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+] → [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.