From b9ea572e19d3fdc4ebf5f8bc9f8c8bb9ccfe621b Mon Sep 17 00:00:00 2001 From: Jonas Herzig Date: Sat, 12 Aug 2017 11:34:06 +0200 Subject: [PATCH] Try to handle invalid ffmpeg arguments more gracefully (fixes #77) Instead of outright crashing, if the user has modified the ffmpeg arguments, they are given the chance to re-try with the defaults. --- .../com/replaymod/render/VideoWriter.java | 51 +++++++- .../replaymod/render/gui/GuiExportFailed.java | 109 ++++++++++++++++++ .../replaymod/render/gui/GuiRenderQueue.java | 66 +++++++---- .../render/gui/GuiRenderSettings.java | 22 ++-- .../render/rendering/VideoRenderer.java | 34 +++++- src/main/resources/assets/replaymod/lang | 2 +- 6 files changed, 244 insertions(+), 40 deletions(-) create mode 100644 src/main/java/com/replaymod/render/gui/GuiExportFailed.java diff --git a/src/main/java/com/replaymod/render/VideoWriter.java b/src/main/java/com/replaymod/render/VideoWriter.java index 4fca0f4f..08ddb9e4 100755 --- a/src/main/java/com/replaymod/render/VideoWriter.java +++ b/src/main/java/com/replaymod/render/VideoWriter.java @@ -2,6 +2,7 @@ package com.replaymod.render; import com.replaymod.render.frame.RGBFrame; import com.replaymod.render.rendering.FrameConsumer; +import com.replaymod.render.rendering.VideoRenderer; import com.replaymod.render.utils.ByteBufferPool; import com.replaymod.render.utils.StreamPipe; import net.minecraft.client.Minecraft; @@ -32,6 +33,7 @@ import static org.apache.commons.lang3.Validate.isTrue; public class VideoWriter implements FrameConsumer { + private final VideoRenderer renderer; private final RenderSettings settings; private final Process process; private final OutputStream outputStream; @@ -41,8 +43,9 @@ public class VideoWriter implements FrameConsumer { private ByteArrayOutputStream ffmpegLog = new ByteArrayOutputStream(4096); - public VideoWriter(final RenderSettings settings) throws IOException { - this.settings = settings; + public VideoWriter(final VideoRenderer renderer) throws IOException { + this.renderer = renderer; + this.settings = renderer.getRenderSettings(); File outputFolder = settings.getOutputFile().getParentFile(); FileUtils.forceMkdir(outputFolder); @@ -59,7 +62,11 @@ public class VideoWriter implements FrameConsumer { String executable = settings.getExportCommand().isEmpty() ? findFFmpeg() : settings.getExportCommand(); LOGGER.info("Starting {} with args: {}", executable, commandArgs); String[] cmdline = new CommandLine(executable).addArguments(commandArgs).toStrings(); - process = new ProcessBuilder(cmdline).directory(outputFolder).start(); + try { + process = new ProcessBuilder(cmdline).directory(outputFolder).start(); + } catch (IOException e) { + throw new NoFFmpegException(e); + } File exportLogFile = new File(Minecraft.getMinecraft().mcDataDir, "export.log"); OutputStream exportLogOut = new TeeOutputStream(new FileOutputStream(exportLogFile), ffmpegLog); new StreamPipe(process.getInputStream(), exportLogOut).start(); @@ -152,6 +159,16 @@ public class VideoWriter implements FrameConsumer { if (aborted) { return; } + try { + // Check whether this is a failure right at the beginning of the rendering process + // or at some later point (ffmpeg won't print the output file until the first frame + // has been written to stdin, so we can't already check for invalid args in ). + getVideoFile(); + } catch (FFmpegStartupException e) { + // Possibly invalid ffmpeg arguments + renderer.setFailure(e); + return; + } CrashReport report = CrashReport.makeCrashReport(t, "Exporting frame"); CrashReportCategory exportDetails = report.makeCategory("Export details"); exportDetails.addCrashSection("Export command", settings.getExportCommand()); @@ -173,7 +190,7 @@ public class VideoWriter implements FrameConsumer { aborted = true; } - public File getVideoFile() { + public File getVideoFile() throws FFmpegStartupException { String log = ffmpegLog.toString(); for (String line : log.split("\n")) { if (line.startsWith("Output #0")) { @@ -181,6 +198,30 @@ public class VideoWriter implements FrameConsumer { return new File(settings.getOutputFile().getParentFile(), fileName); } } - throw new IllegalStateException("No output file found."); + throw new FFmpegStartupException(settings, log); + } + + public static class NoFFmpegException extends IOException { + public NoFFmpegException(Throwable cause) { + super(cause); + } + } + + public static class FFmpegStartupException extends IOException { + private final RenderSettings settings; + private final String log; + + public FFmpegStartupException(RenderSettings settings, String log) { + this.settings = settings; + this.log = log; + } + + public RenderSettings getSettings() { + return settings; + } + + public String getLog() { + return log; + } } } diff --git a/src/main/java/com/replaymod/render/gui/GuiExportFailed.java b/src/main/java/com/replaymod/render/gui/GuiExportFailed.java new file mode 100644 index 00000000..b86e7118 --- /dev/null +++ b/src/main/java/com/replaymod/render/gui/GuiExportFailed.java @@ -0,0 +1,109 @@ +package com.replaymod.render.gui; + +import com.replaymod.render.RenderSettings; +import com.replaymod.render.VideoWriter; +import de.johni0702.minecraft.gui.container.GuiPanel; +import de.johni0702.minecraft.gui.container.GuiScreen; +import de.johni0702.minecraft.gui.container.GuiVerticalList; +import de.johni0702.minecraft.gui.element.GuiButton; +import de.johni0702.minecraft.gui.element.GuiElement; +import de.johni0702.minecraft.gui.element.GuiLabel; +import de.johni0702.minecraft.gui.layout.CustomLayout; +import de.johni0702.minecraft.gui.layout.HorizontalLayout; +import de.johni0702.minecraft.gui.layout.VerticalLayout; +import net.minecraft.crash.CrashReport; +import net.minecraft.crash.CrashReportCategory; +import net.minecraft.util.ReportedException; + +import java.util.Arrays; +import java.util.function.Consumer; + +import static com.replaymod.render.ReplayModRender.LOGGER; + +public class GuiExportFailed extends GuiScreen { + public static GuiExportFailed tryToRecover(VideoWriter.FFmpegStartupException e, Consumer doRestart) { + // Always log the error first + LOGGER.error("Rendering video:", e); + + RenderSettings settings = e.getSettings(); + // Check whether the user has configured some custom ffmpeg arguments + if (settings.getEncodingPreset().getValue().equals(settings.getExportArguments())) { + // If they haven't, then this is probably a faulty ffmpeg installation and there's nothing we can do + CrashReport crashReport = CrashReport.makeCrashReport(e, "Exporting video"); + CrashReportCategory details = crashReport.makeCategory("Export details"); + details.addCrashSection("Settings", settings); + details.addCrashSection("FFmpeg log", e.getLog()); + throw new ReportedException(crashReport); + } else { + // If they have, ask them whether it was intentional + GuiExportFailed gui = new GuiExportFailed(e, doRestart); + gui.display(); + return gui; + } + } + + private final GuiLabel logLabel = new GuiLabel(this) + .setI18nText("replaymod.gui.rendering.error.ffmpeglog"); + private final GuiVerticalList logList = new GuiVerticalList(this).setDrawShadow(true); + private final GuiButton resetButton = new GuiButton().setI18nLabel("gui.yes").setSize(100, 20); + private final GuiButton abortButton = new GuiButton().setI18nLabel("gui.no").setSize(100, 20); + private final GuiPanel info = new GuiPanel(this) + .setLayout(new VerticalLayout().setSpacing(4)) + .addElements(new VerticalLayout.Data(0.5), + new GuiLabel().setI18nText("replaymod.gui.rendering.error.ffmpegargs.1"), + new GuiLabel().setI18nText("replaymod.gui.rendering.error.ffmpegargs.2"), + new GuiLabel(), + new GuiPanel().setLayout(new HorizontalLayout(HorizontalLayout.Alignment.CENTER).setSpacing(5)) + .addElements(null, resetButton, abortButton) + ); + + { + setLayout(new CustomLayout() { + @Override + protected void layout(GuiScreen container, int width, int height) { + pos(info, width/2 - width(info)/2, (height/2 - height(info) - 30) / 2 + 30); + pos(logLabel, width/2 - width(logLabel)/2, height/2 + 4); + pos(logList, 10, y(logLabel) + height(logLabel) + 4); + size(logList, width - 10 - x(logList), height - 10 - y(logList)); + } + }); + + setTitle(new GuiLabel().setI18nText("replaymod.gui.rendering.error.title")); + setBackground(Background.DIRT); + } + + public GuiExportFailed(VideoWriter.FFmpegStartupException e, Consumer doRestart) { + logList.getListPanel().addElements(null, + Arrays.stream(e.getLog().replace("\t", " ").split("\n")) + .map(l -> new GuiLabel().setText(l)) + .toArray(GuiElement[]::new)); + + resetButton.onClick(() -> { + RenderSettings oldSettings = e.getSettings(); + doRestart.accept(new RenderSettings( + oldSettings.getRenderMethod(), + oldSettings.getEncodingPreset(), + oldSettings.getVideoWidth(), + oldSettings.getVideoHeight(), + oldSettings.getFramesPerSecond(), + oldSettings.getBitRate(), + oldSettings.getOutputFile(), + oldSettings.isRenderNameTags(), + oldSettings.isStabilizeYaw(), + oldSettings.isStabilizePitch(), + oldSettings.isStabilizeRoll(), + oldSettings.getChromaKeyingColor(), + oldSettings.isInject360Metadata(), + oldSettings.getAntiAliasing(), + oldSettings.getExportCommand(), + oldSettings.getEncodingPreset().getValue(), + oldSettings.isHighPerformance() + )); + }); + + abortButton.onClick(() -> { + // Assume they know what they're doing + getMinecraft().displayGuiScreen(null); + }); + } +} diff --git a/src/main/java/com/replaymod/render/gui/GuiRenderQueue.java b/src/main/java/com/replaymod/render/gui/GuiRenderQueue.java index 9a96835b..7801479f 100644 --- a/src/main/java/com/replaymod/render/gui/GuiRenderQueue.java +++ b/src/main/java/com/replaymod/render/gui/GuiRenderQueue.java @@ -1,8 +1,11 @@ package com.replaymod.render.gui; +import com.google.common.collect.Iterables; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; +import com.replaymod.core.utils.Utils; import com.replaymod.render.ReplayModRender; +import com.replaymod.render.VideoWriter; import com.replaymod.render.rendering.VideoRenderer; import com.replaymod.render.utils.RenderJob; import com.replaymod.replay.ReplayHandler; @@ -11,6 +14,7 @@ import com.replaymod.replaystudio.util.I18n; import de.johni0702.minecraft.gui.GuiRenderer; import de.johni0702.minecraft.gui.RenderInfo; import de.johni0702.minecraft.gui.container.AbstractGuiClickableContainer; +import de.johni0702.minecraft.gui.container.AbstractGuiScreen; import de.johni0702.minecraft.gui.container.GuiContainer; import de.johni0702.minecraft.gui.container.GuiPanel; import de.johni0702.minecraft.gui.container.GuiVerticalList; @@ -25,12 +29,10 @@ import de.johni0702.minecraft.gui.popup.GuiYesNoPopup; import de.johni0702.minecraft.gui.utils.Colors; import net.minecraft.client.gui.GuiErrorScreen; import net.minecraft.crash.CrashReport; -import net.minecraft.util.ReportedException; import org.lwjgl.util.Dimension; import org.lwjgl.util.ReadableDimension; import javax.annotation.Nullable; -import java.io.IOException; import java.util.List; import static com.replaymod.render.ReplayModRender.LOGGER; @@ -62,7 +64,8 @@ public class GuiRenderQueue extends AbstractGuiPopup { renameButton, removeButton), closeButton); - private final GuiContainer container; + private final AbstractGuiScreen container; + private final ReplayHandler replayHandler; private Entry selectedEntry; { @@ -84,9 +87,10 @@ public class GuiRenderQueue extends AbstractGuiPopup { }).addElements(null, title, list, buttonPanel); } - public GuiRenderQueue(GuiContainer container, GuiRenderSettings guiRenderSettings, ReplayHandler replayHandler, Timeline timeline) { + public GuiRenderQueue(AbstractGuiScreen container, GuiRenderSettings guiRenderSettings, ReplayHandler replayHandler, Timeline timeline) { super(container); this.container = container; + this.replayHandler = replayHandler; LOGGER.trace("Opening render queue popup"); setBackgroundColor(Colors.DARK_TRANSPARENT); @@ -188,30 +192,46 @@ public class GuiRenderQueue extends AbstractGuiPopup { renderButton.onClick(() -> { LOGGER.trace("Render button clicked"); - // Close all GUIs (so settings in GuiRenderSettings are saved) - getMinecraft().displayGuiScreen(null); - // Start rendering - for (RenderJob renderJob : queue) { - LOGGER.info("Starting render job {}", renderJob); - try { - VideoRenderer videoRenderer = new VideoRenderer(renderJob.getSettings(), replayHandler, renderJob.getTimeline()); - videoRenderer.renderVideo(); - } catch (IOException e) { - LOGGER.error("Rendering video:", e); - GuiErrorScreen errorScreen = new GuiErrorScreen(I18n.format("replaymod.gui.rendering.error.title"), - I18n.format("replaymod.gui.rendering.error.message")); - getMinecraft().displayGuiScreen(errorScreen); - return; - } catch (Throwable t) { - CrashReport crashReport = CrashReport.makeCrashReport(t, "Rendering video"); - throw new ReportedException(crashReport); - } - } + processQueue(queue); }); updateButtons(); } + private void processQueue(Iterable queue) { + // Close all GUIs (so settings in GuiRenderSettings are saved) + getMinecraft().displayGuiScreen(null); + // Start rendering + int jobsDone = 0; + for (RenderJob renderJob : queue) { + LOGGER.info("Starting render job {}", renderJob); + try { + VideoRenderer videoRenderer = new VideoRenderer(renderJob.getSettings(), replayHandler, renderJob.getTimeline()); + videoRenderer.renderVideo(); + } catch (VideoWriter.NoFFmpegException e) { + LOGGER.error("Rendering video:", e); + GuiErrorScreen errorScreen = new GuiErrorScreen(I18n.format("replaymod.gui.rendering.error.title"), + I18n.format("replaymod.gui.rendering.error.message")); + getMinecraft().displayGuiScreen(errorScreen); + return; + } catch (VideoWriter.FFmpegStartupException e) { + int jobsToSkip = jobsDone; + GuiExportFailed.tryToRecover(e, newSettings -> { + // Update current job with fixed ffmpeg arguments + renderJob.setSettings(newSettings); + // Restart queue, skipping the already completed jobs + processQueue(Iterables.skip(queue, jobsToSkip)); + }); + return; + } catch (Throwable t) { + Utils.error(LOGGER, this, CrashReport.makeCrashReport(t, "Rendering video"), () -> {}); + container.display(); // Re-show the queue popup and the new error popup + return; + } + jobsDone++; + } + } + @Override public void open() { super.open(); diff --git a/src/main/java/com/replaymod/render/gui/GuiRenderSettings.java b/src/main/java/com/replaymod/render/gui/GuiRenderSettings.java index 3c2faa88..f712dd0d 100644 --- a/src/main/java/com/replaymod/render/gui/GuiRenderSettings.java +++ b/src/main/java/com/replaymod/render/gui/GuiRenderSettings.java @@ -7,6 +7,7 @@ import com.google.gson.GsonBuilder; import com.google.gson.InstanceCreator; import com.replaymod.render.RenderSettings; import com.replaymod.render.ReplayModRender; +import com.replaymod.render.VideoWriter; import com.replaymod.render.rendering.VideoRenderer; import com.replaymod.replay.ReplayHandler; import com.replaymod.replaystudio.pathing.path.Timeline; @@ -29,7 +30,6 @@ import de.johni0702.minecraft.gui.utils.Utils; import net.minecraft.client.gui.GuiErrorScreen; import net.minecraft.client.resources.I18n; import net.minecraft.crash.CrashReport; -import net.minecraft.util.ReportedException; import net.minecraftforge.common.config.Configuration; import net.minecraftforge.common.config.Property; import org.lwjgl.util.Color; @@ -39,11 +39,13 @@ import org.lwjgl.util.ReadableDimension; import javax.annotation.Nullable; import java.io.File; -import java.io.IOException; import java.text.SimpleDateFormat; import java.util.Date; import java.util.Map; +import static com.replaymod.core.utils.Utils.error; +import static com.replaymod.render.ReplayModRender.LOGGER; + public class GuiRenderSettings extends GuiScreen implements Closeable { public final GuiPanel contentPanel = new GuiPanel(this).setBackgroundColor(Colors.DARK_TRANSPARENT); public final GuiVerticalList settingsList = new GuiVerticalList(contentPanel).setDrawSlider(true); @@ -199,15 +201,21 @@ public class GuiRenderSettings extends GuiScreen implements Closeable { try { VideoRenderer videoRenderer = new VideoRenderer(save(false), replayHandler, timeline); videoRenderer.renderVideo(); - } catch (IOException e) { - e.printStackTrace(); - + } catch (VideoWriter.NoFFmpegException e) { + LOGGER.error("Rendering video:", e); GuiErrorScreen errorScreen = new GuiErrorScreen(I18n.format("replaymod.gui.rendering.error.title"), I18n.format("replaymod.gui.rendering.error.message")); getMinecraft().displayGuiScreen(errorScreen); + } catch (VideoWriter.FFmpegStartupException e) { + GuiExportFailed.tryToRecover(e, newSettings -> { + // Update settings with fixed ffmpeg arguments + exportArguments.setText(newSettings.getExportArguments()); + // Restart rendering, this will also save the changed ffmpeg arguments + renderButton.onClick(); + }); } catch (Throwable t) { - CrashReport crashReport = CrashReport.makeCrashReport(t, "Rendering video"); - throw new ReportedException(crashReport); + error(LOGGER, GuiRenderSettings.this, CrashReport.makeCrashReport(t, "Rendering video"), () -> {}); + display(); // Re-show the render settings gui and the new error popup } } }).setSize(100, 20).setI18nLabel("replaymod.gui.render"); diff --git a/src/main/java/com/replaymod/render/rendering/VideoRenderer.java b/src/main/java/com/replaymod/render/rendering/VideoRenderer.java index cf8ab6b2..9b36c0ad 100644 --- a/src/main/java/com/replaymod/render/rendering/VideoRenderer.java +++ b/src/main/java/com/replaymod/render/rendering/VideoRenderer.java @@ -39,6 +39,7 @@ import java.util.concurrent.Future; import java.util.concurrent.FutureTask; import static com.google.common.collect.Iterables.getLast; +import static com.replaymod.render.ReplayModRender.LOGGER; import static net.minecraft.client.renderer.GlStateManager.*; import static org.lwjgl.opengl.GL11.GL_COLOR_BUFFER_BIT; import static org.lwjgl.opengl.GL11.GL_DEPTH_BUFFER_BIT; @@ -66,6 +67,7 @@ public class VideoRenderer implements RenderInfo { private final GuiVideoRenderer gui; private boolean paused; private boolean cancelled; + private volatile Throwable failureCause; private Framebuffer guiFramebuffer; private int displayWidth, displayHeight; @@ -76,7 +78,7 @@ public class VideoRenderer implements RenderInfo { this.timeline = timeline; this.gui = new GuiVideoRenderer(this); this.renderingPipeline = Pipelines.newPipeline(settings.getRenderMethod(), this, - videoWriter = new VideoWriter(settings) { + videoWriter = new VideoWriter(this) { @Override public void consume(RGBFrame frame) { gui.updatePreview(frame); @@ -89,7 +91,7 @@ public class VideoRenderer implements RenderInfo { * Render this video. * @return {@code true} if rendering was successful, {@code false} if the user aborted rendering (or the window was closed) */ - public boolean renderVideo() { + public boolean renderVideo() throws Throwable { FMLCommonHandler.instance().bus().post(new ReplayRenderEvent.Pre(this)); setup(); @@ -138,6 +140,10 @@ public class VideoRenderer implements RenderInfo { FMLCommonHandler.instance().bus().post(new ReplayRenderEvent.Post(this)); + if (failureCause != null) { + throw failureCause; + } + return !cancelled; } @@ -250,7 +256,13 @@ public class VideoRenderer implements RenderInfo { ReplayMod.soundHandler.playRenderSuccessSound(); - new GuiRenderingDone(ReplayModRender.instance, videoWriter.getVideoFile(), totalFrames, settings).display(); + try { + if (!hasFailed()) { + new GuiRenderingDone(ReplayModRender.instance, videoWriter.getVideoFile(), totalFrames, settings).display(); + } + } catch (VideoWriter.FFmpegStartupException e) { + setFailure(e); + } // Finally, resize the Minecraft framebuffer to the actual width/height of the window mc.resize(displayWidth, displayHeight); @@ -326,7 +338,7 @@ public class VideoRenderer implements RenderInfo { return; } } - } while (paused); + } while (paused && !hasFailed()); } private boolean displaySizeChanged() { @@ -367,6 +379,20 @@ public class VideoRenderer implements RenderInfo { renderingPipeline.cancel(); } + public boolean hasFailed() { + return failureCause != null; + } + + public synchronized void setFailure(Throwable cause) { + if (this.failureCause != null) { + LOGGER.error("Further failure during failed rendering: ", cause); + } else { + LOGGER.error("Failure during rendering: ", cause); + this.failureCause = cause; + cancel(); + } + } + private class TimelinePlayer extends AbstractTimelinePlayer { public TimelinePlayer(ReplayHandler replayHandler) { super(replayHandler); diff --git a/src/main/resources/assets/replaymod/lang b/src/main/resources/assets/replaymod/lang index da6f6f8d..c50c65af 160000 --- a/src/main/resources/assets/replaymod/lang +++ b/src/main/resources/assets/replaymod/lang @@ -1 +1 @@ -Subproject commit da6f6f8d123cbda3d04e027948e29161ce302bdc +Subproject commit c50c65af7e987dbff8e7e24c62f50a6bf9ff2360