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

add support for automatically merging encode segments via helper script #219

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nlburgin
Copy link
Contributor

This is in response to #243.

It adds a checkbox to the encode settings window and a boolean to the config structure, and connects them together. When this new setting is enabled, a helper script written in Bash is called to merge all the segments automatically.

For now, the segments are still left intact and not deleted. However, if it's desired to automatically clean up the segments and free up space after writing the merged file, this could be changed just by adding an extra line or two to the Bash script.

It should be noted that the script heavily uses "Bashisms" and won't run on just any generic POSIX shell. You very seldom see a Linux system without Bash installed though, since it's part of the GNU base system, so I don't think of this as introducing a new dependency.

@vadosnaprimer
Copy link
Contributor

If some of those segments have different resolutions or framerates, what happens?

@nlburgin
Copy link
Contributor Author

nlburgin commented Apr 19, 2019

I'm not sure, but I suspect in that case it might be necessary to re-encode manually to stick them together. IIRC the concat demuxer normally requires the videos to be uniform in those respects, so it might fail in cases like that.

Still, right now the worst-case scenario is that the concatenation fails and the segment files are still there. So in its current state, it should still be safe to enable, even in situations where it wouldn't work.

You do have a good point; I guess if the script was going to be altered to automatically delete the segments after merging them, there should be a check in place to make sure it only did that if the merge was successful.

I assume ffmpeg would have a nonzero exit code if there was an issue, so in Bash that would be a simple if statement. I haven't tested that yet though.

Instead of just leaving the copy-pasted one intact 🤦‍♂️
No need to pipe `yes` to overwrite without prompting.
@nlburgin
Copy link
Contributor Author

nlburgin commented Apr 19, 2019

I just tested what actually happens when you try to merge files with differing framerates and resolutions. Surprisingly, it actually merges them without error, at least if the video bitstreams are h264.

I'm not sure if that's good or bad, though, since the merged file may have very poor compatibility. Mpv handles the framerate and resolution transitions seamlessly, but I don't think most other players are so robust against something so unusual. Even VLC stuttered a bit from the resolution shift, in my testing. A lesser player may just decide to crap out entirely...

I can't figure out how to make ffmpeg simply quit with an error if there's a mismatch, which means it can't be used for the basis of an if block. So if another option were added to delete the segments after merging, it would be unsafe in cases where there's a resolution or framerate mismatch.

Though as it stands now, it's still safe because the segments are unconditionally retained, so if the automerged video is broken/weird it can't just ruin an entire encode.

@vadosnaprimer
Copy link
Contributor

That's what I expected the least!

@nlburgin
Copy link
Contributor Author

I looked up the in-depth documentation for the concat demuxer. It had an option called -auto_convert.

Apparently the "conversion" referred to is not a re-encoding, but just messes with the metadata in the bitstream packets or something like that, for h264 (and does nothing for other formats).

Apparently this option was enabled by default. I tried disabling it manually with "-auto_convert 0" but then it still didn't exit with an error, instead it just produced a really invalid file that even mpv couldn't handle properly.

...After that test, I even tried mismatching the codec, and still it chose to produce an invalid file rather than exiting with an error.

It seems the concat demuxer is determined to produce some sort of output file and exit "successfully", come hell or high water.

@vadosnaprimer
Copy link
Contributor

Just add an explicit check then, it has to have a way to tell if the video parameters are different. Lack of proper error seems to be their bug, you decide if you feel like reporting it to them.

@nlburgin
Copy link
Contributor Author

Mkvmerge, unlike ffmpeg, does seem to care if the inputs match. If adding it as a dependency were no obstacle, that would be the simplest option.

Otherwise, an explicit check would likely require parsing the output of ffprobe, and doing so in a way that isn't error-prone might be somewhat complex.

@nlburgin
Copy link
Contributor Author

nlburgin commented Apr 20, 2019

I just figured out that it might be easier to embed the script than have it as a separate file.

I was initially afraid a solution like that might end up as a escape-sequence riddled mess (insert dumb joke about Shulk spamming Backslashes here).

But it turns out C++ supports "raw string" syntax since 2011, so it actually can integrate pretty seamlessly without making a mess of the script and resulting in "write-only code".

That being the case, I don't actually see a downside to moving the script inside a string constant in a header.

nlburgin and others added 3 commits April 19, 2019 21:03
…e project is supposed to have a copy of this at the top.
It worked even though I forgot this, so I assume it was already getting included by one of the other headers. Still, I think it's good practice to explicitly include the correct headers.
@vadosnaprimer
Copy link
Contributor

vadosnaprimer commented Apr 20, 2019

The downside is probably not being able to tweak the "script" that's being run?

@nlburgin
Copy link
Contributor Author

nlburgin commented Apr 20, 2019

Well, it might be worthwhile to specifically support running user-provided scripts instead of just the one.

As I mentioned, the raw string means the script is still reasonably maintainable as a header file, but it might be interesting to support tweaking/replacing it at run-time without recompiling. 🤔

In that case, the new feature would probably be defined less as the ability to merge segments, and more as the ability to run a user-provided script after finishing an encode, with my merge script just being an example.

It might be good in that case to have the user provide a script path in the encode settings window instead of just looking for libTasConcat.sh in the same directory as the libTas executable.

@InfamousKnight
Copy link

Is this pr gonna get merged or just closed and forgotten? I think its pretty handy.

@Randomno
Copy link
Contributor

Would like to see this. Constantly getting PCem encodes with 8 different segments during boot.

@vadosnaprimer
Copy link
Contributor

vadosnaprimer commented May 15, 2024

I've had a thought. Even if I tell ffmpeg to output segments of the same resolution and framerate, they are still split. Maybe that part needs to be fixed on the libtas end first? However how would it know ffmpeg outputs at the same res? libtas is using nut so it doesn't know what the user told ffmpeg to do. Or maybe we just a setting in the encode config that tells it not to split?

@clementgallet
Copy link
Owner

Yes, we could have a setting that prevent the encoder from restart on window resize. It would probably break the encode if the resolution changed.

Or, we could link to libswscale and do a proper resize of pixels to match a given resolution.

@vadosnaprimer
Copy link
Contributor

If done automatically, it'd make sense to also control resize filter and target res, but on the other hand if the user already handles resizing manually why would it ever change the end resolution?

@clementgallet
Copy link
Owner

The split is done because nut format does not allow changing the resolution inside a single stream. So whatever was passed to ffmpeg does not indeed matter. Resizing must be performed by libtas, or it must be done after ffmpeg pass. Or maybe there's an option to tell ffmpeg to append multiple consecutive streams?

@vadosnaprimer
Copy link
Contributor

Or maybe there's an option to tell ffmpeg to append multiple consecutive streams?

@nattthebear do you happen to know?

@nattthebear
Copy link

I don't remember anything special offhand, no.

@vadosnaprimer
Copy link
Contributor

I got this suggestion from Spike:

-filter_complex "[0:v:0][0:a:0][1:v:0][1:a:0][2:v:0][2:a:0]concat=n=3:v=1:a=1[outv][outa]" \ -map "[outv]" -map "[outa]" output.mkv

@Spikestuff
Copy link

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants