Route all shell-outs through the exec helpers; fix two CRAN check findings - #23
Conversation
Two call sites shelled out directly, bypassing R/ffmpeg.R: - .video_frame_count() (align.R) ran ffprobe -count_frames with no exit status check at all. - rms_curve() (rms.R) ran ffmpeg and discarded the status, so a missing binary or an unreadable file silently returned an empty curve instead of an error. Add .run_ffprobe() as the single ffprobe exec site and refactor .probe_field()/.probe_format_field() onto it, dropping the duplicated shQuote + status-check block. Point .video_frame_count() at it too, so a failed probe now reports the ffprobe status; the NA branch still catches a successful-but-empty read. rms_curve() now uses .run_ffmpeg() and errors on a failed pass. The empty-vector return is kept for the case it actually describes: ffmpeg ran fine but wrote no metadata. @return updated to say so. Every shell-out in the package now lives in R/ffmpeg.R.
R/frames.R documented the frame pattern as \code{"frame_\%04d.png"},
pre-escaping the percent the way roxygen2 requires. tinyrox escapes
percent itself (escape_rd() in rd.R), so the escape got applied twice
and the Rd came out as \\%04d: a literal backslash followed by a
comment character, which swallowed the rest of the line.
R CMD build was warning on it:
Warning: newline within quoted string at frames_clip.Rd:17
Warning: man/frames_clip.Rd:29: unexpected section header '\value'
Warning: man/frames_clip.Rd:45: unexpected END_OF_INPUT
Write the percent bare and let tinyrox escape it. Rd now renders
\%04d in both the description and the @param, and the build is clean.
R CMD check --as-cran flagged https://opentimeline.io/ in DESCRIPTION and README.md: it 301s to the AcademySoftwareFoundation GitHub repo, and CRAN asks that permanent redirects be replaced with the target.
.resolve_media() tested for an absolute url with startsWith(url, "/"), which only recognizes POSIX paths. On Windows the media url is C:\...\v.mp4 (or C:/...), which fails that test, so media_dir got prepended and the path came out doubled and malformed: path[1]="...\laydeg\...\laydeg\v.mp4": The filename, directory name, or volume label syntax is incorrect normalizePath(mustWork = TRUE) then errored, and render_timeline() raised instead of returning a command. R CMD check on windows-hr caught it as a hard ERROR in test_render.R; the Linux check passed because the url there is /tmp/... and matches the leading-slash test. Add .is_absolute_path(), which recognizes POSIX (/), Windows drive (C:\ or C:/), and UNC (\\) paths. It's deliberately platform-agnostic (no .Platform$file.sep): an OTIO bundle authored on one OS can be rendered on another, so the same url must classify the same way everywhere. Linux behavior is unchanged (/... still matches). Add Linux-runnable tests for .is_absolute_path() and .resolve_media() covering all three absolute forms plus the relative/join case, so this regression is caught without needing a Windows runner.
Windows re-check done, and it caught a real bugRan Not a test artifact.
Fix (035fcdc): added Clean re-check, all three environments at 0.2.0
The NOTE is "New submission" only. Also filed cornball-ai/tinyrox#31 for the double-escaped |
Real ffmpeg on windows-hr caught this: rms_curve() returned an empty curve. .rms_filter() embedded the ametadata output path as ametadata=print:...:file=<path>, and on Windows that path is a drive path (C:\...\x.txt). ffmpeg's filter-option parser splits on ':', so file=C:\... truncates at the drive colon and the metadata lands in a stray file named "C" in the working directory; the real target never appears and the curve comes back empty. Confirmed on windows-hr, and confirmed the colon-escaping that works for the subtitles *input* filter does NOT rescue the ametadata *output* option (still wrote a stray "C"). Fix: drop file= entirely. ametadata=print with no file writes to ffmpeg's stderr, which .run_ffmpeg() now returns (invisibly) so rms_curve() can parse it. No path in the filtergraph means no escaping to get wrong, and it's less code (no tempfile/on.exit/file.exists). Verified identical output on Linux and Windows: 32 windows, all finite, RMS values matching to 4 decimals. Also add a real at_home burn-in test for .subtitles_filter(): its path escaping was only string-tested, so it had never actually faced Windows ffmpeg. The burn-in confirms the filtergraph accepts an escaped drive path (subtitles='C\:/...'), which the experiment showed is the one escaping ffmpeg accepts for that filter. So both filter-path cases now have real cross-platform coverage.
Ran the actual ffmpeg tests on Windows, and they caught a second bugThe
I tested the escaping variants against real Windows ffmpeg to be sure:
Fix (c0f9b5c): drop Subtitles: was string-tested only, now burn-in tested
Full re-verification at 0.2.0Real-ffmpeg
|
1 participant
Pre-submission pass. Three things.
Every ffmpeg/ffprobe call now goes through
R/ffmpeg.RTwo call sites shelled out directly:
.video_frame_count()(align.R) ranffprobe -count_frameswith no exit status check at all.rms_curve()(rms.R) ran ffmpeg and discarded the status, so a missing binary or an unreadable file silently returned an empty curve instead of erroring.Added
.run_ffprobe()as the single ffprobe exec site and refactored.probe_field()/.probe_format_field()onto it, dropping a duplicated shQuote + status-check block..video_frame_count()now reports the ffprobe status on failure; theNAbranch still catches a successful-but-empty read.rms_curve()uses.run_ffmpeg()and errors on a failed pass. The empty-vector return is kept for the case it actually describes: ffmpeg ran fine but wrote no metadata.@returnupdated to say so.Net: two
system2()calls in the package, both inR/ffmpeg.R.Fixed broken Rd in
frames_clipR/frames.Rpre-escaped the frame pattern as\%04dthe way roxygen2 requires. tinyrox escapes%itself, so it got escaped twice and the Rd came out as\\%04d: a literal backslash followed by a comment character, swallowing the rest of the line.R CMD buildwas warning:Percent is now written bare and tinyrox escapes it. Filing this upstream on tinyrox separately.
Fixed the flagged URL
--as-cranflaggedhttps://opentimeline.io/in DESCRIPTION and README: it 301s to the AcademySoftwareFoundation GitHub repo, and CRAN asks that permanent redirects be replaced with the target.Version
0.1.0.13->0.2.0. PRs #18-#22 addedstill_clip(),frames_clip(),audio_concat(),compose_layout(),normalize_audio(), plus render_timeline stills/sequences. Feature release, and a clean 3-component version for a first CRAN submission.Checks
R CMD check --as-cran, Ubuntu 24.04, R 4.6.0: 0 errors, 0 warnings, 1 NOTE (New submission only)tinytest,at_home = TRUEso the real-exec tests run: 230/230 pass, includingtest_align.R(exercises the new.run_ffprobe),test_exec.R(shell-quoting through the helpers), andtest_rms.RWindows re-check on windows-hr still pending for 0.2.0.