Chromecast: Use std::vector to pass video config in CMA

chromecast/media/cma/pipeline/video_pipeline_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromecast/media/cma/pipeline/video_pipeline_impl.h"
 
 #include "base/bind.h"
 #include "chromecast/media/cma/backend/video_pipeline_device.h"
 #include "chromecast/media/cma/base/buffering_defs.h"
 #include "chromecast/media/cma/base/cma_logging.h"
@@ skipping 83 matching lines @@
 void VideoPipelineImpl::SetCdm(BrowserCdmCast* media_keys) {
   av_pipeline_impl_->SetCdm(media_keys);
 }
 
 void VideoPipelineImpl::SetClient(const VideoPipelineClient& client) {
   video_client_ = client;
   av_pipeline_impl_->SetClient(client.av_pipeline_client);
 }
 
 void VideoPipelineImpl::Initialize(
-    const ::media::VideoDecoderConfig& video_config,
+    const std::vector<::media::VideoDecoderConfig>& configs,
     scoped_ptr<CodedFrameProvider> frame_provider,
     const ::media::PipelineStatusCB& status_cb) {
-  CMALOG(kLogControl) << "VideoPipelineImpl::Initialize "
-                      << video_config.AsHumanReadableString();
+  CMALOG(kLogControl) << __FUNCTION__ << " config (" << configs.size() << ")";
   VideoPipelineDevice::VideoClient client;
   client.natural_size_changed_cb =
       base::Bind(&VideoPipelineImpl::OnNaturalSizeChanged, weak_this_);
   video_device_->SetVideoClient(client);
   if (frame_provider)
     SetCodedFrameProvider(frame_provider.Pass());
 
-  if (!video_device_->SetConfig(
-          DecoderConfigAdapter::ToCastVideoConfig(kPrimary, video_config)) ||
+  // TODO(erickung): Consider better interface to pass multiple video configs

[gunsch, 2015/06/05 00:17:13]

Is there _ever_ a case when we will have more than 2? It seems to me like the
two-stream case is a pretty big exception as it is with a potentially fixed end
date and that this isn't intended to become N configs.

Or is this TODO about it being awkward to pass a config along as a pointer in
VideoConfig? Because if so, I agree, but maybe put the TODO in the VideoConfig
definition?

[erickung1, 2015/06/05 01:04:18]

This TODO is for later. I'll remove it from this CL and will add it to the
header file later on.

+  // to CMA without chaining all of them.
+  DCHECK(configs.size() <= 2);
+  VideoConfig video_config =
+      DecoderConfigAdapter::ToCastVideoConfig(kPrimary, configs[0]);
+  VideoConfig secondary_config;
+  if (configs.size() == 2) {
+    secondary_config = DecoderConfigAdapter::ToCastVideoConfig(kSecondary,

[gunsch, 2015/06/05 00:17:13]

(Thanks for removing the "for" loop, this is a lot more straightforward to
read).

Are we expecting valid configs as inputs? Should this DCHECK that
secondary_config is valid, or possibly fail initialization if they're not?

Or is that the job of VideoPipelineDevice::SetConfig to do? (return false if the
configs are invalid?)

It seems like we should be validating those in our common code before passing to
vendor implementation.

[erickung1, 2015/06/05 01:04:18]

From the original code, VideoPipelineImpl::Initialize() should provide valid
VideoDecoderConfig information. Therefore I don't think we need to add
additional check here.

After converting it to VideoConfig, we don't need to check because
- no IsValidConfig() util function because we don't want to provide such util
function in public header file
- event though we think it's valid config, the platform still can reject the
request and return false.

It's a sync call and straightforward, I think we can leave the decision to OEM
vendor.

[gunsch, 2015/06/05 18:08:16]

If that's the assumption, then how about DCHECK(config.IsValidConfig()) for both
configs then? Then it's clear when reading the code that the configs are always
valid at this point.

[erickung1, 2015/06/05 20:44:20]

Done. I add the DCHECK on audio_pipeline_impl as well

+                                                               configs[1]);
+    video_config.additional_config = &secondary_config;
+  }
+
+  if (!video_device_->SetConfig(video_config) ||
       !av_pipeline_impl_->Initialize()) {
     status_cb.Run(::media::PIPELINE_ERROR_INITIALIZATION_FAILED);
     return;
   }
   av_pipeline_impl_->TransitionToState(AvPipelineImpl::kFlushed);
   status_cb.Run(::media::PIPELINE_OK);
 }
 
 void VideoPipelineImpl::OnUpdateConfig(
     StreamId id,
     const ::media::AudioDecoderConfig& audio_config,
     const ::media::VideoDecoderConfig& video_config) {
+  DCHECK(!audio_config.IsValidConfig());
   if (video_config.IsValidConfig()) {
     CMALOG(kLogControl) << "VideoPipelineImpl::OnUpdateConfig id:" << id << " "
                         << video_config.AsHumanReadableString();
 
     bool success = video_device_->SetConfig(
         DecoderConfigAdapter::ToCastVideoConfig(id, video_config));
     if (!success &&
         !video_client_.av_pipeline_client.playback_error_cb.is_null()) {
       video_client_.av_pipeline_client.playback_error_cb.Run(
           ::media::PIPELINE_ERROR_DECODE);
@@ skipping 30 matching lines @@
   delta_stats.video_frames_dropped =
       current_stats.video_frames_dropped - previous_stats_.video_frames_dropped;
 
   previous_stats_ = current_stats;
 
   video_client_.av_pipeline_client.statistics_cb.Run(delta_stats);
 }
 
 }  // namespace media
 }  // namespace chromecast