[GTK][WPE] BubblewrapLauncher: Expose all V4L2 devices#61600
[GTK][WPE] BubblewrapLauncher: Expose all V4L2 devices#61600webkit-commit-queue merged 1 commit intoWebKit:mainfrom
Conversation
|
EWS run on previous version of this PR (hash 7f06ecb) Details |
|
EWS run on previous version of this PR (hash 2254907) Details |
TingPing
left a comment
There was a problem hiding this comment.
This could all be done with std::filesystem, personally I'm not sure the result is any nicer though.
|
EWS run on previous version of this PR (hash c097985) Details |
IIRC we currently prefer |
csaavedra
left a comment
There was a problem hiding this comment.
For C-style char* the preferred wrapper nowadays is CStringView. Some comments on that line. But I am not sure, perhaps this could be done somehow else instead of glib filesystem API.
| StringView name = StringView::fromLatin1(entry); | ||
| StringView suffix; | ||
|
|
||
| if (name.startsWith("video"_s)) |
There was a problem hiding this comment.
If you use CStringView, here you can use startsWith(name.span(), "video"_s).
There was a problem hiding this comment.
Ended up using WTF::FileSystem, which made string handling easier.
| } | ||
|
|
||
| while (const char* entry = g_dir_read_name(devDir.get())) { | ||
| StringView name = StringView::fromLatin1(entry); |
There was a problem hiding this comment.
Please use CStringView instead.
| StringView suffix; | ||
|
|
||
| if (name.startsWith("video"_s)) | ||
| suffix = name.substring(strlen("video")); |
There was a problem hiding this comment.
Instead of creating another string you can get a span from the CStringView.
| if (name.startsWith("video"_s)) | ||
| suffix = name.substring(strlen("video")); | ||
| else if (name.startsWith("media"_s)) | ||
| suffix = name.substring(strlen("media")); | ||
| else if (name.startsWith("v4l-subdev"_s)) | ||
| suffix = name.substring(strlen("v4l-subdev")); | ||
|
|
||
| if (suffix.isEmpty() || !suffix.containsOnly<isASCIIDigit>()) | ||
| continue; |
There was a problem hiding this comment.
I'm thinking that to avoid all the repetition this could be a small helper function that will match the format you are looking for, then you could have a std::array<ASCIILiteral> with "video", "media", "v4-subdev", etc, and use a lambda to find a match.
|
EWS run on previous version of this PR (hash c8c6f3b) Details |
|
EWS run on previous version of this PR (hash 1cb0c67) Details |
| if (suffix.isEmpty() || !suffix.containsOnly<isASCIIDigit>()) | ||
| continue; |
There was a problem hiding this comment.
You don't really use the suffix, this could be done inside the lambda and make it return a bool if you found a match.
| })); | ||
|
|
||
| for (auto& entry : FileSystem::listDirectory("/dev"_s)) { | ||
| StringView fileName(entry); |
There was a problem hiding this comment.
You can probably do all of this right away with a String, so the StringView is not really needed.
|
EWS run on previous version of this PR (hash 2b7317e) Details |
csaavedra
left a comment
There was a problem hiding this comment.
Still some things to iron out. For correctness I will leave the r+ to Patrick.
|
EWS run on previous version of this PR (hash 25d1933) Details |
|
EWS run on previous version of this PR (hash 6b872f5) Details |
| continue; | ||
|
|
||
| CString path = FileSystem::pathByAppendingComponent("/dev"_s, fileName).utf8(); | ||
| args.appendVector(Vector<CString>({ |
There was a problem hiding this comment.
Vector::appendList() is more efficient, as it avoids having to create an intermediate vector.
There was a problem hiding this comment.
The entire file uses appendVector for this, so I think it'll be best to make that a separate change.
There was a problem hiding this comment.
Are you planning to fix this?
There was a problem hiding this comment.
Looks like you have beat me to it!
|
EWS run on current version of this PR (hash 4140b87) Details |
https://bugs.webkit.org/show_bug.cgi?id=311007 Reviewed by Claudio Saavedra and Patrick Griffis. Although GTK uses xdg-desktop-portal for cameras, it still needs V4L2 devices to be exposed in the sandbox to make V4L2 video decode work. WPE already exposed some nodes, but not enough for some platforms, such as i.MX 8M Quad, where there are 2 camera and 2 VPU nodes. Make both GTK and WPE expose all V4L2 nodes. * Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp: (WebKit::bindV4l): (WebKit::bubblewrapSpawn): Canonical link: https://commits.webkit.org/310201@main
4140b87 to
67bd935
Compare
|
Committed 310201@main (67bd935): https://commits.webkit.org/310201@main Reviewed commits have been landed. Closing PR #61600 and removing active labels. |
|
Backported into |
🧪 api-ios
67bd935
4140b87
🧪 win-tests🧪 api-mac-debug🛠 playstation