VideoConduit code should simply reconfigure the VideoSendStream when possible on a configuration change

RESOLVED FIXED in Firefox 53

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pkerr, Assigned: jesup)

Tracking

53 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
The VideoConduit module has been updated to work with the new Call API found in the branch 49 version of the webrtc.org media stack. In its current form, the Encoder is stopped, re-created, and started for even small changes in codec configuration. When the changes can be handled by the VideoSendStream::ReconfigureVideoEncoder method the update should be made using this mechanism without destroying the current VideoSendStream. Otherwise, a new VideoSendStream will need to be created.
(Reporter)

Comment 1

2 years ago
Created attachment 8822714 [details] [diff] [review]
All fast SendCodec reconfigure when appropriate
(Reporter)

Updated

2 years ago
Attachment #8822714 - Flags: review?(rjesup)
(Assignee)

Comment 2

2 years ago
I believe resolution changes can all be handled without destroying.  Some parts of this have already landed as part of fixing ConfigureSendMediaCodec() to install the new codec params.
(Assignee)

Comment 3

2 years ago
Created attachment 8828361 [details] [diff] [review]
All fast SendCodec reconfigure when appropriate

rebased, and rather modified.  Also I'm assuming we don't need to destroy on a resolution change - Chrome does, but they hadn't fixed the vp8/vp9 things that break on resolution changes - we have
Attachment #8828361 - Flags: review?(na-g)
(Assignee)

Updated

2 years ago
Attachment #8822714 - Attachment is obsolete: true
Attachment #8822714 - Flags: review?(rjesup)
(Assignee)

Updated

2 years ago
Assignee: paulrkerr → rjesup
Status: NEW → ASSIGNED

Comment 4

2 years ago
Comment on attachment 8828361 [details] [diff] [review]
All fast SendCodec reconfigure when appropriate

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

One issue with duplicated call.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +628,5 @@
> +    if (!RequiresNewSendStream(*codecConfig)) {
> +      if (!mSendStream->ReconfigureVideoEncoder(mEncoderConfig.GenerateConfig())) {
> +        CSFLogError(logTag, "%s: ReconfigureVideoEncoder failed", __FUNCTION__);
> +        // This will cause a new encoder to be created by StartTransmitting()
> +        condError = StopTransmitting();

This is redundant with the StopTransmitting call at line 643.
Attachment #8828361 - Flags: review?(na-g) → review-
(Assignee)

Comment 5

2 years ago
Created attachment 8828539 [details] [diff] [review]
All fast SendCodec reconfigure when appropriate

yup, removed that (though stopping twice doesn't hurt)
Attachment #8828539 - Flags: review?(na-g)
(Assignee)

Updated

2 years ago
Attachment #8828361 - Attachment is obsolete: true

Comment 6

2 years ago
Comment on attachment 8828539 [details] [diff] [review]
All fast SendCodec reconfigure when appropriate

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

r+, pending try

Comment 7

2 years ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc8e1a140e2
Use reconfigure whenever possible on video renegotiation r=ng

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cdc8e1a140e2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

2 years ago
Attachment #8828539 - Flags: review?(na-g) → review+
You need to log in before you can comment on or make changes to this bug.