Switch from builtin video sink to qmlglsink #8093
Conversation
|
In some context - yes. I'd not do so and save my time but unfortunately - I can't. My problem is that your patch changes some places that are not related to video sink replacement (like noticeable .qml changes), disables/does not implement existing major features (video stream watchdog for example) and keeps object hierarchy still over-complicated (VideoSurface for example - why would we need that with qmlglsink?). I'm trying to simplify VideoReceiver and keep all existing features working as before. Right now I'm trying to make it working on all QGC platforms and looking to start working on standalone test tool for VideoReceiver - that's annoying to see many video issues and have no instrument for quick automated testing. As for me it looks like that my patch is much closer to release candidate than yours. I propose to use it as basis for qmlglsink integration. If you see any problems with my code - let me know and I'll fix ASAP. |
|
I don't mind working in a feature together with others, please don't think I'm complaining. My idea with my patch series is not just to remove the QtGstreamer code but also so simplify the whole architecture. You can see for instance that I managed to get multiple video streams working with it while the original architecture for video in QGC makes this quite difficult. The VideoSurface is an abomination that I created to be able to start / stop the video streams in the Qml rendering thread, as I was not having videos with a pipeline started outside of it. It's probably something uneeded and that I could rewrite in a better way, I just need to found out how. And checking your code you have done the same thing (with the difference that you created a class that extends QRunnable and a schedule render job) On your side, you used the example code for the qmlsink that uses "objectName" on the Qml - that's one thing that I did not want to do on my code as it's quite unusual for Qml objects to have a name, and feels wrong. I'm not really familiar with GStreamer - I can see that you know your way around it. |
|
Yes, I see your multiple video streams feature and I think that it is something that maintainers would adopt. From your past communications with them it sounds like they prefer smooth transition from existing code to full featured multiple view support (i.e. small step increments keeping QGC not worse than it was before). I think I'm finishing one of these small steps (qmlglsink support) and probably it is where multiple video stream support can start from. Regarding object names - I agree. Adding extra field is not very nice solution. But since it requires two extra lines only I think that it is not so bad for current stage. If you see some nice way to workaround this requirement then drop me a message (you can email me directly) and I'll be happy to add it to my patch. |
|
I just did a revamp of the "qmlglsink" also changing a few bits, I would really appreciate a review from somebody that knows more of GStreamer than I do. Even if we ship your version instead of mine first - as mine deals with more changes in the code architecture. I believe I can transform the VideoSurface code into the VideoReceiver (the only difference is that I would need to remove the VideoReceiver from the VideoManager, and initialize it directly in Qml for the threading issue I commented about) - would will also make things better: VideoManager is bloated and tries to manage things outside of it's scope, so getting rid of it seems a plus. About the "objectName" - it does not only changes two lines in code, you also need to get the rootWindow() and walk thru the object hierarchy to find the ones you are looking for (mostly making impossible to transform the video code in a single app for testing purposes) - that can be problematic if two QObjects have the same name. |
|
I'll look on objectName after finishing with non-Linux supports - qmlglsink is not as smooth as it looks on first sight. |
git-subtree-dir: libs/gst-plugins-good git-subtree-split: 625eb00c063b8bc03bb2eee637599a02abec6a19
|
Latest changes to this PR are adding prebuilt and statically linked qmlglsink to the QGC. It makes QGC working* on Linux, Windows**, macOS, iOS and Android. NOTE that QGC tries to use qmlglsink installed on your system. If qmlglsink is not present - app will fallback to statically linked qmlglsink and will use it. *working - I mean good enough for development use. QGC still misses a few small fixes here and there. One of them - after initial app setup and first time video streaming configuring you need to restart app to make streaming working. It should be trivial to fix and I'll do that shortly. iOS build sometimes crashes during startup. I'm not sure that it is related to qmlglsink. But I'm also not sure that it is not related to that. **Windows - qmlglsink depends on Open GL which was never simple to use on Windows. There is some long lasting bug related to multiple video sink support (see https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/477#note_361958). You need to disable #if at libs/gst-plugins-good/ext/qt/gstqtglutility.cc@224 if you experience issues with video on Windows. I'll continue working on this issue and fix it. |
9e0eb77 flvmux: Use the last DTS for the metadata timestamp git-subtree-dir: libs/gst-plugins-good git-subtree-split: 9e0eb7781076441c9704d2383708e49399e7da9d
…as not created because videoBackgroundComponent was not loaded on startup if video was disabled
|
Fixed video initialization on first startup. I'd ask someone with deep QGC/QML understanding to look on that fix (see andrewvoznytsa@886c968) - basically videoBackgroundComponent has the same life cycle as thermalItem and there should be no any problems. I had to do that to keep qmlglsink initialization simple, in single place on startup. Fixed qmlglsink multiple instances support on Windows. PR sent to upstream (see https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/432) and is under review. I'll continue working on that until it will be fully accepted. From now I expect that this patch makes QGC not worse (from features point of view) than it was before qmlglsink switch and it is in good shape to be considered for merge. Future plans - I plant to remove objectName dependecy as @tcanabrava suggested. It will make video receiver easier to use in different use cases, including multiple video stream support. Also I'll try to improve videoBackgroundComponent initialization and make it on demand, to better manage resources, probably improve startup time and make things right. But it all depends on my other work related to VideoReceiver improvements. I expect to refactor VideoReceiver a bit (and probably VideoManager). And I think that these objectName and intializations improvements will land into repo as separate PRs, during future work. |
|
Andrew, I'm trying to follow the commits but it's quite hard with the way you use git, can you perhaps clear the commits and use git rebase? The current state of the commits is not really easy to follow. |
|
One of private 'quick' commits escaped to public repo some time ago and rebasing is not trivial at this point :( Will see what can I do after fixing CI issues. |
Hi @andrewvoznytsa. Please consider coordinating with our efforts to refactor the video code by elaborating your plans and ideas at #7842. |
I've downloaded the app. Please provide me step-by-step on what to do and will get it done as soon as I could. |
|
FYI: Windows builds installed using the standard installer already set windows up to capture crash mini dumps. The docs here: https://docs.qgroundcontrol.com/en/Support/Support.html explain where the dump files are stored. You can then load these in visual studio debugger and get a stack track of where the crash is. If you need a full dump (which allows you to inspect memory variables and so forth) you'll need to tweak the crash dumpr registry setting to switch from mini to full dump. |
|
@rollysing could you please do what @DonLakeFlyer suggest in #8093 (comment) ? I was not aware of that. Please send me these dump files (*.dmp) and I'll check them here. If we don't get enough information then we'll continue with WinDBG. To find dumps - please look for 'Reporting Crashes from Windows Builds' on https://docs.qgroundcontrol.com/en/Support/Support.html page. |
With a dump file you'll get way more than what you could get remotely getting rolly to follow steps. If a mini-dump doesn't get you enough information. You can ask rolly to futz the registry to get a full dump. A full dump will get you the same thing as if you had captured the crash in the debugger on your own machine. Move around the stack, examine variable and so forth. |
|
The Windows installer write the following reg keys: If you change DumpType in the registry to 2 you get a full dump. Start with a mini which is default though since full dumps are huge. |
|
@andrewvoznytsa Sorry. I've been out of town. Here are the crash dumps of a 02/20/2020 Daily Build. It opened up fine after initial installation. The moment I changed the video option to H.264, it crashed. Hope this helps. Let me know if there's anything you want me to do. Thanks. |
|
Thanks for update @rollysing ! That moment, related to video codec change, was probably fixed and merged into master. If you get Daily build dated later than this comment then you can expect that there is no crash anymore (I hope very much). Could you please check? In the mean time I'll look on crash dump. |
|
@andrewvoznytsa The crash dumps I referenced are from a Daily Build I just downloaded and installed 2 hours ago. So the merge is still crashing once I opted to enable the video option. |
|
It crashes somewhere inside g_slice_alloc() (malloc() equivalent, part of glib which is part of gstreamer). Everything was built without debug info - that's piece of work to rebuild all things with debug info to get something more useful than what I've found (manual stack walking is possible of course, but IMO it has no sense in our case). In all 3 cases it crashed exactly in the same place. If we want to save time then local debugging via Teamviewer can help. |
|
@andrewvoznytsa I'm UTC -05:00 US Eastern Time. Let me know when you'd be available and I'll have it setup and available for you to remote in. Thanks. |
|
@rollysing I'll prepare for this test and let you know (if we do this immediately I'll need to setup all dev tools which I usually use - this does not sound for me as right path to ask you install all these things - will need to see how can I debug that with minimal debug tool set and quickly). In the mean time, we can continue our lottery. I just pushed #8318 which sounds close to what you were reporting. I've doubts that it is also your issue but we can try - it sounds close. I expect that it will be merged shortly and you can try that tomorrow. |
|
A full dump instead of a mini-dump won't provide you enough information? That is about the same as getting it in the debugger yourself. |
|
The problem here is that gstreamer and glib were built without debug info.
VC is unable to show backtrace. I think that full dump also wont help.
Need to try on my side to see what is the easiest way to find what we are
looking for.
…On Wed, 12 Feb 2020 at 19:41, Don Gagne ***@***.***> wrote:
A full dump instead of a mini-dump won't provide you enough information?
That is about the same as getting it in the debugger yourself.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8093?email_source=notifications&email_token=ADKJSBOPW2AXSHWV44R7I2LRCQYDNA5CNFSM4JV65OG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELRWJMY#issuecomment-585327795>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKJSBOHOREDJLJGMBNAJ3DRCQYDNANCNFSM4JV65OGQ>
.
|
|
@rollysing here is debug build for you - https://drive.google.com/open?id=1eGWe-X6fv7n5fW_elpA13on-GqUEClfM It is latest master built in debug mode with symbols. You'll need to unpack that to As first step we can try naive approach - let's run If no - we'll need to continue with teamviewer. I'm here for next 4-6 hours - should be enough to fix that. If you are busy today - we can continue any other day. |
|
Getting this error almost immediately after starting installation: |
|
Debug builds only run on a system with all the debug DLLs loaded. |
|
Sigh. I'll prepare better build - will setup clean VM, collect all required DLLs and post it here. |
|
@rollysing please find missing dll there https://drive.google.com/open?id=1AQYHg3CX7rIj7V-RJpNmkHPM-4riyHUG Next step would be to run WinDBG for AMD64 Now it is your turn :) If this looks like alien technology - give me please teamviewer access and I'll debug it myself (perhaps we can use @DonLakeFlyer as man in the middle to send login details or other contact information). |
|
Don't see why crash dump file isn't the simplest way to go where. With a full crash dump for example you get everything you could do with WinDbg. See the stack trace at crash, move around the stack, look at variables/memory at each point in the stack, link to the source. |
|
@rollysing you can simply send crash dump after unpacking that missing dll if that is easier for you - did not wanted to throw all that WinDbg stuff on your head - it just looked safer for me to get needed crash information via WinDbg and finally fix that issue. We can always return to WinDbg if I'll be unable to find required debug info from crash dump because of some unforeseen problem (most likely everything will be good this time). |
|
By default the QGC Windows installer will set up the registry to collect mini dumps. That is the way to start. If from that you don't get enough information you can switch to full dumps (they are huge). I uploaded some reg files to S3 which will switch between the QGC default and full dumps: |
|
@andrewvoznytsa @DonLakeFlyer Good news! That QGC didn't crash! More importantly, I'm able to open a video overlay and the USB VTx is showing it! There's no crash dump to send so is there a log in this that you want? So, the next step is to incorporate what you have here into the stable version?Let me know. Thanks. |
|
@rollysing don't know if I should be happy or worried even more - let's try latest 4.0 daily build on your side and see if video is still working for you. There are possible two kind of problems:
|
|
@andrewvoznytsa Good news! I just downloaded the daily build and it installed with no problem, opens and am able to have the USB 2.0 Camera show on the overlay. |
|
@rollysing Perfect! Please let me know ASAP when I'll break that :) BTW, did you update something on your system recently? Perhaps older builds work too? |
|
@andrewvoznytsa Good news again! Installed stable 4.0 and it's working! So it looks like she's good from this stable version and on. Next step would be a field test to see how she does. One thing, though, when the USB connection of the PC camera is interrupted and comes back online, the view doesn't come back unless QGC is closed and restarted. Any updates would've been the usual Windows patch Tuesday stuff. So if there's anything from the past ones that you think might have contributed to this success would be it. In the meantime, will report back (don't know when) on the field test. Thank you very much for all the patience and assistance. |
|
@rollysing Good luck with your field test! |
This patch switches QGC from using builtin video sink to qmlglsink (part of GStreamer Good Plug-ins).
This change is part of my work to lower QGC maintenance efforts and to implement standalone test app for video receiver.
Code was tested on Linux (Ubuntu 18.04/Qt 12.6/Gstreamer 1.14.5/qmlglsink - git/master, local build).
Next few days I'll be testing this code on all QGC platforms. So please expect updates (at least for platforms which require static gst linking). Right now it is more like early preview for Linux users - I'd like to hear comments and tweak some parts if there are good reasons.