Remove VideoDecodeAccelerator::SetCdm() and pass the cdm in Initialize()

Remove VideoDecodeAccelerator::SetCdm() and pass the cdm in Initialize()

When SetCdm was written we didn't necessarily have the CDM ID when we called
VDA::Initialize(). Now we do, so we can simplify things by removing SetCdm.

BUG=610520
Committed: https://crrev.com/8b0527c03f250052e4cc41356549279d2ebfce9c
Cr-Commit-Position: refs/heads/master@{#393150}

[watk, 2016-05-09 23:32:30]

Patchset #1 (id:1) has been deleted

[watk, 2016-05-09 23:34:31]

watk@chromium.org changed reviewers:
+ sandersd@chromium.org

[watk, 2016-05-09 23:34:31]

sandersd: please review
timav: FYI and a question: how can I test the CDM initialization path?

[watk, 2016-05-09 23:40:00]

https://codereview.chromium.org/1963903002/diff/20001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator.cc (left):

https://codereview.chromium.org/1963903002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:378: DCHECK(!is_encrypted_ ||
deferred_initialization_pending_);
DCHECK is below now

https://codereview.chromium.org/1963903002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:436: LOG(ERROR) << "Failed to
start thread for AVDA timer";
The timer doesn't run on that thread.

https://codereview.chromium.org/1963903002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:454: void
AndroidVideoDecodeAccelerator::SetCdm(int cdm_id) {
I moved this block because it's now in the private: section, but it's almost
unchanged. The difference is that I flipped the order of defined/!defined
because i think it's clearer, and removed a few lines at the start.

https://codereview.chromium.org/1963903002/diff/20001/media/video/video_decod...
File media/video/video_decode_accelerator.h (left):

https://codereview.chromium.org/1963903002/diff/20001/media/video/video_decod...
media/video/video_decode_accelerator.h:119: Config(const VideoDecoderConfig&
video_decoder_config);
I removed this because it felt clearer to initialize the fields manually. I had
to look at the definition to know which fields it initialized from the passed in
config.

[sandersd (OOO until July 31), 2016-05-10 00:09:54]

lgtm

https://codereview.chromium.org/1963903002/diff/20001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1963903002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:436:
DCHECK(deferred_initialization_pending_);
We should handle this case gracefully (return false).

https://codereview.chromium.org/1963903002/diff/20001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1963903002/diff/20001/media/video/video_decod...
media/video/video_decode_accelerator.h:119: // Intentional converting
constructor.
Newline before comment. Add a TODO.

[Tima Vaisburd, 2016-05-10 00:20:19]

On 2016/05/09 23:34:31, watk wrote:
> sandersd: please review
> timav: FYI and a question: how can I test the CDM initialization path?

Did you try Shaka player (http://shaka-player-demo.appspot.com/demo/)?
They recently updated the UI, select the asset "Tears of Steel (Widewine)" and
press Load

[watk, 2016-05-10 01:10:01]

On 2016/05/10 00:20:19, Tima Vaisburd wrote:
> On 2016/05/09 23:34:31, watk wrote:
> > sandersd: please review
> > timav: FYI and a question: how can I test the CDM initialization path?
> 
> Did you try Shaka player (http://shaka-player-demo.appspot.com/demo/)
> They recently updated the UI, select the asset "Tears of Steel (Widewine)" and
> press Load

Awesome, thanks. I'm hitting the DCHECK here: 

  cdm_for_reference_holding_only_ = media::MojoCdmService::LegacyGetCdm(cdm_id);
  DCHECK(cdm_for_reference_holding_only_);

I haven't spent too long investigating yet though.

[watk, 2016-05-12 00:12:23]

Tested with xhwang's in progress patch to get EME working again, and I
discovered that I'd forgotten to add cdm_id to the param traits. I've added it
and it works.

[watk, 2016-05-12 00:12:41]

The CQ bit was checked by watk@chromium.org to run a CQ dry run

[watk, 2016-05-12 00:12:44]

The CQ bit was unchecked by watk@chromium.org

[watk, 2016-05-12 00:13:43]

The CQ bit was checked by watk@chromium.org

[watk, 2016-05-12 00:13:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1963903002/#ps60001
(title: "Add cdm_id to paramtraits")

[commit-bot: I haz the power, 2016-05-12 00:14:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1963903002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1963903002/60001

[commit-bot: I haz the power, 2016-05-12 02:29:37]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-12 02:30:51]

Description was changed from

==========
Remove VideoDecodeAccelerator::SetCdm() and pass the cdm in Initialize()

When SetCdm was written we didn't necessarily have the CDM ID when we called
VDA::Initialize(). Now we do, so we can simplify things by removing SetCdm.

BUG=610520
==========

to

==========
Remove VideoDecodeAccelerator::SetCdm() and pass the cdm in Initialize()

When SetCdm was written we didn't necessarily have the CDM ID when we called
VDA::Initialize(). Now we do, so we can simplify things by removing SetCdm.

BUG=610520

Committed: https://crrev.com/8b0527c03f250052e4cc41356549279d2ebfce9c
Cr-Commit-Position: refs/heads/master@{#393150}
==========

[commit-bot: I haz the power, 2016-05-12 02:30:52]

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