Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2276913002:
DecoderDatabase: Made several methods nonvirtual to minimize mockable interface (Closed)
|
Created:
4 years, 4 months ago by ossu Modified:
4 years, 3 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemoved virtual from several methods in DecoderDatabase to minimize
the number of points that need to be mocked for testing.
For the now non-virtual methods, DecoderDatabase now does a lookup
through GetDecoderInfo and then delegates to the appropriate method in
the DecoderInfo object, if one is found.
A few other methods were also changed to look up through GetDecoderInfo.
Also moved the audio decoder factory into DecoderInfo, so that
DecoderInfo::GetDecoder can be used directly.
Committed: https://crrev.com/84bc98509be67f5023e6eb663b1877b1c0b3937e
Cr-Commit-Position: refs/heads/master@{#13933}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Made CheckPayloadTypes nonvirtual. Turned the rest of DecoderDatabase getters const. Fixed a couple… #
Total comments: 5
Patch Set 3 : Turned DecoderInfo factory into a raw pointer. Removed two unnecessary comments. #
Dependent Patchsets: Messages
Total messages: 23 (14 generated)
Description was changed from ========== Removed virtual from several methods in DecoderDatabase to minimize the number of points that need to be mocked for testing. For the now non-virtual methods, DecoderDatabase now does a lookup through GetDecoderInfo and then delegates to the appropriate method in the DecoderInfo object, if one is found. A few other methods were also changed to look up through GetDecoderInfo. Also moved the audio decoder factory into DecoderInfo, so that DecoderInfo::GetDecoder can be used directly. ========== to ========== Removed virtual from several methods in DecoderDatabase to minimize the number of points that need to be mocked for testing. For the now non-virtual methods, DecoderDatabase now does a lookup through GetDecoderInfo and then delegates to the appropriate method in the DecoderInfo object, if one is found. A few other methods were also changed to look up through GetDecoderInfo. Also moved the audio decoder factory into DecoderInfo, so that DecoderInfo::GetDecoder can be used directly. ==========
ossu@webrtc.org changed reviewers: + kwiberg@webrtc.org
This is the first part in getting my changes to the ACM, where NetEqDecoder is no longer used internally, into the code base. Several tests needed change to their mocks and I found they required too many changes. Essentially, I'd have to reimplement DecoderDatabase due to the many different entry points for functionality in DecoderDatabase and DecoderInfo. I've tried to simplify the interface while still making mocking useful for just providing a certain set of codecs, for example. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:196: const DecoderInfo *info = GetDecoderInfo(rtp_payload_type); I did not move these in the .cc-file as I did in the header, since that would make Rietveld sad. :( I will do before submitting, though! https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:290: int DecoderDatabase::CheckPayloadTypes(const PacketList& packet_list) const { Looking at the implementation of this, I'm considering making it nonvirtual as well. If you need to mock the behavior to just return true, for example, you'd just have to make a GetDecoderInfo that returns an Info object for anything (which you'd probably need to do anyway in that case.) https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:80: rtc::scoped_refptr<AudioDecoderFactory> factory_; If we can rely on DecoderInfo not outliving the respective DecoderDatabase (I think we can), then AudioDecoderFactory could be a plain pointer instead. Since DecoderInfo is part of DecoderDatabase, I don't think this is conceptually wrong. Were we allowed to, I would've had it as a reference. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:164: // The following are utility methods: they will look up DecoderInfo through I moved these down so that everything virtual is kept together. Should help in seeing what the mockable interface of DecoderDatabase is.
lgtm https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:36: factory_(factory), std::move here, to avoid unnecessary refcount updates? https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:242: } You don't introduce this behavior here, but you should never handle a failed DCHECK. So collapse lines 238-242 to just RTC_DCHECK(old_info); ? https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:272: } Collapse here too? https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:290: int DecoderDatabase::CheckPayloadTypes(const PacketList& packet_list) const { On 2016/08/24 11:15:20, ossu wrote: > Looking at the implementation of this, I'm considering making it nonvirtual as > well. If you need to mock the behavior to just return true, for example, you'd > just have to make a GetDecoderInfo that returns an Info object for anything > (which you'd probably need to do anyway in that case.) Sounds good. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:57: void DropDecoder() const { decoder_.reset(); } Hmm. Why did you make these two const? It seems like they modify the state. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:66: // Returns true if |code_type| is comfort noise. Spelling. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:69: // Returns true if |code_type| is DTMF. Spelling. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:80: rtc::scoped_refptr<AudioDecoderFactory> factory_; On 2016/08/24 11:15:20, ossu wrote: > If we can rely on DecoderInfo not outliving the respective DecoderDatabase (I > think we can), then AudioDecoderFactory could be a plain pointer instead. Since > DecoderInfo is part of DecoderDatabase, I don't think this is conceptually > wrong. Were we allowed to, I would've had it as a reference. I think you can have it as a reference. Mutable references are only disallowed as function arguments IIRC.
https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:36: factory_(factory), On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > std::move here, to avoid unnecessary refcount updates? Oh, right. I believe we usually send scoped_refptrs as const&, so I'll change it to that instead to remove one refcount update. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:196: const DecoderInfo *info = GetDecoderInfo(rtp_payload_type); On 2016/08/24 11:15:20, ossu wrote: > I did not move these in the .cc-file as I did in the header, since that would > make Rietveld sad. :( > I will do before submitting, though! They're moved now. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:242: } On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > You don't introduce this behavior here, but you should never handle a failed > DCHECK. So collapse lines 238-242 to just > > RTC_DCHECK(old_info); > > ? Yeah, I expect that's the right way to go about it. Not sure why it was like this before. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:272: } On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > Collapse here too? Acknowledged. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:290: int DecoderDatabase::CheckPayloadTypes(const PacketList& packet_list) const { On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > On 2016/08/24 11:15:20, ossu wrote: > > Looking at the implementation of this, I'm considering making it nonvirtual as > > well. If you need to mock the behavior to just return true, for example, you'd > > just have to make a GetDecoderInfo that returns an Info object for anything > > (which you'd probably need to do anyway in that case.) > > Sounds good. Acknowledged. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:57: void DropDecoder() const { decoder_.reset(); } On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > Hmm. Why did you make these two const? It seems like they modify the state. My reasoning is as follows: - Some parts of NetEq uses DecoderDatabase through a const reference. They access DecoderInfo (sometimes indirectly) to get info about a decoder. - Getting the associated decoder object isn't materially different from getting other info about the decoder. However, AudioDecoders are created on demand (except when they're not, i.e. passed in directly through the constructor). - Keeping the DecoderDatabase as const there seems like the right thing to do: these methods may not change the database (i.e. add or remove codecs) - only access what is already set up. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:66: // Returns true if |code_type| is comfort noise. On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > Spelling. Ah! Now I see it! Thanks. :) https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:69: // Returns true if |code_type| is DTMF. On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > Spelling. Acknowledged. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:80: rtc::scoped_refptr<AudioDecoderFactory> factory_; On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > On 2016/08/24 11:15:20, ossu wrote: > > If we can rely on DecoderInfo not outliving the respective DecoderDatabase (I > > think we can), then AudioDecoderFactory could be a plain pointer instead. > Since > > DecoderInfo is part of DecoderDatabase, I don't think this is conceptually > > wrong. Were we allowed to, I would've had it as a reference. > > I think you can have it as a reference. Mutable references are only disallowed > as function arguments IIRC. Yeah, but then I'd pass it as a pointer to the constructor, which is the part I don't like; but that's what the code style says. To me, passing a pointer looks a bit like it might transfer ownership, maybe, but passing a reference clearly doesn't. I'll keep it as a scoped_refptr for now, maybe degrade it to a plain pointer later. https://codereview.webrtc.org/2276913002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2276913002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.h:151: virtual AudioDecoder* GetActiveDecoder() const; Considering my reasoning on what const DecoderDatabase& means, I've turned the rest of the getters const as well.
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:36: factory_(factory), On 2016/08/25 10:23:47, ossu wrote: > On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > > std::move here, to avoid unnecessary refcount updates? > > Oh, right. I believe we usually send scoped_refptrs as const&, so I'll change it > to that instead to remove one refcount update. We handle scoped_refptrs less than well in a lot of places. For a function that always saves a reference to its argument, passing the scoped_refptr by value is best (in the sense that it leads to the fewest number of refcount updates in all cases). If the caller keeps a reference, passing by value and passing by reference both cost 1 incref. But if the caller doesn't want to keep a reference, passing by value is free (it's a move) and passing by reference costs 1 incref and 1 decref. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:57: void DropDecoder() const { decoder_.reset(); } On 2016/08/25 10:23:47, ossu wrote: > On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > > Hmm. Why did you make these two const? It seems like they modify the state. > > My reasoning is as follows: > - Some parts of NetEq uses DecoderDatabase through a const reference. They > access DecoderInfo (sometimes indirectly) to get info about a decoder. > - Getting the associated decoder object isn't materially different from getting > other info about the decoder. However, AudioDecoders are created on demand > (except when they're not, i.e. passed in directly through the constructor). > - Keeping the DecoderDatabase as const there seems like the right thing to do: > these methods may not change the database (i.e. add or remove codecs) - only > access what is already set up. Acknowledged. https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:80: rtc::scoped_refptr<AudioDecoderFactory> factory_; On 2016/08/25 10:23:47, ossu wrote: > On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > > On 2016/08/24 11:15:20, ossu wrote: > > > If we can rely on DecoderInfo not outliving the respective DecoderDatabase > (I > > > think we can), then AudioDecoderFactory could be a plain pointer instead. > > Since > > > DecoderInfo is part of DecoderDatabase, I don't think this is conceptually > > > wrong. Were we allowed to, I would've had it as a reference. > > > > I think you can have it as a reference. Mutable references are only disallowed > > as function arguments IIRC. > > Yeah, but then I'd pass it as a pointer to the constructor, which is the part I > don't like; but that's what the code style says. To me, passing a pointer looks > a bit like it might transfer ownership, maybe, but passing a reference clearly > doesn't. I'll keep it as a scoped_refptr for now, maybe degrade it to a plain > pointer later. Passing a raw pointer clearly doesn't transfer ownership. If you were transferring ownership, you'd have used a smart pointer. I know that not all of our code currently works this way, but new code should.
https://codereview.webrtc.org/2276913002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/2276913002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.cc:212: // Decoder not found. This should not be possible. The comment is redundant now. https://codereview.webrtc.org/2276913002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.cc:239: // Decoder not found. This should not be possible. Again, comment redundant.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.h (right): https://codereview.webrtc.org/2276913002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.h:80: rtc::scoped_refptr<AudioDecoderFactory> factory_; On 2016/08/25 10:49:53, kwiberg-webrtc wrote: > On 2016/08/25 10:23:47, ossu wrote: > > On 2016/08/24 22:28:49, kwiberg-webrtc wrote: > > > On 2016/08/24 11:15:20, ossu wrote: > > > > If we can rely on DecoderInfo not outliving the respective DecoderDatabase > > (I > > > > think we can), then AudioDecoderFactory could be a plain pointer instead. > > > Since > > > > DecoderInfo is part of DecoderDatabase, I don't think this is conceptually > > > > wrong. Were we allowed to, I would've had it as a reference. > > > > > > I think you can have it as a reference. Mutable references are only > disallowed > > > as function arguments IIRC. > > > > Yeah, but then I'd pass it as a pointer to the constructor, which is the part > I > > don't like; but that's what the code style says. To me, passing a pointer > looks > > a bit like it might transfer ownership, maybe, but passing a reference clearly > > doesn't. I'll keep it as a scoped_refptr for now, maybe degrade it to a plain > > pointer later. > > Passing a raw pointer clearly doesn't transfer ownership. If you were > transferring ownership, you'd have used a smart pointer. > > I know that not all of our code currently works this way, but new code should. Agreed. But since it's not consistently like that in the code base, it can be a bit ambiguous. Oh, well! https://codereview.webrtc.org/2276913002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/2276913002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.cc:212: // Decoder not found. This should not be possible. On 2016/08/25 10:53:20, kwiberg-webrtc wrote: > The comment is redundant now. Acknowledged. https://codereview.webrtc.org/2276913002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.cc:239: // Decoder not found. This should not be possible. On 2016/08/25 10:53:20, kwiberg-webrtc wrote: > Again, comment redundant. Acknowledged.
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2276913002/#ps40001 (title: "Turned DecoderInfo factory into a raw pointer. Removed two unnecessary comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Removed virtual from several methods in DecoderDatabase to minimize the number of points that need to be mocked for testing. For the now non-virtual methods, DecoderDatabase now does a lookup through GetDecoderInfo and then delegates to the appropriate method in the DecoderInfo object, if one is found. A few other methods were also changed to look up through GetDecoderInfo. Also moved the audio decoder factory into DecoderInfo, so that DecoderInfo::GetDecoder can be used directly. ========== to ========== Removed virtual from several methods in DecoderDatabase to minimize the number of points that need to be mocked for testing. For the now non-virtual methods, DecoderDatabase now does a lookup through GetDecoderInfo and then delegates to the appropriate method in the DecoderInfo object, if one is found. A few other methods were also changed to look up through GetDecoderInfo. Also moved the audio decoder factory into DecoderInfo, so that DecoderInfo::GetDecoder can be used directly. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Removed virtual from several methods in DecoderDatabase to minimize the number of points that need to be mocked for testing. For the now non-virtual methods, DecoderDatabase now does a lookup through GetDecoderInfo and then delegates to the appropriate method in the DecoderInfo object, if one is found. A few other methods were also changed to look up through GetDecoderInfo. Also moved the audio decoder factory into DecoderInfo, so that DecoderInfo::GetDecoder can be used directly. ========== to ========== Removed virtual from several methods in DecoderDatabase to minimize the number of points that need to be mocked for testing. For the now non-virtual methods, DecoderDatabase now does a lookup through GetDecoderInfo and then delegates to the appropriate method in the DecoderInfo object, if one is found. A few other methods were also changed to look up through GetDecoderInfo. Also moved the audio decoder factory into DecoderInfo, so that DecoderInfo::GetDecoder can be used directly. Committed: https://crrev.com/84bc98509be67f5023e6eb663b1877b1c0b3937e Cr-Commit-Position: refs/heads/master@{#13933} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/84bc98509be67f5023e6eb663b1877b1c0b3937e Cr-Commit-Position: refs/heads/master@{#13933} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
