Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15120009:
Pass the sctp command-line flag to Libjingle as an internal-only constraint. (Closed)
|
Created:
7 years, 7 months ago by jiayl Modified:
7 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPass the sctp command-line flag to Libjingle as an internal-only constraint.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201899
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 8
Patch Set 4 : #Patch Set 5 : #Messages
Total messages: 19 (0 generated)
Could you take a look?
I'll leave it up to Justin to OK the Add method. https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... File content/renderer/media/rtc_media_constraints.cc (right): https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... content/renderer/media/rtc_media_constraints.cc:34: if (new_constraint.key.find("internal") == 0) if "internal" is a prefix, should we use StartsWith[ASCII] instead of find()? see base/string_util.h. https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... content/renderer/media/rtc_peer_connection_handler.cc:343: static bool sctp_data_channel_enabled = nit: no need to cache this in a static. initialize is rarely called and we usually don't cache HasSwitch() results. https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... content/renderer/media/rtc_peer_connection_handler.cc:357: // webrtc::MediaConstraintsInterface::kEnbaleSctpDataChannels when s/kEnbale/kEnable https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... content/renderer/media/rtc_peer_connection_handler.cc:358: // the Libjingle change is rolled. when will the libjingle change be rolled? Could we perhaps just roll it before we land this cl?
Thinking about this some more, this seems like a misuse of the constraints to pass parameters to libjingle. Constraints are a part of the javascript API but this change is to mix in other values that must not be set from javascript, into the same string map. Can you share some background for why this approach was chosen? On Tue, May 21, 2013 at 10:03 AM, <tommi@chromium.org> wrote: > I'll leave it up to Justin to OK the Add method. > > > https://codereview.chromium.**org/15120009/diff/5001/** > content/renderer/media/rtc_**media_constraints.cc<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_media_constraints.cc> > File content/renderer/media/rtc_**media_constraints.cc (right): > > https://codereview.chromium.**org/15120009/diff/5001/** > content/renderer/media/rtc_**media_constraints.cc#newcode34<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_media_constraints.cc#newcode34> > content/renderer/media/rtc_**media_constraints.cc:34: if > > (new_constraint.key.find("**internal") == 0) > if "internal" is a prefix, should we use StartsWith[ASCII] instead of > find()? > see base/string_util.h. > > https://codereview.chromium.**org/15120009/diff/5001/** > content/renderer/media/rtc_**peer_connection_handler.cc<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc> > File content/renderer/media/rtc_**peer_connection_handler.cc (right): > > https://codereview.chromium.**org/15120009/diff/5001/** > content/renderer/media/rtc_**peer_connection_handler.cc#**newcode343<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode343> > content/renderer/media/rtc_**peer_connection_handler.cc:**343: static bool > sctp_data_channel_enabled = > nit: no need to cache this in a static. initialize is rarely called and > we usually don't cache HasSwitch() results. > > https://codereview.chromium.**org/15120009/diff/5001/** > content/renderer/media/rtc_**peer_connection_handler.cc#**newcode357<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode357> > content/renderer/media/rtc_**peer_connection_handler.cc:**357: // > webrtc::**MediaConstraintsInterface::**kEnbaleSctpDataChannels when > s/kEnbale/kEnable > > https://codereview.chromium.**org/15120009/diff/5001/** > content/renderer/media/rtc_**peer_connection_handler.cc#**newcode358<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode358> > content/renderer/media/rtc_**peer_connection_handler.cc:**358: // the > Libjingle change is rolled. > when will the libjingle change be rolled? Could we perhaps just roll it > before we land this cl? > > https://codereview.chromium.**org/15120009/<https://codereview.chromium.org/1... >
The decision is made in https://critique.corp.google.com/#review/46620008-p10. A summary: We considered these three options: 1. add a new method SetOption to PeerConnectionInterface 2. add a new constraint kEnableSctpDataChannels; the glue code strips any value set by JS and fills in the value of the command-line flag. 3. Similar to #2, but extend MediaConstraintInterface to have a new category of constraints (named internal/reserved/...?) which is not visible to JS. Justin and Ronghua both think #2 is the best way to solve it. On Tue, May 21, 2013 at 5:01 AM, Tommi <tommi@chromium.org> wrote: > Thinking about this some more, this seems like a misuse of the constraints > to pass parameters to libjingle. > Constraints are a part of the javascript API but this change is to mix in > other values that must not be set from javascript, into the same string map. > Can you share some background for why this approach was chosen? > > > On Tue, May 21, 2013 at 10:03 AM, <tommi@chromium.org> wrote: > >> I'll leave it up to Justin to OK the Add method. >> >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**media_constraints.cc<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_media_constraints.cc> >> File content/renderer/media/rtc_**media_constraints.cc (right): >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**media_constraints.cc#newcode34<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_media_constraints.cc#newcode34> >> content/renderer/media/rtc_**media_constraints.cc:34: if >> >> (new_constraint.key.find("**internal") == 0) >> if "internal" is a prefix, should we use StartsWith[ASCII] instead of >> find()? >> see base/string_util.h. >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**peer_connection_handler.cc<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc> >> File content/renderer/media/rtc_**peer_connection_handler.cc (right): >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**peer_connection_handler.cc#**newcode343<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode343> >> content/renderer/media/rtc_**peer_connection_handler.cc:**343: static >> bool >> sctp_data_channel_enabled = >> nit: no need to cache this in a static. initialize is rarely called and >> we usually don't cache HasSwitch() results. >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**peer_connection_handler.cc#**newcode357<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode357> >> content/renderer/media/rtc_**peer_connection_handler.cc:**357: // >> webrtc::**MediaConstraintsInterface::**kEnbaleSctpDataChannels when >> s/kEnbale/kEnable >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**peer_connection_handler.cc#**newcode358<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode358> >> content/renderer/media/rtc_**peer_connection_handler.cc:**358: // the >> Libjingle change is rolled. >> when will the libjingle change be rolled? Could we perhaps just roll it >> before we land this cl? >> >> https://codereview.chromium.**org/15120009/<https://codereview.chromium.org/1... >> > >
PTAL. Thanks! https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... File content/renderer/media/rtc_media_constraints.cc (right): https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... content/renderer/media/rtc_media_constraints.cc:34: if (new_constraint.key.find("internal") == 0) On 2013/05/21 08:03:24, tommi wrote: > if "internal" is a prefix, should we use StartsWith[ASCII] instead of find()? > see base/string_util.h. Done. https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... content/renderer/media/rtc_peer_connection_handler.cc:343: static bool sctp_data_channel_enabled = On 2013/05/21 08:03:24, tommi wrote: > nit: no need to cache this in a static. initialize is rarely called and we > usually don't cache HasSwitch() results. Done. https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... content/renderer/media/rtc_peer_connection_handler.cc:357: // webrtc::MediaConstraintsInterface::kEnbaleSctpDataChannels when On 2013/05/21 08:03:24, tommi wrote: > s/kEnbale/kEnable Done. https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc... content/renderer/media/rtc_peer_connection_handler.cc:358: // the Libjingle change is rolled. The libjingle CL adding these strings depends on another libjingle CL that is not approved yet. They should be committed and rolled by end of this week. I submitted this CL in parallel per discussion with Justin, in order to minimize the delay and leave enough baking time so that we can remove the flag by M29 cut. On 2013/05/21 08:03:24, tommi wrote: > when will the libjingle change be rolled? Could we perhaps just roll it before > we land this cl?
Tommi, can you commit your CL of adding the switch after you approve this patch?
Understand the concern - I was somewhat hesitant at first, but it seems that otherwise we'd eventually end up creating a parallel "internal constraints" API to pass in the config data (this is the option #1 Jiayang mentions), so I was persuaded. The risk of an internal constraint being set from JS is a valid concern, but I think we can deal with that by performing a strip of any such internal constraints as soon as we see them, and we can back this up with unit tests. Open to other suggestions though if you see an elegant solution. On Tue, May 21, 2013 at 5:01 AM, Tommi <tommi@chromium.org> wrote: > Thinking about this some more, this seems like a misuse of the constraints > to pass parameters to libjingle. > Constraints are a part of the javascript API but this change is to mix in > other values that must not be set from javascript, into the same string map. > Can you share some background for why this approach was chosen? > > > On Tue, May 21, 2013 at 10:03 AM, <tommi@chromium.org> wrote: > >> I'll leave it up to Justin to OK the Add method. >> >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**media_constraints.cc<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_media_constraints.cc> >> File content/renderer/media/rtc_**media_constraints.cc (right): >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**media_constraints.cc#newcode34<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_media_constraints.cc#newcode34> >> content/renderer/media/rtc_**media_constraints.cc:34: if >> >> (new_constraint.key.find("**internal") == 0) >> if "internal" is a prefix, should we use StartsWith[ASCII] instead of >> find()? >> see base/string_util.h. >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**peer_connection_handler.cc<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc> >> File content/renderer/media/rtc_**peer_connection_handler.cc (right): >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**peer_connection_handler.cc#**newcode343<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode343> >> content/renderer/media/rtc_**peer_connection_handler.cc:**343: static >> bool >> sctp_data_channel_enabled = >> nit: no need to cache this in a static. initialize is rarely called and >> we usually don't cache HasSwitch() results. >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**peer_connection_handler.cc#**newcode357<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode357> >> content/renderer/media/rtc_**peer_connection_handler.cc:**357: // >> webrtc::**MediaConstraintsInterface::**kEnbaleSctpDataChannels when >> s/kEnbale/kEnable >> >> https://codereview.chromium.**org/15120009/diff/5001/** >> content/renderer/media/rtc_**peer_connection_handler.cc#**newcode358<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode358> >> content/renderer/media/rtc_**peer_connection_handler.cc:**358: // the >> Libjingle change is rolled. >> when will the libjingle change be rolled? Could we perhaps just roll it >> before we land this cl? >> >> https://codereview.chromium.**org/15120009/<https://codereview.chromium.org/1... >> > >
OK, I discussed this with Henrik as well and it seems that there's a temporary need for something like this so that's fine with me. On Wed, May 22, 2013 at 7:32 AM, Justin Uberti <juberti@google.com> wrote: > Understand the concern - I was somewhat hesitant at first, but it seems > that otherwise we'd eventually end up creating a parallel "internal > constraints" API to pass in the config data (this is the option #1 Jiayang > mentions), so I was persuaded. > > The risk of an internal constraint being set from JS is a valid concern, > but I think we can deal with that by performing a strip of any such > internal constraints as soon as we see them, and we can back this up with > unit tests. > > Open to other suggestions though if you see an elegant solution. > > > On Tue, May 21, 2013 at 5:01 AM, Tommi <tommi@chromium.org> wrote: > >> Thinking about this some more, this seems like a misuse of the >> constraints to pass parameters to libjingle. >> Constraints are a part of the javascript API but this change is to mix in >> other values that must not be set from javascript, into the same string map. >> Can you share some background for why this approach was chosen? >> >> >> On Tue, May 21, 2013 at 10:03 AM, <tommi@chromium.org> wrote: >> >>> I'll leave it up to Justin to OK the Add method. >>> >>> >>> https://codereview.chromium.**org/15120009/diff/5001/** >>> content/renderer/media/rtc_**media_constraints.cc<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_media_constraints.cc> >>> File content/renderer/media/rtc_**media_constraints.cc (right): >>> >>> https://codereview.chromium.**org/15120009/diff/5001/** >>> content/renderer/media/rtc_**media_constraints.cc#newcode34<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_media_constraints.cc#newcode34> >>> content/renderer/media/rtc_**media_constraints.cc:34: if >>> >>> (new_constraint.key.find("**internal") == 0) >>> if "internal" is a prefix, should we use StartsWith[ASCII] instead of >>> find()? >>> see base/string_util.h. >>> >>> https://codereview.chromium.**org/15120009/diff/5001/** >>> content/renderer/media/rtc_**peer_connection_handler.cc<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc> >>> File content/renderer/media/rtc_**peer_connection_handler.cc (right): >>> >>> https://codereview.chromium.**org/15120009/diff/5001/** >>> content/renderer/media/rtc_**peer_connection_handler.cc#**newcode343<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode343> >>> content/renderer/media/rtc_**peer_connection_handler.cc:**343: static >>> bool >>> sctp_data_channel_enabled = >>> nit: no need to cache this in a static. initialize is rarely called and >>> we usually don't cache HasSwitch() results. >>> >>> https://codereview.chromium.**org/15120009/diff/5001/** >>> content/renderer/media/rtc_**peer_connection_handler.cc#**newcode357<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode357> >>> content/renderer/media/rtc_**peer_connection_handler.cc:**357: // >>> webrtc::**MediaConstraintsInterface::**kEnbaleSctpDataChannels when >>> s/kEnbale/kEnable >>> >>> https://codereview.chromium.**org/15120009/diff/5001/** >>> content/renderer/media/rtc_**peer_connection_handler.cc#**newcode358<https://codereview.chromium.org/15120009/diff/5001/content/renderer/media/rtc_peer_connection_handler.cc#newcode358> >>> content/renderer/media/rtc_**peer_connection_handler.cc:**358: // the >>> Libjingle change is rolled. >>> when will the libjingle change be rolled? Could we perhaps just roll it >>> before we land this cl? >>> >>> https://codereview.chromium.**org/15120009/<https://codereview.chromium.org/1... >>> >> >> >
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15120009/12001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
On 2013/05/22 00:38:36, jiayl wrote: > Tommi, > > can you commit your CL of adding the switch after you approve this patch? I checked the commit box, but libjingle hasn't been rolled, so the change isn't going in. Did you mean something else perhaps? (I don't have a CL that adds a switch)
I meant https://codereview.chromium.org/12984005/, which my CL depends on. On 2013/05/22 08:38:21, tommi wrote: > On 2013/05/22 00:38:36, jiayl wrote: > > Tommi, > > > > can you commit your CL of adding the switch after you approve this patch? > > I checked the commit box, but libjingle hasn't been rolled, so the change isn't > going in. Did you mean something else perhaps? (I don't have a CL that adds a > switch)
Ah, that's Tommy's CL, not mine. cc-ing Tommy. On Wed, May 22, 2013 at 6:34 PM, <jiayl@chromium.org> wrote: > I meant https://codereview.chromium.**org/12984005/<https://codereview.chromium.org/1..., > which my CL depends on. > > > > On 2013/05/22 08:38:21, tommi wrote: > >> On 2013/05/22 00:38:36, jiayl wrote: >> > Tommi, >> > >> > can you commit your CL of adding the switch after you approve this >> patch? >> > > I checked the commit box, but libjingle hasn't been rolled, so the change >> > isn't > >> going in. Did you mean something else perhaps? (I don't have a CL that >> adds >> > a > >> switch) >> > > > https://chromiumcodereview.**appspot.com/15120009/<https://chromiumcodereview... >
My patch is now committed as r201795. On 2013/05/22 16:38:29, tommi wrote: > Ah, that's Tommy's CL, not mine. > cc-ing Tommy. > > > On Wed, May 22, 2013 at 6:34 PM, <mailto:jiayl@chromium.org> wrote: > > > I meant > https://codereview.chromium.**org/12984005/%3Chttps://codereview.chromium.org...>, > > which my CL depends on. > > > > > > > > On 2013/05/22 08:38:21, tommi wrote: > > > >> On 2013/05/22 00:38:36, jiayl wrote: > >> > Tommi, > >> > > >> > can you commit your CL of adding the switch after you approve this > >> patch? > >> > > > > I checked the commit box, but libjingle hasn't been rolled, so the change > >> > > isn't > > > >> going in. Did you mean something else perhaps? (I don't have a CL that > >> adds > >> > > a > > > >> switch) > >> > > > > > > > https://chromiumcodereview.**appspot.com/15120009/%3Chttps://chromiumcoderevi...> > >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15120009/12001
Failed to apply patch for content/renderer/media/rtc_media_constraints.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/renderer/media/rtc_media_constraints.cc
Hunk #1 FAILED at 4.
Hunk #2 FAILED at 26.
Hunk #3 succeeded at 71 with fuzz 1 (offset 13 lines).
2 out of 3 hunks FAILED -- saving rejects to file
content/renderer/media/rtc_media_constraints.cc.rej
Patch: content/renderer/media/rtc_media_constraints.cc
Index: content/renderer/media/rtc_media_constraints.cc
diff --git a/content/renderer/media/rtc_media_constraints.cc
b/content/renderer/media/rtc_media_constraints.cc
index
9118152f0bc9230272dacbec4efdb5d26999bc4e..e6fe734f07e63a6ef2082ff4e85e973bcddf94be
100644
--- a/content/renderer/media/rtc_media_constraints.cc
+++ b/content/renderer/media/rtc_media_constraints.cc
@@ -4,7 +4,7 @@
#include "content/renderer/media/rtc_media_constraints.h"
#include "base/logging.h"
-
+#include "base/string_util.h"
#include "content/common/media/media_stream_options.h"
#include
"third_party/WebKit/Source/Platform/chromium/public/WebMediaConstraints.h"
#include "third_party/WebKit/Source/Platform/chromium/public/WebCString.h"
@@ -26,6 +26,14 @@ void GetNativeMediaConstraints(
if (new_constraint.key == kMediaStreamSource ||
new_constraint.key == kMediaStreamSourceId)
continue;
+
+ // Ignore internal constraints set by JS.
+ // TODO(jiayl): replace the hard coded string with
+ // webrtc::MediaConstraintsInterface::kInternalConstraintPrefix when
+ // the Libjingle change is rolled.
+ if (StartsWithASCII(new_constraint.key, "internal", true))
+ continue;
+
DVLOG(3) << "MediaStreamConstraints:" << new_constraint.key
<< " : " << new_constraint.value;
native_constraints->push_back(new_constraint);
@@ -58,4 +66,10 @@ RTCMediaConstraints::GetOptional() const {
return optional_;
}
+void RTCMediaConstraints::AddOptional(
+ const std::string& key, const std::string& value) {
+ webrtc::MediaConstraintsInterface::Constraint new_constraint(key, value);
+ optional_.push_back(new_constraint);
+}
+
} // namespace content
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/15120009/41001
Message was sent while issue was closed.
Change committed as 201899 |
