From 97eff12fffb2a0909c6264ebc83546211e88dde8 Mon Sep 17 00:00:00 2001 From: 20kdc Date: Wed, 20 Dec 2023 16:33:26 +0000 Subject: [PATCH] Android: Reduce use of unwrapping in decoder setup. Logging: errors should be more explicit --- alvr/client_core/src/decoder.rs | 7 -- .../src/platform/android/decoder.rs | 85 ++++++++++--------- alvr/common/src/logging.rs | 4 +- 3 files changed, 49 insertions(+), 47 deletions(-) diff --git a/alvr/client_core/src/decoder.rs b/alvr/client_core/src/decoder.rs index acceaa863a..e1e5340155 100644 --- a/alvr/client_core/src/decoder.rs +++ b/alvr/client_core/src/decoder.rs @@ -7,8 +7,6 @@ use std::time::Duration; #[derive(Clone)] pub struct DecoderInitConfig { pub codec: CodecType, - pub width: i32, - pub height: i32, pub force_software_decoder: bool, pub max_buffering_frames: f32, pub buffering_history_weight: f32, @@ -18,11 +16,6 @@ pub struct DecoderInitConfig { pub static DECODER_INIT_CONFIG: Lazy> = 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, max_buffering_frames: 1.0, buffering_history_weight: 0.9, diff --git a/alvr/client_core/src/platform/android/decoder.rs b/alvr/client_core/src/platform/android/decoder.rs index be100d85a2..463946e141 100644 --- a/alvr/client_core/src/platform/android/decoder.rs +++ b/alvr/client_core/src/platform/android/decoder.rs @@ -1,9 +1,9 @@ use crate::decoder::DecoderInitConfig; use alvr_common::{ - anyhow::{anyhow, bail, Result}, + anyhow::{anyhow, bail, Context, Result}, error, info, parking_lot::{Condvar, Mutex}, - warn, RelaxedAtomic, + show_err, warn, RelaxedAtomic, }; use alvr_session::{CodecType, MediacodecDataType}; use ndk::{ @@ -170,12 +170,14 @@ fn decoder_attempt_setup( 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()?; + decoder + .configure( + &format, + Some(&image_reader.window()?), + MediaCodecDirection::Decoder, + ) + .with_context(|| "failed to configure decoder")?; + decoder.start().with_context(|| "failed to start decoder")?; Ok(decoder) } @@ -261,8 +263,10 @@ pub fn video_decoder_split( let format = MediaFormat::new(); format.set_str("mime", mime); - format.set_i32("width", config.width); - format.set_i32("height", config.height); + // Given https://github.com/alvr-org/ALVR/pull/1933#discussion_r1431902906 - change at own risk. + // It might be harmless, it might not be, but it's definitely a risk. + format.set_i32("width", 512); + format.set_i32("height", 1024); format.set_buffer("csd-0", &csd_0); for (key, value) in &config.options { @@ -284,8 +288,9 @@ pub fn video_decoder_split( match decoder_attempt_setup(config.codec, false, &format, &image_reader) { Ok(d) => Ok(d), Err(e) => { + // would be "warn!" but this is a severe caveat and a pretty major error. error!( - "Attempting software fallback due to error in default decoder: {}", + "Attempting software fallback due to error in default decoder: {:#}", e ); decoder_attempt_setup(config.codec, true, &format, &image_reader) @@ -293,43 +298,47 @@ pub fn video_decoder_split( } }; - let decoder = Arc::new(FakeThreadSafe(preparing_decoder.unwrap())); + if let Ok(prepared_decoder) = preparing_decoder { + let decoder = Arc::new(FakeThreadSafe(prepared_decoder)); - { - let mut decoder_lock = decoder_sink.lock(); + { + let mut decoder_lock = decoder_sink.lock(); - *decoder_lock = Some(Arc::clone(&decoder)); + *decoder_lock = Some(Arc::clone(&decoder)); - decoder_ready_notifier.notify_one(); - } + decoder_ready_notifier.notify_one(); + } - while running.value() { - match decoder.dequeue_output_buffer(Duration::from_millis(1)) { - Ok(DequeuedOutputBufferInfoResult::Buffer(buffer)) => { - // The buffer timestamp is actually nanoseconds - let presentation_time_ns = buffer.info().presentation_time_us(); + while running.value() { + match decoder.dequeue_output_buffer(Duration::from_millis(1)) { + Ok(DequeuedOutputBufferInfoResult::Buffer(buffer)) => { + // The buffer timestamp is actually nanoseconds + let presentation_time_ns = buffer.info().presentation_time_us(); - if let Err(e) = - decoder.release_output_buffer_at_time(buffer, presentation_time_ns) - { - error!("Decoder dequeue error: {e}"); + if let Err(e) = + decoder.release_output_buffer_at_time(buffer, presentation_time_ns) + { + error!("Decoder dequeue error: {e}"); + } } - } - Ok(DequeuedOutputBufferInfoResult::TryAgainLater) => thread::yield_now(), - Ok(i) => info!("Decoder dequeue event: {i:?}"), - Err(e) => { - error!("Decoder dequeue error: {e}"); + Ok(DequeuedOutputBufferInfoResult::TryAgainLater) => thread::yield_now(), + Ok(i) => info!("Decoder dequeue event: {i:?}"), + Err(e) => { + error!("Decoder dequeue error: {e}"); - // lessen logcat flood (just in case) - thread::sleep(Duration::from_millis(50)); + // lessen logcat flood (just in case) + thread::sleep(Duration::from_millis(50)); + } } } - } - // Destroy all resources - decoder_sink.lock().take(); // Make sure the shared ref is deleted first - decoder.stop().unwrap(); - drop(decoder); + // Destroy all resources + decoder_sink.lock().take(); // Make sure the shared ref is deleted first + decoder.stop().unwrap(); + drop(decoder); + } else { + show_err(preparing_decoder); + } image_queue.lock().clear(); error!("FIXME: Leaking Imagereader!"); diff --git a/alvr/common/src/logging.rs b/alvr/common/src/logging.rs index a02f3cc877..bddd00d31a 100644 --- a/alvr/common/src/logging.rs +++ b/alvr/common/src/logging.rs @@ -128,11 +128,11 @@ pub fn show_e_blocking(e: E) { } pub fn show_err(res: Result) -> Option { - res.map_err(|e| show_e_block(e, false)).ok() + res.map_err(|e| show_e_block(format!("{:#}", e), false)).ok() } pub fn show_err_blocking(res: Result) -> Option { - res.map_err(|e| show_e_block(e, true)).ok() + res.map_err(|e| show_e_block(format!("{:#}", e), true)).ok() } pub trait ToAny {