Feature/xcb grabber #912
Merged
Feature/xcb grabber #912
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
m-seker
requested review from
brindosch,
Paulchen-Panther,
Lord-Grey and
tpmodding
and removed request for
brindosch
|
Hello @m-seker 👋 I'm your friendly neighborhood bot and would like to say thank you for So that you and other users can test your changes more quickly, If you make changes to your PR, i create a new link to your workflow artifacts. Best regards, |
m-seker
force-pushed
the
feature/xcb_grabber
branch
from
6b555f8
to
742c05d
Compare
|
Here is your new link to your workflow artifacts. |
m-seker
force-pushed
the
feature/xcb_grabber
branch
from
742c05d
to
4f6063e
Compare
|
Here is your new link to your workflow artifacts. |
m-seker
force-pushed
the
feature/xcb_grabber
branch
from
4f6063e
to
291250d
Compare
|
Here is your new link to your workflow artifacts. |
m-seker
force-pushed
the
feature/xcb_grabber
branch
from
291250d
to
63cea83
Compare
|
Here is your new link to your workflow artifacts. |
m-seker
force-pushed
the
feature/xcb_grabber
branch
from
63cea83
to
20c9fa5
Compare
|
Here is your new link to your workflow artifacts. |
m-seker
commented
Jul 27, 2020
| @@ -0,0 +1,278 @@ | |||
| #.rst: | |||
There was a problem hiding this comment.
I got this file from another project
m-seker
commented
Jul 27, 2020
| @@ -0,0 +1,180 @@ | |||
| #.rst: | |||
There was a problem hiding this comment.
I got this file from another project
m-seker
commented
Jul 27, 2020
| @@ -72,8 +72,13 @@ void X11Grabber::setupResources() | |||
| _shminfo.readOnly = False; | |||
| XShmAttach(_x11Display, &_shminfo); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Fixes a bug in X11 grabber. Image resampler needs to be reinitialized during resource setup (Pixel decimation can't be set on the fly without this fix)
m-seker
commented
Jul 27, 2020
| } | ||
|
|
||
| if (isActive()) | ||
| { | ||
| if (_grabber.updateScreenDimensions() >= 0 ) |
There was a problem hiding this comment.
Is there a reason for calling updateScreenDimensions every time we want to grab a frame ?
m-seker
commented
Jul 27, 2020
| @@ -0,0 +1,436 @@ | |||
| #include <utils/Logger.h> | |||
There was a problem hiding this comment.
I tried to make this file as similar as possible to X11 grabber
m-seker
commented
Jul 27, 2020
| @@ -0,0 +1,38 @@ | |||
| cmake_minimum_required(VERSION 3.0.0) | |||
There was a problem hiding this comment.
copied from hyperion-x11
m-seker
commented
Jul 27, 2020
| @@ -0,0 +1,47 @@ | |||
|
|
|||
There was a problem hiding this comment.
copied from hyperion-x11
m-seker
commented
Jul 27, 2020
| @@ -0,0 +1,115 @@ | |||
|
|
|||
There was a problem hiding this comment.
copied from hyperion-x11
m-seker
commented
Jul 27, 2020
| @@ -0,0 +1,57 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
copied from hyperion-x11
m-seker
commented
Jul 27, 2020
src/hyperiond/hyperiond.cpp
Outdated
| @@ -117,8 +118,6 @@ HyperionDaemon::HyperionDaemon(const QString rootPath, QObject *parent, const bo | |||
| // spawn all Hyperion instances (non blocking) | |||
| _instanceManager->startAll(); | |||
|
|
|||
| //connect(_hyperion,SIGNAL(closing()),this,SLOT(freeObjects())); // TODO for app restart, refactor required | |||
There was a problem hiding this comment.
Already done #894
m-seker
force-pushed
the
feature/xcb_grabber
branch
from
20c9fa5
to
1be3a9a
Compare
|
Here is your new link to your workflow artifacts. |
m-seker
force-pushed
the
feature/xcb_grabber
branch
from
1be3a9a
to
89d2e23
Compare
|
Here is your new link to your workflow artifacts. |
That's interesting. They are set to true next to each other : @Paulchen-Panther do you have any idea why this might happen ? EDIT : I can't reprodice btw. |
I actually didn't think a case where X11 is disabled but XCB is enabed. See my previous post. But I didn't think about a custom compile case. Someone may want to just disable X11 in compile time and leave XCB eanbled. So yes, that's an issue, I'll check. |
|
„ I actually didn't think a case where X11 is disabled but XCB is enabed. See my previous post.“ As you added Xcb as THE alternative to x11, because x11 has limitations, I would have expected that Xcb is the default going forward and has higher priority during configuration. |
|
|
„ That's interesting. They are set to true next to each other :“ Looks like the x11 value was used from the CMake Cache |
I would say let's test XCB first in production before reducing priority of X11. |
|
I do not understand the parameters. Param 1 (RELEASE) is 'x86x64 ' and Param 2 (PLATFORM) is 'x86'? |
|
PLATFORM is definitely wrong |
|
Yes, 2nd Parameter is wrong in the repo. Edit: I just fixed it. This is why I am running with an updated version. I run with x11 or x11-dev as platform Nevertheless, let me rebuild the CMake Cache tomorrow. Seems the behavior is my fault. |
|
@Lord-Grey Warum testest du nicht einfach das Artefakt? Es wurde dafür eingeführt. |
|
@paulchenpanther Ich habe vom Start her getestet. Wenn Scripts angepasst wurden, dann gehören die natürlich zum Testumfang dazu. |
|
Du solltest aber erst die Artefakte testen. Danach kannst du gerne selber kompilieren und testen. Das ist der Sinn der Artefakte. |
|
Trotzdem danke für das testen für das Script. 👍 |
Edit: If X11 and Xcb are enabled during build, this issues does not occur. |
|
Additional Finding: A High Picture decimation is not considered correctly. Set Picture decimation = 30 X11 Output
Xcb Output
|
|
Test Summary: Xcb grabber's main functionality works! Open topics (as X11 supports this correctly):
|
Can you share your logs ? Pixel decimation works OK on me. This must be happening because of an extension that happens to be in my system but not yours, so grabber chooses an alternate path to apply decimation which I don't see EDIT : Resolution detection and selecting XCB as default in case X11 is not available is fixed, see my last commit :88aa4d0 |
m-seker
force-pushed
the
feature/xcb_grabber
branch
from
401b751
to
88aa4d0
Compare
|
Here is your new link to your workflow artifacts. |
|
@m-seker a) Auto detection of resolution change - Works I did a full screenshot:
|
|
Tested amd64 Artifakt under Mint 20. |
|
Here is your new link to your workflow artifacts. |
|
@Lord-Grey Should be fixed with this : e2fb4d0 |
|
Here is your new link to your workflow artifacts. |
Lord-Grey
approved these changes
Aug 3, 2020
Lord-Grey
pushed a commit
to Lord-Grey/hyperion.ng
that referenced
this pull request
Aug 4, 2020
* Add Xcb grabber * update compile instruction Signed-off-by: Paulchen Panther <[email protected]> * Fix problem on resolution change + Make XCB default if X11 is not avaialable * Fix decimation problem Co-authored-by: Paulchen Panther <[email protected]> Co-authored-by: Paulchen Panther <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Add XCB grabber. An alternative for X11 grabber.
libx11 has some inherent problems that can't be fixed (#892)
This PR introduces an alternative. Grabbing via XCB should also be a bit faster. Maybe we can make this default instead of X11 in the future.
What kind of change does this PR introduce? (check at least one)
If changing the UI of web configuration, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing setups:
The PR fulfills these requirements:
Fixes: #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
PLEASE DON'T FORGET TO ADD YOUR CHANGES TO CHANGELOG.MD
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: