Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
163553006:
Cast: Refactoring Cast API's (Closed)
|
Created:
6 years, 10 months ago by mikhal1 Modified:
6 years, 9 months ago Reviewers:
Alpha Left Google CC:
chromium-reviews, hclam+watch_chromium.org, mikhal+watch_chromium.org, hguihot+watch_chromium.org, jasonroberts+watch_google.com, pwestin+watch_google.com, feature-media-reviews_chromium.org, hubbe+watch_chromium.org, miu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCast: Refactoring Cast API's
Main changes:
1. Adding an IntializeAudio and InitializeVideo to CastSender.
2. Separating FrameInput to audio and video which enables initializing each separately.
3. Changing the CastSender and CastReceiver Create functions to return a scoped_ptr.
These changes better align Cast with the Chromium pipeline.
BUG=346822
R=hclam@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255954
Patch Set 1 #
Total comments: 44
Patch Set 2 : Responding to review #
Total comments: 24
Patch Set 3 : Refactoring and responding to review #
Total comments: 27
Patch Set 4 : Updates and rebase #
Total comments: 4
Patch Set 5 : nits + rebase #Patch Set 6 : rebase #Patch Set 7 : yet another rebase #Patch Set 8 : rebase #
Messages
Total messages: 55 (0 generated)
Please review, -Mikhal
I don't really know cast code so take my comments with a grain of salt. https://codereview.chromium.org/163553006/diff/1/chrome/renderer/media/cast_s... File chrome/renderer/media/cast_session_delegate.cc (right): https://codereview.chromium.org/163553006/diff/1/chrome/renderer/media/cast_s... chrome/renderer/media/cast_session_delegate.cc:94: base::Unretained(this)), What makes this Unretained safe? Is it that this bound callback is only invoked on the IO thread and only as a result of calling Initialize{Audio,Video} on the IO thread, and that synchronously, and the dtor DCHECKing that this is only deleted on the IO thread? Because if the answer is yeah, the above, it might be worth describing in a note :) https://codereview.chromium.org/163553006/diff/1/chrome/renderer/media/cast_s... File chrome/renderer/media/cast_session_delegate.h (right): https://codereview.chromium.org/163553006/diff/1/chrome/renderer/media/cast_s... chrome/renderer/media/cast_session_delegate.h:59: // If this callback is called with STATUS_INITIALIZED it will report back update doco https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender.h File media/cast/cast_sender.h (right): https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender.h#new... media/cast/cast_sender.h:34: // This Class is thread safe. "thread safe" usually implies callable from any thread. The new API methods you've added use WeakPtr and thus imply being bound to a particular thread. What's up with that? https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender.h#new... media/cast/cast_sender.h:53: virtual void SetAudioSender(base::WeakPtr<AudioSender> audio_sender) = 0; doco (esp. for public, esp^2 for pure virtual) https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender.h#new... media/cast/cast_sender.h:70: static CastSender* CreateCastSender( This doesn't doco ownership semantics of the returned object. I believe this should instead return a scoped_ptr<CastSender> which would obviate the need for a comment. https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.cc File media/cast/cast_sender_impl.cc (right): https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:37: NOTREACHED(); Then why not simply CHECK(video_sender_); ? https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:44: base::Unretained(this), what makes Unretained safe? (in general, any Unretained should be explained away by a comment somewhere...) https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:53: video_sender_->InsertRawVideoFrame(video_frame, capture_time); What if video_sender_'s been invalidated by this point? I suspect what you really want (instead of the Unretained at l.44) is something more like Bind(&VideoSender::InsertRawVideoFrame, video_sender, video_frame, capture_time) at l.44, and then you can drop this method? https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:56: virtual void InsertAudio(const AudioBus* audio_bus, same comments apply to audio below as video above. https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:95: const CastInitializationCallback& cast_initialization, fwiw in media/ code we've followed a convention that FooCallback param names are suffixed with _cb https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:108: base::Bind(&CastSenderImpl::ReceivedPacket, base::Unretained(this))), I'm guessing Unretained here to avoid a reference cycle, but should doco why it's safe. https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.h File media/cast/cast_sender_impl.h (right): https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.h:26: // This calls is a pure owner class that group all required sending objects s/calls// ? (I bet you can English this comment up ;)) https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.h:57: transport::CastTransportSender* const transport_sender_; // owned by ??? https://codereview.chromium.org/163553006/diff/1/media/cast/test/end2end_unit... File media/cast/test/end2end_unittest.cc (right): https://codereview.chromium.org/163553006/diff/1/media/cast/test/end2end_unit... media/cast/test/end2end_unittest.cc:548: EXPECT_TRUE(end_result); Any desire to check that each status is hit exactly once? https://codereview.chromium.org/163553006/diff/1/media/cast/test/sender.cc File media/cast/test/sender.cc (right): https://codereview.chromium.org/163553006/diff/1/media/cast/test/sender.cc#ne... media/cast/test/sender.cc:302: CHECK(end_result); ditto https://codereview.chromium.org/163553006/diff/1/media/cast/test/sender.cc#ne... media/cast/test/sender.cc:303: VLOG(1) << "Cast Sender initialized"; a bit of an overstatement now
https://codereview.chromium.org/163553006/diff/1/chrome/renderer/media/cast_s... File chrome/renderer/media/cast_session_delegate.cc (right): https://codereview.chromium.org/163553006/diff/1/chrome/renderer/media/cast_s... chrome/renderer/media/cast_session_delegate.cc:94: base::Unretained(this)), I too this this needs an explanation. Please make sure the callback is called on the cast main thread. If it happens asynchronously this should be a weak pointer. https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender.h File media/cast/cast_sender.h (right): https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender.h#new... media/cast/cast_sender.h:70: static CastSender* CreateCastSender( This can just be CastSender::Create(...). CastSender::CreateCastSender is clumsy. https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.cc File media/cast/cast_sender_impl.cc (right): https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:37: NOTREACHED(); NOTREACHED() should be a DCHECK. So this should be DCHECK(video_sender_); https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:44: base::Unretained(this), FrameInput is RefCountedThreadSafe so there's no need to enclose this by base::Unretained. https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:53: video_sender_->InsertRawVideoFrame(video_frame, capture_time); On 2014/02/13 18:02:14, Ami Fischman wrote: > What if video_sender_'s been invalidated by this point? > I suspect what you really want (instead of the Unretained at l.44) is something > more like > Bind(&VideoSender::InsertRawVideoFrame, video_sender, video_frame, capture_time) > at l.44, and then you can drop this method? video_sender_ is a WeakPtr. That's why it's dereferenced in a separate method. https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:119: CHECK(audio_config.use_external_encoder || DCHECK that this is called on the cast main thread. https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:142: CHECK(video_config.use_external_encoder || DCHECK that this is called on the cast main thread.
https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.cc File media/cast/cast_sender_impl.cc (right): https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:53: video_sender_->InsertRawVideoFrame(video_frame, capture_time); On 2014/02/13 19:53:59, Alpha wrote: > On 2014/02/13 18:02:14, Ami Fischman wrote: > > What if video_sender_'s been invalidated by this point? > > I suspect what you really want (instead of the Unretained at l.44) is > something > > more like > > Bind(&VideoSender::InsertRawVideoFrame, video_sender, video_frame, > capture_time) > > at l.44, and then you can drop this method? > > video_sender_ is a WeakPtr. That's why it's dereferenced in a separate method. I don't understand this comment. Bind is magic when given a weakptr (the way I suggested) but the way it's used in deref'd in the separate method now the magic is not used and instead the weakptr is unconditionally deref'd which will crash after invalidation.
https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.cc File media/cast/cast_sender_impl.cc (right): https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:53: video_sender_->InsertRawVideoFrame(video_frame, capture_time); The reason doing a Bind in line 44 doesn't work is because InsertRawVideoFrame can be called on any thread while SetVideoSender is called on the main thread. There's a race condition for accessing video_sender_. And you're right doing a null check for audio_sender_ and video_sender_. It should be done in InsertRawVideoOnMainThread and InsertAudioOnMainThread.
https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.cc File media/cast/cast_sender_impl.cc (right): https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:53: video_sender_->InsertRawVideoFrame(video_frame, capture_time); On 2014/02/13 20:50:35, Alpha wrote: > The reason doing a Bind in line 44 doesn't work is because InsertRawVideoFrame > can be called on any thread while SetVideoSender is called on the main thread. > There's a race condition for accessing video_sender_. I think you misunderstood my suggestion. I'm suggesting the new code should be: cast_environment_->PostTask( CastEnvironment::MAIN, FROM_HERE, base::Bind(&VideoSender::InsertRawVideoFrame, video_sender, video_frame, capture_time));
https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.cc File media/cast/cast_sender_impl.cc (right): https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... media/cast/cast_sender_impl.cc:53: video_sender_->InsertRawVideoFrame(video_frame, capture_time); On 2014/02/13 20:57:31, Ami Fischman wrote: > On 2014/02/13 20:50:35, Alpha wrote: > > The reason doing a Bind in line 44 doesn't work is because InsertRawVideoFrame > > can be called on any thread while SetVideoSender is called on the main thread. > > There's a race condition for accessing video_sender_. > > I think you misunderstood my suggestion. > I'm suggesting the new code should be: > cast_environment_->PostTask( > CastEnvironment::MAIN, > FROM_HERE, > base::Bind(&VideoSender::InsertRawVideoFrame, video_sender, video_frame, > capture_time)); > hclam@ points out in person that if SetVideoSender is called concurrently with or after the Post happens then the bound video_sender_ would be outdated/wrong/NULL. If it can't be guaranteed that SetVideoSender is called only before the first invocation of InsertRawVideoFrame then a helper/trampoline will be required.
On 2014/02/13 21:02:30, Ami Fischman wrote: > https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.cc > File media/cast/cast_sender_impl.cc (right): > > https://codereview.chromium.org/163553006/diff/1/media/cast/cast_sender_impl.... > media/cast/cast_sender_impl.cc:53: > video_sender_->InsertRawVideoFrame(video_frame, capture_time); > On 2014/02/13 20:57:31, Ami Fischman wrote: > > On 2014/02/13 20:50:35, Alpha wrote: > > > The reason doing a Bind in line 44 doesn't work is because > InsertRawVideoFrame > > > can be called on any thread while SetVideoSender is called on the main > thread. > > > There's a race condition for accessing video_sender_. > > > > I think you misunderstood my suggestion. > > I'm suggesting the new code should be: > > cast_environment_->PostTask( > > CastEnvironment::MAIN, > > FROM_HERE, > > base::Bind(&VideoSender::InsertRawVideoFrame, video_sender, video_frame, > > capture_time)); > > > > hclam@ points out in person that if SetVideoSender is called concurrently with > or after the Post happens then the bound video_sender_ would be > outdated/wrong/NULL. > If it can't be guaranteed that SetVideoSender is called only before the first > invocation of InsertRawVideoFrame then a helper/trampoline will be required. While this code works (using a helper method to trampoline to the main thread). What would be a better design is to not have a single FrameInput that proxies audio and video frames. Say if we have a AudioFrameInput and VideoFrameInput interfaces separately in CastSender. Then there's no needs to do this SetAudioSender and SetVideoSender business. Because AudioFrameInput and VideoFrameInput would be constructed with a corresponding AudioSender and VideoSender. We should probably just do this.
PTAL https://chromiumcodereview.appspot.com/163553006/diff/1/chrome/renderer/media... File chrome/renderer/media/cast_session_delegate.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/1/chrome/renderer/media... chrome/renderer/media/cast_session_delegate.cc:94: base::Unretained(this)), Switching to a weak ptr. On 2014/02/13 19:53:59, Alpha wrote: > I too this this needs an explanation. > > Please make sure the callback is called on the cast main thread. If it happens > asynchronously this should be a weak pointer. https://chromiumcodereview.appspot.com/163553006/diff/1/chrome/renderer/media... File chrome/renderer/media/cast_session_delegate.h (right): https://chromiumcodereview.appspot.com/163553006/diff/1/chrome/renderer/media... chrome/renderer/media/cast_session_delegate.h:59: // If this callback is called with STATUS_INITIALIZED it will report back On 2014/02/13 18:02:14, Ami Fischman wrote: > update doco Done. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sender.h File media/cast/cast_sender.h (right): https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender.h:34: // This Class is thread safe. Comment irrelevant. New comments added. On 2014/02/13 18:02:14, Ami Fischman wrote: > "thread safe" usually implies callable from any thread. > The new API methods you've added use WeakPtr and thus imply being bound to a > particular thread. What's up with that? https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender.h:53: virtual void SetAudioSender(base::WeakPtr<AudioSender> audio_sender) = 0; On 2014/02/13 18:02:14, Ami Fischman wrote: > doco (esp. for public, esp^2 for pure virtual) Done. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender.h:70: static CastSender* CreateCastSender( Renamed. On 2014/02/13 19:53:59, Alpha wrote: > This can just be CastSender::Create(...). CastSender::CreateCastSender is > clumsy. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... File media/cast/cast_sender_impl.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender_impl.cc:37: NOTREACHED(); Adding the DCHECKs cause the code to crash when calling the wek_ptr, as it calls audio/video sender on different threads. On 2014/02/13 19:53:59, Alpha wrote: > NOTREACHED() should be a DCHECK. So this should be DCHECK(video_sender_); https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender_impl.cc:53: video_sender_->InsertRawVideoFrame(video_frame, capture_time); Redone. Let me know what you think. On 2014/02/13 21:02:30, Ami Fischman wrote: > On 2014/02/13 20:57:31, Ami Fischman wrote: > > On 2014/02/13 20:50:35, Alpha wrote: > > > The reason doing a Bind in line 44 doesn't work is because > InsertRawVideoFrame > > > can be called on any thread while SetVideoSender is called on the main > thread. > > > There's a race condition for accessing video_sender_. > > > > I think you misunderstood my suggestion. > > I'm suggesting the new code should be: > > cast_environment_->PostTask( > > CastEnvironment::MAIN, > > FROM_HERE, > > base::Bind(&VideoSender::InsertRawVideoFrame, video_sender, video_frame, > > capture_time)); > > > > hclam@ points out in person that if SetVideoSender is called concurrently with > or after the Post happens then the bound video_sender_ would be > outdated/wrong/NULL. > If it can't be guaranteed that SetVideoSender is called only before the first > invocation of InsertRawVideoFrame then a helper/trampoline will be required. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender_impl.cc:56: virtual void InsertAudio(const AudioBus* audio_bus, On 2014/02/13 18:02:14, Ami Fischman wrote: > same comments apply to audio below as video above. Done. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender_impl.cc:95: const CastInitializationCallback& cast_initialization, On 2014/02/13 18:02:14, Ami Fischman wrote: > fwiw in media/ code we've followed a convention that FooCallback param names are > suffixed with _cb Done. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender_impl.cc:108: base::Bind(&CastSenderImpl::ReceivedPacket, base::Unretained(this))), On 2014/02/13 18:02:14, Ami Fischman wrote: > I'm guessing Unretained here to avoid a reference cycle, but should doco why > it's safe. Done. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender_impl.cc:119: CHECK(audio_config.use_external_encoder || On 2014/02/13 19:53:59, Alpha wrote: > DCHECK that this is called on the cast main thread. Done. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender_impl.cc:142: CHECK(video_config.use_external_encoder || On 2014/02/13 19:53:59, Alpha wrote: > DCHECK that this is called on the cast main thread. Done. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... File media/cast/cast_sender_impl.h (right): https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender_impl.h:26: // This calls is a pure owner class that group all required sending objects hmmm, Even I can't understand this one :) Let's see if I can do better. On 2014/02/13 18:02:14, Ami Fischman wrote: > s/calls// ? > (I bet you can English this comment up ;)) https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/cast_sende... media/cast/cast_sender_impl.h:57: transport::CastTransportSender* const transport_sender_; On 2014/02/13 18:02:14, Ami Fischman wrote: > // owned by ??? Done. https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/test/end2e... File media/cast/test/end2end_unittest.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/test/end2e... media/cast/test/end2end_unittest.cc:548: EXPECT_TRUE(end_result); Wasn't aware of one, but since you brought it up.... On 2014/02/13 18:02:14, Ami Fischman wrote: > Any desire to check that each status is hit exactly once? https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/test/sende... File media/cast/test/sender.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/test/sende... media/cast/test/sender.cc:302: CHECK(end_result); Probably an overkill here, as it's just a test app, and not an automated test. On 2014/02/13 18:02:14, Ami Fischman wrote: > ditto https://chromiumcodereview.appspot.com/163553006/diff/1/media/cast/test/sende... media/cast/test/sender.cc:303: VLOG(1) << "Cast Sender initialized"; On 2014/02/13 18:02:14, Ami Fischman wrote: > a bit of an overstatement now Done.
https://chromiumcodereview.appspot.com/163553006/diff/650001/chrome/renderer/... File chrome/renderer/media/cast_session_delegate.h (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/chrome/renderer/... chrome/renderer/media/cast_session_delegate.h:40: class CastSessionDelegate : public base::SupportsWeakPtr<CastSessionDelegate> { Why this SupportsWeakPtr? https://chromiumcodereview.appspot.com/163553006/diff/650001/chrome/renderer/... chrome/renderer/media/cast_session_delegate.h:84: base::WeakPtrFactory<CastSessionDelegate> weak_factory_; doco as bound to io_message_loop_proxy_. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... File media/cast/cast_sender.h (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender.h:9: // the IO thread. However they are allowed to be called from any thread. no longer true https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender.h:73: static CastSender* Create( I think you missed this comment: This doesn't doco ownership semantics of the returned object. I believe this should instead return a scoped_ptr<CastSender> which would obviate the need for a comment. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... File media/cast/cast_sender_impl.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.cc:41: video_sender_, Since InsertRVF() can be called on any thread, what guarantees that the write to video_sender_ in SetVideoSender will be observed here? Sadly I think you need the trampoline back, or else to introduce a mutex around read/write of video_sender_. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.cc:52: audio_sender_, ditto https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.cc:86: // therefore we can call base::Unretained. Class is valid as long as OtherClass is valid doesn't read right to me, because the "validity" being discussed is of instances, not classes. You need to explain how it is guaranteed that packet_receiver_ will not be .Run() after |this| is deleted in order to justify the use of Unretained(this). If my code-skimming is right, that means that whoever called packet_receiver() needs to be told to stop using it before |this| is deleted. But if that's already being done (and the receiver callback is dropped in the process) then I think you can avoid the Unretained by dropping packet_receiver_ entirely and returning a newly-bound (on a refcounted this) callback in packet_receiver() (which will need to be renamed PacketReceiver() to reflect its non-getter-ness). Then you won't have to worry about a ref cycle b/c the callback is dropped during teardown. Of course, that's only ref-cycle-free _if_ the previous paragraph really holds. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.cc:206: return base::Bind(&CastSenderImpl::ReceivedPacket, base::Unretained(this)); lolwat? https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... File media/cast/cast_sender_impl.h (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.h:55: transport_sender_; // Not owned by this class. I was hoping for a positive statement about who _does_ own this. Because that's basically a requirement that that owner outlive this class. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/test/... File media/cast/test/end2end_unittest.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/test/... media/cast/test/end2end_unittest.cc:555: ASSERT_TRUE(false); FAIL() https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/test/... media/cast/test/end2end_unittest.cc:558: EXPECT_GE(1, video_initialization_cnt_); s/GE/EQ/ here and on the previous line? https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/video... File media/cast/video_sender/video_sender.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/video... media/cast/video_sender/video_sender.cc:148: // TODO(pwestin): pass cast_initialization_cb to |video_encoder_| cast_initialization_cb isn't a thing here; hopefully the comment makes sense to cast folk anyway.
New patch uploaded. In addition to the review comments, I made the following main refactoring changes: 1. Separated the FrameInput to audio and video. 2. Changed all Cast interfaces to return a scoped_ptr. PTAL. Thanks, -Mikhal https://chromiumcodereview.appspot.com/163553006/diff/650001/chrome/renderer/... File chrome/renderer/media/cast_session_delegate.h (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/chrome/renderer/... chrome/renderer/media/cast_session_delegate.h:40: class CastSessionDelegate : public base::SupportsWeakPtr<CastSessionDelegate> { On 2014/02/14 18:23:54, Ami Fischman (OOO Feb 19-23) wrote: > Why this SupportsWeakPtr? Done. https://chromiumcodereview.appspot.com/163553006/diff/650001/chrome/renderer/... chrome/renderer/media/cast_session_delegate.h:84: base::WeakPtrFactory<CastSessionDelegate> weak_factory_; On 2014/02/14 18:23:54, Ami Fischman (OOO Feb 19-23) wrote: > doco as bound to io_message_loop_proxy_. Done. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... File media/cast/cast_sender.h (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender.h:9: // the IO thread. However they are allowed to be called from any thread. Code refactored. On 2014/02/14 18:23:54, Ami Fischman (OOO Feb 19-23) wrote: > no longer true https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender.h:73: static CastSender* Create( Done, and matched receiver and transport. On 2014/02/14 18:23:54, Ami Fischman (OOO Feb 19-23) wrote: > I think you missed this comment: > > This doesn't doco ownership semantics of the returned object. > I believe this should instead return a scoped_ptr<CastSender> which would > obviate the need for a comment. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... File media/cast/cast_sender_impl.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.cc:41: video_sender_, Check out the new version! On 2014/02/14 18:23:54, Ami Fischman wrote: > Since InsertRVF() can be called on any thread, what guarantees that the write to > video_sender_ in SetVideoSender will be observed here? Sadly I think you need > the trampoline back, or else to introduce a mutex around read/write of > video_sender_. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.cc:52: audio_sender_, On 2014/02/14 18:23:54, Ami Fischman wrote: > ditto Done. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.cc:86: // therefore we can call base::Unretained. I think you may be on to something here :o Removed, as this is no longer needed (probably part of the bad merge). On 2014/02/14 18:23:54, Ami Fischman wrote: > Class is valid as long as OtherClass is valid > doesn't read right to me, because the "validity" being discussed is of > instances, not classes. > > You need to explain how it is guaranteed that packet_receiver_ will not be > .Run() after |this| is deleted in order to justify the use of Unretained(this). > > If my code-skimming is right, that means that whoever called packet_receiver() > needs to be told to stop using it before |this| is deleted. > > But if that's already being done (and the receiver callback is dropped in the > process) then I think you can avoid the Unretained by dropping packet_receiver_ > entirely and returning a newly-bound (on a refcounted this) callback in > packet_receiver() (which will need to be renamed PacketReceiver() to reflect its > non-getter-ness). Then you won't have to worry about a ref cycle b/c the > callback is dropped during teardown. > Of course, that's only ref-cycle-free _if_ the previous paragraph really holds. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.cc:206: return base::Bind(&CastSenderImpl::ReceivedPacket, base::Unretained(this)); hmmm...somewhat speechless. Bad merge? On 2014/02/14 18:23:54, Ami Fischman wrote: > lolwat? https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... File media/cast/cast_sender_impl.h (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/cast_... media/cast/cast_sender_impl.h:55: transport_sender_; // Not owned by this class. On 2014/02/14 18:23:54, Ami Fischman (OOO Feb 19-23) wrote: > I was hoping for a positive statement about who _does_ own this. Because that's > basically a requirement that that owner outlive this class. Done. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/test/... File media/cast/test/end2end_unittest.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/test/... media/cast/test/end2end_unittest.cc:555: ASSERT_TRUE(false); On 2014/02/14 18:23:54, Ami Fischman wrote: > FAIL() Done. https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/test/... media/cast/test/end2end_unittest.cc:558: EXPECT_GE(1, video_initialization_cnt_); Don't get the comment. Each can be either 0 or 1, depending on the order of the initialization. On 2014/02/14 18:23:54, Ami Fischman wrote: > s/GE/EQ/ here and on the previous line? https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/video... File media/cast/video_sender/video_sender.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/650001/media/cast/video... media/cast/video_sender/video_sender.cc:148: // TODO(pwestin): pass cast_initialization_cb to |video_encoder_| On 2014/02/14 18:23:54, Ami Fischman wrote: > cast_initialization_cb isn't a thing here; hopefully the comment makes sense to > cast folk anyway. Done.
Please hold on the review, as more changes are on their way.
Second thought - All required changes to CastTransportSender will be done in a separate cl (coming soon!). Please review this, and note that it can only be rebased & committed after the transport cl is done.
On 2014/02/18 21:54:29, mikhal1 wrote: > Second thought - All required changes to CastTransportSender will be done in a > separate cl (coming soon!). > Please review this, and note that it can only be rebased & committed after the > transport cl is done. which transport cl?
On 2014/02/18 21:56:12, Alpha wrote: > On 2014/02/18 21:54:29, mikhal1 wrote: > > Second thought - All required changes to CastTransportSender will be done in a > > separate cl (coming soon!). > > Please review this, and note that it can only be rebased & committed after the > > transport cl is done. > > which transport cl? The one coming soon, not uploaded yet.
https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... File chrome/renderer/media/cast_session_delegate.cc (right): https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.cc:109: audio_frame_input_available_callback_.reset( This is very odd. We rarely use a scoped_ptr to hold a callback. This should just be: audio_frame_input_available_callback_ = callback; Also the assignment of callback should come before InitializeAudio is called. https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.cc:118: // Initialize video - set gpu to NULL. nit: This comment should be a TODO to pass in a valid GpuVideoAcceleratorFactories to support hardware video encoding. https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.cc:120: video_frame_input_available_callback_.reset( This should just be: video_frame_input_available_callback_ = callback; This line should be called before InitializeVideo. https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.cc:130: if (audio_frame_input_available_callback_) This should be audio_frame_input_available_callback_.is_null(). https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... File chrome/renderer/media/cast_session_delegate.h (right): https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.h:42: typedef base::Callback<void(const scoped_refptr<media::cast::FrameInput>&)> There's no more FrameInput isn't it? https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.h:78: scoped_ptr<FrameInputAvailableCallback> audio_frame_input_available_callback_; I don't you didn't change this line but this looks very odd. Might as well fix it in this change. There's no need to have a scoped_ptr to a callback. This should just be: FrameInputAvailableCallback audio_frame_input_available_callback_; FrameInputAvailableCallback video_frame_input_available_callback_; https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.h:84: // Weak pointer bound to io_message_loop_proxy_. nit: |io_message_loop_proxy_|. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.h File media/cast/cast_sender.h (right): https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:8: // be accessed from the IO thread. However they may be called from any thread. media/cast has no concept of an "IO" thread. This should be the Cast main thread. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:66: // This Class is thread safe. This is actually not true. You might as well say all methods of CastSender must be accessed on the main thread. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:79: // Can be called from any thread. This can only be called on the main thread because in CastSenderImpl video_frame_input() returns a valid pointer only after InitializeVideo() is called. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:83: // Can be called from any thread. This can only be called on the main thread. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:91: virtual void InitializeAudio(const AudioSenderConfig& audio_config) = 0; These two methods should be called on the main thread as well. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender_... File media/cast/cast_sender_impl.cc (right): https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender_... media/cast/cast_sender_impl.cc:207: return audio_frame_input_; This method is not really thread-safe. I'd just say all methods of CastSender should be called on the main thread (because they actually are).
Uploaded a new patch for review. The CastTransport was handled in a designated cl (https://codereview.chromium.org/174183003), and therefore I could not update this cl without rebasing. Consequently, this patch has a lot of changes, most of which irrelevant to this change. Alpha - I know you're familiar with the code and the recent changes, so let me know if you'd prefer to continue with this cl, or if you'd rather that I start a clean cl. Thanks, -Mikhal https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... File chrome/renderer/media/cast_session_delegate.cc (right): https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.cc:109: audio_frame_input_available_callback_.reset( As mentioned before, resolved when rebased. On 2014/02/18 22:28:13, Alpha wrote: > This is very odd. We rarely use a scoped_ptr to hold a callback. This should > just be: > > audio_frame_input_available_callback_ = callback; > > Also the assignment of callback should come before InitializeAudio is called. https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.cc:118: // Initialize video - set gpu to NULL. On 2014/02/18 22:28:13, Alpha wrote: > nit: This comment should be a TODO to pass in a valid > GpuVideoAcceleratorFactories to support hardware video encoding. > Done. https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.cc:120: video_frame_input_available_callback_.reset( On 2014/02/18 22:28:13, Alpha wrote: > This should just be: > video_frame_input_available_callback_ = callback; > This line should be called before InitializeVideo. Done. https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.cc:130: if (audio_frame_input_available_callback_) On 2014/02/18 22:28:13, Alpha wrote: > This should be audio_frame_input_available_callback_.is_null(). Done. https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... File chrome/renderer/media/cast_session_delegate.h (right): https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.h:42: typedef base::Callback<void(const scoped_refptr<media::cast::FrameInput>&)> On 2014/02/18 22:28:13, Alpha wrote: > There's no more FrameInput isn't it? Done. https://codereview.chromium.org/163553006/diff/940008/chrome/renderer/media/c... chrome/renderer/media/cast_session_delegate.h:78: scoped_ptr<FrameInputAvailableCallback> audio_frame_input_available_callback_; Changes by rebasing. On 2014/02/18 22:28:13, Alpha wrote: > I don't you didn't change this line but this looks very odd. Might as well fix > it in this change. > > There's no need to have a scoped_ptr to a callback. This should just be: > > FrameInputAvailableCallback audio_frame_input_available_callback_; > FrameInputAvailableCallback video_frame_input_available_callback_; https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.h File media/cast/cast_sender.h (right): https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:8: // be accessed from the IO thread. However they may be called from any thread. On 2014/02/18 22:28:13, Alpha wrote: > media/cast has no concept of an "IO" thread. This should be the Cast main > thread. Done. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:66: // This Class is thread safe. On 2014/02/18 22:28:13, Alpha wrote: > This is actually not true. You might as well say all methods of CastSender must > be accessed on the main thread. Done. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:79: // Can be called from any thread. On 2014/02/18 22:28:13, Alpha wrote: > This can only be called on the main thread because in CastSenderImpl > video_frame_input() returns a valid pointer only after InitializeVideo() is > called. Done. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:79: // Can be called from any thread. On 2014/02/18 22:28:13, Alpha wrote: > This can only be called on the main thread because in CastSenderImpl > video_frame_input() returns a valid pointer only after InitializeVideo() is > called. Done. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:83: // Can be called from any thread. On 2014/02/18 22:28:13, Alpha wrote: > This can only be called on the main thread. Done. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:83: // Can be called from any thread. On 2014/02/18 22:28:13, Alpha wrote: > This can only be called on the main thread. Done. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender.... media/cast/cast_sender.h:91: virtual void InitializeAudio(const AudioSenderConfig& audio_config) = 0; On 2014/02/18 22:28:13, Alpha wrote: > These two methods should be called on the main thread as well. Done. https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender_... File media/cast/cast_sender_impl.cc (right): https://codereview.chromium.org/163553006/diff/940008/media/cast/cast_sender_... media/cast/cast_sender_impl.cc:207: return audio_frame_input_; On 2014/02/18 22:28:13, Alpha wrote: > This method is not really thread-safe. I'd just say all methods of CastSender > should be called on the main thread (because they actually are). Done.
ping
Two nits. https://codereview.chromium.org/163553006/diff/1230001/chrome/renderer/media/... File chrome/renderer/media/cast_session_delegate.cc (right): https://codereview.chromium.org/163553006/diff/1230001/chrome/renderer/media/... chrome/renderer/media/cast_session_delegate.cc:107: // local_endpoint_ = local_endpoint; These two lines are not needed. https://codereview.chromium.org/163553006/diff/1230001/media/cast/test/sender.cc File media/cast/test/sender.cc (right): https://codereview.chromium.org/163553006/diff/1230001/media/cast/test/sender... media/cast/test/sender.cc:326: CHECK(end_result); This should give a log message if !end_result is true.
PTAL https://chromiumcodereview.appspot.com/163553006/diff/1230001/chrome/renderer... File chrome/renderer/media/cast_session_delegate.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/1230001/chrome/renderer... chrome/renderer/media/cast_session_delegate.cc:107: // local_endpoint_ = local_endpoint; On 2014/03/06 23:10:04, Alpha wrote: > These two lines are not needed. Done. https://chromiumcodereview.appspot.com/163553006/diff/1230001/media/cast/test... File media/cast/test/sender.cc (right): https://chromiumcodereview.appspot.com/163553006/diff/1230001/media/cast/test... media/cast/test/sender.cc:326: CHECK(end_result); On 2014/03/06 23:10:04, Alpha wrote: > This should give a log message if !end_result is true. Done.
lgtm
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1250001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/renderer/media/cast_session_delegate.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/renderer/media/cast_session_delegate.cc
Hunk #2 FAILED at 62.
Hunk #3 FAILED at 79.
Hunk #5 FAILED at 187.
3 out of 5 hunks FAILED -- saving rejects to file
chrome/renderer/media/cast_session_delegate.cc.rej
Patch: chrome/renderer/media/cast_session_delegate.cc
Index: chrome/renderer/media/cast_session_delegate.cc
diff --git a/chrome/renderer/media/cast_session_delegate.cc
b/chrome/renderer/media/cast_session_delegate.cc
index
f72c682a669c6ef27f412cf7dd239b945fa7faa6..86eb86febeb8a9d74063d97e59a90f25a489c044
100644
--- a/chrome/renderer/media/cast_session_delegate.cc
+++ b/chrome/renderer/media/cast_session_delegate.cc
@@ -43,8 +43,7 @@ const int kMaxAudioEventEntries = kMaxSerializedBytes / 75;
} // namespace
CastSessionDelegate::CastSessionDelegate()
- : transport_configured_(false),
- io_message_loop_proxy_(
+ : io_message_loop_proxy_(
content::RenderThread::Get()->GetIOMessageLoopProxy()),
weak_factory_(this) {
DCHECK(io_message_loop_proxy_);
@@ -63,9 +62,48 @@ CastSessionDelegate::~CastSessionDelegate() {
}
}
-void CastSessionDelegate::Initialize() {
- if (cast_environment_)
- return; // Already initialized.
+void CastSessionDelegate::StartAudio(
+ const AudioSenderConfig& config,
+ const AudioFrameInputAvailableCallback& callback) {
+ DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+
+ audio_frame_input_available_callback_ = callback;
+ media::cast::transport::CastTransportAudioConfig transport_config;
+ transport_config.base.ssrc = config.sender_ssrc;
+ transport_config.codec = config.codec;
+ transport_config.base.rtp_config = config.rtp_config;
+ transport_config.frequency = config.frequency;
+ transport_config.channels = config.channels;
+ cast_transport_->InitializeAudio(transport_config);
+ cast_sender_->InitializeAudio(
+ config,
+ base::Bind(&CastSessionDelegate::InitializationResult,
+ weak_factory_.GetWeakPtr()));
+}
+
+void CastSessionDelegate::StartVideo(
+ const VideoSenderConfig& config,
+ const VideoFrameInputAvailableCallback& callback) {
+ DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+ video_frame_input_available_callback_ = callback;
+
+ media::cast::transport::CastTransportVideoConfig transport_config;
+ transport_config.base.ssrc = config.sender_ssrc;
+ transport_config.codec = config.codec;
+ transport_config.base.rtp_config = config.rtp_config;
+ cast_transport_->InitializeVideo(transport_config);
+ // TODO(mikhal): Pass in a valid GpuVideoAcceleratorFactories to support
+ // hardware video encoding.
+ cast_sender_->InitializeVideo(
+ config,
+ base::Bind(&CastSessionDelegate::InitializationResult,
+ weak_factory_.GetWeakPtr()),
+ NULL /* GPU*/);
+}
+
+void CastSessionDelegate::StartUDP(const net::IPEndPoint& local_endpoint,
+ const net::IPEndPoint& remote_endpoint) {
+ DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
// CastSender uses the renderer's IO thread as the main thread. This reduces
// thread hopping for incoming video frames and outgoing network packets.
@@ -80,39 +118,20 @@ void CastSessionDelegate::Initialize() {
NULL,
base::MessageLoopProxy::current(),
media::cast::GetLoggingConfigWithRawEventsAndStatsEnabled());
-}
-
-void CastSessionDelegate::StartAudio(
- const AudioSenderConfig& config,
- const FrameInputAvailableCallback& callback) {
- DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
-
- audio_config_.reset(new AudioSenderConfig(config));
- video_frame_input_available_callback_ = callback;
- StartSendingInternal();
-}
-
-void CastSessionDelegate::StartVideo(
- const VideoSenderConfig& config,
- const FrameInputAvailableCallback& callback) {
- DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
- audio_frame_input_available_callback_ = callback;
- video_config_.reset(new VideoSenderConfig(config));
- StartSendingInternal();
-}
+ // Rationale for using unretained: The callback cannot be called after the
+ // destruction of CastTransportSenderIPC, and they both share the same
thread.
+ cast_transport_.reset(new CastTransportSenderIPC(
+ local_endpoint,
+ remote_endpoint,
+ base::Bind(&CastSessionDelegate::StatusNotificationCB,
+ base::Unretained(this))));
-void CastSessionDelegate::StartUDP(const net::IPEndPoint& local_endpoint,
- const net::IPEndPoint& remote_endpoint) {
- DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
- transport_configured_ = true;
- local_endpoint_ = local_endpoint;
- remote_endpoint_ = remote_endpoint;
- StartSendingInternal();
+ cast_sender_ = CastSender::Create(cast_environment_, cast_transport_.get());
+ cast_transport_->SetPacketReceiver(cast_sender_->packet_receiver());
}
-void CastSessionDelegate::ToggleLogging(bool is_audio,
- bool enable) {
+void CastSessionDelegate::ToggleLogging(bool is_audio, bool enable) {
DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
if (enable) {
if (is_audio) {
@@ -148,11 +167,12 @@ void CastSessionDelegate::ToggleLogging(bool is_audio,
}
void CastSessionDelegate::GetEventLogsAndReset(
- bool is_audio, const EventLogsCallback& callback) {
+ bool is_audio,
+ const EventLogsCallback& callback) {
DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
- media::cast::EncodingEventSubscriber* subscriber = is_audio ?
- audio_event_subscriber_.get() : video_event_subscriber_.get();
+ media::cast::EncodingEventSubscriber* subscriber =
+ is_audio ? audio_event_subscriber_.get() : video_event_subscriber_.get();
if (!subscriber) {
callback.Run(make_scoped_ptr(new std::string).Pass());
return;
@@ -187,68 +207,20 @@ void CastSessionDelegate::StatusNotificationCB(
// TODO(hubbe): Call javascript UDPTransport error function.
}
-void CastSessionDelegate::StartSendingInternal() {
- DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
-
- // No transport, wait.
- if (!transport_configured_)
- return;
-
- // No audio or video, wait.
- if (!audio_config_ || !video_config_)
- return;
-
- Initialize();
-
- // Rationale for using unretained: The callback cannot be called after the
- // destruction of CastTransportSenderIPC, and they both share the same
thread.
- cast_transport_.reset(new CastTransportSenderIPC(
- local_endpoint_,
- remote_endpoint_,
- base::Bind(&CastSessionDelegate::StatusNotificationCB,
- base::Unretained(this))));
-
- // TODO(hubbe): set config.aes_key and config.aes_iv_mask.
- if (audio_config_) {
- media::cast::transport::CastTransportAudioConfig config;
- config.base.ssrc = audio_config_->sender_ssrc;
- config.codec = audio_config_->codec;
- config.base.rtp_config = audio_config_->rtp_config;
- config.frequency = audio_config_->frequency;
- config.channels = audio_config_->channels;
- cast_transport_->InitializeAudio(config);
- }
- if (video_config_) {
- media::cast::transport::CastTransportVideoConfig config;
- config.base.ssrc = video_config_->sender_ssrc;
- config.codec = video_config_->codec;
- config.base.rtp_config = video_config_->rtp_config;
- cast_transport_->InitializeVideo(config);
- }
-
- cast_sender_.reset(CastSender::CreateCastSender(
- cast_environment_,
- audio_config_.get(),
- video_config_.get(),
- NULL, // GPU.
- base::Bind(&CastSessionDelegate::InitializationResult,
- weak_factory_.GetWeakPtr()),
- cast_transport_.get()));
- cast_transport_->SetPacketReceiver(cast_sender_->packet_receiver());
-}
-
void CastSessionDelegate::InitializationResult(
media::cast::CastInitializationStatus result) const {
DCHECK(cast_sender_);
// TODO(pwestin): handle the error codes.
- if (result == media::cast::STATUS_INITIALIZED) {
+ if (result == media::cast::STATUS_AUDIO_INITIALIZED) {
if (!audio_frame_input_available_callback_.is_null()) {
- audio_frame_input_available_callback_.Run(cast_sender_->frame_input());
+ audio_frame_input_available_callback_.Run(
+ cast_sender_->audio_frame_input());
}
+ } else if (result == media::cast::STATUS_VIDEO_INITIALIZED) {
if (!video_frame_input_available_callback_.is_null()) {
- video_frame_input_available_callback_.Run(cast_sender_->frame_input());
+ video_frame_input_available_callback_.Run(
+ cast_sender_->video_frame_input());
}
}
}
-
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1310001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/renderer/media/cast_session_delegate.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/renderer/media/cast_session_delegate.h
Hunk #1 FAILED at 43.
Hunk #2 succeeded at 75 (offset 3 lines).
Hunk #3 succeeded at 86 (offset 3 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/renderer/media/cast_session_delegate.h.rej
Patch: chrome/renderer/media/cast_session_delegate.h
Index: chrome/renderer/media/cast_session_delegate.h
diff --git a/chrome/renderer/media/cast_session_delegate.h
b/chrome/renderer/media/cast_session_delegate.h
index
9c9767a391dd5cc70a9e3e3777f9135f4bbe12d4..e912db30350e23711659e06f66d5622324527898
100644
--- a/chrome/renderer/media/cast_session_delegate.h
+++ b/chrome/renderer/media/cast_session_delegate.h
@@ -43,24 +43,31 @@ class CastTransportSender;
// thread. All methods are accessible only on the IO thread.
class CastSessionDelegate {
public:
- typedef base::Callback<void(const scoped_refptr<media::cast::FrameInput>&)>
- FrameInputAvailableCallback;
+ typedef base::Callback<void(const scoped_refptr<
+ media::cast::AudioFrameInput>&)> AudioFrameInputAvailableCallback;
+ typedef base::Callback<void(const scoped_refptr<
+ media::cast::VideoFrameInput>&)> VideoFrameInputAvailableCallback;
typedef base::Callback<void(scoped_ptr<std::string>)> EventLogsCallback;
CastSessionDelegate();
virtual ~CastSessionDelegate();
+ // This will start the session by configuring and creating the Cast transport
+ // and the Cast sender.
+ // Must be called before initialization of audio or video.
+ void StartUDP(const net::IPEndPoint& local_endpoint,
+ const net::IPEndPoint& remote_endpoint);
+
// After calling StartAudio() or StartVideo() encoding of that media will
// begin as soon as data is delivered to its sink, if the second method is
// called the first media will be restarted. It is strongly recommended not
to
// deliver any data between calling the two methods.
// It's OK to call only one of the two methods.
+ // StartUDP must be called before these methods.
void StartAudio(const media::cast::AudioSenderConfig& config,
- const FrameInputAvailableCallback& callback);
+ const AudioFrameInputAvailableCallback& callback);
void StartVideo(const media::cast::VideoSenderConfig& config,
- const FrameInputAvailableCallback& callback);
- void StartUDP(const net::IPEndPoint& local_endpoint,
- const net::IPEndPoint& remote_endpoint);
+ const VideoFrameInputAvailableCallback& callback);
void ToggleLogging(bool is_audio, bool enable);
void GetEventLogsAndReset(bool is_audio, const EventLogsCallback& callback);
@@ -72,13 +79,6 @@ class CastSessionDelegate {
void InitializationResult(media::cast::CastInitializationStatus result)
const;
private:
- // Start encoding threads and initialize the CastEnvironment.
- void Initialize(const media::cast::CastLoggingConfig& logging_config);
-
- // Configure CastSender. It is ready to accept audio / video frames after
- // receiving a successful call to InitializationResult.
- void StartSendingInternal();
-
void StatusNotificationCB(
media::cast::transport::CastTransportStatus status);
@@ -90,16 +90,8 @@ class CastSessionDelegate {
scoped_ptr<media::cast::CastSender> cast_sender_;
scoped_ptr<media::cast::transport::CastTransportSender> cast_transport_;
- // Configuration for audio and video.
- scoped_ptr<media::cast::AudioSenderConfig> audio_config_;
- scoped_ptr<media::cast::VideoSenderConfig> video_config_;
-
- FrameInputAvailableCallback audio_frame_input_available_callback_;
- FrameInputAvailableCallback video_frame_input_available_callback_;
-
- net::IPEndPoint local_endpoint_;
- net::IPEndPoint remote_endpoint_;
- bool transport_configured_;
+ AudioFrameInputAvailableCallback audio_frame_input_available_callback_;
+ VideoFrameInputAvailableCallback video_frame_input_available_callback_;
scoped_ptr<media::cast::EncodingEventSubscriber> audio_event_subscriber_;
scoped_ptr<media::cast::EncodingEventSubscriber> video_event_subscriber_;
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1330001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1330001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1330001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1330001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1330001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for media/cast/test/receiver.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file media/cast/test/receiver.cc
Hunk #1 FAILED at 288.
1 out of 1 hunk FAILED -- saving rejects to file
media/cast/test/receiver.cc.rej
Patch: media/cast/test/receiver.cc
Index: media/cast/test/receiver.cc
diff --git a/media/cast/test/receiver.cc b/media/cast/test/receiver.cc
index
6c3097f5f53a67b2c32d69a32f9da85c926a6ab5..802291a7b590dda6f327eee7d6385cd092202855
100644
--- a/media/cast/test/receiver.cc
+++ b/media/cast/test/receiver.cc
@@ -288,9 +288,9 @@ int main(int argc, char** argv) {
local_end_point,
remote_end_point,
base::Bind(&media::cast::UpdateCastTransportStatus)));
- scoped_ptr<media::cast::CastReceiver> cast_receiver(
- media::cast::CastReceiver::CreateCastReceiver(
- cast_environment, audio_config, video_config, transport.get()));
+ scoped_ptr<media::cast::CastReceiver> cast_receiver =
+ media::cast::CastReceiver::Create(
+ cast_environment, audio_config, video_config, transport.get());
// TODO(hubbe): Make the cast receiver do this automatically.
transport->StartReceiving(cast_receiver->packet_receiver());
The CQ bit was checked by mikhal@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1350001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikhal@chromium.org/163553006/1350001
Message was sent while issue was closed.
Committed patchset #8 manually as r255954 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
