diff --git a/src/main/java/net/bramp/ffmpeg/FFcommon.java b/src/main/java/net/bramp/ffmpeg/FFcommon.java index b5c9d3f3..758b49a9 100644 --- a/src/main/java/net/bramp/ffmpeg/FFcommon.java +++ b/src/main/java/net/bramp/ffmpeg/FFcommon.java @@ -72,7 +72,6 @@ protected BufferedReader wrapErrorInReader(Process p) { protected void throwOnError(Process p) throws IOException { try { - // TODO In java 8 use waitFor(long timeout, TimeUnit unit) if (ProcessUtils.waitForWithTimeout(p, 1, TimeUnit.SECONDS) != 0) { // TODO Parse the error throw new IOException(path + " returned non-zero exit status. Check stdout."); @@ -84,7 +83,6 @@ protected void throwOnError(Process p) throws IOException { protected void throwOnError(Process p, FFmpegProbeResult result) throws IOException { try { - // TODO In java 8 use waitFor(long timeout, TimeUnit unit) if (ProcessUtils.waitForWithTimeout(p, 1, TimeUnit.SECONDS) != 0) { // TODO Parse the error final FFmpegError ffmpegError = null == result ? null : result.getError(); diff --git a/src/main/java/net/bramp/ffmpeg/io/ProcessUtils.java b/src/main/java/net/bramp/ffmpeg/io/ProcessUtils.java index f51f8224..129ce42d 100644 --- a/src/main/java/net/bramp/ffmpeg/io/ProcessUtils.java +++ b/src/main/java/net/bramp/ffmpeg/io/ProcessUtils.java @@ -9,39 +9,10 @@ * @author bramp */ public final class ProcessUtils { - private ProcessUtils() { throw new AssertionError("No instances for you!"); } - private static class ProcessThread extends Thread { - final Process p; - boolean finished = false; - int exitValue = -1; - - private ProcessThread(Process p) { - this.p = p; - } - - @Override - public void run() { - try { - exitValue = p.waitFor(); - finished = true; - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - } - - public boolean hasFinished() { - return finished; - } - - public int exitValue() { - return exitValue; - } - } - /** * Waits until a process finishes or a timeout occurs * @@ -54,20 +25,17 @@ public int exitValue() { public static int waitForWithTimeout(final Process p, long timeout, TimeUnit unit) throws TimeoutException { - ProcessThread t = new ProcessThread(p); - t.start(); try { - unit.timedJoin(t, timeout); + p.waitFor(timeout, unit); } catch (InterruptedException e) { - t.interrupt(); Thread.currentThread().interrupt(); } - if (!t.hasFinished()) { + if (p.isAlive()) { throw new TimeoutException("Process did not finish within timeout"); } - return t.exitValue(); + return p.exitValue(); } } diff --git a/src/test/java/net/bramp/ffmpeg/FFprobeTest.java b/src/test/java/net/bramp/ffmpeg/FFprobeTest.java index d211dfea..efe07d38 100644 --- a/src/test/java/net/bramp/ffmpeg/FFprobeTest.java +++ b/src/test/java/net/bramp/ffmpeg/FFprobeTest.java @@ -389,8 +389,9 @@ public void testProbeDivideByZero() throws IOException { } @Test - public void shouldThrowOnErrorWithFFmpegProbeResult() throws InterruptedException { - Mockito.when(mockProcess.waitFor()).thenReturn(-1); + public void shouldThrowOnErrorWithFFmpegProbeResult() { + Mockito.doReturn(1).when(mockProcess).exitValue(); + final FFmpegError error = new FFmpegError(); final FFmpegProbeResult result = new FFmpegProbeResult(); result.error = error; @@ -398,6 +399,36 @@ public void shouldThrowOnErrorWithFFmpegProbeResult() throws InterruptedExceptio assertEquals(error, e.getError()); } + @Test + public void shouldThrowOnErrorEvenIfProbeResultHasNoError() { + Mockito.doReturn(1).when(mockProcess).exitValue(); + + final FFmpegProbeResult result = new FFmpegProbeResult(); + FFmpegException e = assertThrows(FFmpegException.class, () -> ffprobe.throwOnError(mockProcess, result)); + assertNull(e.getError()); + } + + @Test + public void shouldThrowOnErrorEvenIfProbeResultIsNull() { + Mockito.doReturn(1).when(mockProcess).exitValue(); + + FFmpegException e = assertThrows(FFmpegException.class, () -> ffprobe.throwOnError(mockProcess, null)); + assertNull(e.getError()); + } + + @Test + public void testShouldThrowErrorWithoutMock() throws IOException { + FFprobe probe = new FFprobe(); + FFmpegException e = assertThrows(FFmpegException.class, () -> probe.probe("doesnotexist.mp4")); + + assertNotNull(e); + assertNotNull(e.getError()); + + // Intentionally not comparing the values, as those might change for different ffmpeg versions + assertNotNull(e.getError().getString()); + assertNotEquals(0, e.getError().getCode()); + } + @Test public void testProbeSideDataList() throws IOException { FFmpegProbeResult info = ffprobe.probe(Samples.side_data_list); diff --git a/src/test/java/net/bramp/ffmpeg/io/ProcessUtilsTest.java b/src/test/java/net/bramp/ffmpeg/io/ProcessUtilsTest.java new file mode 100644 index 00000000..60b94bf8 --- /dev/null +++ b/src/test/java/net/bramp/ffmpeg/io/ProcessUtilsTest.java @@ -0,0 +1,37 @@ +package net.bramp.ffmpeg.io; + +import net.bramp.ffmpeg.FFmpeg; +import org.junit.Test; + +import java.io.IOException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; + +public class ProcessUtilsTest { + @Test + public void testProcessFinishesBeforeTimeout() throws Exception { + String ffmpeg = new FFmpeg().getPath(); + ProcessBuilder processBuilder = new ProcessBuilder(ffmpeg, "-y", "-v", "quiet", "-f", "lavfi", "-i", "testsrc=duration=1:size=1280x720:rate=10", "-c:v", "libx264", "-t", "1", "output.mp4"); + Process process = processBuilder.start(); + + int exitValue = ProcessUtils.waitForWithTimeout(process, 5, TimeUnit.SECONDS); + + assertEquals(0, exitValue); + } + + @Test + public void testProcessDoesNotFinishBeforeTimeout() throws IOException { + String ffmpeg = new FFmpeg().getPath(); + ProcessBuilder processBuilder = new ProcessBuilder(ffmpeg, "-y", "-v", "quiet", "-f", "lavfi", "-i", "testsrc=duration=10:size=1280x720:rate=30", "-c:v", "libx264", "-t", "10", "output.mp4"); + Process process = processBuilder.start(); + + assertThrows(TimeoutException.class, () -> { + ProcessUtils.waitForWithTimeout(process, 1, TimeUnit.MILLISECONDS); + }); + + process.destroy(); + } +}