Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1818033002:
Embed a cricket::MediaConfig in RTCConfiguration. (Closed)
|
Created:
4 years, 9 months ago by nisse-webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionEmbed a cricket::MediaConfig in RTCConfiguration.
This eliminates some instances rtc:Optional and makes the code
simpler. No changes in defaults or other behaviour are intended.
BUG=webrtc:4906
Committed: https://crrev.com/c36b31b78e6115dcc5783f70f9f37cdbd8e2cbed
Cr-Commit-Position: refs/heads/master@{#12326}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed Harald's comments. #Patch Set 3 : Add TODO comment. #Patch Set 4 : Rebase, and update getters and setters. #Patch Set 5 : Rebased. #
Messages
Total messages: 33 (10 generated)
nisse@webrtc.org changed reviewers: + hta@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org
This reorganizes RTCConfiguration a bit, as we discussed earlier in cl https://codereview.webrtc.org/1717583002. Do you see any way this can break application-specific defaults? My understanding is that the application would first construct an RTCConfiguration, which gets webrtc's defaults for everything. Then it can override any settings it likes with its own defaults. And finally, for each peer connection, the application can apply connection-specific settings (using CopyConstraintsIntoRtcConfiguration or any other means), which will override the defaults.
lgtm although it took me a while to get there. On 2016/03/21 13:52:43, nisse-webrtc wrote: > This reorganizes RTCConfiguration a bit, as we discussed earlier in cl > https://codereview.webrtc.org/1717583002. > > Do you see any way this can break application-specific defaults? > > My understanding is that the application would first construct an > RTCConfiguration, which gets webrtc's defaults for everything. Then it can > override any settings it likes with its own defaults. > > And finally, for each peer connection, the application can apply > connection-specific settings (using CopyConstraintsIntoRtcConfiguration or any > other means), which will override the defaults. Let me analyze.... 4 fields gone from struct RTCConfiguration: - disable_prerenderer_smoothing (bool) - not a constraint, not my concern - enable_dscp - cpu_overuse_detection -> enable_cpu_overuse_detection - suspend_below_min_bitrate Call that uses struct RTCConfiguration are createPeerConnection (2 versions) and SetConfiguration. (Also PeerConnection::Initialize, but that's more-or-less an internal method.) RTCConfiguration is a struct, not a class. And it's created with a constructor without arguments. That means that if constructing an RTCConfiguration is going to fetch defaults, these are either global defaults, or the constructor has grown parameters. Parameters aren't grown, so this is local defaults. And the constructor for RTCConfiguration is still empty, so it doesn't fetch any defaults either. So the only defaults that are being considered are the compiled-in ones in peerconnectioninterface.h. Next question: Does this allow us to do what we could previously do? The removed possibility is to set the 3 fields that were booleans to "not set". The previous code would use the default values from a newly constructed cricket::MediaConfig. This is a struct without a constructor, and is again set from compiled-in defaults. Net conclusion: This CL doesn't change existing behavior. But all the defaults it touches are compiled-in
"Reply" doesn't include the comments. Note: This is a breaking API change. You might want to think about how you roll this in order to make it not break google3 (I would suggest adding the new API, roll, then delete the old one - with google3 automatic import, you don't have the option of making the caller's fixes part of the CL.) https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/mediaconstraintsin... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/mediaconstraintsin... webrtc/api/mediaconstraintsinterface.cc:196: // using a constraint. Is that intentional? I suggest you just change this. It was strange last time I went through this, and it's strange now. This is a smaller CL, so pinpointing this particular issue if it ever arises should be easy. I don't think anyone cares about RTP data channels any more (and Chrome has stopped using the constraints API anyway). https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/mediaconstraintsin... File webrtc/api/mediaconstraintsinterface_unittest.cc (right): https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/mediaconstraintsin... webrtc/api/mediaconstraintsinterface_unittest.cc:25: b.media_config.video.disable_prerenderer_smoothing; The Matches function was written to detect conspicious errors; it's a bug that it doesn't check all fields. Feel free to add or delete fields in the checklist at random. https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectionfact... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectionfact... webrtc/api/peerconnectionfactory.cc:294: std::move(dtls_identity_store), observer)) { Nit: Please run git cl format before submitting.
https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectioninte... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectioninte... webrtc/api/peerconnectioninterface.h:246: struct cricket::MediaConfig media_config; Doesn't this change break chrome and other clients? Also- I am not so happy about moving parts of the config into a sub struct. Why these 4 types and not many of the others such as screencast_min_bintrate or audio_jitter_buffer_max_packets.... I am not so sure this is a step in the right direction.
New version. About the API issues, what can be expected to break? I've tried searching google3 for code accessing members of RTCConfiguration, without finding any (but I didn't find any really good way to narrow down the search). If we need more care here, I think he right way is to follow the RTCConfiguration TODO comment and add getter and setter methods. https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/mediaconstraintsin... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/mediaconstraintsin... webrtc/api/mediaconstraintsinterface.cc:196: // using a constraint. Is that intentional? On 2016/03/21 14:38:21, hta-webrtc wrote: > I suggest you just change this. It was strange last time I went through this, > and it's strange now. This is a smaller CL, so pinpointing this particular issue > if it ever arises should be easy. > > I don't think anyone cares about RTP data channels any more (and Chrome has > stopped using the constraints API anyway). Done. I'm also renaming value --> enable_ipv6, since that's the only remaining use. https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectionfact... File webrtc/api/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectionfact... webrtc/api/peerconnectionfactory.cc:294: std::move(dtls_identity_store), observer)) { On 2016/03/21 14:38:21, hta-webrtc wrote: > Nit: Please run git cl format before submitting. Done. But no changes at this location.
On 2016/03/22 08:31:25, perkj_webrtc wrote: > https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectioninte... > File webrtc/api/peerconnectioninterface.h (right): > > https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectioninte... > webrtc/api/peerconnectioninterface.h:246: struct cricket::MediaConfig > media_config; > Doesn't this change break chrome and other clients? > > Also- I am not so happy about moving parts of the config into a sub struct. Why > these 4 types and not many of the others such as screencast_min_bintrate or > audio_jitter_buffer_max_packets.... I am not so sure this is a step in the > right direction. On one hand, I think it makes the implementation simpler to collect the various flags in the same way as they're actually going to be used. But on the other hand, I see that it's not a very clean interface for applications, since this is implementation details leaking out. So it's a judgement call. To be able to move things around without breaking applications, I think it would make sense to add RTCConfiguration methods to access members, e.g., if the application calls config.set_enable_dscp(true), it doesn't need to know that the flag in question is stored inside the embedded media_config.
On 2016/03/22 09:54:19, nisse-webrtc wrote: > On 2016/03/22 08:31:25, perkj_webrtc wrote: > > > https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectioninte... > > File webrtc/api/peerconnectioninterface.h (right): > > > > > https://codereview.webrtc.org/1818033002/diff/1/webrtc/api/peerconnectioninte... > > webrtc/api/peerconnectioninterface.h:246: struct cricket::MediaConfig > > media_config; > > Doesn't this change break chrome and other clients? > > > > Also- I am not so happy about moving parts of the config into a sub struct. > Why > > these 4 types and not many of the others such as screencast_min_bintrate or > > audio_jitter_buffer_max_packets.... I am not so sure this is a step in the > > right direction. > > On one hand, I think it makes the implementation simpler to collect the various > flags in the same way as they're actually going to be used. But on the other > hand, I see that it's not a very clean interface for applications, since this is > implementation details leaking out. So it's a judgement call. > > To be able to move things around without breaking applications, I think it would > make sense to add RTCConfiguration methods to access members, e.g., if the > application calls config.set_enable_dscp(true), it doesn't need to know that the > flag in question is stored inside the embedded media_config. that make sence
On 2016/03/22 08:33:20, nisse-webrtc wrote: > New version. About the API issues, what can be expected to break? I've tried > searching google3 for code accessing members of RTCConfiguration, without > finding any (but I didn't find any really good way to narrow down the search). Talk to Kjellander about this. I think there's a way to run the Google3 roll as a trial run to see if your CL breaks anything. TAP is quite powerful. Chrome is the one I know will break, but there we can finagle our way in the roll CL. > > If we need more care here, I think he right way is to follow the > RTCConfiguration TODO comment and add getter and setter methods. Won't help in getting at the clients that access this. The 3-step method is to define a new struct called RTCConfigurationv2 (or some less stupid name) with the new struct, add a shim function with the old call signature that converts the old struct to the new struct, and a new API function that has the new argument. Roll this. Then convert the callers. Then remove the RTCConfiguration(v1) struct and the shim function. Exactly the same procedure has to be followed when switching from direct-access to getters/setters, so that won't make your life any simpler (whether it leaves you in a better place depends entirely on whether or not you believe in getters/setters).
On 2016/03/22 10:17:24, hta-webrtc wrote: > On 2016/03/22 08:33:20, nisse-webrtc wrote: > > New version. About the API issues, what can be expected to break? I've tried > > searching google3 for code accessing members of RTCConfiguration, without > > finding any (but I didn't find any really good way to narrow down the search). > > Talk to Kjellander about this. I think there's a way to run the Google3 roll as > a trial run > to see if your CL breaks anything. TAP is quite powerful. > > Chrome is the one I know will break, but there we can finagle our way in the > roll CL. > > > > > If we need more care here, I think he right way is to follow the > > RTCConfiguration TODO comment and add getter and setter methods. > > Won't help in getting at the clients that access this. > The 3-step method is to define a new struct called RTCConfigurationv2 (or some > less stupid name) with the new struct, > add a shim function with the old call signature that converts the old struct to > the new struct, and a new API function that > has the new argument. > > Roll this. > > Then convert the callers. > > Then remove the RTCConfiguration(v1) struct and the shim function. > > Exactly the same procedure has to be followed when switching from direct-access > to getters/setters, so that won't > make your life any simpler (whether it leaves you in a better place depends > entirely on whether or not you believe in getters/setters). I was thinking, that if step 1 is adding getters and setters for all fields which are actually accessed by applications, step 2 is converting applications to use them, then we can reorganize the RTCConfiguration however we please in step 3.
lgtm
So is the plan to add setters and getters later? lgtm if so. Just check that you don't break chrome or an import.
I think the change breaks chrome's content/renderer/media/rtc_peer_connection_handler.cc. So I think I'll have to do it as follows: 1. Write a separate cl which adds getters and setters for the attributes which belong in MediaConfig. 2. Update chrome to use those methods. 3. Rebase and land this cl. Any conventions for getters and setters that I ought to know about? I have also tried the import machinery, and as far as I have found, nothing breaks there.
On 2016/03/29 11:08:02, nisse-webrtc wrote: > I think the change breaks chrome's > content/renderer/media/rtc_peer_connection_handler.cc. So I think I'll have to > do it as follows: > > 1. Write a separate cl which adds getters and setters for the attributes which > belong in MediaConfig. > > 2. Update chrome to use those methods. > > 3. Rebase and land this cl. > > Any conventions for getters and setters that I ought to know about? > > I have also tried the import machinery, and as far as I have found, nothing > breaks there. Step 1 done now.
On 2016/03/31 11:10:51, nisse-webrtc wrote: > On 2016/03/29 11:08:02, nisse-webrtc wrote: > > I think the change breaks chrome's > > content/renderer/media/rtc_peer_connection_handler.cc. So I think I'll have to > > do it as follows: > > > > 1. Write a separate cl which adds getters and setters for the attributes which > > belong in MediaConfig. > > > > 2. Update chrome to use those methods. > > > > 3. Rebase and land this cl. > > > > Any conventions for getters and setters that I ought to know about? > > > > I have also tried the import machinery, and as far as I have found, nothing > > breaks there. > > Step 1 done now. Step 2 done now. I'll try to land this now, and see if it breaks anything.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1818033002/#ps80001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818033002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818033002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/2395)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818033002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818033002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/2397)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818033002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818033002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818033002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818033002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Embed a cricket::MediaConfig in RTCConfiguration. This eliminates some instances rtc:Optional and makes the code simpler. No changes in defaults or other behaviour are intended. BUG=webrtc:4906 ========== to ========== Embed a cricket::MediaConfig in RTCConfiguration. This eliminates some instances rtc:Optional and makes the code simpler. No changes in defaults or other behaviour are intended. BUG=webrtc:4906 Committed: https://crrev.com/c36b31b78e6115dcc5783f70f9f37cdbd8e2cbed Cr-Commit-Position: refs/heads/master@{#12326} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c36b31b78e6115dcc5783f70f9f37cdbd8e2cbed Cr-Commit-Position: refs/heads/master@{#12326} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
