Avidemux Forum

Avidemux => Unix-Like (Linux/Bsd/...) => Topic started by: gilbsgilbs on September 18, 2017, 11:27:33 AM

Title: [Fixed] Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on September 18, 2017, 11:27:33 AM
Hi,

Under i3wm at least, some filter windows shows and hides instantly, making avidemux unusable. I can reproduce this issue consistently with the crop filter:
- i3wm (any version)
- avidemux (2.7.0, qt5 gui, but any version will reproduce)
- Open a video
- Change "Video output" to something different than "Copy" (HEVC x265 for instance)
- Click on Filters
- Double click on "Crop" filter

You will barely be able to see the crop window open and it will close instantly.

I already reported the issue on i3 bug tracker (https://github.com/i3/i3/issues/2939#issuecomment-330153163) and stapelberg, one of the i3 maintainers came up with this workaround:
--- avidemux2-2.7.0/avidemux/qt4/ADM_UIs/src/toolkit.cpp.orig 2017-09-18 10:06:11.238712841 +0200
+++ avidemux2-2.7.0/avidemux/qt4/ADM_UIs/src/toolkit.cpp 2017-09-18 10:06:25.150554818 +0200
@@ -9,8 +9,9 @@

void qtRegisterDialog(QWidget *dialog)
{
- if (widgetStack.count())
- dialog->setParent(widgetStack.top(), Qt::Dialog);
+ if (widgetStack.count()) {
+ //dialog->setParent(widgetStack.top(), Qt::Dialog);
+ }

widgetStack.push(dialog);
}


And pointed out QWidget documentation about setParent method:
QuoteWarning: It is very unlikely that you will ever need this function. If you have a widget that changes its content dynamically, it is far easier to use QStackedWidget.
==> https://doc.qt.io/qt-5/qwidget.html#setParent

Thanks,
Gilbsgilbs
Title: Re: Some filter windows won't display under specific desktop environments
Post by: AQUAR on September 18, 2017, 01:01:18 PM
No such issue on i7wm.
Crop filter can be added, previewed and configured.

Maybe try different display options under preferences.
Title: Re: Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on September 18, 2017, 05:19:30 PM
Hi AQUAR.

I tried to change every display option with no luck.

Oddly enough, I noticed at work today that I'm not able to reproduce the problem with a session manager (such as GDM). Still, the issue remains without session manager.

Thanks!
Title: Re: Some filter windows won't display under specific desktop environments
Post by: AQUAR on September 19, 2017, 01:45:31 AM
Ha, I presumed you were on a I3 windows system.

Linux is not my OS of choice and so no experience using this i3wm tiling manager.
Maybe try turning off any hardware acceleration - if they are turned on.
But from your post it seems to be an issue with the way i3wm operates.
Title: Re: Some filter windows won't display under specific desktop environments
Post by: eumagga0x2a on September 22, 2017, 02:41:15 PM
Please redirect the console output of Avidemux to a file, reproduce the issue and attach the compressed file to your reply. The problem may be not setParent as such but the overloaded function http://doc.qt.io/qt-5/qwidget.html#setParent-1 (http://doc.qt.io/qt-5/qwidget.html#setParent-1) which sets window flags in the same go as used by qtRegisterDialog. If you can build Avidemux from source (which is really easy on Linux), you could try if replacing

dialog->setParent(widgetStack.top(), Qt::Dialog);

with

dialog->setParent(widgetStack.top());

has any effect.

Anyway, this is the first report of that kind I'm aware of. The issue exists neither with less exotic window managers on Linux nor on Mac or Windows.
Title: Re: Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on September 24, 2017, 08:46:01 PM
Hi eumagga0x2a. Many thanks for your answer.

You'll find attached the full avidemux log while reproducing the issue.

I tried to remove the `Qt::Dialog` argument with no luck. Oddly enough, the patch from stapelberg is seemingly not working on my machine either.

Edit: Wow. It's definitely some kind of race condition. Using gdb, if I "break DIA_getCropParams", run, reproduce the issue (it stops on the BP) and "next", the window displays correctly. That's weird.

Thanks.
Title: Re: Some filter windows won't display under specific desktop environments
Post by: eumagga0x2a on September 27, 2017, 07:26:47 PM
Which plugins are affected by the issue apart from crop? Is mplayer delogo affected? What about mplayer eq2?
Title: Re: Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on September 29, 2017, 05:41:12 PM
Hi,
Here is the exhaustive list of affected plugins :

Edit: I didn't realize that I was linking avidemux shared objects to my system's which were unpatched. That's probably why stapelberg workaround din't work for me.
Title: Re: Some filter windows won't display under specific desktop environments
Post by: eumagga0x2a on September 30, 2017, 08:18:42 AM
Presumably, Msharpen should be on the list too. These all are filters which use on-the-fly preview from avidemux/qt4/ADM_UIs/src/DIA_flyDialog.cpp with header avidemux/qt4/ADM_UIs/include/DIA_flyDialogQt4.h, and you are right that the filter window doesn't close on its own (it is not destructed).

You could test if calling adjustCanvasPosition() and canvas->parentWidget()->setMinimumSize(30,30) just after show() in the filter dialog constructor is related to the issue. Just comment them out (the preview will be sort of broken, but that doesn't matter for now). For the crop filter, that would be in avidemux_plugins/ADM_videoFilters6/crop/qt5/DIA_flyCrop.cpp:376 and 377.
Title: Re: Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on September 30, 2017, 10:37:17 AM
I rebuilt everything with correct linking, and I can say with confidence that:
Title: Re: Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on September 30, 2017, 06:17:54 PM
I'm able to reproduce the issue with this tiny snippet:


#include <QtWidgets>

class MainWindow : public QMainWindow {
public:
    MainWindow(QWidget * parent = 0, Qt::WindowFlags flags = 0) {
        show();
        QWidget *subDialog = new QWidget();
        subDialog->show();
        subDialog->setParent(this);
    }
};

int main(int argc, char **argv) {
    QApplication app(argc, argv);
    MainWindow window;
    return app.exec();
}


Appearently, i3 doesn't like when a parent window is set just after it's shown. I'm not a QT or WM expert, but it definitely sounds like a bug on i3 side. I'm going to re-open my ticket upstream (edit: follow-up (https://github.com/i3/i3/issues/2939#issuecomment-333327447)).
Still, is it really a good idea from avidemux to do this? Even worse, is it a good idea to change the parent window multiple times like this?

What do you think of this (to let calee set the parent window if needed):


diff --git a/avidemux/qt4/ADM_UIs/src/toolkit.cpp b/avidemux/qt4/ADM_UIs/src/toolkit.cpp
index 20dbb99a..c9724b13 100644
--- a/avidemux/qt4/ADM_UIs/src/toolkit.cpp
+++ b/avidemux/qt4/ADM_UIs/src/toolkit.cpp
@@ -9,7 +9,7 @@ static QStack<QWidget*> widgetStack;

void qtRegisterDialog(QWidget *dialog)
{
-       if (widgetStack.count())
+       if (widgetStack.count() && !dialog->parentWidget())
                dialog->setParent(widgetStack.top(), Qt::Dialog);

        widgetStack.push(dialog);


A more specific fix would be to check for visibility instead. But this looks to have more side-effects to me.

diff --git a/avidemux/qt4/ADM_UIs/src/toolkit.cpp b/avidemux/qt4/ADM_UIs/src/toolkit.cpp
index 20dbb99a..c9724b13 100644
--- a/avidemux/qt4/ADM_UIs/src/toolkit.cpp
+++ b/avidemux/qt4/ADM_UIs/src/toolkit.cpp
@@ -9,7 +9,7 @@ static QStack<QWidget*> widgetStack;

void qtRegisterDialog(QWidget *dialog)
{
-       if (widgetStack.count())
+       if (widgetStack.count() && dialog->isHidden())
                dialog->setParent(widgetStack.top(), Qt::Dialog);

        widgetStack.push(dialog);


I think it's prefarable to show the window after it's registered. So my final vote goes to something like this:

diff --git a/avidemux/qt4/ADM_UIs/src/toolkit.cpp b/avidemux/qt4/ADM_UIs/src/toolkit.cpp
index 20dbb99a..c9724b13 100644
--- a/avidemux/qt4/ADM_UIs/src/toolkit.cpp
+++ b/avidemux/qt4/ADM_UIs/src/toolkit.cpp
@@ -9,7 +9,7 @@ static QStack<QWidget*> widgetStack;

void qtRegisterDialog(QWidget *dialog)
{
-       if (widgetStack.count())
+       if (widgetStack.count() && !dialog->parentWidget())
                dialog->setParent(widgetStack.top(), Qt::Dialog);

        widgetStack.push(dialog);

diff --git a/avidemux_plugins/ADM_videoFilters6/crop/qt5/DIA_flyCrop.cpp b/avidemux_plugins/ADM_videoFilters6/crop/qt5/DIA_flyCrop.cpp
index 3ba4ec78..e7017484 100644
--- a/avidemux_plugins/ADM_videoFilters6/crop/qt5/DIA_flyCrop.cpp
+++ b/avidemux_plugins/ADM_videoFilters6/crop/qt5/DIA_flyCrop.cpp
@@ -372,7 +372,6 @@ uint32_t width,height;
       SPINNER(Top);
       SPINNER(Bottom);

-    show();
     myCrop->adjustCanvasPosition();
     canvas->parentWidget()->setMinimumSize(30,30); // allow resizing both ways after the dialog has settled
     myCrop->rubber->nestedIgnore--;

diff --git a/avidemux_plugins/ADM_videoFilters6/crop/qt5/Q_crop.cpp b/avidemux_plugins/ADM_videoFilters6/crop/qt5/Q_crop.cpp
index 0d150a21..9746817a 100644
--- a/avidemux_plugins/ADM_videoFilters6/crop/qt5/Q_crop.cpp
+++ b/avidemux_plugins/ADM_videoFilters6/crop/qt5/Q_crop.cpp
@@ -32,6 +32,7 @@ int DIA_getCropParams(        const char *name,crop *param,ADM_coreVideoFilter *in)

     Ui_cropWindow dialog(qtLastRegisteredDialog(), param,in);
     qtRegisterDialog(&dialog);
+    dialog.show();

     if(dialog.exec()==QDialog::Accepted)
     {


And obviously, replicate this on all concerned windows across the project.

Any of these would workaround my issue, and I think they all improve the way avidemux works. Let me know what you think is best and I can submit a PR if you will.


Edit:This doesn't reproduce anything. It's just a normal behavior from QT's setParent function.

QuoteNote: The widget becomes invisible as part of changing its parent, even if it was previously visible. You must call show() to make the widget visible again.

This probably means that the "setParent" call might happen too late under i3.

Thanks :) .
Title: Re: Some filter windows won't display under specific desktop environments
Post by: eumagga0x2a on September 30, 2017, 08:10:51 PM
Before reading your last reply, I pushed a tentative fix (https://github.com/mean00/avidemux2/commit/62831ed65fd459f1ff24f6e4ad6546a80a2ec029) to the git master. The bug was purely on the Avidemux side. It remained undetected for so long because the code never had to actually change window flags.

Thank you very much for your report and input.
Title: Re: Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on September 30, 2017, 08:12:26 PM
Great, thanks :)
Title: Re: Some filter windows won't display under specific desktop environments
Post by: eumagga0x2a on September 30, 2017, 08:16:07 PM
A verification of the tentative fix would be welcome :-)
Title: Re: Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on September 30, 2017, 08:16:58 PM
Quote from: eumagga0x2a on September 30, 2017, 08:16:07 PM
A verification of the tentative fix would be welcome :-)
I was going to post it, but this indeed fixes the issue. Thank you again.
Title: Re: [Fixed] Some filter windows won't display under specific desktop environments
Post by: eumagga0x2a on October 02, 2017, 06:48:44 AM
The analysis and thus the "fix" was wrong, I'll have to revert it. Before I break more stuff, could you please test if

void qtRegisterDialog(QWidget *dialog)
{
    if (widgetStack.count())
        dialog->setParent(widgetStack.top(), Qt::Dialog);
    dialog->hide();
    widgetStack.push(dialog);
}


in avidemux/qt4/ADM_UIs/src/toolkit.cpp works for you?

If the dialog is already visible when exec() (e.g. in DIA_getCropParams()) is called, we get a complete mess with modality and keyboard focus. We must ensure that it is hidden and not interfere with exec().
Title: Re: [Fixed] Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on October 02, 2017, 05:26:55 PM
eumagga0x2a, the fix doesn't work unfortunately. I can reproduce the bug from this post just like before 62831e.
Title: Re: [Fixed] Some filter windows won't display under specific desktop environments
Post by: eumagga0x2a on October 02, 2017, 07:13:59 PM
Could you please just test if adding hide() at the end of the constructor instead of these patches works?
Title: Re: [Fixed] Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on October 02, 2017, 07:22:08 PM
Quote from: eumagga0x2a on October 02, 2017, 07:13:59 PM
Could you please just test if adding hide() at the end of the constructor instead of these patches works?
Nope, didn't work. I have to comment out the "show()" from the constructor to make it work (which is not an option).
Title: Re: [Fixed] Some filter windows won't display under specific desktop environments
Post by: eumagga0x2a on October 03, 2017, 11:42:56 AM
Thank you, I've left the qtRegisterDialog code as is for now and have tried to fix window modality for each affected filter and the preview individually (works on macOS + Linux/gnome-shell + Windows). Could you please test whether these changes haven't regressed the dialog visibility with i3wm?
Title: Re: [Fixed] Some filter windows won't display under specific desktop environments
Post by: gilbsgilbs on October 03, 2017, 05:28:33 PM
Quote from: eumagga0x2a on October 03, 2017, 11:42:56 AM
Thank you, I've left the qtRegisterDialog code as is for now and have tried to fix window modality for each affected filter and the preview individually (works on macOS + Linux/gnome-shell + Windows). Could you please test whether these changes haven't regressed the dialog visibility with i3wm?
That works flawlessly. Tested on 21d0ab. Thank you :) .