Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1623543002:
Refactor RtpSender and SSRCDatabase a bit. (Closed)
|
Created:
4 years, 11 months ago by tommi Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, zhuangzesen_agora.io, tterriberry_mozilla.com, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRefactor RtpSender and SSRCDatabase.
* SSRCDatabase doesn't need to be a global instance, so I've changed it to be a "regular" class (i.e. construct via ctor, not maybe via GetSSRCDatabase( + release via ReturnSSRCDatabase())). If we ever have parallel tests running in the same process, they won't have the problem of using the same ssrc database.
* Made RtpSender a more const. Also added some todos for myself and holmer to look into clarifying the threading model.
* Switched from CriticalSectionWrapper to rtc::CriticalSection
* Changed the random seeding to use TickTime::Now().Ticks() since TimeInMicroseconds() could return 0 when the process was starting. This is what TimeInMicroseconds() does anyway but now we don't need to access a global clock object.
BUG=webrtc:3062
Committed: https://crrev.com/ae695e95a677b76be5a4e0c7051a964103340729
Cr-Commit-Position: refs/heads/master@{#11462}
Patch Set 1 #Patch Set 2 : Fix issue with tick count/clock. #Patch Set 3 : Revert tick change and just use the ticks #Patch Set 4 : Do we even need locks? #Patch Set 5 : Add locking back for voe_auto_test #
Total comments: 13
Patch Set 6 : Add TODOs for Random() and delete dead code from Timing (move to separate cl?) #
Total comments: 1
Patch Set 7 : Move member variables #Patch Set 8 : Remove thread checker due to voe::ChannelOwner #
Total comments: 12
Patch Set 9 : Address comments and revert changes #Patch Set 10 : Remove scoped_ptr include #Patch Set 11 : Fix build? #
Total comments: 8
Patch Set 12 : Address comments #Patch Set 13 : Rebase #
Total comments: 3
Patch Set 14 : Update comment #Patch Set 15 : Update comment and remove the one from ssrc_database.cc #
Messages
Total messages: 65 (13 generated)
Fix issue with tick count/clock.
Revert tick change and just use the ticks
Do we even need locks?
Add locking back for voe_auto_test
Description was changed from ========== Refactor RtpSender and SSRCDatabase a bit. Make more const and use SSRCDatabase differently, no static_instance (e.g. no conflicts between tests and deterministic lifetime). BUG= ========== to ========== Refactor RtpSender and SSRCDatabase. * SSRCDatabase doesn't need to be a global instance, so I've changed it to be a "regular" class (i.e. construct via ctor, not maybe via GetSSRCDatabase( + release via ReturnSSRCDatabase())). If we ever have parallel tests running in the same process, they won't have the problem of using the same ssrc database. * Made RtpSender a more const. Also added some todos for myself and holmer to look into clarifying the threading model. * Switched from CriticalSectionWrapper to rtc::CriticalSection * Changed the random seeding to use TickTime::Now().Ticks() since TimeInMicroseconds() could return 0 when the process was starting. This is what TimeInMicroseconds() does anyway but now we don't need to access a global clock object. BUG= ==========
tommi@webrtc.org changed reviewers: + stefan@webrtc.org
I think the reason for SSRCDatabase being a singleton was to make sure different rtp modules in the same call didn't risk generating the same SSRCs, which would cause conflicts which we may not be handling in the upper layers. I think it might be safe to remove this functionality entirely, because libjingle always supplies its own SSRCs (although I haven't double checked that's always the case). Not sure what's the situation with firefox though? If we decide to make this change (and not remove entirely), wouldn't it make more sense to have the database live in webrtc::internal::Call so that it can be shared for all streams within a call? https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:190: // early on in the process. I think it's because timestamps and sequence numbers should start randomly. https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:194: // TODO(holmer, tommi): Should we DCHECK ^^^ on the 'Can't be 0' part? Feel free to add. According to SsrcDatabase that should be fine. https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:202: // TODO(tommi,holmer: We don't grab locks in the dtor before accessing member TODO(tommi,holmer) I think the only known model for this class is that the user needs to guarantee that all methods have finished when the destructor is called. Agree that it would be good to make it more restricted and document that. https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.h:395: // it could be a regular member variable. I guess the same is true for RTPSenderAudio/Video below too? https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:142: // TODO: These variables should be below the methods. Add an owner to the todo, or could you simply move them? :) https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ssrc_database.cc (right): https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ssrc_database.cc:22: SSRCDatabase::SSRCDatabase() : random_(TickTime::Now().Ticks()) {} Would it make sense to have a static metod in Random to get a seed and move this comment there? Feels like we may end up having this comment in a lot of different places otherwise.
Add TODOs for Random() and delete dead code from Timing (move to separate cl?)
https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:190: // early on in the process. On 2016/01/23 17:38:39, stefan-webrtc (holmer) wrote: > I think it's because timestamps and sequence numbers should start randomly. I might be misunderstanding, so please bear with me :) The timestamp or tick_util.* implementation that's supposed to return the current tick count, actually always returns a very low value on first call and subsequent calls will return a value relative to when the tick count was first fetched. It's actually not that random and doesn't reflect the tick count as it does on other platforms. Maybe it's simply a bug (pbos knows more). The call to srand() here, doesn't affect the Random() class from what I can tell and is also not related to timestamps. So, I don't see why it is needed or why calling it should be done in a particular instance of RtpSender. If the requirement comes from SSRCDatabase, then I'd move it to the ctor of SSRCDatabase. https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:194: // TODO(holmer, tommi): Should we DCHECK ^^^ on the 'Can't be 0' part? On 2016/01/23 17:38:39, stefan-webrtc (holmer) wrote: > Feel free to add. According to SsrcDatabase that should be fine. Done. https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:202: // TODO(tommi,holmer: We don't grab locks in the dtor before accessing member On 2016/01/23 17:38:39, stefan-webrtc (holmer) wrote: > TODO(tommi,holmer) Done > I think the only known model for this class is that the user needs to guarantee > that all methods have finished when the destructor is called. Agree that it > would be good to make it more restricted and document that. OK, I added a thread checker to the dtor to start with, which makes sure that the sender instance is deleted on the same thread it was created on. We can build out from there for other methods. https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.h:395: // it could be a regular member variable. On 2016/01/23 17:38:40, stefan-webrtc (holmer) wrote: > I guess the same is true for RTPSenderAudio/Video below too? Moved the BitrateAggregator class declaration. For RTPSenderAudio/Video we need to keep it as is do to the |bool audio| variable passed to the ctor - or can it be done some other way? https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/ssrc_database.cc (right): https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/ssrc_database.cc:22: SSRCDatabase::SSRCDatabase() : random_(TickTime::Now().Ticks()) {} On 2016/01/23 17:38:40, stefan-webrtc (holmer) wrote: > Would it make sense to have a static metod in Random to get a seed and move this > comment there? Feels like we may end up having this comment in a lot of > different places otherwise. Indeed - looks like all users of Random are calling the ctor with clock_->TimeInMicroseconds(). It would be better to make the Random class take care of this but since it's in webrtc/base, it can't depend on TickUtil, which is in system_wrappers. I'll do it in a follow up CL.
Move member variables
https://codereview.webrtc.org/1623543002/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/1623543002/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:143: SSRCDatabase ssrc_database_; oops, forgot to move this (this TODO was meant for myself before sending out the review request... sorry about that)
Remove thread checker due to voe::ChannelOwner
On 2016/01/24 13:59:31, tommi-webrtc wrote: > Remove thread checker due to voe::ChannelOwner Please see my overall comment in #8 too before doing any more work, since I'm not entirely sure we're doing the right thing here.
On 2016/01/24 14:47:12, stefan-webrtc (holmer) wrote: > On 2016/01/24 13:59:31, tommi-webrtc wrote: > > Remove thread checker due to voe::ChannelOwner > > Please see my overall comment in #8 too before doing any more work, since I'm > not entirely sure we're doing the right thing here. It looked to me like we're always calling CreateSSRC() at least in the ctor. Do you mean that ssrc_forced_ would always be true then? I can check if that's the case but doubt it's the case in tests at least. Would be great if we can just clean this up instead.
On 2016/01/24 15:12:16, tommi-webrtc wrote: > On 2016/01/24 14:47:12, stefan-webrtc (holmer) wrote: > > On 2016/01/24 13:59:31, tommi-webrtc wrote: > > > Remove thread checker due to voe::ChannelOwner > > > > Please see my overall comment in #8 too before doing any more work, since I'm > > not entirely sure we're doing the right thing here. > > It looked to me like we're always calling CreateSSRC() at least in the ctor. > Do you mean that ssrc_forced_ would always be true then? I can check if that's > the case but doubt it's the case in tests at least. > Would be great if we can just clean this up instead. Actually, I think it would be alright to land a cl like this (assuming all changes are agreed upon) since it doesn't change the way things work today but rather clarifies it. Removing the use of GetStaticInstance for example is a good thing since it's basically the only place where it's used aside from tracing.
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
drive-by... https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h File webrtc/base/random.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h#new... webrtc/base/random.h:31: // of TickTime::Now()). Maybe, but the ctor taking a seed is still needed for tests. We want (unit) tests to be reproducible, not randomized (fuzzers are a different matter of course). That, and the issue with global state was the reason to create this class and not use std::rand(). Also, depending on what you actually mean we should use to init the seed, it doesn't ring right to me adding a dependency on TickTime (or Clock) to a PRNG. The PRNG is a platform-independent number crunching utility. Anything we can keep platform independent, we should. Suggest instead: a) Add a TickTime::Get64BitForPRNGSeed() utility function which can be used with whichever PRNG class/impl we have now or in the future. b) If that is too inconvenient, invent a template function which hooks up above utility to a PRNG class.
thanks for taking a look https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h File webrtc/base/random.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h#new... webrtc/base/random.h:31: // of TickTime::Now()). On 2016/01/24 21:34:10, the sun wrote: > Maybe, but the ctor taking a seed is still needed for tests. We want (unit) > tests to be reproducible, not randomized (fuzzers are a different matter of > course). That, and the issue with global state was the reason to create this > class and not use std::rand(). Maybe we can do both? If Random calls an overridable function to get the seed, tests could use that. Today all callers provide a seed from the same source. btw, could you point me to a bug or something related to using this implementation instead of std::rand? I'm not following why this class couldn't use it internally at least. > Also, depending on what you actually mean we should use to init the seed, it > doesn't ring right to me adding a dependency on TickTime (or Clock) to a PRNG. > The PRNG is a platform-independent number crunching utility. Anything we can > keep platform independent, we should. To be clear, this depends on TickTime today indirectly (via Clock) and there's a problem with that on Mac due to how TickTime::Now() is implemented. I don't think I'm following the comment about platform independence though. Sounds like you're reading the comment as being about making the code platform dependent, which is not the point. TickTime is itself cross platform interface but has platform specific implementations. So too would Get64BitForPRNGSeed be, I imagine? > Suggest instead: > a) Add a TickTime::Get64BitForPRNGSeed() utility function which can be used with > whichever PRNG class/impl we have now or in the future. > b) If that is too inconvenient, invent a template function which hooks up above > utility to a PRNG class. How would it be implemented?
mflodman@webrtc.org changed reviewers: + mflodman@webrtc.org
https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:341: SSRCDatabase ssrc_database_; Drive-by and Stefan might already have commented on this, but: - The intention with the database is to ensure no ssrc is reused across all sending and receiving streams. - The new video API already checks this when creating send and receive streams, so there is no real use case for the database in the video engine. - VoiceEngine might still depend on this being checked across streams though, so this change will then break currently use VoiceEngine functionality.
On 2016/01/25 07:26:19, mflodman wrote: > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right): > > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:341: SSRCDatabase ssrc_database_; > Drive-by and Stefan might already have commented on this, but: > - The intention with the database is to ensure no ssrc is reused across all > sending and receiving streams. > - The new video API already checks this when creating send and receive streams, > so there is no real use case for the database in the video engine. > - VoiceEngine might still depend on this being checked across streams though, so > this change will then break currently use VoiceEngine functionality. (apologies for spamming - replying again via rietveld) I'm assuming the problem has to do with the lifetime of the ModuleRtpRtcpImpl instance, is that right? If so, would it make more sense for the SSRCDatabase instance to be a part of RtpRtcp::Configuration?
On 2016/01/25 08:17:50, tommi-webrtc wrote: > On 2016/01/25 07:26:19, mflodman wrote: > > > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > > File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right): > > > > > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > > webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:341: SSRCDatabase > ssrc_database_; > > Drive-by and Stefan might already have commented on this, but: > > - The intention with the database is to ensure no ssrc is reused across all > > sending and receiving streams. > > - The new video API already checks this when creating send and receive > streams, > > so there is no real use case for the database in the video engine. > > - VoiceEngine might still depend on this being checked across streams though, > so > > this change will then break currently use VoiceEngine functionality. > > (apologies for spamming - replying again via rietveld) > > I'm assuming the problem has to do with the lifetime of the ModuleRtpRtcpImpl > instance, is that right? > If so, would it make more sense for the SSRCDatabase instance to be a part of > RtpRtcp::Configuration? No, the problem is that a call consisting of several audio and video streams must ensure that there are no ssrc conflicts. ModuleRtpRtcpImpl and RtpRtcp::Configuration is per channel, i.e., per stream, and if we have a local instance per stream we can't make any guarantees that there won't be any conflicts. The new Call/VideoSendStream API guarantees this on stream configuration and actually doesn't use SSRCDatabase at all. This is however not the case for voice engine, so there could be a reason to keep it around for voice for the time being.
On 2016/01/25 08:38:24, stefan-webrtc (holmer) wrote: > On 2016/01/25 08:17:50, tommi-webrtc wrote: > > On 2016/01/25 07:26:19, mflodman wrote: > > > > > > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > > > File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right): > > > > > > > > > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > > > webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:341: SSRCDatabase > > ssrc_database_; > > > Drive-by and Stefan might already have commented on this, but: > > > - The intention with the database is to ensure no ssrc is reused across all > > > sending and receiving streams. > > > - The new video API already checks this when creating send and receive > > streams, > > > so there is no real use case for the database in the video engine. > > > - VoiceEngine might still depend on this being checked across streams > though, > > so > > > this change will then break currently use VoiceEngine functionality. > > > > (apologies for spamming - replying again via rietveld) > > > > I'm assuming the problem has to do with the lifetime of the ModuleRtpRtcpImpl > > instance, is that right? > > If so, would it make more sense for the SSRCDatabase instance to be a part of > > RtpRtcp::Configuration? > > No, the problem is that a call consisting of several audio and video streams > must ensure that there are no ssrc conflicts. ModuleRtpRtcpImpl and > RtpRtcp::Configuration is per channel, i.e., per stream, and if we have a local > instance per stream we can't make any guarantees that there won't be any > conflicts. > > The new Call/VideoSendStream API guarantees this on stream configuration and > actually doesn't use SSRCDatabase at all. This is however not the case for voice > engine, so there could be a reason to keep it around for voice for the time > being. OK, do you think that someone else should take this over or is there anything I can do to address this? What I most care about is to remove use of StaticInstance, since it's an anti pattern that we have and this is the last place we're using it in non-test code. Well, that and fixing the tick count issue on Mac that I discovered :) oh and deleting dead code... and switch over to rtc::CriticalSection... and probably more. okbye.
Remove dead code from webrtc/base/timing.*
Patchset #9 (id:160001) has been deleted
On 2016/01/25 09:33:53, tommi-webrtc wrote: > On 2016/01/25 08:38:24, stefan-webrtc (holmer) wrote: > > On 2016/01/25 08:17:50, tommi-webrtc wrote: > > > On 2016/01/25 07:26:19, mflodman wrote: > > > > > > > > > > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > > > > File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > > > > webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:341: SSRCDatabase > > > ssrc_database_; > > > > Drive-by and Stefan might already have commented on this, but: > > > > - The intention with the database is to ensure no ssrc is reused across > all > > > > sending and receiving streams. > > > > - The new video API already checks this when creating send and receive > > > streams, > > > > so there is no real use case for the database in the video engine. > > > > - VoiceEngine might still depend on this being checked across streams > > though, > > > so > > > > this change will then break currently use VoiceEngine functionality. > > > > > > (apologies for spamming - replying again via rietveld) > > > > > > I'm assuming the problem has to do with the lifetime of the > ModuleRtpRtcpImpl > > > instance, is that right? > > > If so, would it make more sense for the SSRCDatabase instance to be a part > of > > > RtpRtcp::Configuration? > > > > No, the problem is that a call consisting of several audio and video streams > > must ensure that there are no ssrc conflicts. ModuleRtpRtcpImpl and > > RtpRtcp::Configuration is per channel, i.e., per stream, and if we have a > local > > instance per stream we can't make any guarantees that there won't be any > > conflicts. > > > > The new Call/VideoSendStream API guarantees this on stream configuration and > > actually doesn't use SSRCDatabase at all. This is however not the case for > voice > > engine, so there could be a reason to keep it around for voice for the time > > being. > > OK, do you think that someone else should take this over or is there anything I > can do to address this? > What I most care about is to remove use of StaticInstance, since it's an anti > pattern that we have and this is the last place we're using it in non-test code. > Well, that and fixing the tick count issue on Mac that I discovered :) oh and > deleting dead code... and switch over to rtc::CriticalSection... and probably > more. okbye. It all depends on voice engine. I think there are two options: 1. Voice engine no longer needs this to be a StaticInstance to guarantee unique ssrcs. In this case we can remove SSRCDatabase completely. 2. Voice engine still needs it, and in that case it still has to be a StaticInstance. Solenberg might now the state of voice engine.
https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:190: // early on in the process. On 2016/01/24 11:51:49, tommi-webrtc wrote: > On 2016/01/23 17:38:39, stefan-webrtc (holmer) wrote: > > I think it's because timestamps and sequence numbers should start randomly. > > I might be misunderstanding, so please bear with me :) > > The timestamp or tick_util.* implementation that's supposed to return the > current tick count, actually always returns a very low value on first call and > subsequent calls will return a value relative to when the tick count was first > fetched. It's actually not that random and doesn't reflect the tick count as it > does on other platforms. Maybe it's simply a bug (pbos knows more). > > The call to srand() here, doesn't affect the Random() class from what I can tell > and is also not related to timestamps. So, I don't see why it is needed or why > calling it should be done in a particular instance of RtpSender. If the > requirement comes from SSRCDatabase, then I'd move it to the ctor of > SSRCDatabase. I didn't actually read the code, but apparently sequence_number_ and timestamp_ are set using random_ nowadays too. However, on line 607 below rand() is still used. https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/1623543002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.h:395: // it could be a regular member variable. On 2016/01/24 11:51:49, tommi-webrtc wrote: > On 2016/01/23 17:38:40, stefan-webrtc (holmer) wrote: > > I guess the same is true for RTPSenderAudio/Video below too? > > Moved the BitrateAggregator class declaration. > > For RTPSenderAudio/Video we need to keep it as is do to the |bool audio| > variable passed to the ctor - or can it be done some other way? Ah, ok. Let's leave it as is for now then. https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h File webrtc/base/random.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h#new... webrtc/base/random.h:31: // of TickTime::Now()). On 2016/01/24 21:54:35, tommi-webrtc wrote: > On 2016/01/24 21:34:10, the sun wrote: > > Maybe, but the ctor taking a seed is still needed for tests. We want (unit) > > tests to be reproducible, not randomized (fuzzers are a different matter of > > course). That, and the issue with global state was the reason to create this > > class and not use std::rand(). > > Maybe we can do both? If Random calls an overridable function to get the seed, > tests could use that. Today all callers provide a seed from the same source. > > btw, could you point me to a bug or something related to using this > implementation instead of std::rand? I'm not following why this class couldn't > use it internally at least. > > > Also, depending on what you actually mean we should use to init the seed, it > > doesn't ring right to me adding a dependency on TickTime (or Clock) to a PRNG. > > The PRNG is a platform-independent number crunching utility. Anything we can > > keep platform independent, we should. > > To be clear, this depends on TickTime today indirectly (via Clock) and there's a > problem with that on Mac due to how TickTime::Now() is implemented. > > I don't think I'm following the comment about platform independence though. > Sounds like you're reading the comment as being about making the code platform > dependent, which is not the point. TickTime is itself cross platform interface > but has platform specific implementations. So too would Get64BitForPRNGSeed be, > I imagine? > > > Suggest instead: > > a) Add a TickTime::Get64BitForPRNGSeed() utility function which can be used > with > > whichever PRNG class/impl we have now or in the future. > > b) If that is too inconvenient, invent a template function which hooks up > above > > utility to a PRNG class. > > How would it be implemented? I think what solenberg is saying is that it would be better to have a static Get64BitForPRNGSeed() in TickTime (using TickTime::Now().Ticks()) than in Random. I can live with both. I agree with solenberg that we still want to be able to pass a seed to this constructor in the future for tests.
On 2016/01/25 10:09:04, stefan-webrtc (holmer) wrote: > On 2016/01/25 09:33:53, tommi-webrtc wrote: > > On 2016/01/25 08:38:24, stefan-webrtc (holmer) wrote: > > > On 2016/01/25 08:17:50, tommi-webrtc wrote: > > > > On 2016/01/25 07:26:19, mflodman wrote: > > > > > > > > > > > > > > > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > > > > > File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... > > > > > webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:341: SSRCDatabase > > > > ssrc_database_; > > > > > Drive-by and Stefan might already have commented on this, but: > > > > > - The intention with the database is to ensure no ssrc is reused across > > all > > > > > sending and receiving streams. > > > > > - The new video API already checks this when creating send and receive > > > > streams, > > > > > so there is no real use case for the database in the video engine. > > > > > - VoiceEngine might still depend on this being checked across streams > > > though, > > > > so > > > > > this change will then break currently use VoiceEngine functionality. > > > > > > > > (apologies for spamming - replying again via rietveld) > > > > > > > > I'm assuming the problem has to do with the lifetime of the > > ModuleRtpRtcpImpl > > > > instance, is that right? > > > > If so, would it make more sense for the SSRCDatabase instance to be a part > > of > > > > RtpRtcp::Configuration? > > > > > > No, the problem is that a call consisting of several audio and video streams > > > must ensure that there are no ssrc conflicts. ModuleRtpRtcpImpl and > > > RtpRtcp::Configuration is per channel, i.e., per stream, and if we have a > > local > > > instance per stream we can't make any guarantees that there won't be any > > > conflicts. > > > > > > The new Call/VideoSendStream API guarantees this on stream configuration and > > > actually doesn't use SSRCDatabase at all. This is however not the case for > > voice > > > engine, so there could be a reason to keep it around for voice for the time > > > being. > > > > OK, do you think that someone else should take this over or is there anything > I > > can do to address this? > > What I most care about is to remove use of StaticInstance, since it's an anti > > pattern that we have and this is the last place we're using it in non-test > code. > > Well, that and fixing the tick count issue on Mac that I discovered :) oh and > > deleting dead code... and switch over to rtc::CriticalSection... and probably > > more. okbye. > > It all depends on voice engine. I think there are two options: > > 1. Voice engine no longer needs this to be a StaticInstance to guarantee unique > ssrcs. In this case we can remove SSRCDatabase completely. > > 2. Voice engine still needs it, and in that case it still has to be a > StaticInstance. > > Solenberg might now the state of voice engine. libjingle checks if an SSRC is free before trying to create either of VoE::Channel, AudioSendStream, AudioReceiveStream. Then the Call methods to create the streams will DCHECK that the audio_[send|receive]_ssrcs_ maps weren't populated in for that SSRC. But I believe the SSRCDatabase (or at least a random SSRC) is still used for RRs if no send stream has been associated with the VoE::Channel. There was a bug relating to this (https://code.google.com/p/webrtc/issues/detail?id=4740 and https://code.google.com/p/chromium/issues/detail?id=547661) and it is fixed in libjingle and in the VoE2 API, however we still have the legacy VoE.
It's confusing these days... I don't see all of your comments in the code. Let me know if you're expecting input on something I've missed. https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h File webrtc/base/random.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h#new... webrtc/base/random.h:31: // of TickTime::Now()). On 2016/01/24 21:54:35, tommi-webrtc wrote: > On 2016/01/24 21:34:10, the sun wrote: > > Maybe, but the ctor taking a seed is still needed for tests. We want (unit) > > tests to be reproducible, not randomized (fuzzers are a different matter of > > course). That, and the issue with global state was the reason to create this > > class and not use std::rand(). > > Maybe we can do both? If Random calls an overridable function to get the seed, > tests could use that. Today all callers provide a seed from the same source. No, tests use constant seeds. I can spot three uses of Random outside of unit tests. Requiring the tests to override a seed function sounds like more code+work than adding the one call to fetch a seed in rtp_sender, rtcp_sender and ssrc_database. > btw, could you point me to a bug or something related to using this > implementation instead of std::rand? I'm not following why this class couldn't > use it internally at least. Random was originally created for the BWE simulation framework, since std::rand(): 1) is platform specific (potentially different test sequences on different platforms) 2) uses global state (scheduling impacts actual test sequence) It has since been picked up and used in other places, mainly in tests. Recently, terelius@ refined the interface and implementation. Personally I think it's very nice not needing to do "std::rand() % range" everywhere. > > Also, depending on what you actually mean we should use to init the seed, it > > doesn't ring right to me adding a dependency on TickTime (or Clock) to a PRNG. > > The PRNG is a platform-independent number crunching utility. Anything we can > > keep platform independent, we should. > > To be clear, this depends on TickTime today indirectly (via Clock) and there's a > problem with that on Mac due to how TickTime::Now() is implemented. > > I don't think I'm following the comment about platform independence though. > Sounds like you're reading the comment as being about making the code platform > dependent, which is not the point. TickTime is itself cross platform interface > but has platform specific implementations. So too would Get64BitForPRNGSeed be, > I imagine? Platform independence (in this context) == "take the source files and feed them to a C++ compiler without having to muck with compiler settings" - a.k.a. portable C++. So in my mind, Random is portable, but TickTime is not (it requires the right compiler+flags+include paths), even if the interface certainly is. In any case, adding a dependency from Random to TickTime, makes Random less encapsulated and less reusable. I would rather add that link outside of either module. > > Suggest instead: > > a) Add a TickTime::Get64BitForPRNGSeed() utility function which can be used > with > > whichever PRNG class/impl we have now or in the future. > > b) If that is too inconvenient, invent a template function which hooks up > above > > utility to a PRNG class. > > How would it be implemented? template<typename PRNG> PRNG InitPrng() { return PRNG(TickTime::Get64BitForPRNGSeed()); } InitPrng<Random>() (assumes state is copyable of course) https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:38: class BitrateAggregator { Make internal to RTPSender or put in its own .h/.cc pair. https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:475: SSRCDatabase* const ssrc_db_; Add GUARDED_BY https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ssrc_database.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ssrc_database.h:18: #include "webrtc/base/scoped_ptr.h" dd
Address comments and revert changes
Remove scoped_ptr include
Thanks for the feedback everyone and thanks for saving me from shooting us in the foot :) So, I've reverted most of the changes but left in some of the comments that we're still discussing in case there's something that I'm still missing or left to work out. One change that's still in there related to the ssrc_database_ variable is changing it to const and removing GUARD_BY. Let me know what you think (see rationale below). https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h File webrtc/base/random.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/base/random.h#new... webrtc/base/random.h:31: // of TickTime::Now()). On 2016/01/25 11:24:57, the sun wrote: > On 2016/01/24 21:54:35, tommi-webrtc wrote: > > On 2016/01/24 21:34:10, the sun wrote: > > > Maybe, but the ctor taking a seed is still needed for tests. We want (unit) > > > tests to be reproducible, not randomized (fuzzers are a different matter of > > > course). That, and the issue with global state was the reason to create this > > > class and not use std::rand(). > > > > Maybe we can do both? If Random calls an overridable function to get the > seed, > > tests could use that. Today all callers provide a seed from the same source. > > No, tests use constant seeds. I can spot three uses of Random outside of unit > tests. Requiring the tests to override a seed function sounds like more > code+work than adding the one call to fetch a seed in rtp_sender, rtcp_sender > and ssrc_database. > > > btw, could you point me to a bug or something related to using this > > implementation instead of std::rand? I'm not following why this class > couldn't > > use it internally at least. > > Random was originally created for the BWE simulation framework, since > std::rand(): > 1) is platform specific (potentially different test sequences on different > platforms) > 2) uses global state (scheduling impacts actual test sequence) > > It has since been picked up and used in other places, mainly in tests. Recently, > terelius@ refined the interface and implementation. Personally I think it's very > nice not needing to do "std::rand() % range" everywhere. > > > > Also, depending on what you actually mean we should use to init the seed, it > > > doesn't ring right to me adding a dependency on TickTime (or Clock) to a > PRNG. > > > The PRNG is a platform-independent number crunching utility. Anything we can > > > keep platform independent, we should. > > > > To be clear, this depends on TickTime today indirectly (via Clock) and there's > a > > problem with that on Mac due to how TickTime::Now() is implemented. > > > > I don't think I'm following the comment about platform independence though. > > Sounds like you're reading the comment as being about making the code platform > > dependent, which is not the point. TickTime is itself cross platform > interface > > but has platform specific implementations. So too would Get64BitForPRNGSeed > be, > > I imagine? > > Platform independence (in this context) == "take the source files and feed them > to a C++ compiler without having to muck with compiler settings" - a.k.a. > portable C++. So in my mind, Random is portable, but TickTime is not (it > requires the right compiler+flags+include paths), even if the interface > certainly is. > > In any case, adding a dependency from Random to TickTime, makes Random less > encapsulated and less reusable. I would rather add that link outside of either > module. > > > > Suggest instead: > > > a) Add a TickTime::Get64BitForPRNGSeed() utility function which can be used > > with > > > whichever PRNG class/impl we have now or in the future. > > > b) If that is too inconvenient, invent a template function which hooks up > > above > > > utility to a PRNG class. > > > > How would it be implemented? > > template<typename PRNG> PRNG InitPrng() { > return PRNG(TickTime::Get64BitForPRNGSeed()); > } > > InitPrng<Random>() > > (assumes state is copyable of course) Great, I think I get it now :) I'll take a closer look at this at some later point (or someone else can too of course). Note that Random can't depend on TickCount as I point out in the comment, but I think I get where you're coming from. Sounds like you agree with the part about "works the same way across platforms" too. https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:38: class BitrateAggregator { On 2016/01/25 11:24:57, the sun wrote: > Make internal to RTPSender or put in its own .h/.cc pair. Made internal to RTPSender. https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:475: SSRCDatabase* const ssrc_db_; On 2016/01/25 11:24:57, the sun wrote: > Add GUARDED_BY I don't think we need or should do that actually. The variable is const and the implementation uses internal locking. Adding GUARDED_BY requires double locking (which we already do, but I think we could do without it). https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ssrc_database.h (right): https://codereview.webrtc.org/1623543002/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ssrc_database.h:18: #include "webrtc/base/scoped_ptr.h" On 2016/01/25 11:24:57, the sun wrote: > dd Done.
and of course it doesn't compile worth a damn... running low on batteries right now, so flying blind... actually literally flying right now. sorry about that.
Fix build?
https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1658: RTC_DCHECK(ssrc_); I prefer > 0 or != 0. https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1709: RTC_DCHECK(ssrc_); same here https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ssrc_database.cc (right): https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ssrc_database.cc:29: // the best design performance wise. Should this mention getting rid of the static instance, and in that case, should it mention removing this class entirely once voice engine no longer relies on this being a static instance? https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ssrc_database.cc:58: // in webrtc/system_wrappers/include/. As mentioned in earlier comments, an option would be to move this to TickTime::GenerateRandomSeed() so that we can write: random_(GenerateRandomSeed()) I can live with this being resolved later though.
Description was changed from ========== Refactor RtpSender and SSRCDatabase. * SSRCDatabase doesn't need to be a global instance, so I've changed it to be a "regular" class (i.e. construct via ctor, not maybe via GetSSRCDatabase( + release via ReturnSSRCDatabase())). If we ever have parallel tests running in the same process, they won't have the problem of using the same ssrc database. * Made RtpSender a more const. Also added some todos for myself and holmer to look into clarifying the threading model. * Switched from CriticalSectionWrapper to rtc::CriticalSection * Changed the random seeding to use TickTime::Now().Ticks() since TimeInMicroseconds() could return 0 when the process was starting. This is what TimeInMicroseconds() does anyway but now we don't need to access a global clock object. BUG= ========== to ========== Refactor RtpSender and SSRCDatabase. * SSRCDatabase doesn't need to be a global instance, so I've changed it to be a "regular" class (i.e. construct via ctor, not maybe via GetSSRCDatabase( + release via ReturnSSRCDatabase())). If we ever have parallel tests running in the same process, they won't have the problem of using the same ssrc database. * Made RtpSender a more const. Also added some todos for myself and holmer to look into clarifying the threading model. * Switched from CriticalSectionWrapper to rtc::CriticalSection * Changed the random seeding to use TickTime::Now().Ticks() since TimeInMicroseconds() could return 0 when the process was starting. This is what TimeInMicroseconds() does anyway but now we don't need to access a global clock object. BUG=webrtc:3062 ==========
Address comments
https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1658: RTC_DCHECK(ssrc_); On 2016/01/26 13:19:52, stefan-webrtc (holmer) wrote: > I prefer > 0 or != 0. Done. https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1709: RTC_DCHECK(ssrc_); On 2016/01/26 13:19:52, stefan-webrtc (holmer) wrote: > same here Done. https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ssrc_database.cc (right): https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ssrc_database.cc:29: // the best design performance wise. On 2016/01/26 13:19:52, stefan-webrtc (holmer) wrote: > Should this mention getting rid of the static instance, and in that case, should > it mention removing this class entirely once voice engine no longer relies on > this being a static instance? Comment updated and moved to the header. https://codereview.webrtc.org/1623543002/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ssrc_database.cc:58: // in webrtc/system_wrappers/include/. On 2016/01/26 13:19:52, stefan-webrtc (holmer) wrote: > As mentioned in earlier comments, an option would be to move this to > TickTime::GenerateRandomSeed() so that we can write: > random_(GenerateRandomSeed()) > > I can live with this being resolved later though. Updated comment. Erik has been making some improvements to timing code in webrtc/base so I think we should be able to add something like a GenerateRandomSeed() function there.
Rebase
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623543002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623543002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h File webrtc/base/random.h (right): https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h#new... webrtc/base/random.h:24: // TODO(tommi): Change this so that the seed is initialized internally. Please remove this comment. I believe the right design is for the PRNG to *not* know how to initialize its seed. The PRNG *client* knows how it wants to do that.
https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h File webrtc/base/random.h (right): https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h#new... webrtc/base/random.h:24: // TODO(tommi): Change this so that the seed is initialized internally. On 2016/02/01 10:41:28, the sun wrote: > Please remove this comment. I believe the right design is for the PRNG to *not* > know how to initialize its seed. The PRNG *client* knows how it wants to do > that. Yeah, so, I'm on the fence about that. On one side I agree on the other, much of the code has been calling a method that generates a poor quality seed. If we could have a special method for generating the seed that's specifically for that, then I think we should do that. Knowing what is a good seed for a PRNG is something I'd expect to be in the area of the PRNG code, but not something all clients of PRNG should know how to do.
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h File webrtc/base/random.h (right): https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h#new... webrtc/base/random.h:24: // TODO(tommi): Change this so that the seed is initialized internally. On 2016/02/01 17:32:03, tommi-webrtc wrote: > On 2016/02/01 10:41:28, the sun wrote: > > Please remove this comment. I believe the right design is for the PRNG to > *not* > > know how to initialize its seed. The PRNG *client* knows how it wants to do > > that. > > Yeah, so, I'm on the fence about that. On one side I agree on the other, much > of the code has been calling a method that generates a poor quality seed. If we > could have a special method for generating the seed that's specifically for > that, then I think we should do that. Knowing what is a good seed for a PRNG is > something I'd expect to be in the area of the PRNG code, but not something all > clients of PRNG should know how to do. Sounds like a case for having two constructors where one who doesn't care can just use the default ctor?
On 2016/02/01 17:59:25, pbos-webrtc wrote: > https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h > File webrtc/base/random.h (right): > > https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h#new... > webrtc/base/random.h:24: // TODO(tommi): Change this so that the seed is > initialized internally. > On 2016/02/01 17:32:03, tommi-webrtc wrote: > > On 2016/02/01 10:41:28, the sun wrote: > > > Please remove this comment. I believe the right design is for the PRNG to > > *not* > > > know how to initialize its seed. The PRNG *client* knows how it wants to do > > > that. > > > > Yeah, so, I'm on the fence about that. On one side I agree on the other, much > > of the code has been calling a method that generates a poor quality seed. If > we > > could have a special method for generating the seed that's specifically for > > that, then I think we should do that. Knowing what is a good seed for a PRNG > is > > something I'd expect to be in the area of the PRNG code, but not something all > > clients of PRNG should know how to do. > > Sounds like a case for having two constructors where one who doesn't care can > just use the default ctor? Yup, that was the suggestion.
On 2016/02/01 18:05:17, tommi-webrtc wrote: > On 2016/02/01 17:59:25, pbos-webrtc wrote: > > https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h > > File webrtc/base/random.h (right): > > > > > https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h#new... > > webrtc/base/random.h:24: // TODO(tommi): Change this so that the seed is > > initialized internally. > > On 2016/02/01 17:32:03, tommi-webrtc wrote: > > > On 2016/02/01 10:41:28, the sun wrote: > > > > Please remove this comment. I believe the right design is for the PRNG to > > > *not* > > > > know how to initialize its seed. The PRNG *client* knows how it wants to > do > > > > that. > > > > > > Yeah, so, I'm on the fence about that. On one side I agree on the other, > much > > > of the code has been calling a method that generates a poor quality seed. > If > > we > > > could have a special method for generating the seed that's specifically for > > > that, then I think we should do that. Knowing what is a good seed for a > PRNG > > is > > > something I'd expect to be in the area of the PRNG code, but not something > all > > > clients of PRNG should know how to do. > > > > Sounds like a case for having two constructors where one who doesn't care can > > just use the default ctor? > > Yup, that was the suggestion. Sounded to me (from the phrasing in the TODO) like the suggestion was to remove the explicit seed initialization altogether (e.g. change the current, rather than adding a default). Maybe hence the confusion, but I dunno. Can you rephrase it as such?
Update comment
On 2016/02/01 18:08:07, pbos-webrtc wrote: > On 2016/02/01 18:05:17, tommi-webrtc wrote: > > On 2016/02/01 17:59:25, pbos-webrtc wrote: > > > https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h > > > File webrtc/base/random.h (right): > > > > > > > > > https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h#new... > > > webrtc/base/random.h:24: // TODO(tommi): Change this so that the seed is > > > initialized internally. > > > On 2016/02/01 17:32:03, tommi-webrtc wrote: > > > > On 2016/02/01 10:41:28, the sun wrote: > > > > > Please remove this comment. I believe the right design is for the PRNG > to > > > > *not* > > > > > know how to initialize its seed. The PRNG *client* knows how it wants to > > do > > > > > that. > > > > > > > > Yeah, so, I'm on the fence about that. On one side I agree on the other, > > much > > > > of the code has been calling a method that generates a poor quality seed. > > If > > > we > > > > could have a special method for generating the seed that's specifically > for > > > > that, then I think we should do that. Knowing what is a good seed for a > > PRNG > > > is > > > > something I'd expect to be in the area of the PRNG code, but not something > > all > > > > clients of PRNG should know how to do. > > > > > > Sounds like a case for having two constructors where one who doesn't care > can > > > just use the default ctor? > > > > Yup, that was the suggestion. > > Sounded to me (from the phrasing in the TODO) like the suggestion was to remove > the explicit seed initialization altogether (e.g. change the current, rather > than adding a default). Maybe hence the confusion, but I dunno. Can you rephrase > it as such? Absolutely. I gave it a try, ptal. Fredrik - if you could take a look and see if it makes sense now then that would be great. I can also drop the comment altogether but in an effort to not sweep the problem with how callers currently generate a seed, under the rug, I took Peter's suggestion.
On 2016/02/01 20:55:08, tommi-webrtc wrote: > On 2016/02/01 18:08:07, pbos-webrtc wrote: > > On 2016/02/01 18:05:17, tommi-webrtc wrote: > > > On 2016/02/01 17:59:25, pbos-webrtc wrote: > > > > https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h > > > > File webrtc/base/random.h (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1623543002/diff/260001/webrtc/base/random.h#new... > > > > webrtc/base/random.h:24: // TODO(tommi): Change this so that the seed is > > > > initialized internally. > > > > On 2016/02/01 17:32:03, tommi-webrtc wrote: > > > > > On 2016/02/01 10:41:28, the sun wrote: > > > > > > Please remove this comment. I believe the right design is for the PRNG > > to > > > > > *not* > > > > > > know how to initialize its seed. The PRNG *client* knows how it wants > to > > > do > > > > > > that. > > > > > > > > > > Yeah, so, I'm on the fence about that. On one side I agree on the > other, > > > much > > > > > of the code has been calling a method that generates a poor quality > seed. > > > If > > > > we > > > > > could have a special method for generating the seed that's specifically > > for > > > > > that, then I think we should do that. Knowing what is a good seed for a > > > PRNG > > > > is > > > > > something I'd expect to be in the area of the PRNG code, but not > something > > > all > > > > > clients of PRNG should know how to do. > > > > > > > > Sounds like a case for having two constructors where one who doesn't care > > can > > > > just use the default ctor? > > > > > > Yup, that was the suggestion. > > > > Sounded to me (from the phrasing in the TODO) like the suggestion was to > remove > > the explicit seed initialization altogether (e.g. change the current, rather > > than adding a default). Maybe hence the confusion, but I dunno. Can you > rephrase > > it as such? > > Absolutely. I gave it a try, ptal. > > Fredrik - if you could take a look and see if it makes sense now then that would > be great. I can also drop the comment altogether but in an effort to not sweep > the problem with how callers currently generate a seed, under the rug, I took > Peter's suggestion. Two nits: 1) I think the comment in Random.h is better now. Maybe it can even refer to this CL for future recap of the discussion? 2) The (almost identical) comment in ssrc_database.cc can be shortened or removed though. [I'm ok with the CL after those changes.] Regarding internal init or not, I see what you mean and I think your arguments are absolutely valid. However, on the counter side, I would argue that adding a default ctor lends the class to be misused in tests, with flakiness as a consequence. That, in addition to Random depending on platform specific code, weighs heavier to me than the added convenience and safety that the default ctor would add. 1) Random is no crypto quality PRNG; the benefit of using a proper seed is limited. 2) Outside of tests it is used in only 3 places; the clients will already depend on base/ so adding the call to the special-yet-to-be-conceived-seed-function in those places amounts to, possibly, 3 extra #include lines. Adding the default ctor is in fact more code than that, plus the added implicit dependency. 3) Did I say anything about an unnatural dependency which breaks encapsulation? :) I will say no more in this matter.
LGTM
comment lgtm to me as well
Update comment and remove the one from ssrc_database.cc
Great. Thanks for the feedback. I've updated the comment, removed the duplicate one and am sending it to the cq.
The CQ bit was checked by tommi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, solenberg@webrtc.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1623543002/#ps300001 (title: "Update comment and remove the one from ssrc_database.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623543002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623543002/300001
Message was sent while issue was closed.
Description was changed from ========== Refactor RtpSender and SSRCDatabase. * SSRCDatabase doesn't need to be a global instance, so I've changed it to be a "regular" class (i.e. construct via ctor, not maybe via GetSSRCDatabase( + release via ReturnSSRCDatabase())). If we ever have parallel tests running in the same process, they won't have the problem of using the same ssrc database. * Made RtpSender a more const. Also added some todos for myself and holmer to look into clarifying the threading model. * Switched from CriticalSectionWrapper to rtc::CriticalSection * Changed the random seeding to use TickTime::Now().Ticks() since TimeInMicroseconds() could return 0 when the process was starting. This is what TimeInMicroseconds() does anyway but now we don't need to access a global clock object. BUG=webrtc:3062 ========== to ========== Refactor RtpSender and SSRCDatabase. * SSRCDatabase doesn't need to be a global instance, so I've changed it to be a "regular" class (i.e. construct via ctor, not maybe via GetSSRCDatabase( + release via ReturnSSRCDatabase())). If we ever have parallel tests running in the same process, they won't have the problem of using the same ssrc database. * Made RtpSender a more const. Also added some todos for myself and holmer to look into clarifying the threading model. * Switched from CriticalSectionWrapper to rtc::CriticalSection * Changed the random seeding to use TickTime::Now().Ticks() since TimeInMicroseconds() could return 0 when the process was starting. This is what TimeInMicroseconds() does anyway but now we don't need to access a global clock object. BUG=webrtc:3062 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Refactor RtpSender and SSRCDatabase. * SSRCDatabase doesn't need to be a global instance, so I've changed it to be a "regular" class (i.e. construct via ctor, not maybe via GetSSRCDatabase( + release via ReturnSSRCDatabase())). If we ever have parallel tests running in the same process, they won't have the problem of using the same ssrc database. * Made RtpSender a more const. Also added some todos for myself and holmer to look into clarifying the threading model. * Switched from CriticalSectionWrapper to rtc::CriticalSection * Changed the random seeding to use TickTime::Now().Ticks() since TimeInMicroseconds() could return 0 when the process was starting. This is what TimeInMicroseconds() does anyway but now we don't need to access a global clock object. BUG=webrtc:3062 ========== to ========== Refactor RtpSender and SSRCDatabase. * SSRCDatabase doesn't need to be a global instance, so I've changed it to be a "regular" class (i.e. construct via ctor, not maybe via GetSSRCDatabase( + release via ReturnSSRCDatabase())). If we ever have parallel tests running in the same process, they won't have the problem of using the same ssrc database. * Made RtpSender a more const. Also added some todos for myself and holmer to look into clarifying the threading model. * Switched from CriticalSectionWrapper to rtc::CriticalSection * Changed the random seeding to use TickTime::Now().Ticks() since TimeInMicroseconds() could return 0 when the process was starting. This is what TimeInMicroseconds() does anyway but now we don't need to access a global clock object. BUG=webrtc:3062 Committed: https://crrev.com/ae695e95a677b76be5a4e0c7051a964103340729 Cr-Commit-Position: refs/heads/master@{#11462} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/ae695e95a677b76be5a4e0c7051a964103340729 Cr-Commit-Position: refs/heads/master@{#11462} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
