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.

Sign up for GitHub

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
merged 9 commits into from
Jan 3, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
70a61c2
Android: When the hardware/default decoder fails to initialize, fall …
20kdc Dec 17, 2023
ab65b94
Forgot to "cargo fmt --all", thank you CI
20kdc Dec 17, 2023
f8fdf7f
Android: Reduce unwraps in decoder setup to one unified place so that…
20kdc Dec 18, 2023
fc739bd
Android: decoder.rs: cargo fmt --all
20kdc Dec 18, 2023
ebdc6ad
Add option to force software decoding
20kdc Dec 18, 2023
97eff12
Android: Reduce use of unwrapping in decoder setup. Logging: errors s…
20kdc Dec 20, 2023
d9a8731
Settings: Move force_software_decoder above mediacodec_extra_options,…
20kdc Dec 20, 2023
d3645fe
Revert logging changes made for Android decoder error messages
20kdc Dec 20, 2023
41bb3a7
Android: Use match instead of if-let for decoder error handling
20kdc Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions alvr/client_core/src/decoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
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,
zmerp 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 Down
113 changes: 81 additions & 32 deletions alvr/client_core/src/platform/android/decoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,43 @@ impl Drop for VideoDecoderSource {
}
}

// Configure & start a MediaCodec.
fn decoder_configure_and_start(
decoder: MediaCodec,
format: &MediaFormat,
image_reader: &ImageReader,
) -> Result<MediaCodec> {
decoder.configure(
&format,
Some(&image_reader.window().unwrap()),
MediaCodecDirection::Decoder,
)?;
decoder.start()?;
Ok(decoder)
}

// Creates a hardware (or assumed hardware) MediaCodec and then configure and start it.
fn decoder_try_hardware(
mime: &str,
format: &MediaFormat,
image_reader: &ImageReader,
) -> Result<MediaCodec> {
let tmp = MediaCodec::from_decoder_type(&mime)
.ok_or(anyhow!("unable to find decoder for mime type: {}", &mime))?;
decoder_configure_and_start(tmp, &format, &image_reader)
}

// Creates a software MediaCodec and then configure and start it.
fn decoder_try_software(
codec_name: &str,
format: &MediaFormat,
image_reader: &ImageReader,
) -> Result<MediaCodec> {
let tmp = MediaCodec::from_codec_name(&codec_name)
.ok_or(anyhow!("no such codec: {}", &codec_name))?;
decoder_configure_and_start(tmp, &format, &image_reader)
}
zmerp marked this conversation as resolved.
Show resolved Hide resolved

// Create a sink/source pair
pub fn video_decoder_split(
config: DecoderInitConfig,
Expand All @@ -167,26 +204,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 +259,51 @@ 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 = match config.codec {
CodecType::H264 => "video/avc",
CodecType::Hevc => "video/hevc",
};

let sw_codec_name = match config.codec {
CodecType::H264 => "OMX.google.h264.decoder",
CodecType::Hevc => "OMX.google.hevc.decoder",
};

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_try_software(sw_codec_name, &format, &image_reader).unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

Instead of unwrapping you can pass the error upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code is within a thread. I'm not sure where it'd go. I am cleaning up to reduce the amount of unwrapping, but there still will be "an unwrap". In addition, trying to de-unwrapify the code results in loss of which specific call failed, which could be bad for debugging.

Copy link
Member

Choose a reason for hiding this comment

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

You could use the function alvr_common::show_err() to print the error. The error per se might not be interesting, but otherwise the unwrap() will crash the thread silently, which is worse.

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

} else {
// Hardware decoders sometimes fail at the CSD-0.
// May as well fall back if this occurs.
match decoder_try_hardware(mime, &format, &image_reader) {
Ok(d) => d,
Err(e) => {
error!(
"Attempting software fallback due to error in default decoder: {}",
e
);
decoder_try_software(sw_codec_name, &format, &image_reader).unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

Same here

}
}
};

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

{
let mut decoder_lock = decoder_sink.lock();
Expand Down
Loading