Skip to content

Conversation

@jonoomph
Copy link
Member

@jonoomph jonoomph commented Jan 5, 2020

This PR represents the release branch for libopenshot, version 0.2.4 (SO 18), and will be used during the release testing. Feel free to comment, or send PR's for this release branch.

Related PR's:

ferdnyc and others added 30 commits August 25, 2019 04:05
- Use same `html_code` and `css_code`
- Sync up offsets and gravity (BOTTOM_RIGHT)
- Create 1280x720 frames
- Generate 100 frames of output video
Makes nearly all `file_path` arguments optional, unless the exception deals
with file operations specifically (e.g. InvalidFile still requires file_path)
Add a Python example in src/examples/Example.py
Extend openshot::Fraction in the Python wrapper, to permit:
```py
f = openshot.Fraction(16, 9)
float(f) # returns 1.777777777
int(f) # returns 2
print(f) # outputs "16:9"
dict(f.GetMap()) # in Python dict form
Codacy pointed out that there's absolutely no reason to be
importing the sys module in this code.
Example.py: Remove useless `import sys`
- Don't precede with two blank lines
- "Decodiing" was misspelled
- Less cryptic text
Instead of the hardcoded `/home/jonathan/...` input path, load the input file from TEST_MEDIA_PATH, defined as in `tests/CMakeLists.txt`
We don't _just_ use the ZeroMQ library, we also use the cppzmq bindings.
Previously we weren't detecting for those at all. This adds a check for
its configs, and adds its target to the openshot target linking (even
though it's a header-only library).
Our copy of the module is outdated, and fails detecting Python 3.8. (Besides, we shouldn't be mirroring standard CMake modules.)
@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 9, 2020

And AppImages built with 22793e2 have libresvg.so linked properly! \o/ Now to actually test the color-inversion fix...

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 9, 2020

@jonoomph Whoop! Well, we're closer. Now I get a C++ traceback/crash in the AppImage, trying to drag an SVG title file from Project Files to the Timeline.

But I'm not sure that one's my fault, because this is the traceback:

QSslSocket: cannot resolve CRYPTO_num_locks
QSslSocket: cannot resolve CRYPTO_set_id_callback
QSslSocket: cannot resolve CRYPTO_set_locking_callback
QSslSocket: cannot resolve ERR_free_strings
QSslSocket: cannot resolve sk_new_null
QSslSocket: cannot resolve sk_push
QSslSocket: cannot resolve sk_free
QSslSocket: cannot resolve sk_num
QSslSocket: cannot resolve sk_pop_free
QSslSocket: cannot resolve sk_value
QSslSocket: cannot resolve SSL_library_init
QSslSocket: cannot resolve SSL_load_error_strings
QSslSocket: cannot resolve SSLv23_client_method
QSslSocket: cannot resolve SSLv23_server_method
QSslSocket: cannot resolve X509_STORE_CTX_get_chain
QSslSocket: cannot resolve OPENSSL_add_all_algorithms_noconf
QSslSocket: cannot resolve OPENSSL_add_all_algorithms_conf
QSslSocket: cannot resolve SSLeay
QSslSocket: cannot resolve SSLeay_version
QSslSocket: cannot call unresolved function CRYPTO_num_locks
QSslSocket: cannot call unresolved function CRYPTO_set_id_callback
QSslSocket: cannot call unresolved function CRYPTO_set_locking_callback
QSslSocket: cannot call unresolved function SSL_library_init
QSslSocket: cannot call unresolved function SSLv23_client_method
QSslSocket: cannot call unresolved function sk_num
Caught signal 11 (SIGSEGV)
---- Unhandled Exception: Stack Trace ----
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 (                                           + 0xd745c)  [0x7f7b2597a45c]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 ( QSslCertificate::fromData(QByteArray const&, QSsl::EncodingFormat)  + 0x25  )  [0x7f7b2597a665]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 ( QSslCertificate::fromPath(QString const&, QSsl::EncodingFormat, QRegExp::PatternSyntax)  + 0x69a )  [0x7f7b2597ed6a]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 (                                           + 0xec4e4)  [0x7f7b2598f4e4]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 (                                           + 0xec840)  [0x7f7b2598f840]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 ( QSslCertificate::QSslCertificate(QByteArray const&, QSsl::EncodingFormat)  + 0x78  )  [0x7f7b2597a1b8]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 ( QSslConfiguration::QSslConfiguration()    + 0x39  )  [0x7f7b2597fe99]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 (                                           + 0x796b3)  [0x7f7b2591c6b3]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 (                                           + 0x81ff5)  [0x7f7b25924ff5]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 ( QNetworkAccessManager::createRequest(QNetworkAccessManager::Operation, QNetworkRequest const&, QIODevice*)  + 0x2ec )  [0x7f7b258fd51c]
  /tmp/.mount_GcpFEu/usr/bin/libQt5Network.so.5 ( QNetworkAccessManager::get(QNetworkRequest const&)  + 0x14  )  [0x7f7b258fa164]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0xacdacb)  [0x7f7b1e801acb]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0xacdbe8)  [0x7f7b1e801be8]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0xaca280)  [0x7f7b1e7fe280]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0xacae91)  [0x7f7b1e7fee91]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0xacb9ee)  [0x7f7b1e7ff9ee]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0xac7075)  [0x7f7b1e7fb075]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0xa0685c)  [0x7f7b1e73a85c]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x8f016a)  [0x7f7b1e62416a]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x8f44ce)  [0x7f7b1e6284ce]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x8f47f0)  [0x7f7b1e6287f0]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x89b5e1)  [0x7f7b1e5cf5e1]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x8ab582)  [0x7f7b1e5df582]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x8aba11)  [0x7f7b1e5dfa11]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x8d9969)  [0x7f7b1e60d969]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x128fce1)  [0x7f7b1efc3ce1]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x14c701f)  [0x7f7b1f1fb01f]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x146d556)  [0x7f7b1f1a1556]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x14cab1e)  [0x7f7b1f1feb1e]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x14cac9e)  [0x7f7b1f1fec9e]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0x14cadd2)  [0x7f7b1f1fedd2]
  /tmp/.mount_GcpFEu/usr/bin/libQt5WebKit.so.5 (                                           + 0xca9af6)  [0x7f7b1e9ddaf6]
  [0x7f7ac7fff265]                        
---- End of Stack Trace ----
QSslSocket: cannot call unresolved function CRYPTO_num_locks
QSslSocket: cannot call unresolved function CRYPTO_set_id_callback
QSslSocket: cannot call unresolved function CRYPTO_set_locking_callback
QSslSocket: cannot call unresolved function ERR_free_strings
QMutex: destroying locked mutex
Caught signal 11 (SIGSEGV)
---- Unhandled Exception: Stack Trace ----
zsh: segmentation fault (core dumped)  ./OpenShot-v2.4.4-dev2-1578555150-a8b65f57-f38389fc-x86_64.AppImage

And I happened to notice these messages in the builder logs of the last few linux builds (note the WARNINGs):

      freeze:INFO Execution path: /home/gitlab-runner/builds/5cd61c66/0/OpenShot/openshot-qt/freeze.py
      freeze:WARNING /usr/lib/x86_64-linux-gnu/libssl.so.1.0.0: not found, skipping
      freeze:WARNING /usr/lib/x86_64-linux-gnu/libcrypto.so.1.0.0: not found, skipping
      freeze:WARNING /usr/lib/x86_64-linux-gnu/libthread-2.0.so: not found, skipping
<module 'cx_Freeze' from '/usr/local/lib/python3.4/dist-packages/cx_Freeze-4.3.4-py3.4-linux-x86_64.egg/cx_Freeze/__init__.py'>
Copying modules to openshot_qt directory: /home/gitlab-runner/builds/5cd61c66/0/OpenShot/openshot-qt/openshot_qt
Loaded modules from openshot_qt directory: /home/gitlab-runner/builds/5cd61c66/0/OpenShot/openshot-qt/openshot_qt
running build
running build_exe

Path/versioning issue on the linux builder, perhaps?

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 9, 2020

Indeed, from exploring the AppImage I can say that it does not contain libssl.so.1.0.0 or libcrypto.so.1.0.0, but at least _ssl.cpython-34m-x86_64-linux-gnu.so is looking for them:

$ cd /tmp/.mount_avJWHX

$ ls usr/bin/*ssl*
usr/bin/_ssl.cpython-34m-x86_64-linux-gnu.so

$ ldd usr/bin/*ssl*
	linux-vdso.so.1 (0x00007ffc602dd000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f00f2980000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f00f27b7000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f00f2c18000)
	libssl.so.1.0.0 => not found
	libcrypto.so.1.0.0 => not found

(My local system offers only libssl3.so, libssl.so.1.0.2o, and libssl.so.1.1.d.)

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 9, 2020

And I happened to notice these messages in the builder logs of the last few linux builds (note the WARNINGs):

Hmm, those are WARNINGs (instead of fatal errors) only in the wake of OpenShot/openshot-qt#3106, which was intended to make freeze.py more usable for other developers / on other systems — not to paper over issues on the build servers.

Maybe I should add a command-line flag that will make those WARNINGs fatal again. We can use that on the GitLab CI platforms, to make sure that no incomplete builds sneak through.

@jonoomph
Copy link
Member Author

jonoomph commented Jan 9, 2020

@ferdnyc, here are those files on the Linux build server:

ubuntu@ip-172-30-0-113:/usr/bin$ ldd openssl
	linux-vdso.so.1 =>  (0x00007ffce7173000)
	libssl.so.1.0.0 => /lib/x86_64-linux-gnu/libssl.so.1.0.0 (0x00007f694bf57000)
	libcrypto.so.1.0.0 => /lib/x86_64-linux-gnu/libcrypto.so.1.0.0 (0x00007f694bb7a000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f694b7b1000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f694b5ad000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f694c1b6000)

@jonoomph
Copy link
Member Author

jonoomph commented Jan 9, 2020

AppImages now launch, and resvg is updated, but I'm still seeing the inverted gradient colors. Hmmm... Can anyone else confirm?

@jonoomph
Copy link
Member Author

jonoomph commented Jan 9, 2020

✔️ Mac: resvg works nicely, supports alpha gradients, and all titles work (including the default font)
✔️ Windows: resvg also works correctly now
❌ Linux: resvg still produces inverted colors, but alpha gradients now work.

Screenshot from 2020-01-09 15-20-40

@jonoomph
Copy link
Member Author

jonoomph commented Jan 9, 2020

@ferdnyc Perhaps the Linux version of Qt5 is too old, and has some other issue. We are currently running: qt5 version: 5.2.1 on it. I'm going to backup the Linux builder, and see if I can upgrade Qt5, sip, and the python3 bindings.

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 10, 2020

@jonoomph Agreed, if it wasn't the image format, then I'm inclined to just blame Qt 5.2 in general.

@RazrFalcon
Copy link

RazrFalcon commented Jan 11, 2020

@ferdnyc Do you convert produced image from ARGB_Premultipled to RGBA8888 manually? Maybe automatic conversion is broken in this version of Qt? I still insist that this issue is not related to resvg.

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 11, 2020

@ferdnyc Do you convert produced image from ARGB_Premultipled to RGBA8888 manually? Maybe automatic conversion is broken in this version of Qt? I still insist that this issue is not related to resvg.

I absolutely agree with you! Especially since it turns out that I'm an idiot. Twice over.

When I made the change to our resvg code, I fixed the image-loading code, but missed that there was a copy-paste version of the exact same conversion code in QtImageReader::GetFrame(), the code that handles conversion of the image data for the previewer/encoder. That was still using the old code.

So, it's fully expected that nothing would've changed in the AppImage builds made after my previous "fix" (which fixed only the wrong code, effectively). The commits I just pushed make the same change to the GetFrame() code, which is what matters. So, this may be fixed "for real" now.

Of course, the build host is down for a failed update of Qt, since we were thinking that was the problem. So it'll be hard to confirm that immediately.

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 11, 2020

What we do now, though, for the record, is this:

// Scale image smaller (or use a previous scaled image)
if (!cached_image || (max_size.width() != max_width || max_size.height() != max_height)) {
bool rendered = false;
#if USE_RESVG == 1
// If defined and found in CMake, utilize the libresvg for parsing
// SVG files and rasterizing them to QImages.
// Only use resvg for files ending in '.svg' or '.svgz'
if (path.toLower().endsWith(".svg") || path.toLower().endsWith(".svgz")) {
ResvgRenderer renderer(path);
if (renderer.isValid()) {
// Scale SVG size to keep aspect ratio, and fill the max_size as best as possible
QSize svg_size(renderer.defaultSize().width(), renderer.defaultSize().height());
svg_size.scale(max_width, max_height, Qt::KeepAspectRatio);
// Create empty QImage
cached_image = std::shared_ptr<QImage>(new QImage(QSize(svg_size.width(), svg_size.height()), QImage::Format_ARGB32_Premultiplied));
cached_image->fill(Qt::transparent);
// Render SVG into QImage
QPainter p(cached_image.get());
renderer.render(&p);
p.end();
rendered = true;
}
}
#endif
if (!rendered) {
// We need to resize the original image to a smaller image (for performance reasons)
// Only do this once, to prevent tons of unneeded scaling operations
cached_image = std::shared_ptr<QImage>(new QImage(image->scaled(max_width, max_height, Qt::KeepAspectRatio, Qt::SmoothTransformation)));
}
cached_image = std::shared_ptr<QImage>(new QImage(cached_image->convertToFormat(QImage::Format_RGBA8888)));

Ugh, that's almost impossible to read with GitHub's page formatting.

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 11, 2020

Basically, we set up a buffer that's (now) explicitly in QImage::Format_ARGB32_Premultiplied format, and let resvg render into that. Later, no matter what the image format started as (in case we loaded it from a file, in which case it could be in arbitrary format), we unconditionally convert to QImage::Format_RGBA8888.

(Edit: Corrected buffer format)

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 14, 2020

So, I think we should hold off on this release for just around a week longer, based on what I've discovered today.

Let me preface this by saying that I've been sitting on it for hours, trying to figure out where I made some mistake or misinterpreted some data, because I didn't even believe it myself. I figured I must've made an error somewhere. But the more I test, the more unbelievable the results get, and I've finally decided to accept that I might just be right about this.

Qt are not kidding around, about the performance differences between various QImage formats.

The argb-image-format branch in my fork contains the develop branch code, with only two changes:

  1. I made the modification I mentioned above, to use setOpacity() instead of bit-hammering the alpha levels
  2. I search-and-replaced every instance of QImage::Format_RGBA8888 in the code with QImage::Format_ARGB32_Premultiplied.

Then I wrote some benchmark scripts, to compare the code on this branch to the modified argb-image-format code. At first the results were pretty inconclusive, it's not like just changing the image format magically made GetImage() noticeably faster, or anything. So I had to get more creative.

I created a complex project in OpenShot, where "complex" means that from the very first frame, there are always FIVE tracks with video running on them, all with their Alpha properties set for semi-transparency, and with keyframed transforms constantly going on. There are videos rotating, other videos scaling and sliding around behind them, a couple of the middle videos shrink down and walk around the screen as they fade in and out of visibility... it's a chaotic, incomprehensible mess, in short.

I saved that project, removed the history from the file, and then ran Export time trials in the GUI. Using the same OpenShot code (the release branch over there), I PYTHONPATH loaded builds of both libopenshot branches' code, opened the saved project (set up for 1280x720 @ 30fps), and immediately performed an Export with the same parameters: Medium-quality MP4, video-only, frames 1-300, without GPU accel.

branch Time to export
release-20200105 18.72 seconds ❌
argb-image-format 13.14 seconds ✔️

Like I said, I didn't believe it either. So I modified my benchmark scripts to use the new project, and set them up to test the encode path specifically: create a Timeline, set up a Writer, and then repeatedly do timeline.GetFrame(); writer.WriteFrame(); for each frame from 1-300, four times over. (So, the total export was 1200 frames.)

Then I ran best-of-three timing trials between the two branches, now performing three tests:

  1. Repeatedly GetFrame(47); SaveFrame('/dev/null/'); repeat 100 times
  2. GetFrame() a random frame#, SaveFrame('/dev/null/'); repeat 60 times
  3. The sequential GetFrame()->WriteFrame() test over frames 1-300 four times

(Each result is the best of three runs.)

branch Test 1 Test 2 Test 3
release-20200105 5.047s ✔️ 52.215s 56.275s
argb-image-format 5.307s 39.927s ✔️ 32.642s ✔️

I keep running it, and the results are the same: Whether in benchmarks or interactively, for complex compositing operations QImage/QPainter using ARGB32_Premultiplied is somewhere on the order of THIRTY TO FORTY PERCENT faster than RGBA8888.

Now, there's an obvious issue with this, currently: The colors are all out of whack. To complete the format-change, I have to go through the code, find all the places where we bit-hammer the image bytes directly, and reorder them from RGBA to ARGB. (Or, more likely, rewrite the code to use Qt's qRgba(), because it will conform to the image format automatically. So even if we were to change the format again in the future, the code wouldn't have to change.)

It'll take a bit of work, and a few days to get it right probably. But... I mean... it sounds worth it, no!?

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 14, 2020

The colors are all out of whack.

Actually, the funny thing is, that's only true of the preview widget. The exported colors are perfect, the way we deliver the image data to FFmpeg must be able to handle the format change.

I used this technique to diff the results of both of the OpenShot exports above, and the image was solid gray (no differences at all) except for one video I included that's cropped to a non-standard size of 430px × 720px. That video makes OpenShot very unhappy, and results in a several-pixels-wide band of video noise on its right edge. Because that area was random noise, it didn't match up between the two videos, just like it wouldn't match up between two different exports from the SAME code.

But other than that, the output videos were 100% identical. One of them just got written 5½ seconds faster than the other.

@jonoomph
Copy link
Member Author

@ferdnyc This sounds very promising indeed!!! Yes, let's hold off the release for a few more days and see how this turns out. 🤞 Please let me know when the branch is ready for some testing, and I'll give is a try on all 3 OSes.

@SuslikV
Copy link
Contributor

SuslikV commented Jan 15, 2020

...except for one video I included that's cropped to a non-standard size of 430px × 720px. That video makes OpenShot very unhappy, and results in a several-pixels-wide band of video noise on its right edge...

Last time it was mentioned here: OpenShot/openshot-qt#3152

@SuslikV
Copy link
Contributor

SuslikV commented Jan 15, 2020

Depending on how Preview looks for you, you may try to change the windows/video_widget.py of OpenShot by adding to present() function next code:

self.current_image.reinterpretAsFormat(QImage.Format_ARGB32_Premultiplied)

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 20, 2020

Depending on how Preview looks for you, you may try to change the windows/video_widget.py of OpenShot by adding to present() function next code:

self.current_image.reinterpretAsFormat(QImage.Format_ARGB32_Premultiplied)

Unfortunately we can't use that, as it's only present in Qt 5.9 and later. But openshot::Frame::Save() was also writing out color-swapped PNG files (just not color-swapped video frames), so I think it was purely a bug in libopenshot. That format should be the default for any Qt image-processing functions, anyway, so I can't imagine openshot-qt would need to explicitly request it.

Regardless, the code wasn't coming together as quickly as I'd hoped, as our direct bit-access makes a format change somewhat complicated. So @jonoomph and I decided to postpone it for after the release. Once the release is done I'll get my in-progress changes into a more coherent state and create a branch off of develop in the project repo (or repos, if openshot-qt does also require changes), so that others can review and contribute.

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 20, 2020

I did want to ask @jonoomph if he's seen #416, though, and might want to incorporate it into the 0.2.4 release — as I said in the PR, it made a huge difference in preview playback on my system, and we've had more reports come in (OpenShot/openshot-qt#965) just today about poor preview performance.

@jonoomph
Copy link
Member Author

jonoomph commented Feb 7, 2020

Looking at #416 now. I'm a little concerned about the memory requirements of this change. I'm gonna test it real quick though.

… not see any crazy memory demands from this.
@jonoomph
Copy link
Member Author

jonoomph commented Feb 7, 2020

Okay, in my tests this worked great. About to merge for final release

@jonoomph jonoomph changed the title WIP: Release of libopenshot 0.2.4 (SO 18) Release of libopenshot 0.2.4 (SO 18) Feb 8, 2020
@jonoomph jonoomph merged commit 63e28a0 into master Feb 8, 2020
@jonoomph jonoomph deleted the release-20200105 branch August 17, 2020 19:11
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.

9 participants