Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1551813002:
Storing raw audio sink for default audio track. (Closed)
|
Created:
4 years, 11 months ago by Taylor Brandstetter Modified:
4 years, 11 months ago CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionStoring raw audio sink for default audio track.
BUG=webrtc:5250
Committed: https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99
Cr-Commit-Position: refs/heads/master@{#11230}
Committed: https://crrev.com/884f58523a75ace3239805759147948b7e868aea
Cr-Commit-Position: refs/heads/master@{#11275}
Patch Set 1 #Patch Set 2 : Adding include back. #
Total comments: 2
Patch Set 3 : Changing to scoped_refptr for AudioSinkInterface. #Patch Set 4 : Adding unit tests for SetRawAudioSink. #
Total comments: 19
Patch Set 5 : Addressing solenberg@'s comments. #
Total comments: 1
Patch Set 6 : Returning const ref from fake call class. #Patch Set 7 : Fixing Windows compile error. #Patch Set 8 : Going back to scoped_ptr, and using proxy object to handle shared ownership. #Patch Set 9 : Minor cleanup. #
Total comments: 10
Patch Set 10 : Adding back "if (default_sink_)" when creating default stream. #
Total comments: 4
Patch Set 11 : Removing obsolete "RefCountedObject" and adding an RTC_DCHECK. #
Messages
Total messages: 49 (14 generated)
deadbeef@webrtc.org changed reviewers: + pbos@webrtc.org, pthatcher@webrtc.org
pbos and/or pthatcher: PTAL. This is very similar to how "default_recv_volume_" works, or "default_renderer_" in WebRtcVideoChannel2. I'm not sure how it should be unit tested, though. I don't see any unit tests that touch SetRawAudioSink.
For unit tests, I think you can just do something like: SetRawAudioSink(0, fake_sink); OnPacketReceived(crafted_rtp_packet_with_ssrc_1); // triggers default stream VerifyRawAudioSinkOfRecvStream(1, fake_sink); RemoveReceiveStream(1); OnPacketReceived(crafted_rtp_packet_with_ssrc_2); // triggers default stream VerifyRawAudioSinkOfRecvStream(2, fake_sink); I'm guessing that implementing VerifyRawAudioSinkOfRecvStream by putting in the right getters/fakes will be the most work. https://codereview.webrtc.org/1551813002/diff/20001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1551813002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2190: SetRawAudioSink(default_recv_ssrc_, std::move(default_sink_)); This looks different than default_recv_volume_ in that default_recv_volume_ continues to be set even after the default stream is added, so that if the default stream is removed and re-added, the new default stream can get the same volume. But this is moving ownership of the audio sink, so if the default stream were removed, the default sink would be destroyed. There are a couple of solution to this, but I don't really like any of them: - When the default stream is removed, transfer ownership back, and comment when default_sink_ is set and when it is not in the header file. - Use refcounting - Always retain owernship of sinks in this class instead of lower below. - Never destroy the default receive stream once created. Instead, just remove it from the map. Out of them all, the first looks the best to me right now. What do you think?
pbos@webrtc.org changed reviewers: + solenberg@webrtc.org
+r solenberg who's better suited to take a look at this than me
Ok; unit tests for SetRawAudioSink added, and switched to scoped_refptr to address the issue Peter noticed. https://codereview.webrtc.org/1551813002/diff/20001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1551813002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2190: SetRawAudioSink(default_recv_ssrc_, std::move(default_sink_)); On 2015/12/30 16:31:50, pthatcher1 wrote: > This looks different than default_recv_volume_ in that default_recv_volume_ > continues to be set even after the default stream is added, so that if the > default stream is removed and re-added, the new default stream can get the same > volume. But this is moving ownership of the audio sink, so if the default > stream were removed, the default sink would be destroyed. > > There are a couple of solution to this, but I don't really like any of them: > - When the default stream is removed, transfer ownership back, and comment when > default_sink_ is set and when it is not in the header file. > - Use refcounting > - Always retain owernship of sinks in this class instead of lower below. > - Never destroy the default receive stream once created. Instead, just remove > it from the map. > > > Out of them all, the first looks the best to me right now. What do you think? The first solution seems a little fragile. And it means we'd need to have a "GetSink"/"ReleaseSink" method through all the layers. Personally I don't see any problem with using a scoped_refptr, other than that it's a little unintuitive at first glance.
lgtm
Nice, thanks for doing this. Just a few minor comments. https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... File talk/app/webrtc/mediastreamprovider.h (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... talk/app/webrtc/mediastreamprovider.h:78: rtc::scoped_refptr<webrtc::AudioSinkInterface> sink) = 0; Assuming we'll be replacing this with shared_ptr once we can use that, is it then still a good idea to pass by value? scoped_refptr just contains one pointer, but shared_ptr has at least two. Should we pass by const & instead? https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2189: if (default_sink_) { No need for the conditional, setting a null sink should work. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2424: SetRawAudioSink(default_recv_ssrc_, sink); Nice, but will generate duplicate log lines. I'd prefer this block to be: if (ssrc == 0) { default_sink = sink; if (default_recv_ssrc_ == -1) { return; } ssrc = static_cast<uint32_t>(default_recv_ssrc_); } https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.h:272: // Sink for default stream, which may be set before the stream exists. nit: default->unsignalled since that is the terminology used above, for volume etc. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3254: TEST_F(WebRtcVoiceEngineTestFake, SetRawAudioSink) { nit: move these tests up to the other WebRtcVoiceEngineTestFake test cases. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3265: EXPECT_TRUE(channel_->AddRecvStream(stream)); EXPECT_TRUE(channel_->AddRecvStream( cricket::StreamParams::CreateLegacy(kSsrc1))); https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3269: EXPECT_EQ(fake_sink, call_.GetAudioReceiveStream(kSsrc1)->sink()); EXPECT_EQ(fake_sink, GetRecvStream(kSsrc1).sink()); and below. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3284: nit: Add comment: // Create default channel.
https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3269: EXPECT_EQ(fake_sink, call_.GetAudioReceiveStream(kSsrc1)->sink()); On 2016/01/08 13:14:26, the sun wrote: > EXPECT_EQ(fake_sink, GetRecvStream(kSsrc1).sink()); > > and below. Oops, that function isn't committed yet (https://codereview.webrtc.org/1499903002/diff/80001/talk/media/webrtc/webrtcv...). Keep as is or copy from the referenced CL...
https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... File talk/app/webrtc/mediastreamprovider.h (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... talk/app/webrtc/mediastreamprovider.h:78: rtc::scoped_refptr<webrtc::AudioSinkInterface> sink) = 0; On 2016/01/08 13:14:26, the sun wrote: > Assuming we'll be replacing this with shared_ptr once we can use that, is it > then still a good idea to pass by value? scoped_refptr just contains one > pointer, but shared_ptr has at least two. Should we pass by const & instead? const & would indeed save some unnecessary copying; this is just the pattern I was used to seeing. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2189: if (default_sink_) { On 2016/01/08 13:14:26, the sun wrote: > No need for the conditional, setting a null sink should work. Done. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:2424: SetRawAudioSink(default_recv_ssrc_, sink); On 2016/01/08 13:14:26, the sun wrote: > Nice, but will generate duplicate log lines. I'd prefer this block to be: > > if (ssrc == 0) { > default_sink = sink; > if (default_recv_ssrc_ == -1) { > return; > } > ssrc = static_cast<uint32_t>(default_recv_ssrc_); > } Done. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.h:272: // Sink for default stream, which may be set before the stream exists. On 2016/01/08 13:14:26, the sun wrote: > nit: default->unsignalled since that is the terminology used above, for volume > etc. Done. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3254: TEST_F(WebRtcVoiceEngineTestFake, SetRawAudioSink) { On 2016/01/08 13:14:27, the sun wrote: > nit: move these tests up to the other WebRtcVoiceEngineTestFake test cases. Done. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3265: EXPECT_TRUE(channel_->AddRecvStream(stream)); On 2016/01/08 13:14:26, the sun wrote: > EXPECT_TRUE(channel_->AddRecvStream( > cricket::StreamParams::CreateLegacy(kSsrc1))); Done. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3269: EXPECT_EQ(fake_sink, call_.GetAudioReceiveStream(kSsrc1)->sink()); On 2016/01/08 13:26:19, the sun wrote: > On 2016/01/08 13:14:26, the sun wrote: > > EXPECT_EQ(fake_sink, GetRecvStream(kSsrc1).sink()); > > > > and below. > > Oops, that function isn't committed yet > (https://codereview.webrtc.org/1499903002/diff/80001/talk/media/webrtc/webrtcv...). > Keep as is or copy from the referenced CL... Ok, copied. https://codereview.webrtc.org/1551813002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine_unittest.cc:3284: On 2016/01/08 13:14:26, the sun wrote: > nit: Add comment: > // Create default channel. Done.
LGTM. Thank you! https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... File talk/app/webrtc/mediastreamprovider.h (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... talk/app/webrtc/mediastreamprovider.h:78: rtc::scoped_refptr<webrtc::AudioSinkInterface> sink) = 0; On 2016/01/08 19:46:58, Taylor Brandstetter wrote: > On 2016/01/08 13:14:26, the sun wrote: > > Assuming we'll be replacing this with shared_ptr once we can use that, is it > > then still a good idea to pass by value? scoped_refptr just contains one > > pointer, but shared_ptr has at least two. Should we pass by const & instead? > > const & would indeed save some unnecessary copying; this is just the pattern I > was used to seeing. For scoped_ptr (and current scoped_ref_ptr) it won't matter since they just contain a single pointer; the compiler is pretty smart. :) Thanks for updating though! https://codereview.webrtc.org/1551813002/diff/80001/talk/media/webrtc/fakeweb... File talk/media/webrtc/fakewebrtccall.h (right): https://codereview.webrtc.org/1551813002/diff/80001/talk/media/webrtc/fakeweb... talk/media/webrtc/fakewebrtccall.h:92: rtc::scoped_refptr<webrtc::AudioSinkInterface> sink() const { return sink_; } super nit: could return a const &
deadbeef@webrtc.org changed reviewers: + tommi@webrtc.org
tommi: Could you take a look at audio_receive_stream.h? It looks like I'm still missing an l-g-t-m from that.
lgtm for audio_receive_stream.* https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... File talk/app/webrtc/mediastreamprovider.h (right): https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... talk/app/webrtc/mediastreamprovider.h:78: rtc::scoped_refptr<webrtc::AudioSinkInterface> sink) = 0; On 2016/01/11 10:38:27, the sun wrote: > On 2016/01/08 19:46:58, Taylor Brandstetter wrote: > > On 2016/01/08 13:14:26, the sun wrote: > > > Assuming we'll be replacing this with shared_ptr once we can use that, is it > > > then still a good idea to pass by value? scoped_refptr just contains one > > > pointer, but shared_ptr has at least two. Should we pass by const & instead? > > > > const & would indeed save some unnecessary copying; this is just the pattern I > > was used to seeing. > > For scoped_ptr (and current scoped_ref_ptr) it won't matter since they just > contain a single pointer; the compiler is pretty smart. :) Thanks for updating > though! Passing scoped_refptr by value will trigger an extra pair of AddRef/Release calls which can mean unnecessary interlocked operations. That + virtual calls, so should be passed by const ref. Thanks for catching Fredrik.
On 2016/01/12 08:58:06, tommi-webrtc wrote: > lgtm for audio_receive_stream.* > > https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... > File talk/app/webrtc/mediastreamprovider.h (right): > > https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... > talk/app/webrtc/mediastreamprovider.h:78: > rtc::scoped_refptr<webrtc::AudioSinkInterface> sink) = 0; > On 2016/01/11 10:38:27, the sun wrote: > > On 2016/01/08 19:46:58, Taylor Brandstetter wrote: > > > On 2016/01/08 13:14:26, the sun wrote: > > > > Assuming we'll be replacing this with shared_ptr once we can use that, is > it > > > > then still a good idea to pass by value? scoped_refptr just contains one > > > > pointer, but shared_ptr has at least two. Should we pass by const & > instead? > > > > > > const & would indeed save some unnecessary copying; this is just the pattern > I > > > was used to seeing. > > > > For scoped_ptr (and current scoped_ref_ptr) it won't matter since they just > > contain a single pointer; the compiler is pretty smart. :) Thanks for updating > > though! > > Passing scoped_refptr by value will trigger an extra pair of AddRef/Release > calls which can mean unnecessary interlocked operations. That + virtual calls, > so should be passed by const ref. Thanks for catching Fredrik. Doh! Thanks Tommi! :) Now I see that switching to shared_ptr will rid us of the virtual calls too, although still need to follow the pointer to the refcount so that may not amount to anything in practice...
On 2016/01/12 10:10:12, the sun wrote: > On 2016/01/12 08:58:06, tommi-webrtc wrote: > > lgtm for audio_receive_stream.* > > > > > https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... > > File talk/app/webrtc/mediastreamprovider.h (right): > > > > > https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... > > talk/app/webrtc/mediastreamprovider.h:78: > > rtc::scoped_refptr<webrtc::AudioSinkInterface> sink) = 0; > > On 2016/01/11 10:38:27, the sun wrote: > > > On 2016/01/08 19:46:58, Taylor Brandstetter wrote: > > > > On 2016/01/08 13:14:26, the sun wrote: > > > > > Assuming we'll be replacing this with shared_ptr once we can use that, > is > > it > > > > > then still a good idea to pass by value? scoped_refptr just contains one > > > > > pointer, but shared_ptr has at least two. Should we pass by const & > > instead? > > > > > > > > const & would indeed save some unnecessary copying; this is just the > pattern > > I > > > > was used to seeing. > > > > > > For scoped_ptr (and current scoped_ref_ptr) it won't matter since they just > > > contain a single pointer; the compiler is pretty smart. :) Thanks for > updating > > > though! > > > > Passing scoped_refptr by value will trigger an extra pair of AddRef/Release > > calls which can mean unnecessary interlocked operations. That + virtual calls, > > so should be passed by const ref. Thanks for catching Fredrik. > > Doh! Thanks Tommi! :) > > Now I see that switching to shared_ptr will rid us of the virtual calls too, > although still need to follow the pointer to the refcount so that may not amount > to anything in practice... Why does the RefCountedObject need to have virtual AddRef/Release?
On 2016/01/12 10:15:03, pbos-webrtc wrote: > On 2016/01/12 10:10:12, the sun wrote: > > On 2016/01/12 08:58:06, tommi-webrtc wrote: > > > lgtm for audio_receive_stream.* > > > > > > > > > https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... > > > File talk/app/webrtc/mediastreamprovider.h (right): > > > > > > > > > https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... > > > talk/app/webrtc/mediastreamprovider.h:78: > > > rtc::scoped_refptr<webrtc::AudioSinkInterface> sink) = 0; > > > On 2016/01/11 10:38:27, the sun wrote: > > > > On 2016/01/08 19:46:58, Taylor Brandstetter wrote: > > > > > On 2016/01/08 13:14:26, the sun wrote: > > > > > > Assuming we'll be replacing this with shared_ptr once we can use that, > > is > > > it > > > > > > then still a good idea to pass by value? scoped_refptr just contains > one > > > > > > pointer, but shared_ptr has at least two. Should we pass by const & > > > instead? > > > > > > > > > > const & would indeed save some unnecessary copying; this is just the > > pattern > > > I > > > > > was used to seeing. > > > > > > > > For scoped_ptr (and current scoped_ref_ptr) it won't matter since they > just > > > > contain a single pointer; the compiler is pretty smart. :) Thanks for > > updating > > > > though! > > > > > > Passing scoped_refptr by value will trigger an extra pair of AddRef/Release > > > calls which can mean unnecessary interlocked operations. That + virtual > calls, > > > so should be passed by const ref. Thanks for catching Fredrik. > > > > Doh! Thanks Tommi! :) > > > > Now I see that switching to shared_ptr will rid us of the virtual calls too, > > although still need to follow the pointer to the refcount so that may not > amount > > to anything in practice... > > Why does the RefCountedObject need to have virtual AddRef/Release? It really should be 'override'. RefCountedObject was not written to be a part of the public interface that the recipient of an object pointer sees. It was written to implement a virtual AddRef/Release contract for the interface that the recipient sees. For example: class FooInterface { public: virtual void Splat() = 0; virtual int AddRef() = 0; virtual int Release() = 0; }; class BarInterface { public: scoped_refptr<FooInterface> GetFoo() = 0; virtual int AddRef() = 0; virtual int Release() = 0; }; class FooImpl : public FooInterface { public: // Override for FooInterface void Splat() override { printf("splatted!"); } // Override for BarInterface scoped_refptr<FooInterface> GetFoo() { return this; } }; scoped_refptr<BarInterface> GetBar() { // RefCountedObject just makes life easier for us here. // It implements AddRef/Release for both BarInterface and FooInterface in a single counter. // The implementation of RefCountedObject is though completely private from both // the interfaces as well as FooImpl. return new RefCountedObject<FooImpl>(); } void main() { GetBar()->GetFoo()->Splat(); }
On 2016/01/12 10:47:58, tommi-webrtc wrote: > On 2016/01/12 10:15:03, pbos-webrtc wrote: > > On 2016/01/12 10:10:12, the sun wrote: > > > On 2016/01/12 08:58:06, tommi-webrtc wrote: > > > > lgtm for audio_receive_stream.* > > > > > > > > > > > > > > https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... > > > > File talk/app/webrtc/mediastreamprovider.h (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/1551813002/diff/60001/talk/app/webrtc/mediastre... > > > > talk/app/webrtc/mediastreamprovider.h:78: > > > > rtc::scoped_refptr<webrtc::AudioSinkInterface> sink) = 0; > > > > On 2016/01/11 10:38:27, the sun wrote: > > > > > On 2016/01/08 19:46:58, Taylor Brandstetter wrote: > > > > > > On 2016/01/08 13:14:26, the sun wrote: > > > > > > > Assuming we'll be replacing this with shared_ptr once we can use > that, > > > is > > > > it > > > > > > > then still a good idea to pass by value? scoped_refptr just contains > > one > > > > > > > pointer, but shared_ptr has at least two. Should we pass by const & > > > > instead? > > > > > > > > > > > > const & would indeed save some unnecessary copying; this is just the > > > pattern > > > > I > > > > > > was used to seeing. > > > > > > > > > > For scoped_ptr (and current scoped_ref_ptr) it won't matter since they > > just > > > > > contain a single pointer; the compiler is pretty smart. :) Thanks for > > > updating > > > > > though! > > > > > > > > Passing scoped_refptr by value will trigger an extra pair of > AddRef/Release > > > > calls which can mean unnecessary interlocked operations. That + virtual > > calls, > > > > so should be passed by const ref. Thanks for catching Fredrik. > > > > > > Doh! Thanks Tommi! :) > > > > > > Now I see that switching to shared_ptr will rid us of the virtual calls too, > > > although still need to follow the pointer to the refcount so that may not > > amount > > > to anything in practice... > > > > Why does the RefCountedObject need to have virtual AddRef/Release? > > It really should be 'override'. RefCountedObject was not written to be a part > of the public interface that the recipient of an object pointer sees. It was > written to implement a virtual AddRef/Release contract for the interface that > the recipient sees. > For example: > > class FooInterface { > public: > virtual void Splat() = 0; > virtual int AddRef() = 0; > virtual int Release() = 0; > }; > > class BarInterface { > public: > scoped_refptr<FooInterface> GetFoo() = 0; > virtual int AddRef() = 0; > virtual int Release() = 0; > }; > > class FooImpl : public FooInterface { > public: > // Override for FooInterface > void Splat() override { > printf("splatted!"); > } > > // Override for BarInterface > scoped_refptr<FooInterface> GetFoo() { > return this; > } > }; > > scoped_refptr<BarInterface> GetBar() { > // RefCountedObject just makes life easier for us here. > // It implements AddRef/Release for both BarInterface and FooInterface in a > single counter. > // The implementation of RefCountedObject is though completely private from > both > // the interfaces as well as FooImpl. > return new RefCountedObject<FooImpl>(); > } > > void main() { > GetBar()->GetFoo()->Splat(); > } oops, FooImpl was supposed to inherit from BarInterface too. class FooImpl : public FooInterface, public BarInterface { ... };
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.chromium.org/1551813002/#ps120001 (title: "Fixing Windows compile error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1551813002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551813002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by deadbeef@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1551813002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551813002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Storing raw audio sink for default audio track. BUG=webrtc:5250 ========== to ========== Storing raw audio sink for default audio track. BUG=webrtc:5250 Committed: https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 Cr-Commit-Position: refs/heads/master@{#11230} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 Cr-Commit-Position: refs/heads/master@{#11230}
Message was sent while issue was closed.
On 2016/01/13 00:45:44, commit-bot: I haz the power wrote: > Patchset 7 (id:??) landed as > https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 > Cr-Commit-Position: refs/heads/master@{#11230} Sorry, I just now saw that this CL changes the AudioSinkInterface to be refcounted. That changes the semantics quite a bit and I think we might need to revert this. I should have taken a look at more files. See for example the dtor of RemoteAudioSource::Sink. The contract there is that when the sink is deleted, it means that it has been detached from the source. With the added reference counting, the lifetime is no longer deterministic and that will cause problems.
Message was sent while issue was closed.
On 2016/01/13 14:18:15, tommi-webrtc wrote: > On 2016/01/13 00:45:44, commit-bot: I haz the power wrote: > > Patchset 7 (id:??) landed as > > https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 > > Cr-Commit-Position: refs/heads/master@{#11230} > > Sorry, I just now saw that this CL changes the AudioSinkInterface to be > refcounted. That changes the semantics quite a bit and I think we might need to > revert this. I should have taken a look at more files. > > See for example the dtor of RemoteAudioSource::Sink. The contract there is that > when the sink is deleted, it means that it has been detached from the source. > With the added reference counting, the lifetime is no longer deterministic and > that will cause problems. As a proposal for how to do this differently, I suggest: * Store the scoped_ptr to the default sink in WebRtcVoiceMediaChannel as you're doing, but not ref counted. * Whenever a default ssrc is set up, create a new sink, which is a special sink implementation for the default ssrc. * This new sink is basically just a shim on top of the default sink that's ultimately owned by WebRtcVoiceMediaChannel. * Whenever a default ssrc goes away, the shim is deleted, but the default sink is not. It will implicitly enter an 'unused' state though, which might be useful down the line. There's a lifetime issue to be aware of though. Maybe it's not a problem if there are already guarantees that WebRtcVoiceMediaChannel always outlives any default ssrc channel. If such a guarantee exists, then the shim will always be deleted before WebRtcVoiceMediaChannel and therefore before the default sink goes out of scope. If there are not such guarantees, then there needs to be a way to either for the shim to null out its pointer to the default sink, or, if we pass ownership to the shim, to let the shim hand back ownership to WebRtcVoiceMediaChannel if it exists (and not of course, if it doesn't exist). We can discuss in more detail if needed. However, since M49 cut is coming up, we need to resolve this today or tomorrow.
Message was sent while issue was closed.
On 2016/01/13 15:02:54, tommi-webrtc wrote: > On 2016/01/13 14:18:15, tommi-webrtc wrote: > > On 2016/01/13 00:45:44, commit-bot: I haz the power wrote: > > > Patchset 7 (id:??) landed as > > > https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 > > > Cr-Commit-Position: refs/heads/master@{#11230} > > > > Sorry, I just now saw that this CL changes the AudioSinkInterface to be > > refcounted. That changes the semantics quite a bit and I think we might need > to > > revert this. I should have taken a look at more files. > > > > See for example the dtor of RemoteAudioSource::Sink. The contract there is > that > > when the sink is deleted, it means that it has been detached from the source. > > With the added reference counting, the lifetime is no longer deterministic and > > that will cause problems. > > As a proposal for how to do this differently, I suggest: > > * Store the scoped_ptr to the default sink in WebRtcVoiceMediaChannel as you're > doing, but not ref counted. > * Whenever a default ssrc is set up, create a new sink, which is a special sink > implementation for the default ssrc. > * This new sink is basically just a shim on top of the default sink that's > ultimately owned by WebRtcVoiceMediaChannel. > * Whenever a default ssrc goes away, the shim is deleted, but the default sink > is not. It will implicitly enter an 'unused' state though, which might be > useful down the line. > > There's a lifetime issue to be aware of though. Maybe it's not a problem if > there are already guarantees that WebRtcVoiceMediaChannel always outlives any > default ssrc channel. If such a guarantee exists, then the shim will always be > deleted before WebRtcVoiceMediaChannel and therefore before the default sink > goes out of scope. If there are not such guarantees, then there needs to be a > way to either for the shim to null out its pointer to the default sink, or, if > we pass ownership to the shim, to let the shim hand back ownership to > WebRtcVoiceMediaChannel if it exists (and not of course, if it doesn't exist). > > We can discuss in more detail if needed. However, since M49 cut is coming up, > we need to resolve this today or tomorrow. WVoMC deletes any of its receive streams, default or not: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
Message was sent while issue was closed.
On 2016/01/13 15:02:54, tommi-webrtc wrote: > On 2016/01/13 14:18:15, tommi-webrtc wrote: > > On 2016/01/13 00:45:44, commit-bot: I haz the power wrote: > > > Patchset 7 (id:??) landed as > > > https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 > > > Cr-Commit-Position: refs/heads/master@{#11230} > > > > Sorry, I just now saw that this CL changes the AudioSinkInterface to be > > refcounted. That changes the semantics quite a bit and I think we might need > to > > revert this. I should have taken a look at more files. > > > > See for example the dtor of RemoteAudioSource::Sink. The contract there is > that > > when the sink is deleted, it means that it has been detached from the source. > > With the added reference counting, the lifetime is no longer deterministic and > > that will cause problems. > > As a proposal for how to do this differently, I suggest: > > * Store the scoped_ptr to the default sink in WebRtcVoiceMediaChannel as you're > doing, but not ref counted. > * Whenever a default ssrc is set up, create a new sink, which is a special sink > implementation for the default ssrc. > * This new sink is basically just a shim on top of the default sink that's > ultimately owned by WebRtcVoiceMediaChannel. > * Whenever a default ssrc goes away, the shim is deleted, but the default sink > is not. It will implicitly enter an 'unused' state though, which might be > useful down the line. > > There's a lifetime issue to be aware of though. Maybe it's not a problem if > there are already guarantees that WebRtcVoiceMediaChannel always outlives any > default ssrc channel. If such a guarantee exists, then the shim will always be > deleted before WebRtcVoiceMediaChannel and therefore before the default sink > goes out of scope. If there are not such guarantees, then there needs to be a > way to either for the shim to null out its pointer to the default sink, or, if > we pass ownership to the shim, to let the shim hand back ownership to > WebRtcVoiceMediaChannel if it exists (and not of course, if it doesn't exist). > > We can discuss in more detail if needed. However, since M49 cut is coming up, > we need to resolve this today or tomorrow. I don't see how the lifetime is any less deterministic with reference counting. The sink just goes from having 1 owner to having 2 owners (in the case of unsignaled SSRCs). You could argue that someone may forget to release a reference, but someone could just as easily forget to delete the Sink if it isn't ref-counted. The shim idea should work fine as an alternative, though. And the advantage of that approach is that it's a small change for a small corner case (as opposed to changing scoped_ptr to scoped_refptr *everywhere*). I'll revert this CL and do that.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1588693002/ by deadbeef@webrtc.org. The reason for reverting is: tommi pointed out that using a refptr for the sink may cause issues. Will reland with a slightly different approach..
Message was sent while issue was closed.
Description was changed from ========== Storing raw audio sink for default audio track. BUG=webrtc:5250 Committed: https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 Cr-Commit-Position: refs/heads/master@{#11230} ========== to ========== Storing raw audio sink for default audio track. BUG=webrtc:5250 Committed: https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 Cr-Commit-Position: refs/heads/master@{#11230} ==========
tommi: I implemented your suggestion of a proxy object. Does this look ok now?
Re "I don't see how the lifetime is any less deterministic with reference counting.": - For the default ssrc, which is really a 'virtual' ssrc that can cover multiple actual streams, there's not really a difference. However, for non-default ssrc's, or rather, when sinks are explicitly set for known ssrcs, there is a difference. Chrome sets sinks when streams exist (doesn't use the default ssrc) and depends on knowing when the streams go away in order to signal that the source has ended. That's the case I'm worried about regressing and because there is a specific contract with the lifetime of a sink being tied to the lifetime of the source, using reference counting breaks that contract. Wheter or not we should actually have a '0' ssrc, is debatable. We're setting up state for a stream that might never exist, mapping a 0 ssrc to an 'actual' ssrc so there may be two ways to address the same ssrc and it looks like we're assuming that an ssrc value of 0 is illegal in the protocol which I learned today is not true. So it might be a better approach to leave it up to the caller to hook up to whatever streams come and go and not try to do too much magic so to speak. I think we might be making things complicated for us even though (I think) that the idea with a default ssrc has been to simplify things. Thanks for reacting so quickly, reverting the other change and writing up a new one! Really appreciate it. lgtm! https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:2201: SetRawAudioSink(default_recv_ssrc_, std::move(proxy_sink)); Do we actually want to make this call if default_sink_ is null? I was thinking we'd just do something like if (default_sink_) SetRawAudioSink(default_recv_ssrc_, new ProxySink(default_sink_.get())); In either case, there's a concern that we might be overriding/disconnecting a different sink if one was already assigned. I guess we can check that later if it's indeed a possibility. https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:2438: default_sink_ = std::move(sink); nit: do you mind moving this above the check for -1 and use default_sink_ like you do above? For a while there I read this as |sink| going out of scope and that the proxy would have a bogus pointer. https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine_unittest.cc:75: class FakeAudioSink : public rtc::RefCountedObject<webrtc::AudioSinkInterface> { no need for refcounted?
https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:2201: SetRawAudioSink(default_recv_ssrc_, std::move(proxy_sink)); On 2016/01/13 23:14:24, tommi-webrtc wrote: > Do we actually want to make this call if default_sink_ is null? I was thinking > we'd just do something like > > if (default_sink_) > SetRawAudioSink(default_recv_ssrc_, new ProxySink(default_sink_.get())); > > In either case, there's a concern that we might be overriding/disconnecting a > different sink if one was already assigned. I guess we can check that later if > it's indeed a possibility. I had "if (default_sink_)" originally, but I was suggested to change it. I'll put it back since it makes more sense now. Also, at this point it will be impossible to override an existing sink, since the stream was just created (inside AddRecvStream). https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:2438: default_sink_ = std::move(sink); On 2016/01/13 23:14:24, tommi-webrtc wrote: > nit: do you mind moving this above the check for -1 and use default_sink_ like > you do above? For a while there I read this as |sink| going out of scope and > that the proxy would have a bogus pointer. Is the sink accessed on any other threads? If so I can't move this line, since it will implicitly destroy the existing default sink (if there is one). The new sink needs to be set before the old is destroyed.
lgtm with nits https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:2201: SetRawAudioSink(default_recv_ssrc_, std::move(proxy_sink)); On 2016/01/13 23:55:12, Taylor Brandstetter wrote: > On 2016/01/13 23:14:24, tommi-webrtc wrote: > > Do we actually want to make this call if default_sink_ is null? I was > thinking > > we'd just do something like > > > > if (default_sink_) > > SetRawAudioSink(default_recv_ssrc_, new ProxySink(default_sink_.get())); > > > > In either case, there's a concern that we might be overriding/disconnecting a > > different sink if one was already assigned. I guess we can check that later > if > > it's indeed a possibility. > > I had "if (default_sink_)" originally, but I was suggested to change it. I'll > put it back since it makes more sense now. Yes, now that we can avoid allocating an object it makes sense. > Also, at this point it will be impossible to override an existing sink, since > the stream was just created (inside AddRecvStream). https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:2438: default_sink_ = std::move(sink); On 2016/01/13 23:55:12, Taylor Brandstetter wrote: > On 2016/01/13 23:14:24, tommi-webrtc wrote: > > nit: do you mind moving this above the check for -1 and use default_sink_ like > > you do above? For a while there I read this as |sink| going out of scope and > > that the proxy would have a bogus pointer. > > Is the sink accessed on any other threads? If so I can't move this line, since > it will implicitly destroy the existing default sink (if there is one). The new > sink needs to be set before the old is destroyed. I'd still like you to avoid the duplicate log lines that will result from recursively calling SetRawAudioSink. https://codereview.chromium.org/1551813002/diff/180001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1551813002/diff/180001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:137: ProxySink(AudioSinkInterface* sink) : sink_(sink) {} nit: explicit add RTC_DCHECK(sink_); in the ctor body. https://codereview.chromium.org/1551813002/diff/180001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.chromium.org/1551813002/diff/180001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine_unittest.cc:75: class FakeAudioSink : public rtc::RefCountedObject<webrtc::AudioSinkInterface> { RefCountedObject not needed anymore
tommi@chromium.org changed reviewers: + tommi@chromium.org
lgtm https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:2438: default_sink_ = std::move(sink); On 2016/01/13 23:55:12, Taylor Brandstetter wrote: > On 2016/01/13 23:14:24, tommi-webrtc wrote: > > nit: do you mind moving this above the check for -1 and use default_sink_ like > > you do above? For a while there I read this as |sink| going out of scope and > > that the proxy would have a bogus pointer. > > Is the sink accessed on any other threads? If so I can't move this line, since > it will implicitly destroy the existing default sink (if there is one). The new > sink needs to be set before the old is destroyed. ah of course
https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:2438: default_sink_ = std::move(sink); On 2016/01/14 09:24:39, the sun wrote: > On 2016/01/13 23:55:12, Taylor Brandstetter wrote: > > On 2016/01/13 23:14:24, tommi-webrtc wrote: > > > nit: do you mind moving this above the check for -1 and use default_sink_ > like > > > you do above? For a while there I read this as |sink| going out of scope > and > > > that the proxy would have a bogus pointer. > > > > Is the sink accessed on any other threads? If so I can't move this line, since > > it will implicitly destroy the existing default sink (if there is one). The > new > > sink needs to be set before the old is destroyed. > > I'd still like you to avoid the duplicate log lines that will result from > recursively calling SetRawAudioSink. My reasoning is that SetRenderer does the same thing: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... That's why I added the SSRC to the log, so you'll see: "SetRawAudioSink: ssrc 0" "SetRawAudioSink: ssrc X" Which I've found is actually more helpful than having a single log line. https://codereview.chromium.org/1551813002/diff/180001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1551813002/diff/180001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:137: ProxySink(AudioSinkInterface* sink) : sink_(sink) {} On 2016/01/14 09:24:39, the sun wrote: > nit: explicit > > add > RTC_DCHECK(sink_); > in the ctor body. Done. https://codereview.chromium.org/1551813002/diff/180001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right): https://codereview.chromium.org/1551813002/diff/180001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine_unittest.cc:75: class FakeAudioSink : public rtc::RefCountedObject<webrtc::AudioSinkInterface> { On 2016/01/14 09:24:39, the sun wrote: > RefCountedObject not needed anymore Done.
lgtm https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.chromium.org/1551813002/diff/160001/talk/media/webrtc/webr... talk/media/webrtc/webrtcvoiceengine.cc:2438: default_sink_ = std::move(sink); On 2016/01/14 15:48:47, Taylor Brandstetter wrote: > On 2016/01/14 09:24:39, the sun wrote: > > On 2016/01/13 23:55:12, Taylor Brandstetter wrote: > > > On 2016/01/13 23:14:24, tommi-webrtc wrote: > > > > nit: do you mind moving this above the check for -1 and use default_sink_ > > like > > > > you do above? For a while there I read this as |sink| going out of scope > > and > > > > that the proxy would have a bogus pointer. > > > > > > Is the sink accessed on any other threads? If so I can't move this line, > since > > > it will implicitly destroy the existing default sink (if there is one). The > > new > > > sink needs to be set before the old is destroyed. > > > > I'd still like you to avoid the duplicate log lines that will result from > > recursively calling SetRawAudioSink. > > My reasoning is that SetRenderer does the same thing: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > That's why I added the SSRC to the log, so you'll see: > "SetRawAudioSink: ssrc 0" > "SetRawAudioSink: ssrc X" > > Which I've found is actually more helpful than having a single log line. Acknowledged.
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, tommi@webrtc.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1551813002/#ps200001 (title: "Removing obsolete "RefCountedObject" and adding an RTC_DCHECK.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1551813002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551813002/200001
Message was sent while issue was closed.
Description was changed from ========== Storing raw audio sink for default audio track. BUG=webrtc:5250 Committed: https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 Cr-Commit-Position: refs/heads/master@{#11230} ========== to ========== Storing raw audio sink for default audio track. BUG=webrtc:5250 Committed: https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 Cr-Commit-Position: refs/heads/master@{#11230} ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Storing raw audio sink for default audio track. BUG=webrtc:5250 Committed: https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 Cr-Commit-Position: refs/heads/master@{#11230} ========== to ========== Storing raw audio sink for default audio track. BUG=webrtc:5250 Committed: https://crrev.com/e591f9377f33f3f725a30faecd1bef1a71fa6b99 Cr-Commit-Position: refs/heads/master@{#11230} Committed: https://crrev.com/884f58523a75ace3239805759147948b7e868aea Cr-Commit-Position: refs/heads/master@{#11275} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/884f58523a75ace3239805759147948b7e868aea Cr-Commit-Position: refs/heads/master@{#11275} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
