Repository navigation
Conversation
rom1v
reviewed
Dec 18, 2024
rom1v
left a comment
rom1v
left a comment
Collaborator
There was a problem hiding this comment.
Thank you 👍
I just read a first pass, I will check in more details later.
It makes a difference only on some Samsung devices, is that right?
| } | ||
| } | ||
| if (!isBackwardCompatible) { | ||
| continue; |
Collaborator
There was a problem hiding this comment.
We should do that even with the CameraManager then (the current implementation), correct?
Contributor
Author
There was a problem hiding this comment.
Correct. On my Xiaomi devices, there are two invalid cameras ("facing" is "external" and fails to open) that can be filtered by this capability. However, they are hidden in CameraManager.getCameraIdList. I don't know if any OS returns non-backward-compatible cameras from CameraManager
|
|
||
| List<String> cameraIds = new ArrayList<>(); | ||
| for (CameraStatus cameraStatus : cameraStatuses) { | ||
| if (cameraStatus.status != ICameraServiceListener.STATUS_NOT_PRESENT && cameraStatus.status != ICameraServiceListener.STATUS_ENUMERATING) { |
Collaborator
There was a problem hiding this comment.
Contrary to CameraManager.getCameraIdList(), shouldHideCamera() is not called. Is it on purpose to show camera that are wrongly hidden?
Contributor
Author
There was a problem hiding this comment.
Yes. shouldHideCamera was added recently, so for example when a companion device app added a virtual camera, it won't be seen by normal apps, I don't know if anyone is doing that, but it should be interesting to try to include them.
Contributor
Author
It shouldn't affect Samsung devices, because It will affect Xiaomi devices (#4392 (comment)) and LineageOS (#4392 (comment)) |
Collaborator
|
While I was making tests for this PR with a Xiaomi Redmi Note 13, I noticed that dc2fcc4 (#5659) already fixes the problem. On On Can you confirm? |
Contributor
Author
|
I can confirm On Xiaomi Mi 11: On Redmi K70 Pro: Check for |
Collaborator
|
Thank you for your test 👍
Maybe we can keep camera 8 and 9 listed, but without printing the exception (and also disable it for other errors): diff --git server/src/main/java/com/genymobile/scrcpy/util/LogUtils.java server/src/main/java/com/genymobile/scrcpy/util/LogUtils.java
index 088be7e7d..76bf503e1 100644
--- server/src/main/java/com/genymobile/scrcpy/util/LogUtils.java
+++ server/src/main/java/com/genymobile/scrcpy/util/LogUtils.java
@@ -141,11 +141,12 @@ public final class LogUtils {
try {
// Capture frame rates for low-FPS mode are the same for every resolution
Range<Integer>[] lowFpsRanges = characteristics.get(CameraCharacteristics.CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES);
- SortedSet<Integer> uniqueLowFps = getUniqueSet(lowFpsRanges);
- builder.append(", fps=").append(uniqueLowFps);
- } catch (Exception e) {
+ if (lowFpsRanges != null) {
+ SortedSet<Integer> uniqueLowFps = getUniqueSet(lowFpsRanges);
+ builder.append(", fps=").append(uniqueLowFps);
+ }
+ } catch (RuntimeException e) {
// Some devices may provide invalid ranges, causing an IllegalArgumentException "lower must be less than or equal to upper"
- Ln.w("Could not get available frame rates for camera " + id, e);
}
builder.append(')'); |
Contributor
Author
|
I think cameras 8 and 9 don't work at all, so not worth listing
|
Collaborator
|
OK, is it sufficient: diff --git server/src/main/java/com/genymobile/scrcpy/util/LogUtils.java server/src/main/java/com/genymobile/scrcpy/util/LogUtils.java
index 088be7e7d..81d499209 100644
--- server/src/main/java/com/genymobile/scrcpy/util/LogUtils.java
+++ server/src/main/java/com/genymobile/scrcpy/util/LogUtils.java
@@ -120,6 +120,23 @@ public final class LogUtils {
}
}
+ private static boolean isCameraBackwardCompatible(CameraCharacteristics characteristics) {
+ int[] capabilities = characteristics.get(CameraCharacteristics.REQUEST_AVAILABLE_CAPABILITIES);
+ if (capabilities == null) {
+ return false;
+ }
+
+ // Ignore depth cameras as suggested by official documentation
+ // <https://developer.android.com/media/camera/camera2/camera-enumeration>
+ for (int capability : capabilities) {
+ if (capability == CameraCharacteristics.REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
public static String buildCameraListMessage(boolean includeSizes) {
StringBuilder builder = new StringBuilder("List of cameras:");
CameraManager cameraManager = ServiceManager.getCameraManager();
@@ -132,6 +149,10 @@ public final class LogUtils {
builder.append("\n --camera-id=").append(id);
CameraCharacteristics characteristics = cameraManager.getCameraCharacteristics(id);
+ if (!isCameraBackwardCompatible(characteristics)) {
+ continue;
+ }
+
int facing = characteristics.get(CameraCharacteristics.LENS_FACING);
builder.append(" (").append(getCameraFacingName(facing)).append(", ");
In other words, is it still necessary on try {
…
} catch (IllegalArgumentException ignore) {
// Samsung devices might throw an IllegalArgumentException
// when getting camera characteristics for hidden cameras
}? |
Contributor
Author
|
That diff still prints the IDs ( I think using a try-catch (with |
Collaborator
|
OK, so I removed this part: https://github.com/Genymobile/scrcpy/pull/5669/files#diff-b0c97217600727b27d9e22e3e26e2f68cbec7ac8a6666e16f8cf91e7080898ffR169-R171 Please check the |
Contributor
Author
|
|
Collaborator
|
Thank you, I close this PR since CameraService is not necessary (at least for now). |
2 tasks done
bartsaintgermain
pushed a commit
to bartsaintgermain/scrcpy
that referenced
this pull request
Apr 11, 2025
PR Genymobile#5669 <Genymobile#5669> Signed-off-by: Romain Vimont <rom@rom1v.com>
bartsaintgermain
pushed a commit
to bartsaintgermain/scrcpy
that referenced
this pull request
Apr 11, 2025
Do not report an error if the returned FPS ranges array is null. Refs Genymobile#5669 <Genymobile#5669>
This file contains hidden or 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
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.
This is a cleaned-up version of #4392 (comment).
I'm using similar code in my project to enumerate cameras, then use Scrcpy to mirror them, but recently 2337f52 broke this usage.
I think it should rather list and accept non-functional camera IDs, than rejecting functional ones.
This doesn't include the code to handle logical vs. physical cameras.