Fix Qt screenhot crash

[m-seker]

Summary

resize() is forgotten for image before using

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Docs
• Build-related changes
• Other, please describe:

If changing the UI of web configuration, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

• Yes
• No

If yes, please describe the impact and migration path for existing setups:

The PR fulfills these requirements:

• When resolving a specific issue, it's referenced in the PR's body (e.g. Fixes: #xxx[,#xxx], where "xxx" is the issue number)

If adding a new feature, the PR's description includes:

• A convincing reason for adding this feature
• Related documents have been updated (docs/docs/en)
• Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

[hyperion-project]

Hello @m-seker 👋

I'm your friendly neighborhood bot and would like to say thank you for
submitting a pull request to Hyperion!

So that you and other users can test your changes more quickly,
you can find your workflow artifacts here.

If you make changes to your PR, i create a new link to your workflow artifacts.

Best regards,
Hyperion-Project

[Paulchen-Panther]

Then you were slower than me. : smile
I already have the QT and DX changes in DirectX PR. How do we want to handle this?
9d142c8

[m-seker]

Then you were slower than me. : smile
I already have the QT and DX changes in DirectX PR. How do we want to handle this?
9d142c8

haha then I m just closing this

[Lord-Grey]

Then you were slower than me. : smile
I already have the QT and DX changes in DirectX PR. How do we want to handle this?
9d142c8

@Paulchen-Panther Hey, I guess you are mixing things. You fixed that QtGrabber does not write a screenshot,
while @m-seker fixed a core dump related to some image processing.

I think merging is required rather than closing this one.

[tpmodding]

If you want to merge it, then please dont forget to add it to the changelog

[Paulchen-Panther]

Then you were slower than me. : smile
I already have the QT and DX changes in DirectX PR. How do we want to handle this?
9d142c8

@Paulchen-Panther Hey, I guess you are mixing things. You fixed that QtGrabber does not write a screenshot,
while @m-seker fixed a core dump related to some image processing.

I think merging is required rather than closing this one.

Kann ich so nicht unterschreiben.
Das einzige was m-seker geändert hat ist, eine Überprüfung einzubauen ob das Target Image in der Größe geändert werden muss oder nicht. (was sowieso auch in der QTWrapper Function capture() enthalten ist.) Im QT Grabber ändert er erst kurz vor dem schreiben des Target Images die Größe, falls es notwendig ist. Da kommen wir zum nächsten Thema. Warum baut m-seker die Überprüfung nicht in die Image.h? Warum muss jedesmal an irgend einer Stelle zuvor erst geprüft werden wenn es die Image Klasse erledigen könnte.
Noch eine Frage. Du hast dir den Start Post von m-seker durchgelesen? Da steht "resize() is forgotten for image before using", und genau das gleiche macht mein Fix auch. Also wo ist jetzt das Problem???????

[Lord-Grey]

Also wo ist jetzt das Problem???????

Kein Problem... Ich hatte nur den QtGrabber im aktuellen Master aufgerufen und er ist gecored in Klassen, in denen m-seker zuletzt Anpassungen gemacht hatte. Darum hatte ich ihn angesprochen und er hat diesen Fix gemacht.
Für den Grabber ist Dein Fix sicherlich gleichwertig. Mein Punkt war ja nur, dass es noch zwei andere Files hier im PR gibt, die außerhalb des Grabbers Kontexts sind. Für mich sah es von außen so aus, dass der PR nicht ganz deckungsgleich mit den dem anderen Fix ist und deshalb hatte ich einen Merge der beiden PRs vorgeschlagen.
Wenn es einen besseren Weg z.B. via Image.h für die Stabilität gibt, hast Du meine Unterstützung...

[Paulchen-Panther]

@Lord-Grey
Danke fürs Feedback. Wir fragen am besten @m-seker.

@m-seker
Why not outsource the check of the target image size to the Image::resize instead of having to list it again and again in the code?

[Lord-Grey]

@Paulchen-Panther Zusatzfrage: In dem capture() Fix wird jetzt resize gemacht (wie bei dem anderen Fix), es gibt aber auch noch zusätzlich ein Signal (emit sig_screenshot(_screenshot);). Für mich zum Verständnis... Ist das notwendig? Oder Overhead?

[Paulchen-Panther]

Das Signal wird mit keinen Slot verbunden wenn ein Screenshot angefordert wird. Dies wird nur gesendet wenn der externe Grabber genutzt wird.

hyperion.ng/src/hyperion-qt/hyperion-qt.cpp

Line 92 in bfb50b8

QObject::connect(&grabber, SIGNAL(sig_screenshot(const Image<ColorRgb> &)), &flatbuf, SLOT(setImage(Image<ColorRgb>)));

Sollte also kein Problem sein. QT sendet das Signal im screenshot modus nicht. Du wirst kein Unterschied in der Geschwindigkeit merken wenn du ein Screenshot erstellst. Mit oder ohne Signal. Kann also bleiben.

[Lord-Grey]

Das Signal wird mit keinen Slot verbunden wenn ein Screenshot angefordert wird. Dies wird nur gesendet wenn der externe Grabber genutzt wird.

hyperion.ng/src/hyperion-qt/hyperion-qt.cpp

Line 92 in bfb50b8

QObject::connect(&grabber, SIGNAL(sig_screenshot(const Image<ColorRgb> &)), &flatbuf, SLOT(setImage(Image<ColorRgb>)));

Sollte also kein Problem sein. QT sendet das Signal im screenshot modus nicht. Du wirst kein Unterschied in der Geschwindigkeit merken wenn du ein Screenshot erstellst. Mit oder ohne Signal. Kann also bleiben.

Danke!

[Paulchen-Panther]

Das Signal wird mit keinen Slot verbunden wenn ein Screenshot angefordert wird. Dies wird nur gesendet wenn der externe Grabber genutzt wird.

hyperion.ng/src/hyperion-qt/hyperion-qt.cpp

Line 92 in bfb50b8

QObject::connect(&grabber, SIGNAL(sig_screenshot(const Image<ColorRgb> &)), &flatbuf, SLOT(setImage(Image<ColorRgb>)));

Sollte also kein Problem sein. QT sendet das Signal im screenshot modus nicht. Du wirst kein Unterschied in der Geschwindigkeit merken wenn du ein Screenshot erstellst. Mit oder ohne Signal. Kann also bleiben.

Danke!

Du könntest natürlich auch eine Abfrage bei jedem Capture() machen ob der HyperionManager == null ist und dann erst das emit abfeuern aber das ist genauso Overhead. Die Abfrage braucht bestimmt mehr Rechenzeit als ein einfaches emit ohne Slot.

edit: Kannst du gerne mit der integrierten Klasse Profile.h testen.

[m-seker]

@Paulchen-Panther @Lord-Grey The reason I check size before resizing the image is because of the last tweak I did.
Image class now uses implicit sharing, a.k.a. copy-on-write. It basically works on the principle that if you call a non-const method, then it assumes you are changing the state of the internal data so a copy of internal data should be made (in case the data is really shared, which is not in our case). For more reading : https://doc.qt.io/qt-5/implicit-sharing.html

So resize() works like this :
resize() -> this is a non const method, if shared first copy internal data then resize, else just resize.

Maybe I was just trying to elimniate the small overhead of checking if the data is shared or not, I don't really remember. In short, they are not necessary.