From caaacc8f92eee8c948d1d6d8c9571d84846b7fc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADtor=20Vasconcellos?= Date: Fri, 30 Jun 2023 08:00:04 -0300 Subject: [PATCH] [ENG-866, ENG-535] Thumbnailer fixes (double-free, thumbnails for portrait videos) (#1059) * Fix double-free during MovieDecoder Drop - Fix broken thumbnails for portrait videos - Fix some inconsistencies between our MovieDecoder impl and ffmpegthumbnailer impl - Make sd-desktop-macos use the correct macOS deployment target * fmt --------- Co-authored-by: Jamie Pine <32987599+jamiepine@users.noreply.github.com> Co-authored-by: jake <77554505+brxken128@users.noreply.github.com> Co-authored-by: Utku <74243531+utkubakir@users.noreply.github.com> --- Cargo.lock | 1 + apps/desktop/crates/macos/build.rs | 14 ++- crates/ffmpeg/Cargo.toml | 1 + crates/ffmpeg/src/movie_decoder.rs | 152 ++++++++++------------------- crates/ffmpeg/src/thumbnailer.rs | 14 ++- crates/ffmpeg/src/video_frame.rs | 6 +- 6 files changed, 77 insertions(+), 111 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dcdbeea40..669e2cede 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7167,6 +7167,7 @@ dependencies = [ "tempfile", "thiserror", "tokio", + "tracing 0.1.37", "webp", ] diff --git a/apps/desktop/crates/macos/build.rs b/apps/desktop/crates/macos/build.rs index b4dd4fb57..9cd65bd52 100644 --- a/apps/desktop/crates/macos/build.rs +++ b/apps/desktop/crates/macos/build.rs @@ -1,6 +1,14 @@ +#[cfg(target_os = "macos")] +use std::env; + fn main() { #[cfg(target_os = "macos")] - swift_rs::SwiftLinker::new("10.15") - .with_package("sd-desktop-macos", "./") - .link(); + { + let deployment_target = + env::var("MACOSX_DEPLOYMENT_TARGET").unwrap_or_else(|_| String::from("10.15")); + + swift_rs::SwiftLinker::new(deployment_target.as_str()) + .with_package("sd-desktop-macos", "./") + .link(); + } } diff --git a/crates/ffmpeg/Cargo.toml b/crates/ffmpeg/Cargo.toml index 244a12f7e..e1efe2159 100644 --- a/crates/ffmpeg/Cargo.toml +++ b/crates/ffmpeg/Cargo.toml @@ -13,6 +13,7 @@ edition = { workspace = true } [dependencies] ffmpeg-sys-next = "6.0.1" +tracing = "0.1.37" thiserror = "1.0.40" webp = "0.2.2" diff --git a/crates/ffmpeg/src/movie_decoder.rs b/crates/ffmpeg/src/movie_decoder.rs index 42a695cfa..2b741eee9 100644 --- a/crates/ffmpeg/src/movie_decoder.rs +++ b/crates/ffmpeg/src/movie_decoder.rs @@ -6,26 +6,23 @@ use crate::{ use ffmpeg_sys_next::{ av_buffersink_get_frame, av_buffersrc_write_frame, av_dict_get, av_display_rotation_get, - av_frame_alloc, av_frame_free, av_guess_sample_aspect_ratio, av_packet_alloc, av_packet_free, - av_packet_unref, av_read_frame, av_seek_frame, av_stream_get_side_data, avcodec_alloc_context3, - avcodec_find_decoder, avcodec_flush_buffers, avcodec_free_context, avcodec_open2, - avcodec_parameters_to_context, avcodec_receive_frame, avcodec_send_packet, - avfilter_get_by_name, avfilter_graph_alloc, avfilter_graph_config, - avfilter_graph_create_filter, avfilter_graph_free, avfilter_link, avformat_close_input, - avformat_find_stream_info, avformat_open_input, AVCodec, AVCodecContext, AVCodecID, - AVFilterContext, AVFilterGraph, AVFormatContext, AVFrame, AVMediaType, AVPacket, + av_frame_alloc, av_frame_free, av_packet_alloc, av_packet_free, av_packet_unref, av_read_frame, + av_seek_frame, av_stream_get_side_data, avcodec_alloc_context3, avcodec_find_decoder, + avcodec_flush_buffers, avcodec_free_context, avcodec_open2, avcodec_parameters_to_context, + avcodec_receive_frame, avcodec_send_packet, avfilter_get_by_name, avfilter_graph_alloc, + avfilter_graph_config, avfilter_graph_create_filter, avfilter_graph_free, avfilter_link, + avformat_close_input, avformat_find_stream_info, avformat_open_input, AVCodec, AVCodecContext, + AVCodecID, AVFilterContext, AVFilterGraph, AVFormatContext, AVFrame, AVMediaType, AVPacket, AVPacketSideDataType, AVRational, AVStream, AVERROR, AVERROR_EOF, AVPROBE_SCORE_MAX, AV_DICT_IGNORE_SUFFIX, AV_TIME_BASE, EAGAIN, }; use std::{ - ffi::{c_int, CString}, + ffi::{CStr, CString}, fmt::Write, path::Path, time::Duration, }; -const AVERROR_EAGAIN: c_int = AVERROR(EAGAIN); - #[derive(Debug, Clone, Copy)] pub(crate) enum ThumbnailSize { Dimensions { width: u32, height: u32 }, @@ -139,7 +136,7 @@ impl MovieDecoder { pub(crate) fn seek(&mut self, seconds: i64) -> Result<(), ThumbnailerError> { if !self.allow_seek { - return Err(ThumbnailerError::SeekNotAllowed); + return Ok(()); } let timestamp = (AV_TIME_BASE as i64).checked_mul(seconds).unwrap_or(0); @@ -204,7 +201,7 @@ impl MovieDecoder { let mut new_frame = FfmpegFrame::new()?; let mut attempts = 0; let mut ret = unsafe { av_buffersink_get_frame(self.filter_sink, new_frame.as_mut_ptr()) }; - while ret == AVERROR_EAGAIN && attempts < 10 { + while ret == AVERROR(EAGAIN) && attempts < 10 { self.decode_video_frame()?; check_error( unsafe { av_buffersrc_write_frame(self.filter_source, self.frame) }, @@ -251,7 +248,10 @@ impl MovieDecoder { std::slice::from_raw_parts((*new_frame.as_mut_ptr()).data[0], frame_data_size) }); - unsafe { avfilter_graph_free(&mut self.filter_graph) }; + if !self.filter_graph.is_null() { + unsafe { avfilter_graph_free(&mut self.filter_graph) }; + self.filter_graph = std::ptr::null_mut(); + } Ok(()) } @@ -336,24 +336,21 @@ impl MovieDecoder { AV_DICT_IGNORE_SUFFIX, ) }; + if tag.is_null() { break; } - if unsafe { - CString::from_raw((*tag).key) - .to_str() - .expect("Found non-UTF-8 path") == "filename" - && CString::from_raw((*tag).value) - .to_str() - .expect("Found non-UTF-8 path") - .starts_with("cover.") - } { - if embedded_data_streams.is_empty() { - embedded_data_streams.push(stream_idx); - } else { - embedded_data_streams[0] = stream_idx; + + // WARNING: NEVER use CString with foreign raw pointer (causes double-free) + let key = unsafe { CStr::from_ptr((*tag).key) }.to_str(); + if let Ok(key) = key { + let value = unsafe { CStr::from_ptr((*tag).value) }.to_str(); + if let Ok(value) = value { + if key == "filename" && value == "cover." { + embedded_data_streams.insert(0, stream_idx); + continue; + } } - continue; } } } @@ -389,7 +386,7 @@ impl MovieDecoder { self.packet = unsafe { av_packet_alloc() }; while frames_available && !frame_decoded { - frames_available = unsafe { av_read_frame(self.format_context, self.packet) == 0 }; + frames_available = unsafe { av_read_frame(self.format_context, self.packet) >= 0 }; if frames_available { frame_decoded = unsafe { (*self.packet).stream_index } == self.video_stream_index; if !frame_decoded { @@ -420,11 +417,11 @@ impl MovieDecoder { match unsafe { avcodec_receive_frame(self.video_codec_context, self.frame) } { 0 => Ok(true), - AVERROR_EAGAIN => Ok(false), - e => Err(ThumbnailerError::FfmpegWithReason( + e if e != AVERROR(EAGAIN) => Err(ThumbnailerError::FfmpegWithReason( FfmpegError::from(e), "Failed to receive frame from decoder".to_string(), )), + _ => Ok(false), } } @@ -592,8 +589,7 @@ impl MovieDecoder { if size.is_none() { return Ok("w=0:h=0".to_string()); } - - let size = size.unwrap(); + let size = size.expect("Size should have been checked for None"); match size { ThumbnailSize::Dimensions { width, height } => { @@ -605,77 +601,26 @@ impl MovieDecoder { } } + if scaled_width <= 0 { + scaled_width = -1 + } + + if scaled_height <= 0 { + scaled_height = -1 + } + let mut scale = String::new(); - if scaled_width != -1 && scaled_height != -1 { - let _ = write!(scale, "w={scaled_width}:h={scaled_height}"); - if maintain_aspect_ratio { - let _ = write!(scale, ":force_original_aspect_ratio=decrease"); - } - } else if !maintain_aspect_ratio { - if scaled_width == -1 { - let _ = write!(scale, "w={scaled_height}:h={scaled_height}"); - } else { - let _ = write!(scale, "w={scaled_width}:h={scaled_width}"); - } - } else { - let size_int = if scaled_height == -1 { - scaled_width - } else { - scaled_height - }; + write!(scale, "w={scaled_width}:h={scaled_height}") + .expect("Write of const string should work"); - let anamorphic; - let aspect_ratio; - unsafe { - scaled_width = (*self.video_codec_context).width; - scaled_height = (*self.video_codec_context).height; - - aspect_ratio = av_guess_sample_aspect_ratio( - self.format_context, - self.video_stream, - self.frame, - ); - anamorphic = aspect_ratio.num != 0 && aspect_ratio.num != aspect_ratio.den; - } - - if anamorphic { - scaled_width = scaled_width * aspect_ratio.num / aspect_ratio.den; - - if size_int != 0 { - if scaled_height > scaled_width { - scaled_width = scaled_width * size_int / scaled_height; - scaled_height = size_int; - } else { - scaled_height = scaled_height * size_int / scaled_width; - scaled_width = size_int; - } - } - - let _ = write!(scale, "w={scaled_width}:h={scaled_height}"); - } else if scaled_height > scaled_width { - let _ = write!( - scale, - "w=-1:h={}", - if size_int == 0 { - scaled_height - } else { - size_int - } - ); - } else { - let _ = write!( - scale, - "w={}:h=-1", - if size_int == 0 { - scaled_width - } else { - size_int - } - ); - } + if maintain_aspect_ratio { + write!(scale, ":force_original_aspect_ratio=decrease") + .expect("Write of const string should work"); } + // TODO: Handle anamorphic videos + Ok(scale) } @@ -723,8 +668,8 @@ impl Drop for MovieDecoder { unsafe { av_packet_unref(self.packet); av_packet_free(&mut self.packet); - self.packet = std::ptr::null_mut(); } + self.packet = std::ptr::null_mut(); } if !self.frame.is_null() { @@ -732,6 +677,7 @@ impl Drop for MovieDecoder { av_frame_free(&mut self.frame); self.frame = std::ptr::null_mut(); } + self.frame = std::ptr::null_mut(); } self.video_stream_index = -1; @@ -757,9 +703,9 @@ fn setup_filter( graph_ctx: *mut AVFilterGraph, error_message: &str, ) -> Result<(), ThumbnailerError> { - let filter_name_cstr = CString::new(filter_name).unwrap(); - let filter_setup_name_cstr = CString::new(filter_setup_name).unwrap(); - let args_cstr = CString::new(args).unwrap(); + let filter_name_cstr = CString::new(filter_name).expect("CString from str"); + let filter_setup_name_cstr = CString::new(filter_setup_name).expect("CString from str"); + let args_cstr = CString::new(args).expect("CString from str"); check_error( unsafe { diff --git a/crates/ffmpeg/src/thumbnailer.rs b/crates/ffmpeg/src/thumbnailer.rs index a52d7fd6c..a10327cb4 100644 --- a/crates/ffmpeg/src/thumbnailer.rs +++ b/crates/ffmpeg/src/thumbnailer.rs @@ -2,6 +2,7 @@ use crate::{film_strip_filter, MovieDecoder, ThumbnailSize, ThumbnailerError, Vi use std::{ops::Deref, path::Path}; use tokio::{fs, task::spawn_blocking}; +use tracing::error; use webp::Encoder; /// `Thumbnailer` struct holds data from a `ThumbnailerBuilder`, exposing methods @@ -40,15 +41,22 @@ impl Thumbnailer { let quality = self.builder.quality; spawn_blocking(move || -> Result, ThumbnailerError> { - let mut decoder = MovieDecoder::new(video_file_path, prefer_embedded_metadata)?; + let mut decoder = MovieDecoder::new(video_file_path.clone(), prefer_embedded_metadata)?; // We actually have to decode a frame to get some metadata before we can start decoding for real decoder.decode_video_frame()?; if !decoder.embedded_metadata_is_available() { - decoder.seek( + let result = decoder.seek( (decoder.get_video_duration().as_secs() as f32 * seek_percentage).round() as i64, - )?; + ); + + if let Err(err) = result { + error!("Failed to seek: {err:#?}"); + // seeking failed, try the first frame again + decoder = MovieDecoder::new(video_file_path, prefer_embedded_metadata)?; + decoder.decode_video_frame()?; + } } let mut video_frame = VideoFrame::default(); diff --git a/crates/ffmpeg/src/video_frame.rs b/crates/ffmpeg/src/video_frame.rs index bceaa1e96..22764294a 100644 --- a/crates/ffmpeg/src/video_frame.rs +++ b/crates/ffmpeg/src/video_frame.rs @@ -36,7 +36,9 @@ impl FfmpegFrame { impl Drop for FfmpegFrame { fn drop(&mut self) { - unsafe { av_frame_free(&mut self.data) }; - self.data = std::ptr::null_mut(); + if !self.data.is_null() { + unsafe { av_frame_free(&mut self.data) }; + self.data = std::ptr::null_mut(); + } } }