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

Issue 827673 link

Starred by 4 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Decoder selection doesn't always wait for CDM to be set

Project Member Reported by xhw...@chromium.org, Mar 30 2018

Issue description

Today, if the initial decoder config is encrypted, the media pipeline will wait for a CDM to be set to start decoder selection process [1].

However, if the initial decoder config is clear, the media pipeline will not wait for the CDM. But then if we hit a config change and the new config is encrypted, we'll just proceed with normal decoder (re)selection logic, which will fail if a CDM still is not available.

To fix this, we can move the "no CDM then don't start initialize" logic from RendererImpl to DecoderSelector, so that we can trigger this logic even during decoder reselection.

We have browser test coverage on switching between clear and encrypted streams, but we don't have tests that test the case where a CDM is attached later. If we fix it, we should also add test coverage there.

Note that this can be worked around by JS player by attaching a MediaKeys promptly before we hit the encrypted stream, which is the best practice anyways IMHO. Maybe this can also be fixed in the spec land to require MediaKeys to be set before play() if the stream can be encrypted (not necessarily needs to wait for license exchange to complete though).

[1] https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=b12efe560b80ff0d3370e1dea848b860d831a600&l=153
 

Comment 1 by xhw...@chromium.org, Mar 30 2018

Status: Assigned (was: Available)

Comment 2 by xhw...@chromium.org, Apr 13 2018

Issue 832953 has been merged into this issue.

Comment 3 by xhw...@chromium.org, Apr 13 2018

Status: Started (was: Assigned)
Looks like we could actually cause crash because of this. I'll work on a fix.

Comment 4 by xhw...@chromium.org, Apr 13 2018

Labels: -Pri-2 -M-69 FoundIn-65 M-68 Pri-1
The crash only happens when:
- The first config is clear, then
- We switch to encrypted config, and
- By the time we hit encrypted config, there's still no CDM attached.

The crash rate is still low. 519 crashes on M64 and above.

Crash link: https://crash.corp.google.com/browse?q=product.name%3D'Chrome'%20AND%20EXISTS%20(SELECT%201%20FROM%20UNNEST(CrashedStackTrace.StackFrame)%20WITH%20OFFSET%20as%20position%20WHERE%20FunctionName%20like%20'media%3A%3A%25'%20AND%20position%3D1)%20AND%20product.Version%3E'64.0.0.0'%20AND%20expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D'media%3A%3ADecryptingVideoDecoder%3A%3AInitialize'&stbtiq=&reportid=&index=0

Comment 5 by xhw...@chromium.org, Apr 13 2018

For easy merge, I'll first stop the crash, so that these crash cases will become playback failure (decoder selection logic).

Then I'll look at a more comprehensive fix: move the "no CDM then don't start initialize" logic from RendererImpl to DecoderSelector, so that we can trigger this logic even during decoder reselection.
Labels: -M-68 Target-69

Comment 7 by xhw...@chromium.org, Jun 29 2018

Labels: -Target-69 Target-70
Labels: M-70
Cc: sande...@chromium.org
Labels: -Pri-1 -M-70 M-71 Pri-2
Labels: Target-71
Labels: M-70
Labels: -M-70
Labels: -Target-70
Labels: -M-71 -Target-71 Target-72 M-72
Status: Assigned (was: Started)
Labels: -M-72 -Target-72 Target-74 M-74
Labels: -Pri-2 -Target-74 -M-74 Pri-3
Re #5: The original crash has been fixed by dalecurtis@ in f0451434f1eb0cbcf324794ba549d36e370cfc63:
https://chromium-review.googlesource.com/c/chromium/src/+/952207/

So now if we start with clear stream without a CDM, and switch to an encrypted stream (with CDM set somewhere in between), playback will fail because the DecoderSelector won't be able to pick up the CDM.

We can pass CdmContext all the way down to DecoderSelector so that it can pick up the CDM whenever it's set, or add a callback for DecoderSelector to get the CdmContext so that it can even wait for the CDM to be set (we used to have code like that). But again, it's always recommended for JS to set the CDM before play() to avoid this sort of issues, especially since not all browsers would support setting CDM after playback starts.

With that, I'll set this to be P3, and remove the milestone. We can revisit later if needed.

Sign in to add a comment