Commit ac8b0d15 authored by Dale Curtis's avatar Dale Curtis Committed by Commit Bot

Speculative MCVD fix: Bind OnBuffersAvailableCB on known good thread.

Crash dumps are unclear and the issue seems resolved on Pixel hardware,
but some earlier stack traces suggest we're getting a null TaskRunner
when attempting to BindToCurrentLoop during CreateCodec(). As such
move binding to a known good thread.

This should be reverted if the issue is not fixed. If it is fixed
there's additional work to be done to understand the consequences
of CreateCodec() being called on an unexpected thread.

BUG=873094
TEST=none

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: I2928d7866c1f40de63f526ee7393d2d8b90884f2
Reviewed-on: https://chromium-review.googlesource.com/1175092Reviewed-by: default avatarThomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583067}
parent ac818c23
...@@ -204,6 +204,13 @@ void MediaCodecVideoDecoder::Initialize( ...@@ -204,6 +204,13 @@ void MediaCodecVideoDecoder::Initialize(
ExtractSpsAndPps(config.extra_data(), &csd0_, &csd1_); ExtractSpsAndPps(config.extra_data(), &csd0_, &csd1_);
#endif #endif
// Use the asynchronous API if we can.
if (first_init && device_info_->IsAsyncApiSupported()) {
on_buffers_available_cb_ = BindToCurrentLoop(
base::BindRepeating(&MediaCodecVideoDecoder::StartTimerOrPumpCodec,
weak_factory_.GetWeakPtr()));
}
// We only support setting CDM at first initialization. Even if the initial // We only support setting CDM at first initialization. Even if the initial
// config is clear, we'll still try to set CDM since we may switch to an // config is clear, we'll still try to set CDM since we may switch to an
// encrypted config later. // encrypted config later.
...@@ -437,13 +444,10 @@ void MediaCodecVideoDecoder::CreateCodec() { ...@@ -437,13 +444,10 @@ void MediaCodecVideoDecoder::CreateCodec() {
config->initial_expected_coded_size = decoder_config_.coded_size(); config->initial_expected_coded_size = decoder_config_.coded_size();
config->surface_bundle = target_surface_bundle_; config->surface_bundle = target_surface_bundle_;
// Use the asynchronous API if we can. // TODO(dalecurtis): We should be able to bind the callback directly instead
if (device_info_->IsAsyncApiSupported()) { // of storing it as a class member, but there's speculation that CreateCodec()
using_async_api_ = true; // is getting called on the wrong thread. https://crbug.com/873094.
config->on_buffers_available_cb = BindToCurrentLoop( config->on_buffers_available_cb = on_buffers_available_cb_;
base::BindRepeating(&MediaCodecVideoDecoder::StartTimerOrPumpCodec,
weak_factory_.GetWeakPtr()));
}
// Note that this might be the same surface bundle that we've been using, if // Note that this might be the same surface bundle that we've been using, if
// we're reinitializing the codec without changing surfaces. That's fine. // we're reinitializing the codec without changing surfaces. That's fine.
...@@ -465,7 +469,7 @@ void MediaCodecVideoDecoder::OnCodecConfigured( ...@@ -465,7 +469,7 @@ void MediaCodecVideoDecoder::OnCodecConfigured(
codec_ = std::make_unique<CodecWrapper>( codec_ = std::make_unique<CodecWrapper>(
CodecSurfacePair(std::move(codec), std::move(surface_bundle)), CodecSurfacePair(std::move(codec), std::move(surface_bundle)),
base::BindRepeating(&OutputBufferReleased, using_async_api_, base::BindRepeating(&OutputBufferReleased, !!on_buffers_available_cb_,
BindToCurrentLoop(base::BindRepeating( BindToCurrentLoop(base::BindRepeating(
&MediaCodecVideoDecoder::StartTimerOrPumpCodec, &MediaCodecVideoDecoder::StartTimerOrPumpCodec,
weak_factory_.GetWeakPtr())))); weak_factory_.GetWeakPtr()))));
...@@ -532,7 +536,7 @@ void MediaCodecVideoDecoder::PumpCodec(bool force_start_timer) { ...@@ -532,7 +536,7 @@ void MediaCodecVideoDecoder::PumpCodec(bool force_start_timer) {
did_work = true; did_work = true;
} while (did_input || did_output); } while (did_input || did_output);
if (using_async_api_) if (on_buffers_available_cb_)
return; return;
if (did_work || force_start_timer) if (did_work || force_start_timer)
...@@ -546,7 +550,7 @@ void MediaCodecVideoDecoder::StartTimerOrPumpCodec() { ...@@ -546,7 +550,7 @@ void MediaCodecVideoDecoder::StartTimerOrPumpCodec() {
if (state_ != State::kRunning) if (state_ != State::kRunning)
return; return;
if (using_async_api_) { if (on_buffers_available_cb_) {
PumpCodec(false); PumpCodec(false);
return; return;
} }
...@@ -568,7 +572,7 @@ void MediaCodecVideoDecoder::StartTimerOrPumpCodec() { ...@@ -568,7 +572,7 @@ void MediaCodecVideoDecoder::StartTimerOrPumpCodec() {
void MediaCodecVideoDecoder::StopTimerIfIdle() { void MediaCodecVideoDecoder::StopTimerIfIdle() {
DVLOG(4) << __func__; DVLOG(4) << __func__;
DCHECK(!using_async_api_); DCHECK(!on_buffers_available_cb_);
// Stop the timer if we've been idle for one second. Chosen arbitrarily. // Stop the timer if we've been idle for one second. Chosen arbitrarily.
const auto kTimeout = base::TimeDelta::FromSeconds(1); const auto kTimeout = base::TimeDelta::FromSeconds(1);
......
...@@ -281,7 +281,8 @@ class MEDIA_GPU_EXPORT MediaCodecVideoDecoder ...@@ -281,7 +281,8 @@ class MEDIA_GPU_EXPORT MediaCodecVideoDecoder
// Do we need a hw-secure codec? // Do we need a hw-secure codec?
bool requires_secure_codec_ = false; bool requires_secure_codec_ = false;
bool using_async_api_ = false; // If non-null, we're using the asynchronous media codec API.
base::RepeatingClosure on_buffers_available_cb_;
// Optional crypto object from the Cdm. // Optional crypto object from the Cdm.
base::android::ScopedJavaGlobalRef<jobject> media_crypto_; base::android::ScopedJavaGlobalRef<jobject> media_crypto_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment