Software-Mac/spacedrive
Software-Mac/spacedrive
1
0
Fork 0
mirror of https://github.com/spacedriveapp/spacedrive synced 2024-07-04 11:03:27 +00:00
Code Issues Packages Projects Releases Wiki Activity

[ENG-866, ENG-535] Thumbnailer fixes (double-free, thumbnails for portrait videos) (#1059)

Browse source 
* 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>
This commit is contained in:
Vítor Vasconcellos 2023-06-30 08:00:04 -03:00 committed by GitHub
parent a04da012ed
commit caaacc8f92
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 77 additions and 111 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
Cargo.lock generated
View file

@ -7167,6 +7167,7 @@ dependencies = [
"tempfile",
"thiserror",
"tokio",
"tracing 0.1.37",
"webp",
]

14
apps/desktop/crates/macos/build.rs
View file

@ -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();
}
}

1
crates/ffmpeg/Cargo.toml
View file

@ -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"

152
crates/ffmpeg/src/movie_decoder.rs
View file

@ -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 {

14
crates/ffmpeg/src/thumbnailer.rs
View file

@ -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<Vec<u8>, 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();

6
crates/ffmpeg/src/video_frame.rs
View file

@ -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();
}
}
}