New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 840586 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Clear to Encrypted H264 video switching fails on Chrome for Android

Project Member Reported by xhw...@chromium.org, May 7 2018

Issue description

OS: Android

What steps will reproduce the problem?
(1) Start a playback that switch from clear H264 to encrypted H264

What is the expected result?
Playback works before and after the switch.

What happens instead?
Playback stalls after the switch.

Please use labels and text to provide additional information.
Since it's H264, we first choose GpuVideoDecoder to handle the clear stream. Then we switch to encrypted steram. Typically this will trigger a config change, which will trigger a video decoder reinitializtion. However, GpuVideoDecoder doesn't implement the typical decoder reinitialization logic, but instead assumes there's no work to be done for reinitialization. Also, we didn't pass the |cdm_id| when we initialized GpuVideoDecoder for the first time, so the VDA is setup only to handle clear streams, causing the stall.


 
To fix the issue, we should always pass the |cdm_id| down to the VDA, and always InitializeCdm() in AVDA, even if the stream is clear.
Cc: kdevarakonda@google.com
Project Member

Comment 3 by bugdroid1@chromium.org, May 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e38f04dc127aa41495090915a253c66bc6414045

commit e38f04dc127aa41495090915a253c66bc6414045
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Thu May 10 06:47:28 2018

media: Add Media.EME.MediaCryptoAvailable UMA

Bug:  840586 
Change-Id: I0d91bde150d6e3755df3a3577862149298eaa61f
Reviewed-on: https://chromium-review.googlesource.com/1050934
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557469}
[modify] https://crrev.com/e38f04dc127aa41495090915a253c66bc6414045/media/base/android/media_drm_bridge.cc
[modify] https://crrev.com/e38f04dc127aa41495090915a253c66bc6414045/tools/metrics/histograms/histograms.xml

Project Member

Comment 4 by bugdroid1@chromium.org, May 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/63e76603b38c73c3d5b644f197f2f4f9b11967aa

commit 63e76603b38c73c3d5b644f197f2f4f9b11967aa
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Fri May 11 02:50:24 2018

media: Fix clear/encrypted switching in GpuVideoDecoder

- Fix GpuVideoDecoder::Initialize() so that even if the stream is clear
  we still pass down the CDM ID. This is needed for handling future
  encrypted streams since GpuVideoDecoder doesn't do full-fledged
  reinitialization.
- Fix AndroidVideoDecodeAccelerator so that even if the stream is clear
  we still requrest MediaCrypto, for the same reason.

Bug:  840586 
Test: Manually tested
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I99c4a86001aa75924f93d8ee634fd5136a85e2c1
Reviewed-on: https://chromium-review.googlesource.com/1050434
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557774}
[modify] https://crrev.com/63e76603b38c73c3d5b644f197f2f4f9b11967aa/media/filters/gpu_video_decoder.cc
[modify] https://crrev.com/63e76603b38c73c3d5b644f197f2f4f9b11967aa/media/gpu/android/android_video_decode_accelerator.cc
[modify] https://crrev.com/63e76603b38c73c3d5b644f197f2f4f9b11967aa/media/gpu/ipc/service/vda_video_decoder.cc

Project Member

Comment 5 by bugdroid1@chromium.org, May 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f036cc34a1f213adb3862f939bb9269f2078a42

commit 9f036cc34a1f213adb3862f939bb9269f2078a42
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Wed May 16 19:40:54 2018

media: Only allow CDM setting in initial init in MediaCodecVideoDecoder

Even though a new |cdm_context| is passed in re-initialization, the
current MediaCodecVideoDecoder (MCVD) code assumes SetCdm() only happens
in the first initialization. Instead of trying to fix MCVD to support
setting CDM on a later config change, we'll only allow CDM setting in
the initial initialization.

More precisely:
- SetCdm() is only triggered in initial initialization, if |cdm_context|
  is non-null, even if the initial config is clear.
- In a re-initialization, if the config is encrypted, and we don't have
  a MediaCrypto already, we'll simply fail, even if a non-null
  |cdm_context| is passed in.

For the reference, "Support of switching [between clear and encrypted]
when MediaKeys is set after the playback starts, [or ...], is a quality
of implementation issue" [1].

We may apply this to our media pipeline and all decoders to simplify our
code. The cost for a JS player to setMediaKeys() before playback starts
is really minimal (no need to even start license exchange).

There are some other changes in this CL:
- Fix AVDA so that if MediaCrytoContext is not available but the stream,
  is clear, we'll not try to initialize CDM.
- Misc logging fixes based on media logging guidance [2].

[1] https://github.com/w3c/encrypted-media/pull/374/files
[2] https://chromium.googlesource.com/chromium/src/+/master/media/README.md#logging

Bug:  840586 
Test: Manually tested switching from clear to encrypted H264.
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I584a21dc992ae8bf13794b9310cb2a549e73bade
Reviewed-on: https://chromium-review.googlesource.com/1056490
Reviewed-by: Frank Liberato <liberato@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559234}
[modify] https://crrev.com/9f036cc34a1f213adb3862f939bb9269f2078a42/media/base/android/mock_media_crypto_context.cc
[modify] https://crrev.com/9f036cc34a1f213adb3862f939bb9269f2078a42/media/base/android/mock_media_crypto_context.h
[modify] https://crrev.com/9f036cc34a1f213adb3862f939bb9269f2078a42/media/filters/decoder_stream.cc
[modify] https://crrev.com/9f036cc34a1f213adb3862f939bb9269f2078a42/media/gpu/android/android_video_decode_accelerator.cc
[modify] https://crrev.com/9f036cc34a1f213adb3862f939bb9269f2078a42/media/gpu/android/media_codec_video_decoder.cc
[modify] https://crrev.com/9f036cc34a1f213adb3862f939bb9269f2078a42/media/gpu/android/media_codec_video_decoder_unittest.cc
[modify] https://crrev.com/9f036cc34a1f213adb3862f939bb9269f2078a42/media/mojo/clients/mojo_video_decoder.cc
[modify] https://crrev.com/9f036cc34a1f213adb3862f939bb9269f2078a42/media/mojo/services/mojo_video_decoder_service.cc

Comment 6 by xhw...@chromium.org, Jun 22 2018

Status: Fixed (was: Started)

Sign in to add a comment