-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Client orientation (resolves #218) #1151
Conversation
PR #1151 <#1151> Signed-off-by: Romain Vimont <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work!
The behavior is not what I had in mind for #218 (I was thinking about rotating only the window on the client, possibly at any time using shortcuts), but I like the one you propose (and they are not incompatible anyway).
In particular, the rotation is taken into account when recording, and it allows to record with constant frame size (changing frame size on rotation often does not survive re-encoding).
Globally, I'm ok with the PR ✔️ Moreover, the code is clean and documentation and tests are updated 👍
Since there are many orientations, at least:
- device orientation (the Android orientation)
- video orientation (the orientation in which the video is captured and sent to the client, this is the target of this PR)
- window orientation (how the received video is rotated by the client)
I would suggest a more explicit parameter name, like --lock-video-orientation
or --force-video-orientation
.
I merged the documentation fix on master
. But this PR should target dev
branch instead (the idea is that master
only contains the last releases + bugfixes, and the prebuilt server from the last release must work with the client code, because it's the main page on GitHub).
app/src/cli.c
Outdated
@@ -351,6 +371,7 @@ scrcpy_parse_args(struct scrcpy_cli_args *args, int argc, char *argv[]) { | |||
{"help", no_argument, NULL, 'h'}, | |||
{"max-fps", required_argument, NULL, OPT_MAX_FPS}, | |||
{"max-size", required_argument, NULL, 'm'}, | |||
{"orientation", required_argument, NULL, 'o'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's an "advanced" option, I'd prefer not to "consume" a short option (letter) for it.
I initially assigned a short letter for everything, but since few releases, I assigned only long options to "advanced" features.
app/src/cli.c
Outdated
@@ -417,6 +438,11 @@ scrcpy_parse_args(struct scrcpy_cli_args *args, int argc, char *argv[]) { | |||
return false; | |||
} | |||
break; | |||
case 'o': | |||
if(!parse_orientation(optarg, &opts->orientation)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (
(with a space)
app/src/server.c
Outdated
@@ -124,9 +124,11 @@ execute_server(struct server *server, const struct server_params *params) { | |||
char max_size_string[6]; | |||
char bit_rate_string[11]; | |||
char max_fps_string[6]; | |||
char orientation_string[4]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-128\0
is 5 chars long :)
(OK, this value should not be possible here, but then 4
is also unexpected)
@@ -169,7 +172,30 @@ private static Position readPosition(ByteBuffer buffer) { | |||
int y = buffer.getInt(); | |||
int screenWidth = toUnsigned(buffer.getShort()); | |||
int screenHeight = toUnsigned(buffer.getShort()); | |||
return new Position(x, y, screenWidth, screenHeight); | |||
return rotatePosition(x, y, screenWidth, screenHeight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ControlMessageReader
is only responsible for deserializing the messages from the client, it should not manage rotation.
I suggest to do this in Device.getPhysicalPoint()
instead.
static Rect flipRect(Rect crop) { | ||
return new Rect(crop.top, crop.left, crop.bottom, crop.right); | ||
crop.set(crop.top, crop.left, crop.bottom, crop.right); | ||
return crop; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
The parameter should not be modified (it could have unexpected side effects).
@@ -27,23 +27,27 @@ | |||
|
|||
private int bitRate; | |||
private int maxFps; | |||
private int clientOrientation; | |||
private int rotationOffset = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(in Java, 0 is the default value for fields)
@@ -134,6 +139,13 @@ private void writeFrameMeta(FileDescriptor fd, MediaCodec.BufferInfo bufferInfo, | |||
IO.writeFully(fd, headerBuffer); | |||
} | |||
|
|||
private void setRotationOffset(int rotation) { | |||
if(clientOrientation != -1) {// user has requested orientation | |||
rotationOffset = rotation + clientOrientation % 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rotationOffset = (rotation + clientOrientation) % 4;
format.setInteger(MediaFormat.KEY_WIDTH, width); | ||
format.setInteger(MediaFormat.KEY_HEIGHT, height); | ||
private static void setSize(MediaFormat format, int orientation, int width, int height) { | ||
if(orientation % 2 == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (
@@ -134,6 +139,13 @@ private void writeFrameMeta(FileDescriptor fd, MediaCodec.BufferInfo bufferInfo, | |||
IO.writeFully(fd, headerBuffer); | |||
} | |||
|
|||
private void setRotationOffset(int rotation) { | |||
if(clientOrientation != -1) {// user has requested orientation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (
and { //
} | ||
|
||
public ScreenInfo withRotation(int newRotation) { | ||
if ((rotation + newRotation) % 2 == 0) { // 180s don't need flipping | ||
return this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then the returned ScreenInfo
will not have the correct rotation
value.
Thank you for the prompt review. I implemented the changes you proposed and merged with Dev. This is my first pull request and I can't find a way to change the base branch from my end.. If you want me I can make a new one to the correct branch. Thanks again, |
😱 don't merge branches in a PR/patchset :) You should rebase on If you want, just push force your last commit before the merge (keep this PR on (I will try to find time to review the changes next week) |
1a63a91
to
6395636
Compare
Sorry about that. I hope things are fixed now. Looking forward to your review. Thanks, |
Hi, I had some time tonight \o/ I squashed your 2 last commits, and applied the following changes:
My commit on I would appreciate if you could review my changes and test :) Here is the range-diff (diff of diffs between your 2 commits squashed and my commit which includes my changes, it can be hard to read but it gives the full changes): range-diff
|
Thank you for the review. I am ok with all changes and everything works for me. Only thing is that you should also swap lines 201 and 202 at cli.c so the default values are reported correctly. Thank you so much, |
Cool, thank you for your review and tests 👍
Good catch! I will rebase on Thanks! |
PR #1151 <#1151> Signed-off-by: Romain Vimont <[email protected]>
I tested on tablet, it works 🎉 I rebased and merged on Thank you for your contribution. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review. I am ok with all changes and everything works for me. Only thing is that you should also swap lines 201 and 202 at cli.c so the default values are reported correctly.
Thank you so much,
George
Thank to you bro, you mean i can fix my rotation problem? How to fix it?
Keep the window in landscape orientation: | ||
|
||
```bash | ||
scrcpy --orientation 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tryed
ctrl+r
C:\scr>scrcpy --orientation 3
scrcpy: unknown option -- orientation
C:\scr>scrcpy --lock-video-orientation 1
scrcpy: unknown option -- lock-video-orientation
but not go in landscape in some apps like this
https://i.imgur.com/lKPzlEg.jpg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using the dev
branch? This is not merged into master
yet.
Cheers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, any guide for build exe for win10 64bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how to fix this error?
root@debian:/home/debian/Downloads/scr/scr# make -f Makefile.CrossWindows
fatal: Not a git repository (or any of the parent directories): .git
./gradlew clean
BUILD SUCCESSFUL in 1s
1 actionable task: 1 up-to-date
rm -rf "build-server" "build-win32" "build-win64" \
"build-win32-noconsole" "build-win64-noconsole" "dist"
[ -d "build-server" ] || ( mkdir "build-server" && \
meson "build-server" \
--buildtype release -Dcompile_app=false )
The Meson build system
Version: 0.53.2
Source dir: /home/debian/Downloads/scr/scr
Build dir: /home/debian/Downloads/scr/scr/build-server
Build type: native build
Project name: scrcpy
Project version: 1.12.1
C compiler for the host machine: cc (gcc 6.3.0 "cc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516")
C linker for the host machine: cc ld.bfd 2.28
Host machine cpu family: x86_64
Host machine cpu: x86_64
Program ./scripts/build-wrapper.sh found: YES (/bin/bash /home/debian/Downloads/scr/scr/server/./scripts/build-wrapper.sh)
WARNING: Project targeting '>= 0.37' but tried to use feature introduced in '0.48.0': console arg in custom_target
DEPRECATION: build_always is deprecated. Combine build_by_default and build_always_stale instead.
Build targets in project: 2
WARNING: Project specifies a minimum meson_version '>= 0.37' but uses features which were added in newer versions:
* 0.48.0: {'console arg in custom_target'}
Found ninja-1.7.2 at /usr/bin/ninja
ninja -C "build-server"
ninja: Entering directory `build-server'
[0/1] Generating scrcpy-server with a custom command.
(not invoking gradle, since we are root)
make -C prebuilt-deps prepare-win32
make[1]: Entering directory '/home/debian/Downloads/scr/scr/prebuilt-deps'
make[1]: execvp: ./prepare-dep: Permission denied
Makefile:33: recipe for target 'prepare-sdl2' failed
make[1]: *** [prepare-sdl2] Error 127
make[1]: Leaving directory '/home/debian/Downloads/scr/scr/prebuilt-deps'
Makefile.CrossWindows:51: recipe for target 'prepare-deps-win32' failed
make: [prepare-deps-win32] Error 2 (ignored)
[ -d "build-win32" ] || ( mkdir "build-win32" && \
meson "build-win32" \
--cross-file cross_win32.txt \
--buildtype release --strip -Db_lto=true \
-Dcrossbuild_windows=true \
-Dcompile_server=false \
-Dportable=true )
The Meson build system
Version: 0.53.2
Source dir: /home/debian/Downloads/scr/scr
Build dir: /home/debian/Downloads/scr/scr/build-win32
Build type: cross build
Project name: scrcpy
Project version: 1.12.1
C compiler for the build machine: cc (gcc 6.3.0 "cc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516")
C linker for the build machine: cc ld.bfd 2.28
C compiler for the host machine: /usr/bin/i686-w64-mingw32-gcc (gcc 6.3.0 "i686-w64-mingw32-gcc (GCC) 6.3.0 20170516")
C linker for the host machine: /usr/bin/i686-w64-mingw32-gcc ld.bfd 2.28
Build machine cpu family: x86_64
Build machine cpu: x86_64
Host machine cpu family: x86
Host machine cpu: i686
Target machine cpu family: x86
Target machine cpu: i686
app/meson.build:46:4: ERROR: Include dir ../prebuilt-deps/SDL2-2.0.10/i686-w64-mingw32/include does not exist.
A full log can be found at /home/debian/Downloads/scr/scr/build-win32/meson-logs/meson-log.txt
Makefile.CrossWindows:54: recipe for target 'build-win32' failed
make: *** [build-win32] Error 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not related to the pull request. I don't know how to build for windows either. I know you should not be building as root though. Please ask your question in an appropriate thread so people can help you.
PS: You should also checkout the dev
branch before you run the build since --lock-video-orientation
is what you are after.
Thanks
@Stamoulohta I noticed that the initial video size was incorrect if the video orientation was locked in an orientation different from the device. Concretely, the client initialized the window to 1080x1920, but immediately (after the first frame) it reset to 1920x1080. I refactored a bit and implemented changes to send the correct initial size: Thank you. |
@rom1v Surely! I will have some time later today I think. I'll get back to you as soon as possible. |
Did you find some time to test? :) |
Unfortunately not :( I haven't forgot about it but It's been a bit hectic these last days here in Greece. I hope this week we will reach a relative normality and we'll be able to go on with our lives. Sorry about that, |
No problem, I understand. I merged it into dev. If you encounter any problem when you test, we'll fix them :) |
Great! |
During PR #1151, this field has been moved to ScreenInfo, but has not been removed from ScreenEncoder.
I've added an option to keep the client window in a specific orientation. This helps in recording and in utilizing small screens.
I've tried my best to update any relevant documentation and test files. I could not update the Korean documentation files though.
Thanks,
George