Openshot-qt: Last Daily = Corrupted Export File

[nanohtpc]

OpenShot-v2.4.4-dev2-1564951228-90a56104-f17a7ed4-x86_64

= file corrupted

OpenShot-v2.4.4-dev2-1564930379-90a56104-f17a7ed4-x86_64

= file Ok...

Win10 x64

[ferdnyc]

@nanohtpc

• What were your export settings?
• Does the issue still occur if you use a different codec?
• "Corrupted" how, exactly?
• What did you use to determine the file was corrupted?
• Did you check your C:\Users\_____\.openshot_qt/openshot-qt.log to see if there were any error messages logged by the encode?

[SuslikV]

@ferdnyc Do you remember the change that changed recent files list paths? Look how it looks now (slashes vs backslashes):

the root of the problem is here I think (Edit: not the PR about the recent list changes, of course). The issue itself is similar to the export error you got (no moov box).

[ferdnyc]

@SuslikV

That change just happened because add_to_recent_files now calls os.path.abspath() on the path string, which normalizes it to OS path separators. Python on Windows has always supported either slash format interchangeably. That shouldn't have any effect on anything.

If the issue was the slash direction, it would only occur on Windows. So, if the issue really is the missing moov, then it's something happening cross-platform. I actually have a suspicion about what it might be, while I was chasing -Wall errors I noticed some dodgy code in libopenshot.

[ferdnyc]

cc: @jonoomph

If the issue is ffmpeg failing to reopen the output file, I think this is the bad code right here:

OpenShot/libopenshot src/FFmpegWriter.cpp#L517-L528:

// Write the file header (after the options are set)
void FFmpegWriter::WriteHeader() {
if (!info.has_audio && !info.has_video)
    throw InvalidOptions("No video or audio options have been set.  You must set has_video or has_audio (or both).", path);


// Open the output file, if needed
if (!(fmt->flags & AVFMT_NOFILE)) {
    if (avio_open(&oc->pb, path.c_str(), AVIO_FLAG_WRITE) < 0)
        throw InvalidFile("Could not open or write file.", path);
}


// Force the output filename (which doesn't always happen for some reason)
snprintf(oc->AV_FILENAME, sizeof(oc->AV_FILENAME), "%s", path.c_str());

Notice that it's using the size of oc->AV_FILENAME as the buffer size? That means if the buffer isn't _already_ big enough to fit the entire filepath, it'll end up truncated, which is exactly what I was seeing in OpenShot/libopenshot#292.

[ferdnyc]

Mmm, nope, I might be wrong about that. It looks like AvFormatContext.filename has a fixed size of 1024 characters, which is plenty big enough. Unless something else is truncating it somehow.

[ferdnyc]

Is it possible enabling the faststart option is somehow causing the filename allocation to be truncated in the header?

[ferdnyc]

Mmm, nope, I might be wrong about that. It looks like AvFormatContext.filename has a fixed size of 1024 characters, which is plenty big enough. Unless something else is truncating it somehow.

Ooh!! _BUT_, while it's true that filename is a char[1024], that's also a deprecated field, and in FFmpeg 3.4+ AV_FILENAME will be set to url (AvFormatContext.url) which does NOT have a fixed length! I'm back to thinking that's where the problem is.

[ferdnyc]

OpenShot/libopenshot#298 fixed this for me, on linux. I no longer get the second-pass "can't reopen file" errors, and the resulting video has its moov block properly moved to the start of the file.

[ferdnyc]

OpenShot/libopenshot#298 passed Travis, so I'm going to plan to merge it ASAP...

But I definitely wouldn't mind anyone else testing it first, and confirming that it (a) doesn't break FFmpegWriter, and (b) fixes the bad-export problem, before I merge.

[SuslikV]

Yeah, pointer vs fixed string... 8 vs 1024. Minimum possible path in Windows is C:\1.mp that is shorter than required C:\1.mp4 at least. But in linux @ferdnyc you may try to output to shorter path, isn't it?

The thing is that there is no error in the log of OpenShot during export. So, if export fails inside the FFmpeg's processing - Windows user will never knew about it.

[ferdnyc]

The thing is that there is no error in the log of OpenShot during export. So, if export fails inside the FFmpeg's processing - Windows user will never knew about it.

Yeah, a lot of ffmpeg's codecs output messages directly to STDERR. Those messages need to be captured better.

[SuslikV]

By "redirection" to ZmqLogger?

Also, there is one more place where context's pointers used with false assumption of robustness, I think. That delayed my PR about color range, color matrix etc. flagging.

[ferdnyc]

Also, there is one more place where context's pointers used with false assumption of robustness, I think. That delayed my PR about color range, color matrix etc. flagging.

You'll have to point me at a line# in a file.

It's easy to see how the AV_FILENAME thing happened. The fixed buffer size _used_ to be a perfectly safe assumption, until FFmpeg 3.4, and whoever updated the abstraction for AV_FILENAME must've just not spotted the fact that there's one place where it's used to write the value, and not just read it.

Plus, it's been like that for at least a YEAR, probably, and it never affected anything until faststart came along. It's only because it implicitly turns on 2-pass encoding, that it suddenly matters that we were corrupting the in-memory filename _after_ opening the file. Until then, it never affected anything.

By "redirection" to ZmqLogger?

Basically, yeah — not redirection _per se_, because the zmq message format is very rigid, and ffmpeg's error output is anything but (it's even colorized)... but at least some sort of proxy-notification of the fact that there was error output, if nothing else.

And perhaps also some sort of optional redirection into an export-process logfile stored alongside the output. That's a common pattern for modular encoding software, used by HandBrake among others.

[SuslikV]

@nanohtpc While it is closed, the fix will appear only in the next build of the application that will use new modified libopenshot library (it is unknown when it happens).
Due to nature of the issue the fix was applied to linked library of the OpenShot.

[ferdnyc]

Oh, it's already there, if you visit the daily tag on GitHub at https://github.com/OpenShot/openshot-qt/releases/tag/daily — you're looking for OpenShot-v2.4.4-dev2-1565113187-05c21ae2-f17a7ed4-x86_64.exe for Win64. (The newest builds are at the bottom of the list on Page 2.)

Not sure how long it takes https://openshot.org/download/ to update its list.

[ferdnyc]

Oops, actually, the openshot.org download page just updated. So, "Not long" is the answer.