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.

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

Packager fails to remove old media segments if both HLS and DASH is being used for LIVE segmentation #1367

Open
tamasgp opened this issue Mar 12, 2024 · 2 comments
Labels
component: DASH The issue involves DASH output component: HLS The issue involves HLS output priority: P1 Big impact or workaround impractical; resolve before feature release type: bug Something isn't working correctly
Milestone

Comments

@tamasgp
Copy link

tamasgp commented Mar 12, 2024

System info

Operating System: Ubuntu 22.04.2 LTS
Shaka Packager Version: 5ee2b7f-release

Issue and steps to reproduce the problem

Packager Command:
packager-linux-x64 in=udp://230.2.2.222:1111?interface=172.22.238.253&reuse=1,stream=audio,drm_label=AUDIO,init_segment=audio_init.mp4,segment_template=audio_$Number$.m4s,playlist_name=/mnt/life/audio.m3u8,hls_group_id=audio in=udp://230.2.2.222:1111?interface=172.22.238.253&reuse=1,stream=video,init_segment=p00_init.mp4,segment_template=p00_$Number$.m4s,playlist_name=p00.m3u8 in=udp://230.2.2.222:1112?interface=172.22.238.253&reuse=1,stream=video,init_segment=p01_init.mp4,segment_template=p01_$Number$.m4s,playlist_name=p01.m3u8 in=udp://230.2.2.222:1113?interface=172.22.238.253&reuse=1,stream=video,init_segment=p02_init.mp4,segment_template=p02_$Number$.m4s,playlist_name=p02.m3u8 --hls_master_playlist_output /mnt/life/life.m3u8 --hls_playlist_type LIVE --segment_duration 6.4 --preserved_segments_outside_live_window 2 --time_shift_buffer_depth 32 --mpd_output /mnt/life/life.mpd --suggested_presentation_delay 25

What is the expected result?

Old (outside the live window) media segments should be removed without an error.

What happens instead?

In case of using both HLS and DASH manifest generation, both formats' "RemoveOldSegment" function is being triggered. The media segments are removed by the first format's invocation, but the other format shows a warning message and keeps the segment filenames in a list (to try to remove them later). This "segments_to_be_removed_ list" will grow forever, which can lead to OOM / memory leak.

Media segments should be removed only once in a common code section, or if the file removal is being failed with a "not found" error, then the filename should be popd from the removal queue.

@joeyparrish joeyparrish added type: bug Something isn't working correctly component: HLS The issue involves HLS output priority: P1 Big impact or workaround impractical; resolve before feature release component: DASH The issue involves DASH output labels Mar 12, 2024
@github-actions github-actions bot added this to the v3.0 milestone Mar 12, 2024
@cosmin cosmin modified the milestones: v3.0, v3.1 Apr 26, 2024
@cosmin cosmin removed this from the v3.1 milestone May 7, 2024
@github-actions github-actions bot added this to the v3.2 milestone May 7, 2024
@cosmin cosmin modified the milestones: v3.2, Backlog May 12, 2024
@acris5
Copy link

acris5 commented Sep 5, 2024

This can easily be fixed with a patch:

diff --git a/packager/file/local_file.cc b/packager/file/local_file.cc
index 5ddc304e..37144e2d 100644
--- a/packager/file/local_file.cc
+++ b/packager/file/local_file.cc
@@ -139,7 +139,9 @@ bool LocalFile::Delete(const char* file_name) {
   auto file_path = std::filesystem::u8path(file_name);
   std::error_code ec;
   // On error (ec truthy), remove() will return false anyway.
-  return std::filesystem::remove(file_path, ec);
+  if (std::filesystem::exists(file_path))
+    return std::filesystem::remove(file_path, ec);
+  else return true;
 }
 
 }  // namespace shaka

@joeyparrish
Copy link
Member

@acris5, thanks for the patch. Would you be willing to make a PR?

Also, would it work to just ignore the return value of remove() instead of checking exists()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: DASH The issue involves DASH output component: HLS The issue involves HLS output priority: P1 Big impact or workaround impractical; resolve before feature release type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

4 participants