Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Android: When the hardware/default decoder fails to initialize, fall back to software. #1933

Merged
2 changes: 1 addition & 1 deletion alvr/client_core/src/connection.rs
Expand Up @@ -457,7 +457,7 @@ fn connection_pipeline(

match maybe_packet {
Ok(ServerControlPacket::InitializeDecoder(config)) => {
decoder::create_decoder(config);
decoder::create_decoder(config, settings.video.force_software_decoder);
}
Ok(ServerControlPacket::Restarting) => {
info!("{SERVER_RESTART_MESSAGE}");
Expand Down
12 changes: 11 additions & 1 deletion alvr/client_core/src/decoder.rs
Expand Up @@ -7,6 +7,9 @@ use std::time::Duration;
#[derive(Clone)]
pub struct DecoderInitConfig {
pub codec: CodecType,
pub width: i32,
pub height: i32,
pub force_software_decoder: bool,
zarik5 marked this conversation as resolved.
Show resolved Hide resolved
pub max_buffering_frames: f32,
pub buffering_history_weight: f32,
pub options: Vec<(String, MediacodecDataType)>,
Expand All @@ -15,6 +18,12 @@ pub struct DecoderInitConfig {
pub static DECODER_INIT_CONFIG: Lazy<Mutex<DecoderInitConfig>> = Lazy::new(|| {
Mutex::new(DecoderInitConfig {
codec: CodecType::H264,
// TODO: Actually get the real values for these and update accordingly.
// These are inherited from the previous code to avoid breaking anything.
// Would also be nice if force_software_decoder was configurable.
width: 512,
height: 1024,
force_software_decoder: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked it really matters? Video resolution is extracted from SPS/PPS (CSD-0).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that it, for certain, really matters. In my testing it does not aid my particular case. But given that this whole thing started because of codec breakage, I feel there is merit to setting these properties correctly. If they didn't somehow matter, why even set them in the first place?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code was translated as faithfully as possible from ancient java code. We are not in touch with the original author. I think this was used to mitigate some decoder implementation edge cases.
Since we are not changing the values at all, I think we should not add the width an height fields in DecoderInitConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as of 97eff12

max_buffering_frames: 1.0,
buffering_history_weight: 0.9,
options: vec![],
Expand All @@ -29,9 +38,10 @@ pub static DECODER_SOURCE: alvr_common::OptLazy<crate::platform::VideoDecoderSou

pub static EXTERNAL_DECODER: RelaxedAtomic = RelaxedAtomic::new(false);

pub fn create_decoder(lazy_config: DecoderInitializationConfig) {
pub fn create_decoder(lazy_config: DecoderInitializationConfig, force_software_decoder: bool) {
let mut config = DECODER_INIT_CONFIG.lock();
config.codec = lazy_config.codec;
config.force_software_decoder = force_software_decoder;

if EXTERNAL_DECODER.value() {
EVENT_QUEUE
Expand Down
103 changes: 71 additions & 32 deletions alvr/client_core/src/platform/android/decoder.rs
@@ -1,6 +1,6 @@
use crate::decoder::DecoderInitConfig;
use alvr_common::{
anyhow::{bail, Result},
anyhow::{anyhow, bail, Result},
error, info,
parking_lot::{Condvar, Mutex},
warn, RelaxedAtomic,
Expand Down Expand Up @@ -144,6 +144,41 @@ impl Drop for VideoDecoderSource {
}
}

fn mime_for_codec(codec: CodecType) -> &'static str {
match codec {
CodecType::H264 => "video/avc",
CodecType::Hevc => "video/hevc",
}
}

// Attempts to create a MediaCodec, and then configure and start it.
fn decoder_attempt_setup(
codec_type: CodecType,
is_software: bool,
format: &MediaFormat,
image_reader: &ImageReader,
) -> Result<MediaCodec> {
let decoder = if is_software {
let sw_codec_name = match codec_type {
CodecType::H264 => "OMX.google.h264.decoder",
CodecType::Hevc => "OMX.google.hevc.decoder",
};
MediaCodec::from_codec_name(&sw_codec_name)
.ok_or(anyhow!("no such codec: {}", &sw_codec_name))?
} else {
let mime = mime_for_codec(codec_type);
MediaCodec::from_decoder_type(&mime)
.ok_or(anyhow!("unable to find decoder for mime type: {}", &mime))?
};
decoder.configure(
&format,
Some(&image_reader.window()?),
MediaCodecDirection::Decoder,
)?;
decoder.start()?;
Ok(decoder)
}

// Create a sink/source pair
pub fn video_decoder_split(
config: DecoderInitConfig,
Expand All @@ -167,26 +202,6 @@ pub fn video_decoder_split(
// 2x: keep the target buffering in the middle of the max amount of queuable frames
let available_buffering_frames = (2. * config.max_buffering_frames).ceil() as usize;

let mime = match config.codec {
CodecType::H264 => "video/avc",
CodecType::Hevc => "video/hevc",
};

let format = MediaFormat::new();
format.set_str("mime", mime);
format.set_i32("width", 512);
format.set_i32("height", 1024);
format.set_buffer("csd-0", &csd_0);

for (key, value) in &config.options {
match value {
MediacodecDataType::Float(value) => format.set_f32(key, *value),
MediacodecDataType::Int32(value) => format.set_i32(key, *value),
MediacodecDataType::Int64(value) => format.set_i64(key, *value),
MediacodecDataType::String(value) => format.set_str(key, value),
}
}

let mut image_reader = ImageReader::new_with_usage(
1,
1,
Expand Down Expand Up @@ -242,19 +257,43 @@ pub fn video_decoder_split(
.set_buffer_removed_listener(Box::new(|_, _| ()))
.unwrap();

let decoder = Arc::new(FakeThreadSafe(
MediaCodec::from_decoder_type(&mime).unwrap(),
));
let mime = mime_for_codec(config.codec);

let format = MediaFormat::new();
format.set_str("mime", mime);
format.set_i32("width", config.width);
format.set_i32("height", config.height);
format.set_buffer("csd-0", &csd_0);

for (key, value) in &config.options {
match value {
MediacodecDataType::Float(value) => format.set_f32(key, *value),
MediacodecDataType::Int32(value) => format.set_i32(key, *value),
MediacodecDataType::Int64(value) => format.set_i64(key, *value),
MediacodecDataType::String(value) => format.set_str(key, value),
}
}

info!("Using AMediaCoded format:{} ", format);
decoder
.configure(
&format,
Some(&image_reader.window().unwrap()),
MediaCodecDirection::Decoder,
)
.unwrap();
decoder.start().unwrap();

let preparing_decoder = if config.force_software_decoder {
decoder_attempt_setup(config.codec, true, &format, &image_reader)
} else {
// Hardware decoders sometimes fail at the CSD-0.
// May as well fall back if this occurs.
match decoder_attempt_setup(config.codec, false, &format, &image_reader) {
Ok(d) => Ok(d),
Err(e) => {
error!(
"Attempting software fallback due to error in default decoder: {}",
e
);
decoder_attempt_setup(config.codec, true, &format, &image_reader)
}
}
};

let decoder = Arc::new(FakeThreadSafe(preparing_decoder.unwrap()));

{
let mut decoder_lock = decoder_sink.lock();
Expand Down
6 changes: 6 additions & 0 deletions alvr/session/src/settings.rs
Expand Up @@ -479,6 +479,11 @@ pub struct VideoConfig {

pub clientside_foveation: Switch<ClientsideFoveationConfig>,

#[schema(strings(
help = "Attempts to use a software decoder on the device. Slow, but may work around broken codecs."
))]
pub force_software_decoder: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this just above mediacodec_extra_options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as of d9a8731.


#[schema(flag = "steamvr-restart")]
pub color_correction: Switch<ColorCorrectionConfig>,
}
Expand Down Expand Up @@ -1272,6 +1277,7 @@ pub fn session_settings_default() -> SettingsDefault {
vertical_offset_deg: 0.0,
},
},
force_software_decoder: false,
color_correction: SwitchDefault {
enabled: true,
content: ColorCorrectionConfigDefault {
Expand Down