[Fixed] Some filter windows won't display under specific desktop environments

[gilbsgilbs]

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:

Code Select Expand

--- 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:

Warning: 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

[AQUAR]

No such issue on i7wm.
Crop filter can be added, previewed and configured.

Maybe try different display options under preferences.

[gilbsgilbs]

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!

[AQUAR]

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.

[eumagga0x2a]

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 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

Code Select Expand

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

with

Code Select Expand

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.

[gilbsgilbs]

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.

[eumagga0x2a]

Which plugins are affected by the issue apart from crop? Is mplayer delogo affected? What about mplayer eq2?

[gilbsgilbs]

Hi,
Here is the exhaustive list of affected plugins :

• Add logo
• Blacken borders
• Crop
• ChromaShift
• Contrast
• MPlayer eq2
• MPlayer Hue
• Asharp
• MPlayer delogo2

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.

[eumagga0x2a]

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.

[gilbsgilbs]

I rebuilt everything with correct linking, and I can say with confidence that:

• Unlike what I said, stapelberg's workaround actually works
• Unlike what I said, the workaround you suggested of removing "Qt::Dialog" parameter from the setParent call also works
• Commenting out DIA_flyCrop.cpp:376-377 has no effect on the issue

[gilbsgilbs]

I'm able to reproduce the issue with this tiny snippet:

Code Select Expand


#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).
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):

Code Select Expand


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.

Code Select Expand


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:

Code Select Expand


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.

Note: 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 .

[eumagga0x2a]

Before reading your last reply, I pushed a tentative fix 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.

[gilbsgilbs]

Great, thanks

[eumagga0x2a]

A verification of the tentative fix would be welcome :-)

[gilbsgilbs]

A verification of the tentative fix would be welcome :-)

I was going to post it, but this indeed fixes the issue. Thank you again.