core: refactor capture devices #7492
Conversation
hardening
requested review from
rpadaki,
sardination,
jkarthic and
npip99
as code owners
github-actions
bot
added
the
📁 Repo: protocol
hardening
force-pushed
the
david/capture_dev_plus_test
branch
2 times, most recently
from
55fd123
to
21542af
Compare
| @@ -305,27 +256,23 @@ static void update_current_device(WhistServerState* state, WhistTimer* statistic | |||
| LOG_INFO("Received an update capture device request to dimensions %dx%d with DPI %d", | |||
| true_width, true_height, state->client_dpi); | |||
|
|
|||
| // If a device already exists, we should reconfigure or destroy it | |||
| if (device != NULL) { | |||
There was a problem hiding this comment.
in retry_capture_screen(), device will be set to NULL after decided to retry.
But in this line, we remove the if (device != NULL) { here, then the code will always run into reconfigure_capture_device(), which has a FATAL_ASSERT inside:
FATAL_ASSERT(device && "NULL device was passed into reconfigure_capture_device!");
So after your change, the retry_capture_screen() will always trigger a FATAL, which makes it a useless function. Is this as expected?
There was a problem hiding this comment.
Correct, with the new impl device can't be NULL and that shall be an assert. Gonna correct that.
There was a problem hiding this comment.
looks like this functional bug hasn't been fixed yet
There was a problem hiding this comment.
I've treated that at the device level
There was a problem hiding this comment.
might worth a re-review if the commit history can be recovered
hardening
force-pushed
the
david/capture_dev_plus_test
branch
from
21542af
to
af0b0d3
Compare
Codecov Report
@@ Coverage Diff @@
## dev #7492 +/- ##
==========================================
- Coverage 56.13% 55.42% -0.71%
==========================================
Files 158 166 +8
Lines 32519 33836 +1317
==========================================
+ Hits 18255 18755 +500
- Misses 14010 14827 +817
Partials 254 254
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report at Codecov.
|
hardening
force-pushed
the
david/capture_dev_plus_test
branch
2 times, most recently
from
6af6f82
to
f68fd7d
Compare
Protocol End-to-End Streaming Test ResultsExperiments SummaryExpand Summary✅ Experiment 1 - Bandwidth: Unbounded, Delay: None, Packet Drops: None, Queue limit: default. Download logs: aws s3 cp s3://whist-e2e-protocol-test-logs/david/capture_dev_plus_test/2022_11_22@17-56-44/ 2022_11_22@17-56-44/ --recursive✅ Experiment 2 - Bandwidth: variable between 15Mbit and 30Mbit, Delay: 10 ms, Packet Drops: None, Queue Limit: None, Conditions change over time? Yes, frequency is variable between 1000 ms and 2000 ms. Download logs: aws s3 cp s3://whist-e2e-protocol-test-logs/david/capture_dev_plus_test/2022_11_22@18-02-23/ 2022_11_22@18-02-23/ --recursive✅ Experiment 3 - Bandwidth: variable between 10Mbit and 20Mbit, Delay: 10 ms, Packet Drops: None, Queue Limit: None, Conditions change over time? Yes, frequency is variable between 500 ms and 2000 ms. Download logs: aws s3 cp s3://whist-e2e-protocol-test-logs/david/capture_dev_plus_test/2022_11_22@18-06-30/ 2022_11_22@18-06-30/ --recursive❌ Experiment 4 - Bandwidth: 10Mbit, Delay: 10 ms, Packet Drops: None, Queue Limit: 100 packets, Conditions change over time? No.. Download logs: aws s3 cp s3://whist-e2e-protocol-test-logs/david/capture_dev_plus_test/2022_11_22@18-10-37/ 2022_11_22@18-10-37/ --recursiveFull Results: link here |
hardening
force-pushed
the
david/capture_dev_plus_test
branch
9 times, most recently
from
66f196d
to
cceef3f
Compare
| } | ||
| infos->last_capture_device = NVIDIA_DEVICE; | ||
| } | ||
| return ret; |
There was a problem hiding this comment.
In the original implementation, if nvidia capture fails, the capture will smoothly switch to X11 capture (and nvidia capture will be re-created on next change of dimension). The failture will not be returned to upper level.
But in the new implementation, the failure will be returned to the upper level, causing upper level to destory both the nvidia_capture and x11 capture:
There is no mechainsim of smoothly failing back to X11 capture any more, which can cause more interruption for the user experience and looks like a regression.
Why do we make this change?
There was a problem hiding this comment.
That's a good point, even if I'm not sure if the old mechanism was effectively working, I'll try to redo the mechanism that goes back to X11 (and retry NV capture).
There was a problem hiding this comment.
yes the nvx11 capture device is more robust to that, when nv capture fails we automatically fallback to X11. So if we ever return -1 it means that X11 backend is also broken.
|
@wangyu- a bunch of good remarks, will treat them |
hardening
force-pushed
the
david/capture_dev_plus_test
branch
2 times, most recently
from
d3d7e24
to
a051134
Compare
This big refactor abstract capture devices to prepare things for the weston backend. The refactoring make it possible to have multiple capture device at the same time (capture device is not fixed at build time)
hardening
force-pushed
the
david/capture_dev_plus_test
branch
from
a051134
to
73138ff
Compare
|
@hardening could you please not squeese all commits into one immediately after addressed the reviews? it makes reviewer very hard to know what you have changed since last review. @hardening can you recover the commit history? |
|
@wangyu- Yeah sorry I'm rebasing and commit amending to have a clean log, but this has the drawback that you loose changes. So I don't think I can get back the changes since last commit (although I'm not a git expert) |
This big refactor abstract capture devices to prepare things for the weston backend. The refactoring make it possible to have multiple capture device at the same time (capture device is not fixed at build time)
Ticket(s) Closed
Description
Implementation
Documentation & Tests Added
Testing Instructions
PR Checklist