From c4dd08dff0c28fe0d3235f4b755d6dda382b7ad8 Mon Sep 17 00:00:00 2001
From: Alexander Plavin <alexander@plav.in>
Date: Fri, 9 Aug 2024 10:16:45 +0200
Subject: [PATCH] don't remove underlying VideoStream file when doing save()
 (#3883)

* don't remove underlying VideoStream file when doing save()

* remove VideoStream underlying file when cleaning the object

* Update CHANGELOG.md

---------

Co-authored-by: Simon <sdanisch@protonmail.com>
---
 CHANGELOG.md                |  1 +
 CairoMakie/test/runtests.jl | 14 ++++++++++++--
 src/ffmpeg-util.jl          |  9 ++++++---
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cc2431bf53f..10f7595744c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## [Unreleased]
 
+- Don't remove underlying VideoStream file when doing save() [#3883](https://github.com/MakieOrg/Makie.jl/pull/3883).
 - Fix label/legend for plotlist [#4079](https://github.com/MakieOrg/Makie.jl/pull/4079).
 - Fix wrong order for colors in RPRMakie [#4098](https://github.com/MakieOrg/Makie.jl/pull/4098).
 - Fixed incorrect distance calculation in `pick_closest` in WGLMakie [#4082](https://github.com/MakieOrg/Makie.jl/pull/4082).
diff --git a/CairoMakie/test/runtests.jl b/CairoMakie/test/runtests.jl
index 38dfce23287..cbff42c9098 100644
--- a/CairoMakie/test/runtests.jl
+++ b/CairoMakie/test/runtests.jl
@@ -123,15 +123,25 @@ end
     N = 3
     points = Observable(Point2f[])
     f, ax, pl = scatter(points, axis=(type=Axis, aspect=DataAspect(), limits=(0.4, N + 0.6, 0.4, N + 0.6),), figure=(size=(600, 800),))
+
     vio = Makie.VideoStream(f; format="mp4", px_per_unit=2.0, backend=CairoMakie)
+    tmp_path = vio.path
+
     @test vio.screen isa CairoMakie.Screen{CairoMakie.IMAGE}
     @test size(vio.screen) == size(f.scene) .* 2
     @test vio.screen.device_scaling_factor == 2.0
 
     Makie.recordframe!(vio)
     save("test.mp4", vio)
-    @test isfile("test.mp4") # Make sure no error etc
-    rm("test.mp4")
+    save("test_2.mkv", vio)
+    save("test_3.mp4", vio)
+    # make sure all files are correctly saved:
+    @test all(isfile, ["test.mp4", "test_2.mkv", "test_3.mp4"])
+    @test filesize("test.mp4") == filesize("test_3.mp4") > 3000
+    @test filesize("test.mp4") != filesize("test_2.mkv") > 3000
+    rm.(["test.mp4", "test_2.mkv", "test_3.mp4"])
+    finalize(vio); yield()
+    @test !isfile(tmp_path)
 end
 
 @testset "plotlist no ambiguity (#4038)" begin
diff --git a/src/ffmpeg-util.jl b/src/ffmpeg-util.jl
index ec280481b85..322c09d4406 100644
--- a/src/ffmpeg-util.jl
+++ b/src/ffmpeg-util.jl
@@ -178,7 +178,7 @@ function to_ffmpeg_cmd(vso::VideoStreamOptions, xdim::Integer=0, ydim::Integer=0
 end
 
 
-struct VideoStream
+mutable struct VideoStream
     io::Base.PipeEndpoint
     process::Base.Process
     screen::MakieScreen
@@ -229,7 +229,11 @@ function VideoStream(fig::FigureLike;
     vso = VideoStreamOptions(format, framerate, compression, profile, pixel_format, loop, loglevel, "pipe:0", true)
     cmd = to_ffmpeg_cmd(vso, xdim, ydim)
     process = open(`$(FFMPEG_jll.ffmpeg()) $cmd $path`, "w")
-    return VideoStream(process.in, process, screen, buffer, abspath(path), vso)
+    result = VideoStream(process.in, process, screen, buffer, abspath(path), vso)
+    finalizer(result) do x
+        @async rm(x.path; force=true)
+    end
+    return result
 end
 
 """
@@ -267,7 +271,6 @@ function save(path::String, io::VideoStream; video_options...)
     else
         cp(io.path, path; force=true)
     end
-    rm(io.path)
     return path
 end