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

Develop to Master #363

Merged
merged 12 commits into from
Sep 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions .github/workflows/codeql-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,24 +40,24 @@ jobs:

steps:
- name: Checkout repository
uses: actions/checkout@v2
uses: actions/checkout@v4

- uses: actions/setup-java@v2
- uses: actions/setup-java@v3
with:
distribution: 'adopt'
java-version: 8
java-version: 17

# Initializes the CodeQL tools for scanning.
- name: Initialize CodeQL
uses: github/codeql-action/init@v1
uses: github/codeql-action/init@v2
with:
languages: ${{ matrix.language }}
# If you wish to specify custom queries, you can do so here or in a config file.
# By default, queries listed here will override any specified in a config file.
# Prefix the list here with "+" to use these queries and those in the config file.
# queries: ./path/to/local/query, your-org/your-repo/queries@main

- uses: actions/cache@v2
- uses: actions/cache@v3
with:
# be careful not to include ~/.m2/settings.xml which contains credentials
path: |
Expand All @@ -70,4 +70,4 @@ jobs:
run: bash mvnw clean install -B -DskipTests

- name: Perform CodeQL Analysis
uses: github/codeql-action/analyze@v1
uses: github/codeql-action/analyze@v2
6 changes: 3 additions & 3 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v4

- uses: actions/setup-java@v2
- uses: actions/setup-java@v3
with:
distribution: 'adopt'
java-version: 11

- uses: actions/cache@v2
- uses: actions/cache@v3
with:
# be careful not to include ~/.m2/settings.xml which contains credentials
path: |
Expand Down
13 changes: 7 additions & 6 deletions .github/workflows/sonar-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,26 +26,27 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v4
with:
fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis

- name: Set up JDK 11
uses: actions/setup-java@v1
- name: Set up JDK
uses: actions/setup-java@v3
with:
java-version: 11
distribution: 'adopt'
java-version: 17

- name: Install ffmpeg on Ubuntu
run: sudo apt-get update && sudo apt-get install -y ffmpeg && ffmpeg -version

- name: Cache SonarCloud packages
uses: actions/cache@v1
uses: actions/cache@v3
with:
path: ~/.sonar/cache
key: ${{ runner.os }}-sonar
restore-keys: ${{ runner.os }}-sonar

- uses: actions/cache@v2
- uses: actions/cache@v3
with:
# be careful not to include ~/.m2/settings.xml which contains credentials
path: |
Expand Down
14 changes: 7 additions & 7 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,16 @@ jobs:
fail-fast: false

steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v4

- name: Set up JDK
uses: actions/setup-java@v2
uses: actions/setup-java@v3
with:
distribution: 'adopt'
java-version: ${{ matrix.java-version }}

- name: Cache Maven Packages
uses: actions/cache@v2
uses: actions/cache@v3
with:
# be careful not to include ~/.m2/settings.xml which contains credentials
path: |
Expand All @@ -49,7 +49,7 @@ jobs:
restore-keys: ${{ runner.os }}-m2

- name: Cache Test Artifacts
uses: actions/cache@v2
uses: actions/cache@v3
with:
path: |
.artifacts
Expand Down Expand Up @@ -82,18 +82,18 @@ jobs:
test-release:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v4

- name: Set up JDK
uses: actions/setup-java@v2
uses: actions/setup-java@v3
with:
distribution: 'adopt'
java-version: 11

- name: Set up RANDOM GPG key
run: gpg --quick-generate-key --batch --passphrase '' test42

- uses: actions/cache@v2
- uses: actions/cache@v3
with:
# be careful not to include ~/.m2/settings.xml which contains credentials
path: |
Expand Down
8 changes: 4 additions & 4 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.3.1</version>
<version>3.5.0</version>
<configuration>
<source>8</source>
</configuration>
Expand Down Expand Up @@ -194,7 +194,7 @@
<plugin>
<groupId>org.sonatype.plugins</groupId>
<artifactId>nexus-staging-maven-plugin</artifactId>
<version>1.6.8</version>
<version>1.6.13</version>
<extensions>true</extensions>
<configuration>
<serverId>ossrh</serverId>
Expand All @@ -212,7 +212,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.8.1</version>
<version>3.11.0</version>
<configuration>
<source>8</source>
<target>8</target>
Expand Down Expand Up @@ -251,7 +251,7 @@
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>9.2.1</version>
<version>10.12.3</version>
</dependency>
</dependencies>
<executions>
Expand Down
36 changes: 31 additions & 5 deletions src/main/java/com/github/kokorin/jaffree/ffmpeg/FFmpeg.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public class FFmpeg {

private LogLevel logLevel = LogLevel.INFO;
private String contextName = null;
private Integer executorTimeoutMillis = null;

private final Path executable;

Expand Down Expand Up @@ -387,6 +388,26 @@ public FFmpeg setContextName(final String contextName) {
return this;
}

/**
* Overrides the default {@link com.github.kokorin.jaffree.process.Executor} timeout.
* <p>
* Most normal use cases will easily complete within the default timeout. It is not recommended
* to set an explicit timeout value unless you have actually experienced unwanted timeouts.
* <p>
* A value of 0 will disable the timeout.
* That is, Jaffree will wait indefinitely for the Executor to complete.
*
* @param executorTimeoutMillis the custom executor timeout in milliseconds
* @return this
*/
public FFmpeg setExecutorTimeoutMillis(final int executorTimeoutMillis) {
if (executorTimeoutMillis < 0) {
throw new IllegalArgumentException("Executor timeout cannot be negative");
}
this.executorTimeoutMillis = executorTimeoutMillis;
return this;
}

/**
* Starts synchronous ffmpeg execution.
* <p>
Expand Down Expand Up @@ -480,11 +501,16 @@ protected ProcessHandler<FFmpegResult> createProcessHandler() {
helpers.add(progressHelper);
}

return new ProcessHandler<FFmpegResult>(executable, contextName)
.setStdErrReader(createStdErrReader(outputListener))
.setStdOutReader(createStdOutReader())
.setHelpers(helpers)
.setArguments(buildArguments());
ProcessHandler<FFmpegResult> processHandler =
new ProcessHandler<FFmpegResult>(executable, contextName)
.setStdErrReader(createStdErrReader(outputListener))
.setStdOutReader(createStdOutReader())
.setHelpers(helpers)
.setArguments(buildArguments());
if (executorTimeoutMillis != null) {
processHandler.setExecutorTimeoutMillis(executorTimeoutMillis);
}
return processHandler;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ public class ProcessHandler<T> {
private List<ProcessHelper> helpers = null;
private Stopper stopper = null;
private List<String> arguments = Collections.emptyList();
private int executorTimeoutMillis = DEFAULT_EXECUTOR_TIMEOUT_MILLIS;

private static final int EXECUTOR_TIMEOUT_MILLIS = 10_000;
private static final int DEFAULT_EXECUTOR_TIMEOUT_MILLIS = 10_000;
private static final Logger LOGGER = LoggerFactory.getLogger(ProcessHandler.class);

/**
Expand Down Expand Up @@ -120,6 +121,20 @@ public synchronized ProcessHandler<T> setArguments(final List<String> arguments)
return this;
}

/**
* Overrides the default Executor timeout.
* <p>
* A value of 0 is interpreted as "wait indefinitely".
*
* @param executorTimeoutMillis the new Executor timeout in milliseconds
*/
public void setExecutorTimeoutMillis(final int executorTimeoutMillis) {
if (executorTimeoutMillis < 0) {
throw new IllegalArgumentException("Executor timeout cannot be negative");
}
this.executorTimeoutMillis = executorTimeoutMillis;
}

/**
* Executes a program.
* <p>
Expand Down Expand Up @@ -172,15 +187,15 @@ public synchronized T execute() {
*/
protected T interactWithProcess(final Process process) {
AtomicReference<T> resultRef = new AtomicReference<>();
Integer status = null;
final int status;
Executor executor = startExecution(process, resultRef);

try {
LOGGER.info("Waiting for process to finish");
status = process.waitFor();
LOGGER.info("Process has finished with status: {}", status);

waitForExecutorToStop(executor, EXECUTOR_TIMEOUT_MILLIS);
waitForExecutorToStop(executor, executorTimeoutMillis);
} catch (InterruptedException e) {
LOGGER.warn("Process has been interrupted");
if (stopper != null) {
Expand All @@ -198,7 +213,7 @@ protected T interactWithProcess(final Process process) {
"Failed to execute, exception appeared in one of helper threads", exceptions);
}

if (!Integer.valueOf(0).equals(status)) {
if (status != 0) {
throw new JaffreeAbnormalExitException(
"Process execution has ended with non-zero status: " + status
+ ". Check logs for detailed error message.",
Expand Down Expand Up @@ -308,9 +323,10 @@ private static void waitForExecutorToStop(final Executor executor, final long ti
throws InterruptedException {
LOGGER.debug("Waiting for Executor to stop");

long waitStarted = System.currentTimeMillis();
final long waitStarted = System.currentTimeMillis();
do {
if (System.currentTimeMillis() - waitStarted > timeoutMillis) {
// Zero timeout means "wait indefinitely"
if (timeoutMillis > 0 && System.currentTimeMillis() - waitStarted > timeoutMillis) {
LOGGER.warn("Executor hasn't stopped in {} millis, won't wait longer",
timeoutMillis);
break;
Expand Down
34 changes: 25 additions & 9 deletions src/test/java/com/github/kokorin/jaffree/ffmpeg/FFmpegTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.hamcrest.core.AllOf;
import org.hamcrest.core.StringContains;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -379,16 +380,31 @@ public void testSizeLimit() throws Exception {
Path tempDir = Files.createTempDirectory("jaffree");
Path outputPath = tempDir.resolve(Artifacts.VIDEO_MP4.getFileName());

FFmpegResult result = FFmpeg.atPath(Config.FFMPEG_BIN)
.addInput(UrlInput.fromPath(Artifacts.VIDEO_MP4))
.addOutput(UrlOutput
.toPath(outputPath)
.copyAllCodecs()
.setSizeLimit(1_000_000L)
)
.execute();
final AtomicBoolean muxingErrorDetected = new AtomicBoolean(false);
OutputListener outputListener = message -> {
if (message.endsWith("Error muxing a packet")) {
LOGGER.warn("Detected a muxing error, which indicates ffmpeg bug #10327");
muxingErrorDetected.set(true);
}
};

Assert.assertNotNull(result);
try {
FFmpegResult result = FFmpeg.atPath(Config.FFMPEG_BIN)
.addInput(UrlInput.fromPath(Artifacts.VIDEO_MP4))
.setOutputListener(outputListener)
.addOutput(UrlOutput
.toPath(outputPath)
.copyAllCodecs()
.setSizeLimit(1_000_000L)
)
.execute();
Assert.assertNotNull(result);
} catch (JaffreeAbnormalExitException ex) {
// Detect ffmpeg bug "Error muxing a packet when limit file size parameter is set"
// https://trac.ffmpeg.org/ticket/10327
Assume.assumeFalse("Hit ffmpeg bug #10327 - we will ignore this test", muxingErrorDetected.get());
Assert.fail("Abnormal exit for limit file size");
}

long outputSize = Files.size(outputPath);
assertTrue(outputSize > 900_000);
Expand Down