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

Chromecast: use multiple video configs in CMA

- The CL also remove space in front of ::media namespace.

BUG= internal b/18865809

Committed: https://crrev.com/f4e0b8865f3f0969f50587c31508c99557127f28
Cr-Commit-Position: refs/heads/master@{#333225}

[erickung1, 2015-06-04 10:05:02]

erickung@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@chromium.org, servolk@chromium.org

[gunsch, 2015-06-04 16:55:22]

pre-review comments:

1) do you have a BUG= for this?

2) "Use std::vector" in the description doesn't provide a lot of context, maybe
something like "Use multiple video configs in CMA" ?

[gunsch, 2015-06-04 17:03:43]

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/fi...
File chromecast/media/cma/filters/cma_renderer.cc (right):

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/fi...
chromecast/media/cma/filters/cma_renderer.cc:324:
std::vector<::media::VideoDecoderConfig> configs;
Alright, this looks like the start. Is there a TODO here, or an explanation of
how we might get >1 configs?

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.cc:130:
DCHECK(!video_config.IsValidConfig());
Hm, is this actually safe? Do we only ever pass in an empty video config?

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:115: VideoConfig
*video_configs = new VideoConfig[configs.size()];
scoped_ptr has an array templatization that handles array delete. Prefer that
over raw pointers for this.

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:121:
video_configs[i-1].additional_config = &video_configs[i];
I don't understand this. So you allow for N video configs, but this reads to me
like you're assuming it's actually exactly 2, and that you're going to pass one
VideoConfig object to the backend. Can you clarify?

If we're expecting no more than two everywhere, we might pass around
primary/secondary everywhere, or regularly DCHECK(configs.size() <= 2).

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:124: bool result =
video_device_->SetConfig(video_configs[0]);
Yeah, see, you do a complicated loop above, but then only pass down the first
VideoConfig object. Just handle two, e.g.

VideoConfig video_config = ToCastVideoConfig(0, configs[0])
VideoConfig secondary_config;
if (configs.size() == 2)
  secondary_config = ToCastVideoConfig(1, configs[1]);
  video_config.additional_config = &secondary_config;
}
video_device_->SetConfig(video_config);

[erickung1, 2015-06-04 20:29:20]

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/fi...
File chromecast/media/cma/filters/cma_renderer.cc (right):

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/fi...
chromecast/media/cma/filters/cma_renderer.cc:324:
std::vector<::media::VideoDecoderConfig> configs;
On 2015/06/04 17:03:42, gunsch wrote:
> Alright, this looks like the start. Is there a TODO here, or an explanation of
> how we might get >1 configs?

There will be another file, please see internal CL. Thanks.

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.cc:130:
DCHECK(!video_config.IsValidConfig());
On 2015/06/04 17:03:42, gunsch wrote:
> Hm, is this actually safe? Do we only ever pass in an empty video config?

Yes. the config only comes with buffer if there is a change.
it doesn't get video config if receiving audio buffer
The reason we have both audio/video config here is because we try to reuse same
code for both video/audio.

We should add this for check.

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:115: VideoConfig
*video_configs = new VideoConfig[configs.size()];
On 2015/06/04 17:03:43, gunsch wrote:
> scoped_ptr has an array templatization that handles array delete. Prefer that
> over raw pointers for this.

I removed the code here using loop and assume at most there will be two for now

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:121:
video_configs[i-1].additional_config = &video_configs[i];
On 2015/06/04 17:03:43, gunsch wrote:
> I don't understand this. So you allow for N video configs, but this reads to
me
> like you're assuming it's actually exactly 2, and that you're going to pass
one
> VideoConfig object to the backend. Can you clarify?
> 
> If we're expecting no more than two everywhere, we might pass around
> primary/secondary everywhere, or regularly DCHECK(configs.size() <= 2).
The original thought is to use programming solution to chain the data, and don't
limit the code to support only 2. But I think I have plan to refactor the
interface better without chaining the data later on. put a TODO here

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:124: bool result =
video_device_->SetConfig(video_configs[0]);
On 2015/06/04 17:03:43, gunsch wrote:
> Yeah, see, you do a complicated loop above, but then only pass down the first
> VideoConfig object. Just handle two, e.g.
> 
> VideoConfig video_config = ToCastVideoConfig(0, configs[0])
> VideoConfig secondary_config;
> if (configs.size() == 2)
>   secondary_config = ToCastVideoConfig(1, configs[1]);
>   video_config.additional_config = &secondary_config;
> }
> video_device_->SetConfig(video_config);

Done.

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

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.cc:130:
DCHECK(!video_config.IsValidConfig());
On 2015/06/04 20:29:20, erickung1 wrote:
> On 2015/06/04 17:03:42, gunsch wrote:
> > Hm, is this actually safe? Do we only ever pass in an empty video config?
> 
> Yes. the config only comes with buffer if there is a change.
> it doesn't get video config if receiving audio buffer
> The reason we have both audio/video config here is because we try to reuse
same
> code for both video/audio.
> 
> We should add this for check.

What do you mean by "we should add this for check"?

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:115: // TODO(erickung):
Consider better interface to pass multiple video configs
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?

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:122: secondary_config =
DecoderConfigAdapter::ToCastVideoConfig(kSecondary,
(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:19]

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.cc:130:
DCHECK(!video_config.IsValidConfig());
On 2015/06/05 00:17:12, gunsch wrote:
> On 2015/06/04 20:29:20, erickung1 wrote:
> > On 2015/06/04 17:03:42, gunsch wrote:
> > > Hm, is this actually safe? Do we only ever pass in an empty video config?
> > 
> > Yes. the config only comes with buffer if there is a change.
> > it doesn't get video config if receiving audio buffer
> > The reason we have both audio/video config here is because we try to reuse
> same
> > code for both video/audio.
> > 
> > We should add this for check.
> 
> What do you mean by "we should add this for check"?

We should put DCHECK() here, because it's a bug and should never happen

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:115: // TODO(erickung):
Consider better interface to pass multiple video configs
On 2015/06/05 00:17:13, gunsch wrote:
> 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?
This TODO is for later. I'll remove it from this CL and will add it to the
header file later on.

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:122: secondary_config =
DecoderConfigAdapter::ToCastVideoConfig(kSecondary,
On 2015/06/05 00:17:13, gunsch wrote:
> (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.

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]

lgtm % adding DCHECKs

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:122: secondary_config =
DecoderConfigAdapter::ToCastVideoConfig(kSecondary,
On 2015/06/05 01:04:18, erickung1 wrote:
> On 2015/06/05 00:17:13, gunsch wrote:
> > (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.
> 
> 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.

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]

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:122: secondary_config =
DecoderConfigAdapter::ToCastVideoConfig(kSecondary,
On 2015/06/05 18:08:16, gunsch wrote:
> On 2015/06/05 01:04:18, erickung1 wrote:
> > On 2015/06/05 00:17:13, gunsch wrote:
> > > (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.
> > 
> > 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.
> 
> 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.

Done. I add the DCHECK on audio_pipeline_impl as well

[erickung1, 2015-06-05 20:44:28]

The CQ bit was checked by erickung@chromium.org

[erickung1, 2015-06-05 20:44:28]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1148253006/#ps80001
(title: "Add more DCHECK")

[commit-bot: I haz the power, 2015-06-05 20:46:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148253006/80001

[commit-bot: I haz the power, 2015-06-05 20:59:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 20:59:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erickung1, 2015-06-05 22:03:50]

erickung@chromium.org changed reviewers:
+ dcheng@chromium.org, tsepez@chromium.org

[erickung1, 2015-06-05 22:05:00]

Hi Daniel and Tom,

Can you help to review chromecast/common/media/cma_messages.h ?

Thanks.

[Tom Sepez, 2015-06-05 22:07:00]

Messages LGTM

[erickung1, 2015-06-05 22:07:37]

The CQ bit was checked by erickung@chromium.org

[commit-bot: I haz the power, 2015-06-05 22:09:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148253006/80001

[commit-bot: I haz the power, 2015-06-05 22:19:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 22:19:14]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erickung1, 2015-06-05 22:24:34]

The CQ bit was checked by erickung@chromium.org

[commit-bot: I haz the power, 2015-06-05 22:25:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148253006/80001

[commit-bot: I haz the power, 2015-06-05 22:36:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 22:36:56]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erickung1, 2015-06-05 23:34:36]

The CQ bit was checked by erickung@chromium.org

[erickung1, 2015-06-05 23:34:36]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1148253006/#ps100001
(title: "Rebase to TOT")

[commit-bot: I haz the power, 2015-06-05 23:36:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148253006/100001

[dcheng, 2015-06-05 23:41:18]

not lgtm

https://codereview.chromium.org/1148253006/diff/80001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/80001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:118:
DecoderConfigAdapter::ToCastVideoConfig(kPrimary, configs[0]);
What process is this running in? It looks like this is in the browser? If so,
the DCHECK is not sufficient.

[dcheng, 2015-06-05 23:41:29]

The CQ bit was unchecked by dcheng@chromium.org

[erickung1, 2015-06-05 23:46:28]

On 2015/06/05 23:41:18, dcheng wrote:
> not lgtm
> 
>
https://codereview.chromium.org/1148253006/diff/80001/chromecast/media/cma/pi...
> File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):
> 
>
https://codereview.chromium.org/1148253006/diff/80001/chromecast/media/cma/pi...
> chromecast/media/cma/pipeline/video_pipeline_impl.cc:118:
> DecoderConfigAdapter::ToCastVideoConfig(kPrimary, configs[0]);
> What process is this running in? It looks like this is in the browser? If so,
> the DCHECK is not sufficient.

Can you explain a little bit why DCHECK is not sufficient?
DCHECK here means it's a bug that data was passed from render process through
browser process using our own CAM IPC, and we should fix that.

[dcheng, 2015-06-05 23:48:53]

On 2015/06/05 at 23:46:28, erickung wrote:
> On 2015/06/05 23:41:18, dcheng wrote:
> > not lgtm
> > 
> >
https://codereview.chromium.org/1148253006/diff/80001/chromecast/media/cma/pi...
> > File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):
> > 
> >
https://codereview.chromium.org/1148253006/diff/80001/chromecast/media/cma/pi...
> > chromecast/media/cma/pipeline/video_pipeline_impl.cc:118:
> > DecoderConfigAdapter::ToCastVideoConfig(kPrimary, configs[0]);
> > What process is this running in? It looks like this is in the browser? If
so,
> > the DCHECK is not sufficient.
> 
> Can you explain a little bit why DCHECK is not sufficient?
> DCHECK here means it's a bug that data was passed from render process through
browser process using our own CAM IPC, and we should fix that.

This is a IPC message from a renderer to the browser right? A compromised
renderer could send a 0-length vector.

[erickung1, 2015-06-06 00:04:17]

https://codereview.chromium.org/1148253006/diff/80001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/80001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:118:
DecoderConfigAdapter::ToCastVideoConfig(kPrimary, configs[0]);
On 2015/06/05 23:41:18, dcheng wrote:
> What process is this running in? It looks like this is in the browser? If so,
> the DCHECK is not sufficient.

Done.
OK. it will return error if the vector is empty.
If not we do DCHECK to make sure the structure passed in is valid or not.

[dcheng, 2015-06-06 00:07:18]

Thanks, LGTM.

In general, we must be very careful with any IPC messages handled by the browser
process: since they are coming from less privileged processes usually, we must
verify all the data: we can't assume that it sent something valid.

[erickung1, 2015-06-06 00:08:56]

The CQ bit was checked by erickung@chromium.org

[erickung1, 2015-06-06 00:08:57]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1148253006/#ps120001
(title: "Return failure if incoming vector of DecoderConfig is empty or not.")

[commit-bot: I haz the power, 2015-06-06 00:10:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148253006/120001

[commit-bot: I haz the power, 2015-06-06 00:58:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-06 00:58:11]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, no build URL)

[erickung1, 2015-06-07 03:34:53]

The CQ bit was checked by erickung@chromium.org

[erickung1, 2015-06-07 03:34:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
dcheng@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1148253006/#ps140001
(title: "Remove DCHECK when config is updated. Will create another CL for the
change")

[commit-bot: I haz the power, 2015-06-07 03:34:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148253006/140001

[commit-bot: I haz the power, 2015-06-07 03:43:06]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-06-07 03:43:56]

Patchset 8 (id:??) landed as
https://crrev.com/f4e0b8865f3f0969f50587c31508c99557127f28
Cr-Commit-Position: refs/heads/master@{#333225}

[erickung1, 2015-06-07 05:20:26]

https://codereview.chromium.org/1148253006/diff/100001/chromecast/media/cma/p...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/1148253006/diff/100001/chromecast/media/cma/p...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:140:
DCHECK(!audio_config.IsValidConfig());
I ended up removing this DCHECK in this CL, same as in audio_pipeline_impl.cc
because two unit test are faileud due to the wrong assumption that both config
will be provided.

I will create another CL to add these DCHECK and change the unit test