Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2692593002:
Media Remoting: End to end integration tests. (Closed)
|
Created:
3 years, 10 months ago by xjz Modified:
3 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org, apacible+watch_chromium.org, erickung+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Remoting: End to end integration tests.
Add end to end integration tests for Media Remoting. Refactors
PipelineIntegrationTest to test both media and media remoting pipeline.
Re-use current tests. No new tests are added in this CL.
BUG=684065
Review-Url: https://codereview.chromium.org/2692593002
Cr-Commit-Position: refs/heads/master@{#462739}
Committed: https://chromium.googlesource.com/chromium/src/+/60d4e0a63eb3f2be26aca1281be29c64b0f5c670
Patch Set 1 #
Total comments: 68
Patch Set 2 : Addressed comments. Rebased. #
Total comments: 25
Patch Set 3 : Addressed comments. #Patch Set 4 : File rename only. #
Messages
Total messages: 32 (21 generated)
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xjz@chromium.org changed reviewers: + miu@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Very nice work here! Sorry it took so long to review. It'll be great to get this in the tree soon. Comments on PS1: https://codereview.chromium.org/2692593002/diff/1/media/remoting/end2end_test... File media/remoting/end2end_test_renderer.cc (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/end2end_test... media/remoting/end2end_test_renderer.cc:63: const SendFrameToSinkCallback& send_frame_to_sink_cb_; typo: Need to remove ampersand here. https://codereview.chromium.org/2692593002/diff/1/media/remoting/end2end_test... media/remoting/end2end_test_renderer.cc:171: LOG(ERROR) << __func__ << ": Media Remoting doesn't support EME for now."; Instead: NOTIMPLEMENTED(); https://codereview.chromium.org/2692593002/diff/1/media/remoting/end2end_test... media/remoting/end2end_test_renderer.cc:206: if (frame.empty()) Why do we check if the frame is empty? Feels like this might hide problems... https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... File media/remoting/remote_receiver.cc (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:77: stream_provider_.reset(new RemoteStreamProvider(rpc_broker_)); if (stream_provider_) { return acquire renderer failed RPC; } stream_provider_.reset(new ...); https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:95: stream_provider_->Initialize( if (!stream_provider_) { return initialize failed RPC; } https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:103: renderer_->Initialize(stream_provider_.get(), this, DCHECK(stream_provider_); https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:120: playback_rate_ = message->double_value(); Per comment in header file, instead of this, how about: void RemoteReceiver::SetPlaybackRate(std::unique_ptr<pb::RpcMessage> message) { const double playback_rate = message->double_value(); DVLOG... renderer_->SetPlaybackRate(playback_rate); if (playback_rate == 0.0) if (time_update_timer_.IsRunning()) { time_update_timer_.Stop(); // Send one final media time update since the sender will not get // any until playback resumes. UpdateMediaTime(); } } else { ScheduleMediaTimeUpdates(); } } ...and the helper method (called from here and from StartPlayingFrom()): void RemoteReceiver::ScheduleMediaTimeUpdates() { if (time_update_timer_.IsRunning()) return; SendMediaTimeUpdate(); // Send one right away too! time_update_timer_.Start(...); } https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:132: if (flush_message.has_audio_count()) { if (stream_provider_) { if (flush_message.has_audio_count()) { stream_provider_->FlushUntil(...); } if (flush_message.has_video_count()) { stream_provider_->FlushUntil(...); } } https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:159: // Schedule period report timer. Per above suggestions, the rest of the method is just a call to ScheduleMediaTimeUpdates(). https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... File media/remoting/remote_receiver.h (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:26: // Simulate a media remoting receiver. Use a media::Renderer to render media Let's take out all code comments that talk about "simulate" and "testing." :) This class, here, is a complete implementation of IPC<-->media::Renderer, which is great! It can be used by anyone wanting a receiver (i.e., not for testing purposes). https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:28: class RemoteReceiver final : public RendererClient { naming: Just "Receiver" (since we're already in the remoting namespace). https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:63: void UpdateMediaTime(); naming: SendMediaTimeUpdate() https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:65: std::unique_ptr<Renderer> renderer_; const https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:66: RpcBroker* rpc_broker_; // Outlives this class. const https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:80: double playback_rate_ = 0; You can remove this. Code only checks whether this is non-zero, and that's always true when time_update_timer_.IsRunning(). (See notes in .cc file, though.) https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... File media/remoting/remote_stream_provider.cc (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.cc:76: DCHECK(callback_message.type() == type_); These four DCHECKs() should be if-statements instead, since they are operating on possibly-invalid data from the remote peer. DCHECKs() are meant to test internal, local assumptions about the state of code; and not assumptions about a remote actor's internal state. Generally, in cases where the sender or receiver discover an inconsistency, they should simply fatal-error and shutdown immediately. Something like: if (callback_message.type() != type_) { SendInitializeRpcFailed("wrong type"); return; } if (!audio_decoder_config_.empty() || !video_decoder_config_.empty() || init_done_callback.is_null()) { SendInitializeRpcFailed("already initialized"); return } https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.cc:92: NOTREACHED(); Replace with something like: SendInitializeRpcFailed("config missing in callback message"); https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.cc:100: DCHECK(read_until_sent_); DCHECK() --> if () ...and rather than me point all these out throughout the file, please go through and fix them all. You may want to employ a common fatal-error-handling routine (like what we did for DemuxerStreamAdapter). https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.cc:197: DCHECK(audio_decoder_config_.size() == 2); This is weird. Looks like we create a whole deque to just store 0, 1, or 2 elements. Feels like we should have a simpler scheme for updating config. Maybe just two class members, like: AudioDecoderConfig audio_decoder_config_; AudioDecoderConfig next_audio_decoder_config_; And, here, then we can just: audio_decoder_config_ = next_audio_decoder_config_; ...and same for video, of course. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... File media/remoting/remote_stream_provider.h (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:22: class RemoteMediaStream final : public DemuxerStream { Consider just forward-declaring this class here, and moving its declaration w/ inlined definition into the .cc file, since RemoteStreamProvider is the public interface for this module. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:22: class RemoteMediaStream final : public DemuxerStream { ditto: Same stuff here: Don't consider this only useful for testing or simulation. And for naming, drop the "Remote." ;) https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:60: RpcBroker* rpc_broker_; // Outlives this class. const https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:61: Type type_ = DemuxerStream::UNKNOWN; const (and no default value, since it is set by ctor) https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:62: int remote_handle_ = RpcBroker::kInvalidHandle; ditto: no default assignments here, since ctor sets these https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:94: class RemoteStreamProvider final : public MediaResource { naming: How about just StreamProvider (you're already in the remoting namespace)? https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:115: RpcBroker* rpc_broker_; const https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:66: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) Why is this dependent on !defined(MOJO_RENDERER)? Can we #include the file regardless? https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:668: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) && !defined(MOJO_RENDERER) Along the lines of my earlier comment, can we just do this everywhere instead: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:874: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) && !defined(MOJO_RENDERER) Instead of duplicating this code for each test fixture, could you do what you did in CommonPipelineIntegrationTest? In fact, maybe this should be moved into a base class, shared by both? https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:939: const PlaybackTestData kADTSRemotingTests[] = { Let's just have one definition with the remoting entries added if the build flag is set. Basically, we want to minimize the "surface area" of the extra test code needed for remoting. That way, it'll be far more likely future changes (by others) to the test code will be made such that they will automatically apply to both the local and remote pipelines without any extra effort. For example, here: const PlaybackTestData kADTSTests[] = { {PipelineType::Media, "bear-audio-main-aac.aac", 0, 2724}, {PipelineType::Media, "bear-audio-lc-aac.aac", 0, 2858}, {PipelineType::Media, "bear-audio-implicit-he-aac-v1.aac", 0, 2812}, {PipelineType::Media, "bear-audio-implicit-he-aac-v2.aac", 0, 3047}, #if BUILDFLAG(ENABLE_MEDIA_REMOTING) {PipelineType::MediaRemoting, "bear-audio-main-aac.aac", 0, 2724}, {PipelineType::MediaRemoting, "bear-audio-lc-aac.aac", 0, 2858}, {PipelineType::MediaRemoting, "bear-audio-implicit-he-aac-v1.aac", 0, 2812}, {PipelineType::MediaRemoting, "bear-audio-implicit-he-aac-v2.aac", 0, 3047}, #endif }; // TODO(chcunningham): Migrate other basic playback tests to TEST_P. INSTANTIATE_TEST_CASE_P(ProprietaryCodecs, BasicPlaybackTest, testing::ValuesIn(kADTSTests)); https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:968: const MSEPlaybackTestData kMediaSourceADTSRemotingTests[] = { ditto: Merge into one parameter array. https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:2198: const IntegrationTestData kIntegrationTests[] = {{PipelineType::Media}}; const IntegrationTestData kIntegrationTests[] = { {PipelineType::Media}, #if BUILDFLAG(ENABLE_MEDIA_REMOTING) {PipelineType::MediaRemoting}, #endif }; https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test_base.h:55: ScopedVector<VideoDecoder> prepend_video_decoders = ScopedVector is deprecated. Please use std::unique_ptr<VideoDecoder[]> instead, throughout this change. https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test_base.h:73: ~PipelineIntegrationTestBase(); Just to make sure the right dtor gets called, let's leave the virtual keyword in here.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks miu@ for reviewing. Addressed your comments. PTAL again. Thanks! https://codereview.chromium.org/2692593002/diff/1/media/remoting/end2end_test... File media/remoting/end2end_test_renderer.cc (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/end2end_test... media/remoting/end2end_test_renderer.cc:63: const SendFrameToSinkCallback& send_frame_to_sink_cb_; On 2017/03/29 01:39:13, miu wrote: > typo: Need to remove ampersand here. Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/end2end_test... media/remoting/end2end_test_renderer.cc:171: LOG(ERROR) << __func__ << ": Media Remoting doesn't support EME for now."; On 2017/03/29 01:39:13, miu wrote: > Instead: > > NOTIMPLEMENTED(); Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/end2end_test... media/remoting/end2end_test_renderer.cc:206: if (frame.empty()) On 2017/03/29 01:39:13, miu wrote: > Why do we check if the frame is empty? Feels like this might hide problems... Done. Removed the check. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... File media/remoting/remote_receiver.cc (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:77: stream_provider_.reset(new RemoteStreamProvider(rpc_broker_)); On 2017/03/29 01:39:13, miu wrote: > if (stream_provider_) { > return acquire renderer failed RPC; > } > stream_provider_.reset(new ...); We don't have a RPC for acquire failure. Called OnError() instead, which will send the error RPC message to sender and ends the remoting session. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:95: stream_provider_->Initialize( On 2017/03/29 01:39:14, miu wrote: > if (!stream_provider_) { > return initialize failed RPC; > } Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:103: renderer_->Initialize(stream_provider_.get(), this, On 2017/03/29 01:39:13, miu wrote: > DCHECK(stream_provider_); Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:120: playback_rate_ = message->double_value(); On 2017/03/29 01:39:14, miu wrote: > Per comment in header file, instead of this, how about: > > void RemoteReceiver::SetPlaybackRate(std::unique_ptr<pb::RpcMessage> message) > { > const double playback_rate = message->double_value(); > DVLOG... > renderer_->SetPlaybackRate(playback_rate); > if (playback_rate == 0.0) > if (time_update_timer_.IsRunning()) { > time_update_timer_.Stop(); > // Send one final media time update since the sender will not get > // any until playback resumes. > UpdateMediaTime(); > } > } else { > ScheduleMediaTimeUpdates(); > } > } > > ...and the helper method (called from here and from StartPlayingFrom()): > > void RemoteReceiver::ScheduleMediaTimeUpdates() { > if (time_update_timer_.IsRunning()) > return; > SendMediaTimeUpdate(); // Send one right away too! > time_update_timer_.Start(...); > } Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:132: if (flush_message.has_audio_count()) { On 2017/03/29 01:39:13, miu wrote: > if (stream_provider_) { > if (flush_message.has_audio_count()) { > stream_provider_->FlushUntil(...); > } > if (flush_message.has_video_count()) { > stream_provider_->FlushUntil(...); > } > } Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.cc:159: // Schedule period report timer. On 2017/03/29 01:39:14, miu wrote: > Per above suggestions, the rest of the method is just a call to > ScheduleMediaTimeUpdates(). Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... File media/remoting/remote_receiver.h (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:26: // Simulate a media remoting receiver. Use a media::Renderer to render media On 2017/03/29 01:39:14, miu wrote: > Let's take out all code comments that talk about "simulate" and "testing." :) > This class, here, is a complete implementation of IPC<-->media::Renderer, which > is great! It can be used by anyone wanting a receiver (i.e., not for testing > purposes). Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:28: class RemoteReceiver final : public RendererClient { On 2017/03/29 01:39:14, miu wrote: > naming: Just "Receiver" (since we're already in the remoting namespace). Done. Will rename the file name to "receiver.*" later when you finish reviewing. :) https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:63: void UpdateMediaTime(); On 2017/03/29 01:39:14, miu wrote: > naming: SendMediaTimeUpdate() Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:65: std::unique_ptr<Renderer> renderer_; On 2017/03/29 01:39:14, miu wrote: > const Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:66: RpcBroker* rpc_broker_; // Outlives this class. On 2017/03/29 01:39:14, miu wrote: > const Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_recei... media/remoting/remote_receiver.h:80: double playback_rate_ = 0; On 2017/03/29 01:39:14, miu wrote: > You can remove this. Code only checks whether this is non-zero, and that's > always true when time_update_timer_.IsRunning(). (See notes in .cc file, > though.) Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... File media/remoting/remote_stream_provider.cc (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.cc:76: DCHECK(callback_message.type() == type_); On 2017/03/29 01:39:14, miu wrote: > These four DCHECKs() should be if-statements instead, since they are operating > on possibly-invalid data from the remote peer. DCHECKs() are meant to test > internal, local assumptions about the state of code; and not assumptions about a > remote actor's internal state. Generally, in cases where the sender or receiver > discover an inconsistency, they should simply fatal-error and shutdown > immediately. > > Something like: > > if (callback_message.type() != type_) { > SendInitializeRpcFailed("wrong type"); > return; > } > if (!audio_decoder_config_.empty() || > !video_decoder_config_.empty() || > init_done_callback.is_null()) { > SendInitializeRpcFailed("already initialized"); > return > } Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.cc:92: NOTREACHED(); On 2017/03/29 01:39:14, miu wrote: > Replace with something like: > > SendInitializeRpcFailed("config missing in callback message"); Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.cc:100: DCHECK(read_until_sent_); On 2017/03/29 01:39:14, miu wrote: > DCHECK() --> if () > > ...and rather than me point all these out throughout the file, please go through > and fix them all. You may want to employ a common fatal-error-handling routine > (like what we did for DemuxerStreamAdapter). Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.cc:197: DCHECK(audio_decoder_config_.size() == 2); On 2017/03/29 01:39:14, miu wrote: > This is weird. Looks like we create a whole deque to just store 0, 1, or 2 > elements. Feels like we should have a simpler scheme for updating config. Maybe > just two class members, like: > > AudioDecoderConfig audio_decoder_config_; > AudioDecoderConfig next_audio_decoder_config_; > > And, here, then we can just: > > audio_decoder_config_ = next_audio_decoder_config_; > > ...and same for video, of course. Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... File media/remoting/remote_stream_provider.h (right): https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:22: class RemoteMediaStream final : public DemuxerStream { On 2017/03/29 01:39:14, miu wrote: > Consider just forward-declaring this class here, and moving its declaration w/ > inlined definition into the .cc file, since RemoteStreamProvider is the public > interface for this module. Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:22: class RemoteMediaStream final : public DemuxerStream { On 2017/03/29 01:39:14, miu wrote: > ditto: Same stuff here: Don't consider this only useful for testing or > simulation. And for naming, drop the "Remote." ;) Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:60: RpcBroker* rpc_broker_; // Outlives this class. On 2017/03/29 01:39:14, miu wrote: > const Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:61: Type type_ = DemuxerStream::UNKNOWN; On 2017/03/29 01:39:14, miu wrote: > const (and no default value, since it is set by ctor) Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:62: int remote_handle_ = RpcBroker::kInvalidHandle; On 2017/03/29 01:39:14, miu wrote: > ditto: no default assignments here, since ctor sets these Done. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:94: class RemoteStreamProvider final : public MediaResource { On 2017/03/29 01:39:14, miu wrote: > naming: How about just StreamProvider (you're already in the remoting > namespace)? Done. Will rename the files to "stream_provider.*" later. https://codereview.chromium.org/2692593002/diff/1/media/remoting/remote_strea... media/remoting/remote_stream_provider.h:115: RpcBroker* rpc_broker_; On 2017/03/29 01:39:14, miu wrote: > const Done. https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:66: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) On 2017/03/29 01:39:15, miu wrote: > Why is this dependent on !defined(MOJO_RENDERER)? Can we #include the file > regardless? Done. It should be ok to remove !defined(MOJO_RENDERER). I added this condition since I thought we don't test remoting when MOJO_RENDERER is enabled, and all mojo tests are actually disabled. https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:668: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) && !defined(MOJO_RENDERER) On 2017/03/29 01:39:15, miu wrote: > Along the lines of my earlier comment, can we just do this everywhere instead: > > #if BUILDFLAG(ENABLE_MEDIA_REMOTING) Done. Now removed this class to base. https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:874: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) && !defined(MOJO_RENDERER) On 2017/03/29 01:39:14, miu wrote: > Instead of duplicating this code for each test fixture, could you do what you > did in CommonPipelineIntegrationTest? > > In fact, maybe this should be moved into a base class, shared by both? Done. https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:939: const PlaybackTestData kADTSRemotingTests[] = { On 2017/03/29 01:39:15, miu wrote: > Let's just have one definition with the remoting entries added if the build flag > is set. Basically, we want to minimize the "surface area" of the extra test code > needed for remoting. That way, it'll be far more likely future changes (by > others) to the test code will be made such that they will automatically apply to > both the local and remote pipelines without any extra effort. For example, here: > > const PlaybackTestData kADTSTests[] = { > {PipelineType::Media, "bear-audio-main-aac.aac", 0, 2724}, > {PipelineType::Media, "bear-audio-lc-aac.aac", 0, 2858}, > {PipelineType::Media, "bear-audio-implicit-he-aac-v1.aac", 0, 2812}, > {PipelineType::Media, "bear-audio-implicit-he-aac-v2.aac", 0, 3047}, > #if BUILDFLAG(ENABLE_MEDIA_REMOTING) > {PipelineType::MediaRemoting, "bear-audio-main-aac.aac", 0, 2724}, > {PipelineType::MediaRemoting, "bear-audio-lc-aac.aac", 0, 2858}, > {PipelineType::MediaRemoting, "bear-audio-implicit-he-aac-v1.aac", 0, 2812}, > {PipelineType::MediaRemoting, "bear-audio-implicit-he-aac-v2.aac", 0, 3047}, > #endif > }; > > // TODO(chcunningham): Migrate other basic playback tests to TEST_P. > INSTANTIATE_TEST_CASE_P(ProprietaryCodecs, > BasicPlaybackTest, > testing::ValuesIn(kADTSTests)); Done. https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:968: const MSEPlaybackTestData kMediaSourceADTSRemotingTests[] = { On 2017/03/29 01:39:15, miu wrote: > ditto: Merge into one parameter array. Done. https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:2198: const IntegrationTestData kIntegrationTests[] = {{PipelineType::Media}}; On 2017/03/29 01:39:14, miu wrote: > const IntegrationTestData kIntegrationTests[] = { > {PipelineType::Media}, > #if BUILDFLAG(ENABLE_MEDIA_REMOTING) > {PipelineType::MediaRemoting}, > #endif > }; Done. https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test_base.h:55: ScopedVector<VideoDecoder> prepend_video_decoders = On 2017/03/29 01:39:15, miu wrote: > ScopedVector is deprecated. Please use std::unique_ptr<VideoDecoder[]> instead, > throughout this change. This involves the refactor of VideoRendererImpl and AudioRendererImpl and many related changes. That's too much for this CL. I prefer leave the refactor to a separate CL. WDYT? https://codereview.chromium.org/2692593002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test_base.h:73: ~PipelineIntegrationTestBase(); On 2017/03/29 01:39:15, miu wrote: > Just to make sure the right dtor gets called, let's leave the virtual keyword in > here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments on PS2: https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_r... File media/remoting/remote_receiver.cc (right): https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_r... media/remoting/remote_receiver.cc:207: base::TimeDelta max_time = media_time; Isn't max_time supposed to be the total time for the track (e.g., length of the whole video)? Is it easy to get this information some other way? https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_r... media/remoting/remote_receiver.cc:208: // Allow some slop to account for delays in scheduling time update tasks. Now that I look at this again, I'm confused. Why is the slop needed? What happens if you don't add the slop? https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... File media/remoting/remote_stream_provider.cc (right): https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... media/remoting/remote_stream_provider.cc:300: if (!read_complete_callback_.is_null()) { Is this possible? There was just a DCHECK(read_complete_callback_.is_null()) before, in the last PS. It seems like this "ignore" logic could cover-up a potential problem. Note: This isn't like the other DCHECK() issue, since this method isn't being called to handle an RPC. This method is called from the local Renderer impl, which has a well-defined API contract (I hope). https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... media/remoting/remote_stream_provider.cc:327: next_audio_decoder_config_ = AudioDecoderConfig(); nit: Setting the next_audio_decoder_config_ to an empty object shouldn't be needed (it's just an extra runtime cost). Though, you could conditionally compile it, as a safety check, when DCHECKs are turned on: #ifdef DCHECK_IS_ON() next_config_ = xxxDecoderConfig(); #endif (...same for the video one below, too) https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... media/remoting/remote_stream_provider.cc:440: if (init_done_callback_.is_null()) { ditto: DCHECK() only here, since this is not handling an RPC (unless the media pipeline may call this more than once?). https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... media/remoting/remote_stream_provider.cc:450: if (init_done_callback_.is_null()) { ditto: DCHECK() only here... https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:835: MaybeCreateRemotingRendererFactory(data.type); nit: GetParam().type would be simpler... https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:835: MaybeCreateRemotingRendererFactory(data.type); In CommonPipelineIntegrationTest, you put this call in the ctor instead of inside each test fixture procedure. Can you do that for BasicPlaybackTest too? https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:872: const PlaybackTestData kADTSTests[] = { nit: Please add a comment to say something like: Any new/changed entries should be made for both the ::Media and ::MediaRemoting types. If you don't think something applies, please contact one of the media/remoting/OWNERS. https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:890: const MSEPlaybackTestData kMediaSourceADTSTests[] = { nit: ditto on adding a "change both ::Media and ::MediaRemoting" comment. https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:934: MaybeCreateRemotingRendererFactory(data.type); nit: GetParam().type would be simpler... https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test_base.h:191: void MaybeCreateRemotingRendererFactory(PipelineType type); Suggestion for making it a bit clearer what's going on here (for those unfamiliar with this): How about this function becomes: // Proxy all control and data flows through a media remoting RPC // pipeline, to test that an end-to-end media remoting pipeline works // the same as a normal, local pipeline. void SetUpRemotingPipeline(); ...and then callers in the test code would: void SetUp() override { parentclass::SetUp(); if (GetParam().type == PipelineType::MediaRemoting) SetUpRemotingPipeline(); }
Addressed miu's comments. PTAL again. Thanks! https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_r... File media/remoting/remote_receiver.cc (right): https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_r... media/remoting/remote_receiver.cc:207: base::TimeDelta max_time = media_time; On 2017/04/03 21:05:01, miu wrote: > Isn't max_time supposed to be the total time for the track (e.g., length of the > whole video)? Is it easy to get this information some other way? Actually, this is now only used to check that it shouldn't be less than |media_time|. Here I did similar to what mojo renderer does: https://cs.chromium.org/chromium/src/media/mojo/services/mojo_renderer_servic... The |max_time| is used there for time interpolation, and it doesn't mean the total time of the track. But we don't do time interpolation for media remoting for now, so this is not used. Changed to just set it equal to |media_time|. https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_r... media/remoting/remote_receiver.cc:208: // Allow some slop to account for delays in scheduling time update tasks. On 2017/04/03 21:05:01, miu wrote: > Now that I look at this again, I'm confused. Why is the slop needed? What > happens if you don't add the slop? Removed the slop. As explained above, it is not needed here. https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... File media/remoting/remote_stream_provider.cc (right): https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... media/remoting/remote_stream_provider.cc:300: if (!read_complete_callback_.is_null()) { On 2017/04/03 21:05:01, miu wrote: > Is this possible? There was just a DCHECK(read_complete_callback_.is_null()) > before, in the last PS. It seems like this "ignore" logic could cover-up a > potential problem. > > Note: This isn't like the other DCHECK() issue, since this method isn't being > called to handle an RPC. This method is called from the local Renderer impl, > which has a well-defined API contract (I hope). Done. Changed it back to DCHECK. https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... media/remoting/remote_stream_provider.cc:327: next_audio_decoder_config_ = AudioDecoderConfig(); On 2017/04/03 21:05:01, miu wrote: > nit: Setting the next_audio_decoder_config_ to an empty object shouldn't be > needed (it's just an extra runtime cost). Though, you could conditionally > compile it, as a safety check, when DCHECKs are turned on: > > #ifdef DCHECK_IS_ON() > next_config_ = xxxDecoderConfig(); > #endif > > (...same for the video one below, too) Done. https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... media/remoting/remote_stream_provider.cc:440: if (init_done_callback_.is_null()) { On 2017/04/03 21:05:01, miu wrote: > ditto: DCHECK() only here, since this is not handling an RPC (unless the media > pipeline may call this more than once?). Done. https://codereview.chromium.org/2692593002/diff/80001/media/remoting/remote_s... media/remoting/remote_stream_provider.cc:450: if (init_done_callback_.is_null()) { On 2017/04/03 21:05:01, miu wrote: > ditto: DCHECK() only here... Done. https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:835: MaybeCreateRemotingRendererFactory(data.type); On 2017/04/03 21:05:01, miu wrote: > In CommonPipelineIntegrationTest, you put this call in the ctor instead of > inside each test fixture procedure. Can you do that for BasicPlaybackTest too? Done. I didn't do this before because there is currently only one text fixture procedure for BasicPlaybackTest/BasicMSEPlaybackTest. :) https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:835: MaybeCreateRemotingRendererFactory(data.type); On 2017/04/03 21:05:01, miu wrote: > nit: GetParam().type would be simpler... Done. https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:872: const PlaybackTestData kADTSTests[] = { On 2017/04/03 21:05:01, miu wrote: > nit: Please add a comment to say something like: > > Any new/changed entries should be made for both the ::Media and ::MediaRemoting > types. If you don't think something applies, please contact one of the > media/remoting/OWNERS. Done. https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:890: const MSEPlaybackTestData kMediaSourceADTSTests[] = { On 2017/04/03 21:05:01, miu wrote: > nit: ditto on adding a "change both ::Media and ::MediaRemoting" comment. Done. https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:934: MaybeCreateRemotingRendererFactory(data.type); On 2017/04/03 21:05:01, miu wrote: > nit: GetParam().type would be simpler... Done. https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test_base.h:191: void MaybeCreateRemotingRendererFactory(PipelineType type); On 2017/04/03 21:05:01, miu wrote: > Suggestion for making it a bit clearer what's going on here (for those > unfamiliar with this): How about this function becomes: > > // Proxy all control and data flows through a media remoting RPC > // pipeline, to test that an end-to-end media remoting pipeline works > // the same as a normal, local pipeline. > void SetUpRemotingPipeline(); > > ...and then callers in the test code would: > > void SetUp() override { > parentclass::SetUp(); > if (GetParam().type == PipelineType::MediaRemoting) > SetUpRemotingPipeline(); > } Done. Called it in tests' ctors.
PS3 lgtm...How about a `git cl try` to see these running on the try bots? ;) https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2692593002/diff/80001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:835: MaybeCreateRemotingRendererFactory(data.type); On 2017/04/04 01:07:56, xjz wrote: > On 2017/04/03 21:05:01, miu wrote: > > In CommonPipelineIntegrationTest, you put this call in the ctor instead of > > inside each test fixture procedure. Can you do that for BasicPlaybackTest too? > > Done. I didn't do this before because there is currently only one text fixture > procedure for BasicPlaybackTest/BasicMSEPlaybackTest. :) OIC...I thought there were two for the same test class (names are almost the same). :) Looks good in the latest patchset (more consistent).
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
xjz@chromium.org changed reviewers: + sandersd@chromium.org
sandersd: Need RS on media/test/*. Re-used current tests to test media remoting pipeline. PTAL. Thanks.
lgtm
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/2692593002/#ps120001 (title: "File rename only.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491518644806960,
"parent_rev": "6280d0d289acfba30fbac97d549dc3e029b6c198", "commit_rev":
"60d4e0a63eb3f2be26aca1281be29c64b0f5c670"}
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: End to end integration tests. Add end to end integration tests for Media Remoting. Refactors PipelineIntegrationTest to test both media and media remoting pipeline. Re-use current tests. No new tests are added in this CL. BUG=684065 ========== to ========== Media Remoting: End to end integration tests. Add end to end integration tests for Media Remoting. Refactors PipelineIntegrationTest to test both media and media remoting pipeline. Re-use current tests. No new tests are added in this CL. BUG=684065 Review-Url: https://codereview.chromium.org/2692593002 Cr-Commit-Position: refs/heads/master@{#462739} Committed: https://chromium.googlesource.com/chromium/src/+/60d4e0a63eb3f2be26aca1281be2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/60d4e0a63eb3f2be26aca1281be2...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:120001) has been created in https://codereview.chromium.org/2804183002/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit identified CL at revision 462739 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
