Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2882433003:
Reduce VideoSendStream recreations due to FlexFEC. (Closed)
|
Created:
3 years, 7 months ago by brandtr Modified:
3 years, 7 months ago Reviewers:
perkj_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReduce VideoSendStream recreations due to FlexFEC.
This CL reduces the number of VideoSendStream recreations during SDP
renegotiation by checking the FlexFEC field trials before, and not after,
the SDP codec diffing logic.
BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2882433003
Cr-Commit-Position: refs/heads/master@{#18211}
Committed: https://chromium.googlesource.com/external/webrtc/+/31bd224f356b6dff18664370abd475351de4c683
Patch Set 1 #
Total comments: 4
Patch Set 2 : perkj comments 1. #
Total comments: 8
Patch Set 3 : perkj comments 2. #Patch Set 4 : Rebase. #
Messages
Total messages: 27 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
brandtr@webrtc.org changed reviewers: + perkj@webrtc.org
Hey Per, would you mind taking a look at this CL? The main difference is that I map the FlexFEC payload type before the diffing logic, rather than after. This will reduce the number of stream recreations for local endpoints that have not enabled the FlexFEC experiment, when the remote endpoint does have the FlexFEC experiment enabled. Also took the opportunity to make some minor cleanups and add some minor tests.
I am a bit confused. Without the experiment we should never advertise flexfec in neither a response or an offer. So why do we need to check the experiment flag in more than during when setting up the possible codecs to advertise? https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1284: if (map_flexfec_recv_ && there should be no flex fec negotiated there right? Regardless of map_flexfec_recv. https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.h:456: // If |map_flexfec| is true, any "flexfec-03" codec payload type will be /s types
On 2017/05/12 08:37:23, perkj_webrtc wrote:
> I am a bit confused. Without the experiment we should never advertise flexfec
in
> neither a response or an offer. So why do we need to check the experiment flag
> in more than during when setting up the possible codecs to advertise?
That's sort of what I did before, but that has the downside that the VSS/VRS
objects will be recreated when renegotiation happens in the opposite order of
the original negotiation. These recreations has a number of downsides related to
them, so that's why I want to reduce them. I do that by adding the |map_flexfec|
parameter to MapCodecs.
The fact that the field trials are split in two ("advertise" vs. "enable
sending") is for historical reasons.
If you have time, we could discuss this F2F during the afternoon?
>
https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvideoengine2.cc (right):
>
>
https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvideoengine2.cc:1284: if (map_flexfec_recv_ &&
> there should be no flex fec negotiated there right? Regardless of
> map_flexfec_recv.
>
>
https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvideoengine2.h (right):
>
>
https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvideoengine2.h:456: // If |map_flexfec| is true, any
> "flexfec-03" codec payload type will be
> /s types
https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1284: if (map_flexfec_recv_ && On 2017/05/12 08:37:23, perkj_webrtc wrote: > there should be no flex fec negotiated there right? Regardless of > map_flexfec_recv. I think that |sp| can contain FlexFEC SSRCs if the remote has added them to its offer SDP, even if the local does not support the FlexFEC payload type in its answer SDP. But you are right in that this check is sort of extraneous; what matters the most is if the FlexFEC payload type is set or not. I added this check to harmonize with the corresponding check on the sender side: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2882433003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.h:456: // If |map_flexfec| is true, any "flexfec-03" codec payload type will be On 2017/05/12 08:37:23, perkj_webrtc wrote: > /s types There should never be multiple FlexFEC payload types. Either there is none, or there is one. Updated the comment to be more clear about that.
https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1616: if (map_flexfec_send) { rename variable or keep function call. is_flexfec_field_trial_. https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2513: bool map_flexfec) { remove and use IsFlexfecFieldTrialEnabled() in here instead. https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.h:457: // |codecs|, its payload type will be mapped to the VideoCodecSettings of the /s all ? https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.h:512: const bool map_flexfec_recv_; Can we add a comment what these two means?
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
On 2017/05/12 12:05:06, perkj_webrtc wrote: > https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:1616: if (map_flexfec_send) { > rename variable or keep function call. is_flexfec_field_trial_. > > https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:2513: bool map_flexfec) { > remove and use IsFlexfecFieldTrialEnabled() in here instead. > > https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvideoengine2.h (right): > > https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.h:457: // |codecs|, its payload type will > be mapped to the VideoCodecSettings of the > /s all ? > > https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.h:512: const bool map_flexfec_recv_; > Can we add a comment what these two means? Thanks for the discussion, it was helpful to clarify what I was trying to do here! Please take a second look. According to the SDP offer/answer spec, it is legal to change the offered codecs in a 2nd offer: https://tools.ietf.org/html/rfc3264#section-8.3.2 The list of media formats used in the session MAY be changed. To do this, the offerer creates a new media description, with the list of media formats in the "m=" line different from the corresponding media stream in the previous SDP. This list MAY include new formats, and MAY remove formats present from the previous SDP. However, in the JSEP spec it seems illegal to do so. https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-20#section-5.2.2 ... In addition, for each non-recycled, non-rejected m= section in the new offer, the following adjustments are made based on the contents of the corresponding m= section in the current remote description, if any: o The m= line and corresponding "a=rtpmap" and "a=fmtp" lines MUST only include codecs present in the most recent answer which have not been excluded by the codec preferences of the associated transceiver. Note that non-JSEP endpoints are not subject to these restrictions, and might offer media formats that were not present in the most recent answer, as specified in [RFC3264], Section 8. Therefore, JSEP implementations MUST be prepared to receive such offers. Are we violating the spec here?
https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1616: if (map_flexfec_send) { On 2017/05/12 12:05:05, perkj_webrtc wrote: > rename variable or keep function call. is_flexfec_field_trial_. Kept function call. https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2513: bool map_flexfec) { On 2017/05/12 12:05:05, perkj_webrtc wrote: > remove and use IsFlexfecFieldTrialEnabled() in here instead. Done. As was clear from our F2F discussion, however, this optimization is only helpful for the sender side. So I moved the logic to GetChangedSendParameters instead. This makes it easier to understand. https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.h:457: // |codecs|, its payload type will be mapped to the VideoCodecSettings of the On 2017/05/12 12:05:06, perkj_webrtc wrote: > /s all ? Moved the logic instead. https://codereview.webrtc.org/2882433003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.h:512: const bool map_flexfec_recv_; On 2017/05/12 12:05:05, perkj_webrtc wrote: > Can we add a comment what these two means? Removed instead.
Description was changed from ========== Reduce VideoSendStream/VideoReceiveStream recreations due to FlexFEC. This CL reduces the number of object recreations during SDP renegotiation by checking the FlexFEC field trials before, and not after, the SDP codec diffing logic. BUG=webrtc:5654 ========== to ========== Reduce VideoSendStream recreations due to FlexFEC. This CL reduces the number of VideoSendStream recreations during SDP renegotiation by checking the FlexFEC field trials before, and not after, the SDP codec diffing logic. BUG=webrtc:5654 ==========
Rebase.
Sorry, missed this. lgtm
On 2017/05/19 06:46:54, perkj_webrtc wrote: > Sorry, missed this. > > lgtm No worries. Thanks.
The CQ bit was checked by brandtr@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by brandtr@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495197931153400,
"parent_rev": "c52bd61f65d04b364e90a672691963d089ca5141", "commit_rev":
"31bd224f356b6dff18664370abd475351de4c683"}
Message was sent while issue was closed.
Description was changed from ========== Reduce VideoSendStream recreations due to FlexFEC. This CL reduces the number of VideoSendStream recreations during SDP renegotiation by checking the FlexFEC field trials before, and not after, the SDP codec diffing logic. BUG=webrtc:5654 ========== to ========== Reduce VideoSendStream recreations due to FlexFEC. This CL reduces the number of VideoSendStream recreations during SDP renegotiation by checking the FlexFEC field trials before, and not after, the SDP codec diffing logic. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2882433003 Cr-Commit-Position: refs/heads/master@{#18211} Committed: https://chromium.googlesource.com/external/webrtc/+/31bd224f356b6dff18664370a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as https://chromium.googlesource.com/external/webrtc/+/31bd224f356b6dff18664370a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
